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

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

[Daniel Gomez]

On Mon, Oct 20, 2025 at 11:51:05AM -0700, Kees Cook wrote:
> On Mon, Oct 20, 2025 at 08:35:53PM +0200, Ricardo Ribalda wrote:
> > Hi Kees
> > 
> > On Mon, 20 Oct 2025 at 20:29, Kees Cook <kees@kernel.org> wrote:
> > >
> > > On Wed, Oct 15, 2025 at 09:33:40AM +0200, Ricardo Ribalda wrote:
> > > > Hi Dan
> > > >
> > > > On Tue, 14 Oct 2025 at 22:45, Kees Cook <kees@kernel.org> wrote:
> > > > >
> > > > > On Tue, Oct 14, 2025 at 08:24:00AM +0200, Ricardo Ribalda wrote:
> > > > > > Hi Kees
> > > > > >
> > > > > > Thanks for the report.
> > > > > >
> > > > > >
> > > > > > On Tue, 14 Oct 2025 at 07:41, Kees Cook <kees@kernel.org> wrote:
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > On October 13, 2025 1:34:20 PM PDT, Patchwork Integration <patchwork@media-ci.org> wrote:
> > > > > > > >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?
> > > > > > >
> > > > > > > >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
> > > > > >
> > > > > > 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/20251010030610.3032147-3-kees@kernel.org/
> > > >
> > > > 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
> > >
> > > 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?
> 
> > 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.)

FYI, the patch is applied to modules-next, so I was planning to send it
for v6.19-rc1.

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

[Matthieu Baerts]

Hi Kees, Dan,

Sorry to react on an oldish patch, but I have a question about it, see
below.

On 10/10/2025 05:06, 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..915f32f7d888 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"); \

When checking MPTCP code on top of Linus tree, I get this new warning
with all MPTCP KUnit tests (net/mptcp/*_test.c), e.g.

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


I'm using Sparse last development version with Dan's commit:

$ sparse --version
v0.6.4-73-gfbdde312

=> fbdde312 ("builtin: implement __builtin_strlen() for constants")


And the two lines causing the warnings don't have "\0":

    72  MODULE_LICENSE("GPL");
    73  MODULE_DESCRIPTION("KUnit tests for MPTCP Crypto");


Am I missing something?

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.

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

[Daniel Gomez]

On 19/12/2025 13.29, Matthieu Baerts wrote:
> $ 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"
> 

FYI, we were discussing the fix here:

https://lore.kernel.org/all/20251211175101.GA3405942@google.com/

Sami provided a fix to the thread that you can test. It'd be good to know if it
works for you too. But we still have some questions as we are not familiar with
the sparse code.

Sami, would you mind sending a patch to the sparse list and then we can ask
questions there?

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

[Dan Carpenter]

On Fri, Dec 19, 2025 at 01:29:21PM +0100, Matthieu Baerts wrote:
> 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/20251211175101.GA3405942@google.com/

regards
dan carpenter

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

[Matthieu Baerts]

Hi Dan, Daniel

On 19/12/2025 13:44, Dan Carpenter wrote:
> On Fri, Dec 19, 2025 at 01:29:21PM +0100, Matthieu Baerts wrote:
>> 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/20251211175101.GA3405942@google.com/

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!

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.