[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <CAK7LNASKHOb5GNPu9XHWxyq-iucxTRkw99_7wL4zgm5HNc6_SA@mail.gmail.com>
Date: Mon, 5 Mar 2018 18:27:44 +0900
From: Masahiro Yamada <yamada.masahiro@...ionext.com>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>,
Kees Cook <keescook@...omium.org>,
Patrick McLean <chutzpah@...too.org>, Emese Revfy <re.emese@...il.com>,
Al Viro <viro@...iv.linux.org.uk>, Bruce Fields <bfields@...hat.com>,
"Darrick J. Wong" <darrick.wong@...cle.com>,
Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
Linux NFS Mailing List <linux-nfs@...r.kernel.org>,
Thorsten Leemhuis <regressions@...mhuis.info>,
"kernel-hardening@...ts.openwall.com" <kernel-hardening@...ts.openwall.com>
Subject: Re: RANDSTRUCT structs need linux/compiler_types.h (Was: [nfsd4]
potentially hardware breaking regression in 4.14-rc and 4.13.11)
Hi Linus,
2018-02-22 7:47 GMT+09:00 Linus Torvalds <torvalds@...ux-foundation.org>:
> On Wed, Feb 21, 2018 at 2:19 PM, Maciej S. Szmigiero
> <mail@...iej.szmigiero.name> wrote:
>>
>> One can see that offsets used to access various members of struct path are
>> different, and also that the original file from step 3 contains an object
>> named "__randomize_layout".
>
> Whee.
>
> Thanks for root-causing this issue, and this syntax of ours is clearly
> *much* too fragile.
>
> We actually have similar issues with some of our other attributes,
> where out nice "helpful" attribute shorthand can end up being just
> silently interpreted as a variable name if they aren't defined in
> time.
>
> For most of our other attributes, it just doesn't matter all that much
> if some user doesn't happen to see the attribute. For
> __randomize_layout, it's obviously very fatal, and silently just
> generates crazy code.
>
> I'm not entirely sure what the right solution is, because it's
> obviously much too easy to miss some #include by mistake. It's easy to
> say "you should always include the proper header", but if a failure to
> do so doesn't end up with any warnings or errors, but just silent bad
> code generation, it's much too fragile.
>
> I wonder if we could change the syntax of that "__randomize_layout"
> thing. Some of our related helper macros (ie
> randomized_struct_fields_start/end) don't have the same problem,
> because if you don't have the define for them, the compiler will
> complain about bad syntax.
>
> And other attribute specifiers we encourage people to put in other
> parts of the type, like __user etc, so they don't have that same
> parsing issue.
>
> I guess one _extreme_ fix for this would be to put
>
> extern struct nostruct __randomize_layout;
>
> in our include/linux/kconfig.h, which I think we end up always
> including first thanks to having it on the command line.
>
> Because if you do that, you actually get an error:
>
> CC [M] fs/nfsd/nfs4xdr.o
> In file included from ./include/linux/fs_struct.h:5:0,
> from fs/nfsd/nfs4xdr.c:36:
> ./include/linux/path.h:11:3: error: conflicting types for ‘__randomize_layout’
> } __randomize_layout;
> ^~~~~~~~~~~~~~~~~~
> In file included from <command-line>:0:0:
> ././include/linux/kconfig.h:8:28: note: previous declaration of
> ‘__randomize_layout’ was here
> extern struct nostruct __randomize_layout;
> ^~~~~~~~~~~~~~~~~~
> make[1]: *** [scripts/Makefile.build:317: fs/nfsd/nfs4xdr.o] Error 1
>
> and we would have figured this out immediately.
>
> Broken example patch appended, in case somebody wants to play with
> something like this or comes up with a better model entirely..
>
> Linus
>
Sorry for chiming in late.
I noticed this thread today,
honestly, the commit made me upset.
Can I suggest another way to make it less fragile?
__attribute((...)) can be placed after 'struct'.
So, we can write:
struct __randomize_layout path {
struct vfsmount *mnt;
struct dentry *dentry;
};
instead of
struct path {
struct vfsmount *mnt;
struct dentry *dentry;
} __randomize_layout;
If we force the former notation,
the undefined __randomize_layout results in a build error
instead of silent broken code generation.
It is true somebody can still place
__randomize_layout after the closing brace,
but can we check this by coccicheck or checkpatch.pl?
(we can describe it in coding style documentation, of course)
IMHO, we should not (ab)use include/linux/kconfig.h
to bring in misc things.
--
Best Regards
Masahiro Yamada
Powered by blists - more mailing lists
Confused about mailing lists and their use?
Read about mailing lists on Wikipedia
and check out these
guidelines on proper formatting of your messages.
