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

[Daniel Gomez <da.gomez@kernel.org>]

On 11/11/2025 12.42, Andy Shevchenko wrote:
> On Wed, Nov 05, 2025 at 02:03:59PM +0100, Daniel Gomez wrote:
>> On 10/10/2025 05.06, Kees Cook wrote:
>>>  v2:
>>>  - use static_assert instead of _Static_assert
>>>  - add Hans's Reviewed-by's
>>>  v1: https://lore.kernel.org/lkml/20251008033844.work.801-kees@kernel.org/
>>>
>>> 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(-)
>>>
>>
>> Reviewed-by: Daniel Gomez <da.gomez@samsung.com>
>>
>> I have also tested a build of v6.18-rc3 + patches using allmodconfig:
>>
>> Tested-by: Daniel Gomez <da.gomez@samsung.com>
> 
> Folks, are you aware that this change blown up the sparse?
> Now there is a "bad constant expression" to each MODULE_*() macro line.

Thanks for the heads up.

I can see this thread:

https://lore.kernel.org/all/D1CBCBE2-3A54-410A-B15C-F1C621F9F56B@kernel.org/#t

And this:

https://lore.kernel.org/linux-sparse/CACePvbVG2KrGQq4cNKV=wbO5h=jp3M0RO1SdfX8kV4OukjPG8A@mail.gmail.com/T/#t

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

Would it be better approach to postpone patch 3 from Kent until sparse is fixed?