Skip to contentSkip to conversation

kconfig.h: Include compiler types to avoid missed struct attributes

PATCH In review Kees Cook 5 participants 17 messages Feb 22, 2018 – Mar 5, 2018

Subsystems (2)
Kbuild User-mode Linux (UML)
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH v3 1/1 kconfig.h: Include compiler types to avoid missed struct attributes

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(+)
include/linux/kconfig.h +3 -0
index fec5076eda91..c5fd4ee776ba 100644
@@ -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 */
Signature · 6 lines
2.7.4


-- 
Kees Cook
Pixel Security
#1 lore raw
Quoted 2 lines · Kees Cook

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

author
#2 lore raw
Quoted 19 lines · Linus Torvalds
Quoted 2 lines · Kees Cook

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

Quoted 7 lines

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

Signature · 2 lines
Kees Cook
Pixel Security
#3 lore raw
Quoted 3 lines · Kees Cook

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

#4 lore raw
In reply to Linus Torvalds
Quoted 11 lines · Linus Torvalds

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

Quoted 7 lines

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

author
#5 lore raw
Quoted 26 lines · Rasmus Villemoes
Quoted 11 lines · Linus Torvalds

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

Quoted 7 lines

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.

Quoted 3 lines

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?

Quoted 3 lines

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

Signature · 2 lines
Kees Cook
Pixel Security
#6 lore raw
In reply to Rasmus Villemoes
Quoted 6 lines · Rasmus Villemoes

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

#7 lore raw
Quoted 2 lines · Linus Torvalds

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

#8 lore raw
In reply to Rasmus Villemoes
Quoted 4 lines · Rasmus Villemoes

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

#9 lore raw
Quoted 4 lines · Rasmus Villemoes

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

#10 lore raw

PATCH 1/3 fixdep: remove stale references to uml-config.h

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(-)
scripts/basic/fixdep.c +0 -9
index fa3d39b6f23b..d7fbe545dd5d 100644
@@ -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");
 }
Signature · 1 line
2.15.1
#11 lore raw

PATCH 2/3 fixdep: remove some false CONFIG_ matches

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(+)
scripts/basic/fixdep.c +5 -0
index d7fbe545dd5d..1b21870d6e7f 100644
@@ -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 == '_'))
Signature · 1 line
2.15.1
#12 lore raw

PATCH 3/3 fixdep: do not ignore kconfig.h

In reply to Rasmus Villemoes

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(-)
scripts/basic/fixdep.c +0 -1
index 1b21870d6e7f..449b68c4c90c 100644
@@ -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");
 }
Signature · 1 line
2.15.1
#13 lore raw

Re: [PATCH 3/3] fixdep: do not ignore kconfig.h

2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:

Quoted 33 lines

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.

Quoted 7 lines

[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!

Quoted 23 lines
---
 scripts/basic/fixdep.c | 1 -
 1 file changed, 1 deletion(-)
scripts/basic/fixdep.c +0 -2
index 1b21870d6e7f..449b68c4c90c 100644
@@ -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

Signature · 2 lines
Best Regards
Masahiro Yamada
#14 lore raw

Re: [PATCH 3/3] fixdep: do not ignore kconfig.h

Quoted 4 lines · Masahiro Yamada

2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:

Quoted 3 lines

kconfig.h was excluded from consideration by fixdep by 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false positive hits

[...]

Quoted 2 lines
Quoted 2 lines

We've already had one case [2] where the lack of include/linux/kconfig.h in the .o.cmd file caused a missing rebuild,

[...]

Quoted 11 lines
Quoted 1 line

[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

#15 lore raw

Re: [PATCH 3/3] fixdep: do not ignore kconfig.h

2018-03-05 17:15 GMT+09:00 Rasmus Villemoes <[email protected]>:

Quoted 21 lines · Masahiro Yamada
Quoted 4 lines

2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:

Quoted 3 lines

kconfig.h was excluded from consideration by fixdep by 6a5be57f0f00 (fixdep: fix extraneous dependencies) to avoid some false positive hits

[...]

Quoted 2 lines
Quoted 2 lines

We've already had one case [2] where the lack of include/linux/kconfig.h in the .o.cmd file caused a missing rebuild,

[...]

Quoted 11 lines
Quoted 1 line

[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)

Quoted 7 lines

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!

Signature · 2 lines
Best Regards
Masahiro Yamada
#16 lore raw

Re: [PATCH 3/3] fixdep: do not ignore kconfig.h

In reply to Rasmus Villemoes

2018-03-01 4:17 GMT+09:00 Rasmus Villemoes <[email protected]>:

Quoted 59 lines

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(-)
scripts/basic/fixdep.c +0 -2
index 1b21870d6e7f..449b68c4c90c 100644
@@ -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

Quoted 4 lines

--
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!

Signature · 2 lines
Best Regards
Masahiro Yamada