module: Add compile-time check for embedded NUL characters
Subsystems (4)
cover module: Add compile-time check for embedded NUL characters
Hi,
A long time ago we had an issue with embedded NUL bytes in MODULE_INFO strings[1]. While this stands out pretty strongly when you look at the code, and we can't do anything about a binary module that just plain lies, we never actually implemented the trivial compile-time check needed to detect it.
Add this check (and fix 2 instances of needless trailing semicolons that this change exposed).
Note that these patches were produced as part of another LLM exercise. This time I wanted to try "what happens if I ask an LLM to go read a specific LWN article and write a patch based on a discussion?" It pretty effortlessly chose and implemented a suggested solution, tested the change, and fixed new build warnings in the process.
Since this was a relatively short session, here's an overview of the prompts involved as I guided it through a clean change and tried to see how it would reason about static_assert vs _Static_assert. (It wanted to use what was most common, not what was the current style -- we may want to update the comment above the static_assert macro to suggest using _Static_assert directly these days...)
I want to fix a weakness in the module info strings. Read about it
here: https://lwn.net/Articles/82305/
Since it's only "info" that we need to check, can you reduce the checks to just that instead of all the other stuff?
I think the change to the comment is redundent, and that should be in a commit log instead. Let's just keep the change to the static assert.
Is "static_assert" the idiomatic way to use a static assert in this code base? I've seen _Static_assert used sometimes.
What's the difference between the two?
Does Linux use C11 by default now?
Then let's not use the wrapper any more.
Do an "allmodconfig all -s" build to verify this works for all modules in the kernel.
Thanks!
-Kees
[1] https://lwn.net/Articles/82305/
Kees Cook (3):
media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
media: radio: si470x: Fix DRIVER_AUTHOR macro definition
module: Add compile-time check for embedded NUL characters
include/linux/moduleparam.h | 3 +++ drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +- drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------ 3 files changed, 10 insertions(+), 7 deletions(-)
Signature · 1 line
2.34.1
PATCH 1/3 media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
The firmware filename macros incorrectly included semicolons in their string literal definitions. Right now, this wasn't causing any real problem, but coming changes to the MODULE_INFO() macro make this more sensitive. Specifically, when used with MODULE_FIRMWARE(), this created syntax errors during macro expansion:
MODULE_FIRMWARE(LME2510_C_S7395);
expands to:
MODULE_INFO(firmware, "dvb-usb-lme2510c-s7395.fw";)
^
syntax errorRemove the trailing semicolons from all six firmware filename macro definitions. Semicolons should only appear at the point of use, not in the macro definition.
---
Cc: Malcolm Priestley <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: <[email protected]>
--- drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/media/usb/dvb-usb-v2/lmedm04.c +6 -6
@@ -70,12 +70,12 @@#include "ts2020.h" -#define LME2510_C_S7395 "dvb-usb-lme2510c-s7395.fw"; -#define LME2510_C_LG "dvb-usb-lme2510c-lg.fw"; -#define LME2510_C_S0194 "dvb-usb-lme2510c-s0194.fw"; -#define LME2510_C_RS2000 "dvb-usb-lme2510c-rs2000.fw"; -#define LME2510_LG "dvb-usb-lme2510-lg.fw"; -#define LME2510_S0194 "dvb-usb-lme2510-s0194.fw"; +#define LME2510_C_S7395 "dvb-usb-lme2510c-s7395.fw" +#define LME2510_C_LG "dvb-usb-lme2510c-lg.fw" +#define LME2510_C_S0194 "dvb-usb-lme2510c-s0194.fw" +#define LME2510_C_RS2000 "dvb-usb-lme2510c-rs2000.fw" +#define LME2510_LG "dvb-usb-lme2510-lg.fw" +#define LME2510_S0194 "dvb-usb-lme2510-s0194.fw"/* debug */ static int dvb_usb_lme2510_debug;
Signature · 1 line
2.34.1
Re: [PATCH 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
Quoted 14 lines · Kees Cook
The firmware filename macros incorrectly included semicolons in their string literal definitions. Right now, this wasn't causing any real problem, but coming changes to the
MODULE_INFO()macro make this more sensitive. Specifically, when used withMODULE_FIRMWARE(), this created syntax errors during macro expansion:MODULE_FIRMWARE(LME2510_C_S7395);expands to:
MODULE_INFO(firmware, "dvb-usb-lme2510c-s7395.fw";) ^ syntax error
Quoted 5 lines
Remove the trailing semicolons from all six firmware filename macro definitions. Semicolons should only appear at the point of use, not in the macro definition.
Signed-off-by Kees Cook <[email protected]>
Quoted 31 lines
---
Cc: Malcolm Priestley <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: <[email protected]>--- drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/media/usb/dvb-usb-v2/lmedm04.c +6
-6@@ -70,12 +70,12 @@#include "ts2020.h" -#define LME2510_C_S7395 "dvb-usb-lme2510c-s7395.fw"; -#define LME2510_C_LG "dvb-usb-lme2510c-lg.fw"; -#define LME2510_C_S0194 "dvb-usb-lme2510c-s0194.fw"; -#define LME2510_C_RS2000 "dvb-usb-lme2510c-rs2000.fw"; -#define LME2510_LG "dvb-usb-lme2510-lg.fw"; -#define LME2510_S0194 "dvb-usb-lme2510-s0194.fw"; +#define LME2510_C_S7395 "dvb-usb-lme2510c-s7395.fw" +#define LME2510_C_LG "dvb-usb-lme2510c-lg.fw" +#define LME2510_C_S0194 "dvb-usb-lme2510c-s0194.fw" +#define LME2510_C_RS2000 "dvb-usb-lme2510c-rs2000.fw" +#define LME2510_LG "dvb-usb-lme2510-lg.fw" +#define LME2510_S0194 "dvb-usb-lme2510-s0194.fw"/* debug */ static int dvb_usb_lme2510_debug;
PATCH 2/3 media: radio: si470x: Fix DRIVER_AUTHOR macro definition
In reply to Kees CookThe DRIVER_AUTHOR macro incorrectly included a semicolon in its string literal definition. Right now, this wasn't causing any real problem, but coming changes to the MODULE_INFO() macro make this more sensitive. Specifically, when used with MODULE_AUTHOR(), this created syntax errors during macro expansion:
MODULE_AUTHOR(DRIVER_AUTHOR);
expands to:
MODULE_INFO(author, "Joonyoung Shim <[email protected]>";)
^
syntax error
Remove the trailing semicolon from the DRIVER_AUTHOR definition. Semicolons should only appear at the point of use, not in the macro definition.
---
Cc: Hans Verkuil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Cc: <[email protected]>
--- drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/media/radio/si470x/radio-si470x-i2c.c +1 -1
@@ -10,7 +10,7 @@/* driver definitions */ -#define DRIVER_AUTHOR "Joonyoung Shim <[email protected]>"; +#define DRIVER_AUTHOR "Joonyoung Shim <[email protected]>"#define DRIVER_CARD "Silicon Labs Si470x FM Radio"#define DRIVER_DESC "I2C radio driver for Si470x FM Radio Receivers"#define DRIVER_VERSION "1.0.2"
Signature · 1 line
2.34.1
Re: [PATCH 2/3] media: radio: si470x: Fix DRIVER_AUTHOR macro definition
Quoted 14 lines · Kees Cook
The
DRIVER_AUTHORmacro incorrectly included a semicolon in its string literal definition. Right now, this wasn't causing any real problem, but coming changes to theMODULE_INFO()macro make this more sensitive. Specifically, when used withMODULE_AUTHOR(), this created syntax errors during macro expansion:MODULE_AUTHOR(DRIVER_AUTHOR);expands to:
MODULE_INFO(author, "Joonyoung Shim <[email protected]>";)
^
syntax error
Quoted 5 lines
Remove the trailing semicolon from the
DRIVER_AUTHORdefinition. Semicolons should only appear at the point of use, not in the macro definition.Signed-off-by Kees Cook <[email protected]>
Quoted 22 lines
---
Cc: Hans Verkuil <[email protected]>
Cc: Mauro Carvalho Chehab <[email protected]>
Cc: Uwe Kleine-König <[email protected]>
Cc: <[email protected]>--- drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)drivers/media/radio/si470x/radio-si470x-i2c.c +1
-1@@ -10,7 +10,7 @@/* driver definitions */ -#define DRIVER_AUTHOR "Joonyoung Shim <[email protected]>"; +#define DRIVER_AUTHOR "Joonyoung Shim <[email protected]>"#define DRIVER_CARD "Silicon Labs Si470x FM Radio"#define DRIVER_DESC "I2C radio driver for Si470x FM Radio Receivers"#define DRIVER_VERSION "1.0.2"
PATCH 3/3 module: Add compile-time check for embedded NUL characters
In reply to Kees CookLong ago, the kernel module license checks were bypassed by embedding a NUL character in the MODULE_LICENSE() string[1]. By using a string like "GPL\0proprietary text", the kernel would only read "GPL" due to C string termination at the NUL byte, allowing proprietary modules to avoid kernel tainting and access GPL-only symbols.
The MODULE_INFO() macro stores these strings in the .modinfo ELF section, and get_next_modinfo() uses strcmp()-family functions which stop at the first NUL. This split the embedded string into two separate .modinfo entries, with only the first part being processed by license_is_gpl_compatible().
Add a compile-time check using _Static_assert that compares the full string length (sizeof - 1) against __builtin_strlen(), which stops at the first NUL. If they differ, compilation fails with a clear error message.
While this check can still be circumvented by modifying the ELF binary post-compilation, it prevents accidental embedded NULs and forces intentional abuse to require deliberate binary manipulation rather than simple source-level tricks.
Build tested with test modules containing both valid and invalid license strings. The check correctly rejects:
MODULE_LICENSE("GPL\0proprietary")
while accepting normal declarations:
MODULE_LICENSE("GPL")
---
Cc: Luis Chamberlain <[email protected]>
Cc: Petr Pavlu <[email protected]>
Cc: Daniel Gomez <[email protected]>
Cc: Sami Tolvanen <[email protected]>
Cc: <[email protected]>
--- include/linux/moduleparam.h | 3 +++ 1 file changed, 3 insertions(+)
include/linux/moduleparam.h +3 -0
@@ -26,6 +26,9 @@/* Generic info of form tag = "info" */ #define MODULE_INFO (tag, info) \ + _Static_assert( \ + sizeof(info) - 1 == __builtin_strlen(info), \ + "MODULE_INFO(" #tag ", ...) contains embedded NUL byte"); \ static const char __UNIQUE_ID(modinfo)[] \ __used __section(".modinfo") __aligned(1) \ = __MODULE_INFO_PREFIX __stringify(tag) "=" info
Signature · 1 line
2.34.1
Petr Pavlu
maintainerQuoted 49 lines · Kees Cook
Long ago, the kernel module license checks were bypassed by embedding a NUL character in the
MODULE_LICENSE()string[1]. By using a string like "GPL\0proprietary text", the kernel would only read "GPL" due to C string termination at the NUL byte, allowing proprietary modules to avoid kernel tainting and access GPL-only symbols.The
MODULE_INFO()macro stores these strings in the .modinfo ELF section, andget_next_modinfo()uses strcmp()-family functions which stop at the first NUL. This split the embedded string into two separate .modinfo entries, with only the first part being processed bylicense_is_gpl_compatible().Add a compile-time check using
_Static_assertthat compares the full string length (sizeof - 1) against__builtin_strlen(), which stops at the first NUL. If they differ, compilation fails with a clear error message.While this check can still be circumvented by modifying the ELF binary post-compilation, it prevents accidental embedded NULs and forces intentional abuse to require deliberate binary manipulation rather than simple source-level tricks.
Build tested with test modules containing both valid and invalid license strings. The check correctly rejects:
MODULE_LICENSE("GPL\0proprietary")while accepting normal declarations:
MODULE_LICENSE("GPL")Link https://lwn.net/Articles/82305/ [1]Suggested-by Rusty Russell <[email protected]>Signed-off-by Kees Cook <[email protected]>---
Cc: Luis Chamberlain <[email protected]>
Cc: Petr Pavlu <[email protected]>
Cc: Daniel Gomez <[email protected]>
Cc: Sami Tolvanen <[email protected]>
Cc: <[email protected]>--- include/linux/moduleparam.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/moduleparam.h +0
-0
include/linux/moduleparam.h line 34
Quoted 9 lines
include/linux/moduleparam.h +3
-0@@ -26,6 +26,9 @@/* Generic info of form tag = "info" */ #define MODULE_INFO (tag, info) \ + _Static_assert( \ + sizeof(info) - 1 == __builtin_strlen(info), \ + "MODULE_INFO(" #tag ", ...) contains embedded NUL byte"); \ static const char __UNIQUE_ID(modinfo)[] \ __used __section(".modinfo") __aligned(1) \ = __MODULE_INFO_PREFIX __stringify(tag) "=" info
Nit: I think it is better to use static_assert() over _Static_assert() for consistency. Note also that C23 [1, 2] introduces static_assert() with the message being optional, which essentially matches the static_assert() macro in include/linux/build_bug.h, and deprecates _Static_assert().
[1] https://en.cppreference.com/w/c/language/_Static_assert.html
[2] https://www.open-std.org/jtc1/sc22/wg14/www/docs/n3220.pdf (C23 similar draft)
Signature · 2 lines
Thanks, Petr
Quoted 22 lines · Kees Cook
Long ago, the kernel module license checks were bypassed by embedding a NUL character in the
MODULE_LICENSE()string[1]. By using a string like "GPL\0proprietary text", the kernel would only read "GPL" due to C string termination at the NUL byte, allowing proprietary modules to avoid kernel tainting and access GPL-only symbols.The
MODULE_INFO()macro stores these strings in the .modinfo ELF section, andget_next_modinfo()uses strcmp()-family functions which stop at the first NUL. This split the embedded string into two separate .modinfo entries, with only the first part being processed bylicense_is_gpl_compatible().Add a compile-time check using
_Static_assertthat compares the full string length (sizeof - 1) against__builtin_strlen(), which stops at the first NUL. If they differ, compilation fails with a clear error message.While this check can still be circumvented by modifying the ELF binary post-compilation, it prevents accidental embedded NULs and forces intentional abuse to require deliberate binary manipulation rather than simple source-level tricks.
Quoted 8 lines
Build tested with test modules containing both valid and invalid license strings. The check correctly rejects:
MODULE_LICENSE("GPL\0proprietary")while accepting normal declarations:
MODULE_LICENSE("GPL")
I did a "make W=1 C=1" and found that sparse is now unhappy with all MODULE_LICENSE(), MODULE_PARM_DESC(), MODULE_DESCRIPTION(), MODULE_AUTHOR() defintions (with no NUL byte).
I see:
error: bad integer constant expression
error: static assertion failed: "MODULE_INFO(parmtype, ...) contains embedded NUL byte"
for every use.
-Tony
Quoted 36 lines · Tony
Quoted 22 lines · Kees Cook
Long ago, the kernel module license checks were bypassed by embedding a NUL character in the
MODULE_LICENSE()string[1]. By using a string like "GPL\0proprietary text", the kernel would only read "GPL" due to C string termination at the NUL byte, allowing proprietary modules to avoid kernel tainting and access GPL-only symbols.The
MODULE_INFO()macro stores these strings in the .modinfo ELF section, andget_next_modinfo()uses strcmp()-family functions which stop at the first NUL. This split the embedded string into two separate .modinfo entries, with only the first part being processed bylicense_is_gpl_compatible().Add a compile-time check using
_Static_assertthat compares the full string length (sizeof - 1) against__builtin_strlen(), which stops at the first NUL. If they differ, compilation fails with a clear error message.While this check can still be circumvented by modifying the ELF binary post-compilation, it prevents accidental embedded NULs and forces intentional abuse to require deliberate binary manipulation rather than simple source-level tricks.
Quoted 8 lines
Build tested with test modules containing both valid and invalid license strings. The check correctly rejects:
MODULE_LICENSE("GPL\0proprietary")while accepting normal declarations:
MODULE_LICENSE("GPL")I did a "make W=1 C=1" and found that sparse is now unhappy with all
MODULE_LICENSE(),MODULE_PARM_DESC(),MODULE_DESCRIPTION(),MODULE_AUTHOR()defintions (with no NUL byte).
Quoted 6 lines
I see:
error: bad integer constant expression
error: static assertion failed: "MODULE_INFO(parmtype, ...)contains embedded NUL byte"for every use.
Likewise, I just got the following kernel test robot report sent to me, where it's warning about MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/
- Eric
Daniel Gomez
maintainerQuoted 39 lines · Eric Biggers
Quoted 38 lines · Tony
Quoted 22 lines · Kees Cook
Long ago, the kernel module license checks were bypassed by embedding a NUL character in the
MODULE_LICENSE()string[1]. By using a string like "GPL\0proprietary text", the kernel would only read "GPL" due to C string termination at the NUL byte, allowing proprietary modules to avoid kernel tainting and access GPL-only symbols.The
MODULE_INFO()macro stores these strings in the .modinfo ELF section, andget_next_modinfo()uses strcmp()-family functions which stop at the first NUL. This split the embedded string into two separate .modinfo entries, with only the first part being processed bylicense_is_gpl_compatible().Add a compile-time check using
_Static_assertthat compares the full string length (sizeof - 1) against__builtin_strlen(), which stops at the first NUL. If they differ, compilation fails with a clear error message.While this check can still be circumvented by modifying the ELF binary post-compilation, it prevents accidental embedded NULs and forces intentional abuse to require deliberate binary manipulation rather than simple source-level tricks.
Quoted 8 lines
Build tested with test modules containing both valid and invalid license strings. The check correctly rejects:
MODULE_LICENSE("GPL\0proprietary")while accepting normal declarations:
MODULE_LICENSE("GPL")I did a "make W=1 C=1" and found that sparse is now unhappy with all
MODULE_LICENSE(),MODULE_PARM_DESC(),MODULE_DESCRIPTION(),MODULE_AUTHOR()defintions (with no NUL byte).I see:
Quoted 8 lines
Quoted 4 lines
error: bad integer constant expression
error: static assertion failed: "MODULE_INFO(parmtype, ...)contains embedded NUL byte"for every use.
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/
Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
https://lore.kernel.org/linux-sparse/CACePvbVG2KrGQq4cNKV=wbO5h=jp3M0RO1SdfX8kV4OukjPG8A@mail.gmail.com/T/#mf838b3e2e3245d88c30a801ea7473d5a5c0eb121
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
Quoted 11 lines
Quoted 3 lines
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
I pulled the latest sparse source and re-checked before reporting. Top commit I have is the one you mention:
fbdde3127b83 ("builtin: implement __builtin_strlen() for constants")
I'm building latest Linus tree from the current merge window (well latest as-of yesterday):
c2f2b01b74be ("Merge tag 'i3c/for-6.19' of git://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux")
-Tony
Quoted 12 lines · Tony
Quoted 11 lines
Quoted 3 lines
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
Quoted 7 lines
I pulled the latest sparse source and re-checked before reporting. Top commit I have is the one you mention:
fbdde3127b83("builtin: implement__builtin_strlen()for constants")I'm building latest Linus tree from the current merge window (well latest as-of yesterday):
c2f2b01b74be("Merge tag 'i3c/for-6.19' ofgit://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux")
I added a debug trace to the new expand_strlen() function added to sparse. It is being called and doing the right thing. My trace says:
len(GPL) = 3
-Tony
Quoted 17 lines · Tony
Quoted 16 lines · Tony
Quoted 11 lines
Quoted 3 lines
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
I pulled the latest sparse source and re-checked before reporting. Top commit I have is the one you mention:
fbdde3127b83("builtin: implement__builtin_strlen()for constants")
Quoted 8 lines
Quoted 3 lines
I'm building latest Linus tree from the current merge window (well latest as-of yesterday):
c2f2b01b74be("Merge tag 'i3c/for-6.19' ofgit://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux")I added a debug trace to the new
expand_strlen()function added to sparse. It is being called and doing the right thing. My trace says:len(GPL) = 3
Simple test case:
$ cat -n s.c
1
2 _Static_assert(sizeof("GPL") - 1 == 3, "sizeof");
3
4 _Static_assert(__builtin_strlen("GPL") == 3, "strlen");$ sparse s.c s.c:4:40: error: bad integer constant expression s.c:4:40: error: static assertion failed: "strlen"
So the "sizeof" bit is OK. But the __builtin_strlen() isn't.
-Tony
Sami Tolvanen
maintainerQuoted 35 lines · Tony
Quoted 17 lines · Tony
Quoted 16 lines · Tony
Quoted 11 lines
Quoted 3 lines
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
I pulled the latest sparse source and re-checked before reporting. Top commit I have is the one you mention:
fbdde3127b83("builtin: implement__builtin_strlen()for constants")Quoted 8 lines
Quoted 3 lines
I'm building latest Linus tree from the current merge window (well latest as-of yesterday):
c2f2b01b74be("Merge tag 'i3c/for-6.19' ofgit://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux")I added a debug trace to the new
expand_strlen()function added to sparse. It is being called and doing the right thing. My trace says:len(GPL) = 3Simple test case:
$ cat -n s.c 1 2 _Static_assert(sizeof("GPL") - 1 == 3, "sizeof"); 3 4 _Static_assert(__builtin_strlen("GPL") == 3, "strlen");
Quoted 5 lines
$ sparse s.c s.c:4:40: error: bad integer constant expression s.c:4:40: error: static assertion failed: "strlen"So the "sizeof" bit is OK. But the
__builtin_strlen()isn't.
This looks like a bug in Sparse. The CEF_ICE flag isn't propagated to the comparison expression, which it presumably should be when both sides are integer constant expressions.
I'm not really familiar enough with Sparse to know whether this is the correct place to handle this case, but this quick hack fixes the issue for me:
expand.c +2 -0
@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr) expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))
Sami
[Added Dan Carpenter and Chris Li]
Dan and Chris,
You're in the git log for sparse wrting/commiting fixes. Can you take a look at this? Sami's fix works for me.
-Tony
Quoted 65 lines · Sami Tolvanen
Quoted 35 lines · Tony
Quoted 17 lines · Tony
Quoted 16 lines · Tony
Quoted 11 lines
Quoted 3 lines
Likewise, I just got the following kernel test robot report sent to me, where it's warning about
MODULE_LICENSE("GPL"):
https://lore.kernel.org/all/[email protected]/Can you both confirm which version of sparse are you using?
My understanding was that this patch fixed that problem:
The patch is already merged into the sparse tree, and I was not able to reproduce the issue.
I pulled the latest sparse source and re-checked before reporting. Top commit I have is the one you mention:
fbdde3127b83("builtin: implement__builtin_strlen()for constants")Quoted 8 lines
Quoted 3 lines
I'm building latest Linus tree from the current merge window (well latest as-of yesterday):
c2f2b01b74be("Merge tag 'i3c/for-6.19' ofgit://git.kernel.org/pub/scm/linux/kernel/git/i3c/linux")I added a debug trace to the new
expand_strlen()function added to sparse. It is being called and doing the right thing. My trace says:len(GPL) = 3Simple test case:
$ cat -n s.c 1 2 _Static_assert(sizeof("GPL") - 1 == 3, "sizeof"); 3 4 _Static_assert(__builtin_strlen("GPL") == 3, "strlen");Quoted 5 lines
$ sparse s.c s.c:4:40: error: bad integer constant expression s.c:4:40: error: static assertion failed: "strlen"So the "sizeof" bit is OK. But the
__builtin_strlen()isn't.This looks like a bug in Sparse. The
CEF_ICEflag isn't propagated to the comparison expression, which it presumably should be when both sides are integer constant expressions.I'm not really familiar enough with Sparse to know whether this is the correct place to handle this case, but this quick hack fixes the issue for me:
expand.c +2
-0@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr) expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))Sami
Quoted 5 lines · Tony
Quoted 5 lines
expand.c +0
-0@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr)
Quoted 8 lines
expand.c static int expand_compare(struct expression *expr) · line 542
Quoted 8 lines
expand.c +2
-0expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))
I'm not an expert in the C standard, but this feels correct to me.
regards,
dan carpenter
Sami Tolvanen
maintainerQuoted 17 lines · Dan Carpenter
Quoted 5 lines · Tony
Quoted 5 lines
expand.c +0
-0@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr)Quoted 8 lines
expand.c static int expand_compare(struct expression *expr) · line 542
Quoted 8 lines
expand.c +2
-0expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))I'm not an expert in the C standard, but this feels correct to me.
It only fixes comparisons though, the problem still exists for other expressions. For example, while _Static_assert(__builtin_strlen("") == 0); works with this change, _Static_assert(!__builtin_strlen("")); still fails. Perhaps there's a better way to fix this than changing each expression expansion function to handle this flag?
Sami
Daniel Gomez
maintainerQuoted 19 lines · Sami Tolvanen
Quoted 17 lines · Dan Carpenter
Quoted 5 lines · Tony
Quoted 5 lines
expand.c +0
-0@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr)Quoted 8 lines
expand.c static int expand_compare(struct expression *expr) · line 542
Quoted 8 lines
expand.c +2
-0expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))I'm not an expert in the C standard, but this feels correct to me.
Quoted 6 lines
It only fixes comparisons though, the problem still exists for other expressions. For example, while
_Static_assert(__builtin_strlen("") == 0);works with this change,_Static_assert(!__builtin_strlen(""));still fails. Perhaps there's a better way to fix this than changing each expression expansion function to handle this flag?
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct symbol_op constant_p_op = {
.evaluate = evaluate_to_int_const_expr,
.expand = expand_constant_p
};
Quoted 2 lines
Sami
Sami Tolvanen
maintainerQuoted 29 lines · Daniel Gomez
Quoted 19 lines · Sami Tolvanen
Quoted 17 lines · Dan Carpenter
Quoted 5 lines · Tony
Quoted 5 lines
expand.c +0
-0@@ -535,6 +535,8 @@ static int expand_compare(struct expression *expr)Quoted 8 lines
expand.c static int expand_compare(struct expression *expr) · line 542
Quoted 8 lines
expand.c +2
-0expr->taint =0 ;return 0 ; } +if (left->flags &CEF_ICE && right->flags &CEF_ICE ) + expr->flags |=CEF_SET_ICE ;if (simplify_cmp_binop (expr, left->ctype))return 0 ;if (simplify_float_cmp (expr, left->ctype))I'm not an expert in the C standard, but this feels correct to me.
Quoted 6 lines
It only fixes comparisons though, the problem still exists for other expressions. For example, while
_Static_assert(__builtin_strlen("") == 0);works with this change,_Static_assert(!__builtin_strlen(""));still fails. Perhaps there's a better way to fix this than changing each expression expansion function to handle this flag?
Quoted 7 lines
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};
Nice catch! This seems to fix the issue for me:
builtin.c +1 -0
@@ -616,6 +616,7 @@ static int expand_strlen(struct expression *expr, int cost) }static struct symbol_op strlen_op = { + .evaluate = evaluate_to_int_const_expr, .expand = expand_strlen, };
I wonder if there are any other __builtin_* functions that need this too? Looks like __builtin_object_size doesn't have this either.
Sami
Quoted 8 lines · Daniel Gomez
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};
I was waiting for you to send this as a patch. I can do it if you need me to.
regards,
dan carpenter
Quoted 4 lines · Dan Carpenter
Quoted 3 lines · Daniel Gomez
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
Quoted 8 lines
Quoted 5 lines
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};I was waiting for you to send this as a patch. I can do it if you need me to.
Dan,
Al Viro thought this was wrong. His alternative patch is here:
-Tony
Quoted 10 lines · Tony
Quoted 9 lines · Dan Carpenter
Quoted 7 lines · Daniel Gomez
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};
Quoted 6 lines
Quoted 2 lines
I was waiting for you to send this as a patch. I can do it if you need me to.
Al Viro thought this was wrong. His alternative patch is here:
Sparse still is PITA as of today, can we get some fix (Al's or alternative) ASAP to be applied and sparse tagged as 0.6.5 so the distros will pack the new version, please?
Signature · 2 lines
With Best Regards, Andy Shevchenko
Quoted 14 lines · Andy Shevchenko
Quoted 13 lines · Tony
Quoted 11 lines · Dan Carpenter
Quoted 7 lines · Daniel Gomez
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};I was waiting for you to send this as a patch. I can do it if you need me to.
Quoted 7 lines
Quoted 3 lines
Al Viro thought this was wrong. His alternative patch is here:
Sparse still is PITA as of today, can we get some fix (Al's or alternative) ASAP to be applied and sparse tagged as 0.6.5 so the distros will pack the new version, please?
Any update on this?
Thanks.
Signature · 1 line
Dmitry
Hi Tony,
Sorry for the late reply.
Quoted 12 lines · Tony
Quoted 10 lines · Dan Carpenter
Quoted 8 lines · Daniel Gomez
Maybe the flag fix just needs to be applied to the evaluation? Other op structs do the same. But Dan's patch did not implement evaluate. E.g.:
static struct
symbol_opconstant_p_op= {
.evaluate =evaluate_to_int_const_expr,
.expand =expand_constant_p
};
Quoted 8 lines
Quoted 2 lines
I was waiting for you to send this as a patch. I can do it if you need me to.
Dan,
Al Viro thought this was wrong. His alternative patch is here:
Hi Al, should I pull from your git repo
https://git.kernel.org/pub/scm/linux/kernel/git/viro/sparse.git/
instead? I saw there is more than one commit. I assume it is ready to pull. Please let me know if I shouldn't.
Thanks
Chris
Quoted 48 lines · Kees Cook
Hi,
A long time ago we had an issue with embedded NUL bytes in
MODULE_INFOstrings[1]. While this stands out pretty strongly when you look at the code, and we can't do anything about a binary module that just plain lies, we never actually implemented the trivial compile-time check needed to detect it.Add this check (and fix 2 instances of needless trailing semicolons that this change exposed).
Note that these patches were produced as part of another LLM exercise. This time I wanted to try "what happens if I ask an LLM to go read a specific LWN article and write a patch based on a discussion?" It pretty effortlessly chose and implemented a suggested solution, tested the change, and fixed new build warnings in the process.
Since this was a relatively short session, here's an overview of the prompts involved as I guided it through a clean change and tried to see how it would reason about
static_assertvs_Static_assert. (It wanted to use what was most common, not what was the current style -- we may want to update the comment above thestatic_assertmacro to suggest using_Static_assertdirectly these days...)I want to fix a weakness in the module info strings. Read about it
here: https://lwn.net/Articles/82305/Since it's only "info" that we need to check, can you reduce the checks to just that instead of all the other stuff?
I think the change to the comment is redundent, and that should be in a commit log instead. Let's just keep the change to the static assert.
Is "
static_assert" the idiomatic way to use a static assert in this code base? I've seen_Static_assertused sometimes.What's the difference between the two?
Does Linux use C11 by default now?
Then let's not use the wrapper any more.
Do an "allmodconfig all -s" build to verify this works for all modules in the kernel.
Thanks!
Quoted 8 lines
-Kees
[1] https://lwn.net/Articles/82305/
Kees Cook (3):
media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
media: radio: si470x: FixDRIVER_AUTHORmacro definition
module: Add compile-time check for embedded NUL characters
I reviewed the two media patches. Feel free to take this series. If you prefer that I take the two media patches, then let me know but it makes more sense in this case that you take all three.
Regards,
Hans
Quoted 6 lines
include/linux/moduleparam.h | 3 +++ drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +- drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------ 3 files changed, 10 insertions(+), 7 deletions(-)