The header files for some structures could get included in such a way
that struct attributes (specifically __randomize_layout from path.h) would
be parsed as variable names instead of attributes. This could lead to
some instances of a structure being unrandomized, causing nasty GPFs, etc.
This patch makes sure the compiler_types.h header is included in
kconfig.h so that we've always got types and struct attributes defined,
since kconfig.h is included from the compiler command line.
Reported-by: Patrick McLean <[email protected]>
Root-caused-by: Maciej S. Szmigiero <[email protected]>
Suggested-by: Linus Torvalds <[email protected]>
Tested-by: Maciej S. Szmigiero <[email protected]>
Fixes: 3859a271a003 ("randstruct: Mark various structs for randomization")
Signed-off-by: Kees Cook <[email protected]>
---
Updated to include Tested-by. Linus, this looks ready to go. I'll send
-stable patches that just fix up path.h.
---
include/linux/kconfig.h | 3 +++
1 file changed, 3 insertions(+)
diff --git a/include/linux/kconfig.h b/include/linux/kconfig.h
index fec5076eda91..c5fd4ee776ba 100644
--- a/include/linux/kconfig.h
+++ b/include/linux/kconfig.h
@@ -64,4 +64,7 @@
*/
#define IS_ENABLED(option) __or(IS_BUILTIN(option), IS_MODULE(option))
+/* Make sure we always have all types and struct attributes defined. */
+#include <linux/compiler_types.h>
+
#endif /* __LINUX_KCONFIG_H */
--
2.7.4
--
Kees Cook
Pixel Security
On Thu, Feb 22, 2018 at 9:41 AM, Kees Cook <[email protected]> wrote:
>
> Updated to include Tested-by. Linus, this looks ready to go.
Ok, applied.
I'm a bit worried that this ends up bypassing our automatic dependency
generation.
Lookie here (in a fully built tree):
find . -name '*.o.cmd' |
xargs grep -L linux/compiler_types.h |
xargs grep -l linux/kconfig.h |
while read i; do
j=$(echo $i | sed 's/\.o.cmd$/\.c/' | sed 's:/\.:/:');
test -f $j && echo $j;
done
shows that a number of files don't end up depending on that header
file, even though it's included (that "grep -l linux/kconfig,h"
triggers on the command itself having that "-include linux/kconfig.h"
line).
It looks like "gcc -M" just doesn't list any files that get included
on the command line with "-include".
Now, there are very *few* files that don't end up eventually including
linux/compiler_types.h _some_ way, and I checked them all, and they
really are so trivial that this doesn't matter.
But it worries me a bit regardless.
Linus
On Thu, Feb 22, 2018 at 10:04 AM, Linus Torvalds
<[email protected]> wrote:
> On Thu, Feb 22, 2018 at 9:41 AM, Kees Cook <[email protected]> wrote:
>>
>> Updated to include Tested-by. Linus, this looks ready to go.
>
> Ok, applied.
>
> I'm a bit worried that this ends up bypassing our automatic dependency
> generation.
>
> Lookie here (in a fully built tree):
>
> find . -name '*.o.cmd' |
> xargs grep -L linux/compiler_types.h |
> xargs grep -l linux/kconfig.h |
> while read i; do
> j=$(echo $i | sed 's/\.o.cmd$/\.c/' | sed 's:/\.:/:');
> test -f $j && echo $j;
> done
>
> shows that a number of files don't end up depending on that header
> file, even though it's included (that "grep -l linux/kconfig,h"
> triggers on the command itself having that "-include linux/kconfig.h"
> line).
>
> It looks like "gcc -M" just doesn't list any files that get included
> on the command line with "-include".
Hmm. But does that mean deps for kconfig.h are broken too? That seems
silly. I'll take a look...
-Kees
--
Kees Cook
Pixel Security
On Thu, Feb 22, 2018 at 11:57 AM, Kees Cook <[email protected]> wrote:
>
> Hmm. But does that mean deps for kconfig.h are broken too? That seems
> silly. I'll take a look...
Yes, kconfig.h itself shares the same problem, but it has generally
been just about the config option testing itself, so you'd normally
never care.
I'm not saying that fixing that too would be wrong, I'm just saying
that linux/compiler_types.h tends to have way more subtle stuff in it,
and get changed in ways that are not directly related to the config
options that we track other ways (ie our dependency making script very
much is all about those config options).
Linus
On 2018-02-22 19:04, Linus Torvalds wrote:
>
> Lookie here (in a fully built tree):
>
> find . -name '*.o.cmd' |
> xargs grep -L linux/compiler_types.h |
> xargs grep -l linux/kconfig.h |
> while read i; do
> j=$(echo $i | sed 's/\.o.cmd$/\.c/' | sed 's:/\.:/:');
> test -f $j && echo $j;
> done
>
> shows that a number of files don't end up depending on that header
> file, even though it's included (that "grep -l linux/kconfig,h"
> triggers on the command itself having that "-include linux/kconfig.h"
> line).
>
> It looks like "gcc -M" just doesn't list any files that get included
> on the command line with "-include".
It does, both per the documentation and testing it. But fixdep
explicitly removes include/linux/kconfig.h along with
include/generated/autoconf.h and a few others. So when you rebuilt after
adding the #include to kconfig.h, I think nothing actually got built,
and no new .o.cmd files got generated.
Doing a clean build does make include/linux/compiler_{types,gcc}.h and
the various fake include/config/.... they "depend" on appear in e.g.
lib/.clz_tab.o.cmd.
The whole point of fixdep and the include/config hierarchy is to be able
to remove the dependency on autoconf.h, but I'm not sure I understand
why kconfig.h itself is also forcibly removed.
Rasmus
On Thu, Feb 22, 2018 at 1:07 PM, Rasmus Villemoes
<[email protected]> wrote:
> On 2018-02-22 19:04, Linus Torvalds wrote:
>>
>> Lookie here (in a fully built tree):
>>
>> find . -name '*.o.cmd' |
>> xargs grep -L linux/compiler_types.h |
>> xargs grep -l linux/kconfig.h |
>> while read i; do
>> j=$(echo $i | sed 's/\.o.cmd$/\.c/' | sed 's:/\.:/:');
>> test -f $j && echo $j;
>> done
>>
>> shows that a number of files don't end up depending on that header
>> file, even though it's included (that "grep -l linux/kconfig,h"
>> triggers on the command itself having that "-include linux/kconfig.h"
>> line).
>>
>> It looks like "gcc -M" just doesn't list any files that get included
>> on the command line with "-include".
>
> It does, both per the documentation and testing it. But fixdep
> explicitly removes include/linux/kconfig.h along with
> include/generated/autoconf.h and a few others. So when you rebuilt after
> adding the #include to kconfig.h, I think nothing actually got built,
> and no new .o.cmd files got generated.
>
> Doing a clean build does make include/linux/compiler_{types,gcc}.h and
> the various fake include/config/.... they "depend" on appear in e.g.
> lib/.clz_tab.o.cmd.
I'm seeing the same. If I do:
$ find . -name '*.o.cmd' | xargs rm
$ make allmodconfig
$ make -j...
...
$ find-command-above...
I get no hits. And doing spot-testing shows dep changes are detected:
$ ls -l fs/nfsd/nfs4xdr.o
-rw-rw-r-- 1 kees kees 276088 Feb 22 13:15 fs/nfsd/nfs4xdr.o
$ make fs/nfsd/nfs4xdr.o
...generated-files-only...
$ ls -l fs/nfsd/nfs4xdr.o
-rw-rw-r-- 1 kees kees 276088 Feb 22 13:15 fs/nfsd/nfs4xdr.o
$ touch include/linux/compiler_types.h
$ make fs/nfsd/nfs4xdr.o
...various...
CC [M] fs/nfsd/nfs4xdr.o
$ ls -l fs/nfsd/nfs4xdr.o
-rw-rw-r-- 1 kees kees 276088 Feb 22 13:21 fs/nfsd/nfs4xdr.o
Am I missing something?
> The whole point of fixdep and the include/config hierarchy is to be able
> to remove the dependency on autoconf.h, but I'm not sure I understand
> why kconfig.h itself is also forcibly removed.
Is there a mode where we're now rebuilding too much?
-Kees
--
Kees Cook
Pixel Security
On Thu, Feb 22, 2018 at 1:07 PM, Rasmus Villemoes
<[email protected]> wrote:
>
> It does, both per the documentation and testing it. But fixdep
> explicitly removes include/linux/kconfig.h along with
> include/generated/autoconf.h and a few others. So when you rebuilt after
> adding the #include to kconfig.h, I think nothing actually got built,
> and no new .o.cmd files got generated.
Hah. So it was the lack of kconfig.h dependency that bit my testing ;)
Linus
On 2018-02-22 22:07, Rasmus Villemoes wrote:
>
> The whole point of fixdep and the include/config hierarchy is to be able
> to remove the dependency on autoconf.h, but I'm not sure I understand
> why kconfig.h itself is also forcibly removed.
<goes digging> Ah, 6a5be57f "fixdep: fix extraneous dependencies", to
work around kconfig.h (naturally) containing the CONFIG_ pattern a few
times. Hm, we should probably make sure that kconfig.h is always on the
dependency list in .o.cmd, but just exclude it from being processed for
the CONFIG_ pattern.
Rasmus
On Thu, Feb 22, 2018 at 1:23 PM, Linus Torvalds
<[email protected]> wrote:
>
> Hah. So it was the lack of kconfig.h dependency that bit my testing ;)
Confirmed. Rasmus was right, doing a full build after cleaning
everything up did fix this. So it's really just <linux/kconfig.h>
itself that is missing from dependencies.
Linus
On Thu, Feb 22, 2018 at 1:34 PM, Rasmus Villemoes
<[email protected]> wrote:
>
> Hm, we should probably make sure that kconfig.h is always on the
> dependency list in .o.cmd, but just exclude it from being processed for
> the CONFIG_ pattern.
That sounds sensible to me.
Not that this likely has ever bit anybody outside of this one case,
but when missing dependencies cause a missed re-compile, the resulting
bugs can be _really_ subtle.
Linus
The string CONFIG_ quite often appears after other alphanumerics,
meaning that that instance cannot be referencing a Kconfig
symbol. Omitting these means make has fewer files to stat() when
deciding what needs to be rebuilt - for a defconfig build, this seems to
remove about 2% of the (wildcard ...) lines from the .o.cmd files.
Signed-off-by: Rasmus Villemoes <[email protected]>
---
scripts/basic/fixdep.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index d7fbe545dd5d..1b21870d6e7f 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -225,8 +225,13 @@ static int str_ends_with(const char *s, int slen, const char *sub)
static void parse_config_file(const char *p)
{
const char *q, *r;
+ const char *start = p;
while ((p = strstr(p, "CONFIG_"))) {
+ if (p > start && (isalnum(p[-1]) || p[-1] == '_')) {
+ p += 7;
+ continue;
+ }
p += 7;
q = p;
while (*q && (isalnum(*q) || *q == '_'))
--
2.15.1
uml-config.h hasn't existed in this decade (87e299e5c750 - x86, um: get
rid of uml-config.h). The few remaining UML_CONFIG instances are defined
directly in terms of their real CONFIG symbol in common-offsets.h, so
unlike when the symbols got defined via a sed script, anything that uses
UML_CONFIG_FOO now should also automatically pick up a dependency on
CONFIG_FOO via the normal fixdep mechanism (since common-offsets.h
should at least recursively be a dependency). Hence I believe we should
actually be able to ignore the HELLO_CONFIG_BOOM cases.
Cc: Al Viro <[email protected]>
Cc: Richard Weinberger <[email protected]>
Cc: [email protected]
Signed-off-by: Rasmus Villemoes <[email protected]>
---
scripts/basic/fixdep.c | 9 ---------
1 file changed, 9 deletions(-)
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index fa3d39b6f23b..d7fbe545dd5d 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -93,14 +93,6 @@
* (Note: it'd be easy to port over the complete mkdep state machine,
* but I don't think the added complexity is worth it)
*/
-/*
- * Note 2: if somebody writes HELLO_CONFIG_BOOM in a file, it will depend onto
- * CONFIG_BOOM. This could seem a bug (not too hard to fix), but please do not
- * fix it! Some UserModeLinux files (look at arch/um/) call CONFIG_BOOM as
- * UML_CONFIG_BOOM, to avoid conflicts with /usr/include/linux/autoconf.h,
- * through arch/um/include/uml-config.h; this fixdep "bug" makes sure that
- * those files will have correct dependencies.
- */
#include <sys/types.h>
#include <sys/stat.h>
@@ -286,7 +278,6 @@ static int is_ignored_file(const char *s, int len)
{
return str_ends_with(s, len, "include/generated/autoconf.h") ||
str_ends_with(s, len, "include/generated/autoksyms.h") ||
- str_ends_with(s, len, "arch/um/include/uml-config.h") ||
str_ends_with(s, len, "include/linux/kconfig.h") ||
str_ends_with(s, len, ".ver");
}
--
2.15.1
kconfig.h was excluded from consideration by fixdep by
6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false
positive hits
(1) include/config/.h
(2) include/config/h.h
(3) include/config/foo.h
(1) occurred because kconfig.h contains the string CONFIG_ in a
comment. However, since dee81e988674 (fixdep: faster CONFIG_ search), we
have a check that the part after CONFIG_ is non-empty, so this does not
happen anymore (and CONFIG_ appears by itself elsewhere, so that check
is worthwhile).
(2) comes from the include guard, __LINUX_KCONFIG_H. But with the
previous patch, we no longer match that either.
That leaves (3), which amounts to one [1] false dependency (aka stat() call
done by make), which I think we can live with:
We've already had one case [2] where the lack of include/linux/kconfig.h in
the .o.cmd file caused a missing rebuild, and while I originally thought
we should just put kconfig.h in the dependency list without parsing it
for the CONFIG_ pattern, we actually do have some real CONFIG_ symbols
mentioned in it, and one can imagine some translation unit that just
does '#ifdef __BIG_ENDIAN' but doesn't through some other header
actually depend on CONFIG_CPU_BIG_ENDIAN - so changing the target
endianness could end up rebuilding the world, minus that small
TU. Quoting Linus,
... when missing dependencies cause a missed re-compile, the resulting
bugs can be _really_ subtle.
[1] well, two, we now also have CONFIG_BOOGER/booger.h - we could change
that to FOO if we care
[2] https://lkml.org/lkml/2018/2/22/838
Cc: Linus Torvalds <[email protected]>
Signed-off-by: Rasmus Villemoes <[email protected]>
---
scripts/basic/fixdep.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
index 1b21870d6e7f..449b68c4c90c 100644
--- a/scripts/basic/fixdep.c
+++ b/scripts/basic/fixdep.c
@@ -283,7 +283,6 @@ static int is_ignored_file(const char *s, int len)
{
return str_ends_with(s, len, "include/generated/autoconf.h") ||
str_ends_with(s, len, "include/generated/autoksyms.h") ||
- str_ends_with(s, len, "include/linux/kconfig.h") ||
str_ends_with(s, len, ".ver");
}
--
2.15.1
2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:
> kconfig.h was excluded from consideration by fixdep by
> 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false
> positive hits
>
> (1) include/config/.h
> (2) include/config/h.h
> (3) include/config/foo.h
>
> (1) occurred because kconfig.h contains the string CONFIG_ in a
> comment. However, since dee81e988674 (fixdep: faster CONFIG_ search), we
> have a check that the part after CONFIG_ is non-empty, so this does not
> happen anymore (and CONFIG_ appears by itself elsewhere, so that check
> is worthwhile).
>
> (2) comes from the include guard, __LINUX_KCONFIG_H. But with the
> previous patch, we no longer match that either.
>
> That leaves (3), which amounts to one [1] false dependency (aka stat() call
> done by make), which I think we can live with:
>
> We've already had one case [2] where the lack of include/linux/kconfig.h in
> the .o.cmd file caused a missing rebuild, and while I originally thought
> we should just put kconfig.h in the dependency list without parsing it
> for the CONFIG_ pattern, we actually do have some real CONFIG_ symbols
> mentioned in it, and one can imagine some translation unit that just
> does '#ifdef __BIG_ENDIAN' but doesn't through some other header
> actually depend on CONFIG_CPU_BIG_ENDIAN - so changing the target
> endianness could end up rebuilding the world, minus that small
> TU. Quoting Linus,
>
> ... when missing dependencies cause a missed re-compile, the resulting
> bugs can be _really_ subtle.
>
> [1] well, two, we now also have CONFIG_BOOGER/booger.h - we could change
> that to FOO if we care
>
> [2] https://lkml.org/lkml/2018/2/22/838
>
> Cc: Linus Torvalds <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
Sorry, I missed to include this series in the Kbuild fixes PR
I sent two days ago.
I was not tracking the randomize-struct thread.
I read [2] and I noticed the background of this series just now.
Hopefully, I will have another opportunity of PR
if this series is necessary for v4.16 (seems so)
Regardless of the randomize-struct issue, this series is great!
> ---
> scripts/basic/fixdep.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index 1b21870d6e7f..449b68c4c90c 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -283,7 +283,6 @@ static int is_ignored_file(const char *s, int len)
> {
> return str_ends_with(s, len, "include/generated/autoconf.h") ||
> str_ends_with(s, len, "include/generated/autoksyms.h") ||
> - str_ends_with(s, len, "include/linux/kconfig.h") ||
> str_ends_with(s, len, ".ver");
> }
>
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Best Regards
Masahiro Yamada
On 5 March 2018 at 05:52, Masahiro Yamada <[email protected]> wrote:
> 2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:
>> kconfig.h was excluded from consideration by fixdep by
>> 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false
>> positive hits
[...]
>> We've already had one case [2] where the lack of include/linux/kconfig.h in
>> the .o.cmd file caused a missing rebuild,
[...]
>> [2] https://lkml.org/lkml/2018/2/22/838
>
>
> Sorry, I missed to include this series in the Kbuild fixes PR
> I sent two days ago.
>
> I was not tracking the randomize-struct thread.
> I read [2] and I noticed the background of this series just now.
>
> Hopefully, I will have another opportunity of PR
> if this series is necessary for v4.16 (seems so)
I should probably have put 3/3 first, if that is 4.16 material, the
other two can obviously wait for 4.17. They don't really depend on
each other apart from overlapping context, and the above commit
message would need slight rewording if put before the others. I can do
that reordering and resend if you wish.
Rasmus
2018-03-05 17:15 GMT+09:00 Rasmus Villemoes <[email protected]>:
> On 5 March 2018 at 05:52, Masahiro Yamada <[email protected]> wrote:
>> 2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:
>>> kconfig.h was excluded from consideration by fixdep by
>>> 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false
>>> positive hits
> [...]
>>> We've already had one case [2] where the lack of include/linux/kconfig.h in
>>> the .o.cmd file caused a missing rebuild,
> [...]
>>> [2] https://lkml.org/lkml/2018/2/22/838
>>
>>
>> Sorry, I missed to include this series in the Kbuild fixes PR
>> I sent two days ago.
>>
>> I was not tracking the randomize-struct thread.
>> I read [2] and I noticed the background of this series just now.
>>
>> Hopefully, I will have another opportunity of PR
>> if this series is necessary for v4.16 (seems so)
>
> I should probably have put 3/3 first, if that is 4.16 material, the
> other two can obviously wait for 4.17. They don't really depend on
> each other apart from overlapping context, and the above commit
> message would need slight rewording if put before the others. I can do
> that reordering and resend if you wish.
>
> Rasmus
1/3 is obviously safe, 2/3 too.
I think the current series is OK for v4.16.
I will queue it up to the fixes branch shortly.
Thanks!
--
Best Regards
Masahiro Yamada
2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:
> kconfig.h was excluded from consideration by fixdep by
> 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false
> positive hits
>
> (1) include/config/.h
> (2) include/config/h.h
> (3) include/config/foo.h
>
> (1) occurred because kconfig.h contains the string CONFIG_ in a
> comment. However, since dee81e988674 (fixdep: faster CONFIG_ search), we
> have a check that the part after CONFIG_ is non-empty, so this does not
> happen anymore (and CONFIG_ appears by itself elsewhere, so that check
> is worthwhile).
>
> (2) comes from the include guard, __LINUX_KCONFIG_H. But with the
> previous patch, we no longer match that either.
>
> That leaves (3), which amounts to one [1] false dependency (aka stat() call
> done by make), which I think we can live with:
>
> We've already had one case [2] where the lack of include/linux/kconfig.h in
> the .o.cmd file caused a missing rebuild, and while I originally thought
> we should just put kconfig.h in the dependency list without parsing it
> for the CONFIG_ pattern, we actually do have some real CONFIG_ symbols
> mentioned in it, and one can imagine some translation unit that just
> does '#ifdef __BIG_ENDIAN' but doesn't through some other header
> actually depend on CONFIG_CPU_BIG_ENDIAN - so changing the target
> endianness could end up rebuilding the world, minus that small
> TU. Quoting Linus,
>
> ... when missing dependencies cause a missed re-compile, the resulting
> bugs can be _really_ subtle.
>
> [1] well, two, we now also have CONFIG_BOOGER/booger.h - we could change
> that to FOO if we care
>
> [2] https://lkml.org/lkml/2018/2/22/838
>
> Cc: Linus Torvalds <[email protected]>
> Signed-off-by: Rasmus Villemoes <[email protected]>
> ---
> scripts/basic/fixdep.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/scripts/basic/fixdep.c b/scripts/basic/fixdep.c
> index 1b21870d6e7f..449b68c4c90c 100644
> --- a/scripts/basic/fixdep.c
> +++ b/scripts/basic/fixdep.c
> @@ -283,7 +283,6 @@ static int is_ignored_file(const char *s, int len)
> {
> return str_ends_with(s, len, "include/generated/autoconf.h") ||
> str_ends_with(s, len, "include/generated/autoksyms.h") ||
> - str_ends_with(s, len, "include/linux/kconfig.h") ||
> str_ends_with(s, len, ".ver");
> }
>
> --
> 2.15.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kbuild" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
The series, applied to linux-kbuild/fixes. Thanks!
--
Best Regards
Masahiro Yamada