module: Add compile-time check for embedded NUL characters

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.

Reviewed-by Hans Verkuil <[email protected]>

Signed-off-by Kees Cook <[email protected]>

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

index 0c510035805b..05c18b6de5c6 100644
@@ -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;

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

Reviewed-by Hans Verkuil <[email protected]>

Signed-off-by Kees Cook <[email protected]>

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

index cdd2ac198f2c..3932a449a1b1 100644
@@ -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"

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 <[email protected]>

Signed-off-by Kees Cook <[email protected]>

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 <[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 +3 -0

index 6907aedc4f74..915f32f7d888 100644
@@ -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

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 <[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 +3 -0

index 6907aedc4f74..915f32f7d888 100644
@@ -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

Quoted 29 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, 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:

Quoted 1 line

MODULE_LICENSE("GPL")

Who will take this series? I can take the first two media patches and someone else can take this last patch, or I can take all, or someone else can take all patches. The media patches already have my 'Reviewed-by'.

Any preferences?

Regards,

Hans

Quoted 4 lines

Link https://lwn.net/Articles/82305/ [1]

Suggested-by Rusty Russell <[email protected]>

Signed-off-by Kees Cook <[email protected]>

Quoted 32 lines · Hans Verkuil

Quoted 30 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, 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")

Quoted 5 lines

Who will take this series? I can take the first two media patches and someone else can take this last patch, or I can take all, or someone else can take all patches. The media patches already have my 'Reviewed-by'.

Any preferences?

I will take patch 3 in modules' tree.

Quoted 11 lines

Regards,

Hans

Quoted 4 lines

Link https://lwn.net/Articles/82305/ [1]

Suggested-by Rusty Russell <[email protected]>

Signed-off-by Kees Cook <[email protected]>

Reviewed-by Daniel Gomez <[email protected]>

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

index 6907aedc4f74..915f32f7d888 100644

$ touch net/mptcp/crypto_test.c && make C=1 net/mptcp/crypto_test.o
  CC [M]  net/mptcp/crypto_test.o
  CHECK   net/mptcp/crypto_test.c
net/mptcp/crypto_test.c:72:1: error: bad integer constant expression
net/mptcp/crypto_test.c:72:1: error: static assertion failed: "MODULE_INFO(license, ...) contains embedded NUL byte"
net/mptcp/crypto_test.c:73:1: error: bad integer constant expression
net/mptcp/crypto_test.c:73:1: error: static assertion failed: "MODULE_INFO(description, ...) contains embedded NUL byte"

net/mptcp/crypto_test.c:72:1: error: bad integer constant expression
net/mptcp/crypto_test.c:72:1: error: static assertion failed: "MODULE_INFO(license, ...) contains embedded NUL byte" net/mptcp/crypto_test.c:73:1: error: bad integer constant expression
net/mptcp/crypto_test.c:73:1: error: static assertion failed: "MODULE_INFO(description, ...) contains embedded NUL byte"

Quoted 4 lines · Matthieu Baerts

net/mptcp/crypto_test.c:72:1: error: bad integer constant expression
net/mptcp/crypto_test.c:72:1: error: static assertion failed: "MODULE_INFO(license, ...) contains embedded NUL byte" net/mptcp/crypto_test.c:73:1: error: bad integer constant expression
net/mptcp/crypto_test.c:73:1: error: static assertion failed: "MODULE_INFO(description, ...) contains embedded NUL byte"

There was a fix for that posted. Let me ping them to see if anyone is planning to send an actual patch.

https://lore.kernel.org/all/[email protected]/

Hi Dan, Daniel

Quoted 8 lines · Dan Carpenter

Quoted 4 lines · Matthieu Baerts

net/mptcp/crypto_test.c:72:1: error: bad integer constant expression
net/mptcp/crypto_test.c:72:1: error: static assertion failed: "MODULE_INFO(license, ...) contains embedded NUL byte" net/mptcp/crypto_test.c:73:1: error: bad integer constant expression
net/mptcp/crypto_test.c:73:1: error: static assertion failed: "MODULE_INFO(description, ...) contains embedded NUL byte"

There was a fix for that posted. Let me ping them to see if anyone is planning to send an actual patch.

Quoted 2 lines

https://lore.kernel.org/all/[email protected]/

Thank you both for your reply! I didn't think about looking at the v1.

I confirm that Sami's patch silences the errors on my side. Thanks!

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

-Kees

--
Kees Cook

Hi Kees

Thanks for the report.

Quoted 18 lines · Kees Cook

Quoted 8 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

Quoted 1 line

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Basically sparse/smatch do not seem to understand the constant.

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

-Kees

--
Kees Cook

Hi Dan

Quoted 33 lines · Kees Cook

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 8 lines

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

Could you take a look at this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

Seems that sparse/smatch are not very happy about __builtin_strlen()

Could you fix support for __builtin_strlen() in your tool?

Once Kees lands his patch it will break all the CIs using sparse/smatch, including media-ci.

Eg:

drivers/media/pci/zoran/zr36060.c:33:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression

Quoted 55 lines · Ricardo Ribalda

Hi Dan

Quoted 33 lines · Kees Cook

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 8 lines

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

Could you take a look at this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

Seems that sparse/smatch are not very happy about __builtin_strlen()

Could you fix support for __builtin_strlen() in your tool?

Once Kees lands his patch it will break all the CIs using sparse/smatch, including media-ci.

Quoted 5 lines

Eg:

drivers/media/pci/zoran/zr36060.c:33:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression

We've waited a decade to get the embedded-NUL check into the modinfo macros, so I'm happy to wait until we can get the CI tooling updated.

Please keep me posted on when the sparse and smatch have been updated and I'll re-send the modinfo patch.

Thanks!

-Kees

--
Kees Cook

Hi Kees

Quoted 63 lines · Kees Cook

Quoted 60 lines · Ricardo Ribalda

Hi Dan

Quoted 33 lines · Kees Cook

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 8 lines

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

Could you take a look at this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

Seems that sparse/smatch are not very happy about __builtin_strlen()

Could you fix support for __builtin_strlen() in your tool?

Once Kees lands his patch it will break all the CIs using sparse/smatch, including media-ci.

Eg:

drivers/media/pci/zoran/zr36060.c:33:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression

Quoted 2 lines

We've waited a decade to get the embedded-NUL check into the modinfo macros, so I'm happy to wait until we can get the CI tooling updated.

For media-ci. It will probably be after 6.19rc1

Basically, when
https://lore.kernel.org/linux-sparse/CACePvbVG2KrGQq4cNKV=wbO5h=jp3M0RO1SdfX8kV4OukjPG8A@mail.gmail.com/T/#t
lands in media-committers.

How did you plan to land this series? via which tree?

if Chris have not landed his fix yet, maybe you can include that patch in your series as 1/X.... That will make every ci happy

Quoted 69 lines · Ricardo Ribalda

Hi Kees

Quoted 65 lines · Kees Cook

Quoted 52 lines · Ricardo Ribalda

Hi Dan

Quoted 33 lines · Kees Cook

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 8 lines

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

Could you take a look at this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

Seems that sparse/smatch are not very happy about __builtin_strlen()

Could you fix support for __builtin_strlen() in your tool?

Quoted 8 lines

Once Kees lands his patch it will break all the CIs using sparse/smatch, including media-ci.

Eg:

drivers/media/pci/zoran/zr36060.c:33:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression

We've waited a decade to get the embedded-NUL check into the modinfo macros, so I'm happy to wait until we can get the CI tooling updated.

Quoted 5 lines

For media-ci. It will probably be after 6.19rc1

Basically, when
https://lore.kernel.org/linux-sparse/CACePvbVG2KrGQq4cNKV=wbO5h=jp3M0RO1SdfX8kV4OukjPG8A@mail.gmail.com/T/#t
lands in media-committers.

That's external to Linux, though; it's a patch for sparse and smatch. How often does the CI rebuild sparse and smatch?

Quoted 2 lines

How did you plan to land this series? via which tree?

I assume it would go either via the modules tree or the hardening tree. (Again, no rush.)

Quoted 2 lines

if Chris have not landed his fix yet, maybe you can include that patch in your series as 1/X.... That will make every ci happy

Since it's a patch for a separate project, I don't think that will work. (Maybe I'm misunderstanding something?)

-Kees

--
Kees Cook

Quoted 66 lines · Ricardo Ribalda

Hi Kees

Quoted 63 lines · Kees Cook

Quoted 60 lines · Ricardo Ribalda

Hi Dan

Quoted 33 lines · Kees Cook

Quoted 31 lines · Ricardo Ribalda

Hi Kees

Thanks for the report.

Quoted 11 lines · Kees Cook

Quoted 7 lines · Patchwork Integration

Dear Kees Cook:

Thanks for your patches! Unfortunately the Media CI robot detected some issues:

# Test static:test-smatch

Quoted 8 lines

Quoted 1 line

drivers/media/usb/usbtv/usbtv-core.c:157:1: error: bad constant expression

Where can I find what this test actually does?

Quoted 2 lines

For more details, check the full report at:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.htm .

This webserver appears to be misconfigured to send compressed output without the right headers? I can't actually view this URL.

I will follow-up with fdo maintainers to figure out what happened. there. On the meantime you can use these url that seems to work:
https://linux-media.pages.freedesktop.org/-/users/patchwork/-/jobs/85913402/artifacts/report.txt
https://gitlab.freedesktop.org/linux-media/users/patchwork/-/jobs/85913398

Quoted 8 lines

Quoted 1 line

Basically sparse/smatch do not seem to understand the constant.

Yeah, I managed to find the actual scripts that are run for the static-sparse/smatch tests. It looks like those tools aren't correctly handling string literals for __builtin_strlen(), which is a constant for constant arguments.

So, that's a C parsing bug in those tools (GCC and Clang are fine).

Could you take a look at this patch:
https://patchwork.linuxtv.org/project/linux-media/patch/[email protected]/

Seems that sparse/smatch are not very happy about __builtin_strlen()

Could you fix support for __builtin_strlen() in your tool?

Once Kees lands his patch it will break all the CIs using sparse/smatch, including media-ci.

Eg:

drivers/media/pci/zoran/zr36060.c:33:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression drivers/media/usb/pvrusb2/pvrusb2-dvb.c:19:1: error: bad constant expression

Quoted 8 lines

Quoted 2 lines

We've waited a decade to get the embedded-NUL check into the modinfo macros, so I'm happy to wait until we can get the CI tooling updated.

For media-ci. It will probably be after 6.19rc1

Basically, when
https://lore.kernel.org/linux-sparse/CACePvbVG2KrGQq4cNKV=wbO5h=jp3M0RO1SdfX8kV4OukjPG8A@mail.gmail.com/T/#t
lands in media-committers.

That's external to Linux, though; it's a patch for sparse and smatch. How often does the CI rebuild sparse and smatch?

Quoted 1 line

How did you plan to land this series? via which tree?

I assume it would go either via the modules tree or the hardening tree. (Again, no rush.)

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Hi!

[...]

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

Quoted 7 lines · Daniel Gomez

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

I'm nervous about this going in alone -- it breaks allmodconfig builds without the media fixes. My intention was to have the media fixes land first...

Should I send the media fixes to linus right away?

-Kees

Quoted 23 lines · Kees Cook

Quoted 7 lines · Daniel Gomez

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

I'm nervous about this going in alone -- it breaks allmodconfig builds without the media fixes. My intention was to have the media fixes land first...

Should I send the media fixes to linus right away?

Quoted 2 lines

-Kees

I can take both patches. But I think it'd make sense to drop patch 3 first and then, apply all 3.

Please, Kees, Hans and Mauro, let me know if this is okay with you.

Quoted 32 lines · Daniel Gomez

Quoted 17 lines · Kees Cook

Quoted 7 lines · Daniel Gomez

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

Quoted 8 lines

I'm nervous about this going in alone -- it breaks allmodconfig builds without the media fixes. My intention was to have the media fixes land first...

Should I send the media fixes to linus right away?

-Kees

I can take both patches. But I think it'd make sense to drop patch 3 first and then, apply all 3.

Quoted 1 line

Please, Kees, Hans and Mauro, let me know if this is okay with you.

I'm fine. If you take it, then I'll drop the media patches from our tree (I merged the media patches yesterday).

Let me know if you take them.

Regards,

Hans

Quoted 38 lines · Hans Verkuil

Quoted 26 lines · Daniel Gomez

Quoted 23 lines · Kees Cook

Quoted 7 lines · Daniel Gomez

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

I'm nervous about this going in alone -- it breaks allmodconfig builds without the media fixes. My intention was to have the media fixes land first...

Should I send the media fixes to linus right away?

Quoted 7 lines

Quoted 2 lines

-Kees

I can take both patches. But I think it'd make sense to drop patch 3 first and then, apply all 3.

Please, Kees, Hans and Mauro, let me know if this is okay with you.

I'm fine. If you take it, then I'll drop the media patches from our tree (I merged the media patches yesterday).

Quoted 1 line

Let me know if you take them.

Thanks, Hans. I merged patch 3 yesterday as well, but since patch order matters in this case, it makes sense to take all of them through the modules tree.

Sorry for the trouble, and thanks Kees, for pointing this out.

Quoted 5 lines

Regards,

Hans

Quoted 41 lines · Daniel Gomez

Quoted 38 lines · Hans Verkuil

Quoted 26 lines · Daniel Gomez

Quoted 23 lines · Kees Cook

Quoted 7 lines · Daniel Gomez

Quoted 5 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Quoted 8 lines

Quoted 3 lines

Hi!

[...]

Applied patch 3, thanks!

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

      commit: 913359754ea821c4d6f6a77e0449b29984099663

I'm nervous about this going in alone -- it breaks allmodconfig builds without the media fixes. My intention was to have the media fixes land first...

Should I send the media fixes to linus right away?

Quoted 7 lines

Quoted 2 lines

-Kees

I can take both patches. But I think it'd make sense to drop patch 3 first and then, apply all 3.

Please, Kees, Hans and Mauro, let me know if this is okay with you.

I'm fine. If you take it, then I'll drop the media patches from our tree (I merged the media patches yesterday).

Quoted 6 lines

Quoted 1 line

Let me know if you take them.

Thanks, Hans. I merged patch 3 yesterday as well, but since patch order matters in this case, it makes sense to take all of them through the modules tree.

Sorry for the trouble, and thanks Kees, for pointing this out.

Kees,

FYI, I have dropped patch 3 from modules. My intention is to merge all 3 patches tomorrow.

I believe Hans has also dropped the patches from the media tree as I do not see them here: https://git.linuxtv.org/media.git/log/

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

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

Quoted 62 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

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

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

Reviewed-by Daniel Gomez <[email protected]>

I have also tested a build of v6.18-rc3 + patches using allmodconfig:

Tested-by Daniel Gomez <[email protected]>

Quoted 62 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

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

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

Reviewed-by Daniel Gomez <[email protected]>

I have also tested a build of v6.18-rc3 + patches using allmodconfig:

Tested-by Daniel Gomez <[email protected]>

Quoted 69 lines · Daniel Gomez

Quoted 67 lines · Kees Cook

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

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

Quoted 5 lines

Reviewed-by Daniel Gomez <[email protected]>

I have also tested a build of v6.18-rc3 + patches using allmodconfig:

Tested-by Daniel Gomez <[email protected]>

Folks, are you aware that this change blown up the sparse? Now there is a "bad constant expression" to each MODULE_*() macro line.

Nice that Uwe is in the Cc list, so IIRC he is Debian maintainer for sparse and perhaps has an influence to it to some extent.

Folks, are you aware that this change blown up the sparse? Now there is a "bad constant expression" to each MODULE_*() macro line.

Nice that Uwe is in the Cc list, so IIRC he is Debian maintainer for sparse and perhaps has an influence to it to some extent.

v2:
- use static_assert instead of _Static_assert
- add Hans's Reviewed-by's
v1: https://lore.kernel.org/lkml/[email protected]/

Hi!

[...]