[PATCH 0/3] module: Add compile-time check for embedded NUL characters

[PATCH 0/3] module: Add compile-time check for embedded NUL characters

[Kees Cook]

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

-- 
2.34.1

[PATCH 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions

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

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 <kees@kernel.org>
---
Cc: Malcolm Priestley <tvboxspy@gmail.com>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: <linux-media@vger.kernel.org>
---
 drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index 0c510035805b..05c18b6de5c6 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -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;
-- 
2.34.1

Re: [PATCH 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions

[Hans Verkuil]

On 08/10/2025 05:59, Kees Cook wrote:
> 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 error
> 
> 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 <kees@kernel.org>

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>

> ---
> Cc: Malcolm Priestley <tvboxspy@gmail.com>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: <linux-media@vger.kernel.org>
> ---
>  drivers/media/usb/dvb-usb-v2/lmedm04.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c b/drivers/media/usb/dvb-usb-v2/lmedm04.c
> index 0c510035805b..05c18b6de5c6 100644
> --- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
> +++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
> @@ -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

[Kees Cook]

The 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 <jy0922.shim@samsung.com>";)
                                                                  ^
                                                       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.

Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Hans Verkuil <hverkuil@kernel.org>
Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Cc: <linux-media@vger.kernel.org>
---
 drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
index cdd2ac198f2c..3932a449a1b1 100644
--- a/drivers/media/radio/si470x/radio-si470x-i2c.c
+++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
@@ -10,7 +10,7 @@
 
 
 /* driver definitions */
-#define DRIVER_AUTHOR "Joonyoung Shim <jy0922.shim@samsung.com>";
+#define DRIVER_AUTHOR "Joonyoung Shim <jy0922.shim@samsung.com>"
 #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"
-- 
2.34.1

Re: [PATCH 2/3] media: radio: si470x: Fix DRIVER_AUTHOR macro definition

[Hans Verkuil]

On 08/10/2025 05:59, Kees Cook wrote:
> The 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 <jy0922.shim@samsung.com>";)
>                                                                   ^
>                                                        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.
> 
> Signed-off-by: Kees Cook <kees@kernel.org>

Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>

> ---
> Cc: Hans Verkuil <hverkuil@kernel.org>
> Cc: Mauro Carvalho Chehab <mchehab@kernel.org>
> Cc: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Cc: <linux-media@vger.kernel.org>
> ---
>  drivers/media/radio/si470x/radio-si470x-i2c.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/radio/si470x/radio-si470x-i2c.c b/drivers/media/radio/si470x/radio-si470x-i2c.c
> index cdd2ac198f2c..3932a449a1b1 100644
> --- a/drivers/media/radio/si470x/radio-si470x-i2c.c
> +++ b/drivers/media/radio/si470x/radio-si470x-i2c.c
> @@ -10,7 +10,7 @@
>  
>  
>  /* driver definitions */
> -#define DRIVER_AUTHOR "Joonyoung Shim <jy0922.shim@samsung.com>";
> +#define DRIVER_AUTHOR "Joonyoung Shim <jy0922.shim@samsung.com>"
>  #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

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

Link: https://lwn.net/Articles/82305/ [1]
Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
Signed-off-by: Kees Cook <kees@kernel.org>
---
Cc: Luis Chamberlain <mcgrof@kernel.org>
Cc: Petr Pavlu <petr.pavlu@suse.com>
Cc: Daniel Gomez <da.gomez@kernel.org>
Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: <linux-modules@vger.kernel.org>
---
 include/linux/moduleparam.h | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 6907aedc4f74..160f1678fafa 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -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
-- 
2.34.1

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Petr Pavlu]

On 10/8/25 5:59 AM, Kees Cook wrote:
> 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, 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")
> 
> Link: https://lwn.net/Articles/82305/ [1]
> Suggested-by: Rusty Russell <rusty@rustcorp.com.au>
> Signed-off-by: Kees Cook <kees@kernel.org>
> ---
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> Cc: Petr Pavlu <petr.pavlu@suse.com>
> Cc: Daniel Gomez <da.gomez@kernel.org>
> Cc: Sami Tolvanen <samitolvanen@google.com>
> Cc: <linux-modules@vger.kernel.org>
> ---
>  include/linux/moduleparam.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 6907aedc4f74..160f1678fafa 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -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)

-- 
Thanks,
Petr

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Luck, Tony]

On Tue, Oct 07, 2025 at 08:59:35PM -0700, Kees Cook wrote:
> 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, 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")

 
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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Eric Biggers]

On Mon, Dec 08, 2025 at 01:05:55PM -0800, Luck, Tony wrote:
> On Tue, Oct 07, 2025 at 08:59:35PM -0700, Kees Cook wrote:
> > 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, 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")
> 
>  
> 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.

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/202512090359.7BkUaiC9-lkp@intel.com/

- Eric

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Daniel Gomez]

On 09/12/2025 09.11, Eric Biggers wrote:
> On Mon, Dec 08, 2025 at 01:05:55PM -0800, Luck, Tony wrote:
>> On Tue, Oct 07, 2025 at 08:59:35PM -0700, Kees Cook wrote:
>>> 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, 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")
>>
>>  
>> 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.
> 
> 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/202512090359.7BkUaiC9-lkp@intel.com/

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.

RE: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Luck, Tony]

>> 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/202512090359.7BkUaiC9-lkp@intel.com/
>
> 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.

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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Luck, Tony]

On Tue, Dec 09, 2025 at 04:20:06PM +0000, Luck, Tony wrote:
> >> 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/202512090359.7BkUaiC9-lkp@intel.com/
> >
> > 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.
> 
> 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")

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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Luck, Tony]

On Tue, Dec 09, 2025 at 08:45:14AM -0800, Luck, Tony wrote:
> On Tue, Dec 09, 2025 at 04:20:06PM +0000, Luck, Tony wrote:
> > >> 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/202512090359.7BkUaiC9-lkp@intel.com/
> > >
> > > 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.
> > 
> > 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")
> 
> 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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Sami Tolvanen]

On Tue, Dec 09, 2025 at 10:29:06AM -0800, Luck, Tony wrote:
> On Tue, Dec 09, 2025 at 08:45:14AM -0800, Luck, Tony wrote:
> > On Tue, Dec 09, 2025 at 04:20:06PM +0000, Luck, Tony wrote:
> > > >> 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/202512090359.7BkUaiC9-lkp@intel.com/
> > > >
> > > > 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.
> > > 
> > > 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")
> > 
> > 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.

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:

diff --git a/expand.c b/expand.c
index f14e7181..71221d35 100644
--- a/expand.c
+++ b/expand.c
@@ -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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Luck, Tony]

[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

On Wed, Dec 10, 2025 at 01:00:20AM +0000, Sami Tolvanen wrote:
> On Tue, Dec 09, 2025 at 10:29:06AM -0800, Luck, Tony wrote:
> > On Tue, Dec 09, 2025 at 08:45:14AM -0800, Luck, Tony wrote:
> > > On Tue, Dec 09, 2025 at 04:20:06PM +0000, Luck, Tony wrote:
> > > > >> 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/202512090359.7BkUaiC9-lkp@intel.com/
> > > > >
> > > > > 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.
> > > > 
> > > > 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")
> > > 
> > > 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.
> 
> 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:
> 
> diff --git a/expand.c b/expand.c
> index f14e7181..71221d35 100644
> --- a/expand.c
> +++ b/expand.c
> @@ -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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Dan Carpenter]

On Wed, Dec 10, 2025 at 02:29:45PM -0800, Luck, Tony wrote:
> > diff --git a/expand.c b/expand.c
> > index f14e7181..71221d35 100644
> > --- a/expand.c
> > +++ b/expand.c
> > @@ -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))

I'm not an expert in the C standard, but this feels correct to me.

regards,
dan carpenter

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Sami Tolvanen]

On Thu, Dec 11, 2025 at 12:28 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>
> On Wed, Dec 10, 2025 at 02:29:45PM -0800, Luck, Tony wrote:
> > > diff --git a/expand.c b/expand.c
> > > index f14e7181..71221d35 100644
> > > --- a/expand.c
> > > +++ b/expand.c
> > > @@ -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))
>
> 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

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Daniel Gomez]

On 12/12/2025 02.03, Sami Tolvanen wrote:
> On Thu, Dec 11, 2025 at 12:28 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
>>
>> On Wed, Dec 10, 2025 at 02:29:45PM -0800, Luck, Tony wrote:
>>>> diff --git a/expand.c b/expand.c
>>>> index f14e7181..71221d35 100644
>>>> --- a/expand.c
>>>> +++ b/expand.c
>>>> @@ -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))
>>
>> 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?

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
};


> 
> Sami

Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters

[Sami Tolvanen]

On Fri, Dec 12, 2025 at 02:30:48AM +0900, Daniel Gomez wrote:
> 
> 
> On 12/12/2025 02.03, Sami Tolvanen wrote:
> > On Thu, Dec 11, 2025 at 12:28 AM Dan Carpenter <dan.carpenter@linaro.org> wrote:
> >>
> >> On Wed, Dec 10, 2025 at 02:29:45PM -0800, Luck, Tony wrote:
> >>>> diff --git a/expand.c b/expand.c
> >>>> index f14e7181..71221d35 100644
> >>>> --- a/expand.c
> >>>> +++ b/expand.c
> >>>> @@ -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))
> >>
> >> 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?
> 
> 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
> };

Nice catch! This seems to fix the issue for me:

diff --git a/builtin.c b/builtin.c
index 9149c43d..7573abf8 100644
--- a/builtin.c
+++ b/builtin.c
@@ -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

Re: [PATCH 0/3] module: Add compile-time check for embedded NUL characters

[Hans Verkuil]

On 08/10/2025 05:59, Kees Cook wrote:
> 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

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

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