* Re: [PATCH v2 3/3] module: Add compile-time check for embedded NUL characters
From: Petr Pavlu @ 2025-10-10 4:19 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Rusty Russell, Daniel Gomez, Sami Tolvanen,
linux-modules, Hans Verkuil, Malcolm Priestley,
Mauro Carvalho Chehab, Hans Verkuil, Uwe Kleine-König,
linux-kernel, linux-media, linux-hardening
In-Reply-To: <20251010030610.3032147-3-kees@kernel.org>
On 10/10/25 5:06 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>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
--
Thanks,
Petr
^ permalink raw reply
* [PATCH v2 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
From: Kees Cook @ 2025-10-10 3:06 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Hans Verkuil, Malcolm Priestley, Mauro Carvalho Chehab,
linux-media, Hans Verkuil, Uwe Kleine-König, Rusty Russell,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, linux-kernel,
linux-modules, linux-hardening
In-Reply-To: <20251010030348.it.784-kees@kernel.org>
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 <hverkuil+cisco@kernel.org>
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
^ permalink raw reply related
* [PATCH v2 3/3] module: Add compile-time check for embedded NUL characters
From: Kees Cook @ 2025-10-10 3:06 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Rusty Russell, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
linux-modules, Hans Verkuil, Malcolm Priestley,
Mauro Carvalho Chehab, Hans Verkuil, Uwe Kleine-König,
linux-kernel, linux-media, linux-hardening
In-Reply-To: <20251010030348.it.784-kees@kernel.org>
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"); \
static const char __UNIQUE_ID(modinfo)[] \
__used __section(".modinfo") __aligned(1) \
= __MODULE_INFO_PREFIX __stringify(tag) "=" info
--
2.34.1
^ permalink raw reply related
* [PATCH v2 2/3] media: radio: si470x: Fix DRIVER_AUTHOR macro definition
From: Kees Cook @ 2025-10-10 3:06 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Hans Verkuil, Hans Verkuil, Mauro Carvalho Chehab,
Uwe Kleine-König, linux-media, Malcolm Priestley,
Rusty Russell, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
linux-kernel, linux-modules, linux-hardening
In-Reply-To: <20251010030348.it.784-kees@kernel.org>
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.
Reviewed-by: Hans Verkuil <hverkuil+cisco@kernel.org>
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
^ permalink raw reply related
* [PATCH v2 0/3] module: Add compile-time check for embedded NUL characters
From: Kees Cook @ 2025-10-10 3:06 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Hans Verkuil, Malcolm Priestley, Mauro Carvalho Chehab,
Hans Verkuil, Uwe Kleine-König, Rusty Russell, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, linux-kernel, linux-media,
linux-modules, linux-hardening
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(-)
--
2.34.1
^ permalink raw reply
* Re: [PATCH v8 7/8] modpost: Create modalias for builtin modules
From: Nicolas Schier @ 2025-10-09 19:52 UTC (permalink / raw)
To: Alexey Gladkov
Cc: Charles Mirabile, da.gomez, linux-kbuild, linux-kernel,
linux-modules, masahiroy, mcgrof, nathan, petr.pavlu,
samitolvanen, sfr
In-Reply-To: <aOToOeNGiaFVM0Ds@example.org>
On Tue, Oct 07, 2025 at 12:15:21PM +0200, Alexey Gladkov wrote:
> On Mon, Oct 06, 2025 at 09:16:37PM -0400, Charles Mirabile wrote:
> > On Thu, Sep 18, 2025 at 10:05:51AM +0200, Alexey Gladkov wrote:
> > > For some modules, modalias is generated using the modpost utility and
> > > the section is added to the module file.
> > >
> > > When a module is added inside vmlinux, modpost does not generate
> > > modalias for such modules and the information is lost.
> > >
> > > As a result kmod (which uses modules.builtin.modinfo in userspace)
> > > cannot determine that modalias is handled by a builtin kernel module.
> > >
> > > $ cat /sys/devices/pci0000:00/0000:00:14.0/modalias
> > > pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
> > >
> > > $ modinfo xhci_pci
> > > name: xhci_pci
> > > filename: (builtin)
> > > license: GPL
> > > file: drivers/usb/host/xhci-pci
> > > description: xHCI PCI Host Controller Driver
> > >
> > > Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> > > modpost if the module is built separately.
> > >
> > > To fix this it is necessary to generate the same modalias for vmlinux as
> > > for the individual modules. Fortunately '.vmlinux.export.o' is already
> > > generated from which '.modinfo' can be extracted in the same way as for
> > > vmlinux.o.
> >
> > Hi -
> >
> > This patch broke RISC-V builds for me. During the final objcopy where the new
> > symbols are supposed to be stripped, an error occurs producing lots of error
> > messages similar to this one:
> >
> > riscv64-linux-gnu-objcopy: not stripping symbol `__mod_device_table__...'
> > because it is named in a relocation
> >
> > It does not occur using defconfig, but I was able to bisect my way to this
> > commit and then reduce my config delta w.r.t defconfig until I landed on:
> >
> > cat > .config <<'EOF'
> > CONFIG_RELOCATABLE=y
> > CONFIG_KASAN=y
> > EOF
> > ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make olddefconfig
> > ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make -j $(nproc)
> > ...
> > LD vmlinux.unstripped
> > NM System.map
> > SORTTAB vmlinux.unstripped
> > CHKREL vmlinux.unstripped
> > OBJCOPY vmlinux
> > OBJCOPY modules.builtin.modinfo
> > GEN modules.builtin
> > riscv64-linux-gnu-objcopy: not stripping symbol `<long symbol name>'
> > because it is named in a relocation
> > <repeats with different symbol names about a dozen times>
> > make[3]: *** [scripts/Makefile.vmlinux:97: vmlinux] Error 1
> > make[3]: *** Deleting file 'vmlinux'
> > make[2]: *** [Makefile:1242: vmlinux] Error 2
> > make[1]: *** [/tmp/linux/Makefile:369: __build_one_by_one] Error 2
> > make: *** [Makefile:248: __sub-make] Error 2
> >
> > I confirmed that reverting this commit fixes the issue.
Thanks for the report!
>
> Hm. Indeed. I haven't found a good solution yet, but you can use the
> following patch to unlock compilation. It won't solve the problem, it will
> only hide it.
>
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -84,7 +84,7 @@ endif
> remove-section-y := .modinfo
> remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
>
> -remove-symbols := -w --strip-symbol='__mod_device_table__*'
> +remove-symbols := -w --strip-unneeded-symbol='__mod_device_table__*'
>
> # To avoid warnings: "empty loadable segment detected at ..." from GNU objcopy,
> # it is necessary to remove the PT_LOAD flag from the segment.
>
Is it problematic to hide that? Otherwise we'd have to revert the
patch, right?
Kind regards,
Nicolas
^ permalink raw reply
* Re: modprobe returns 0 upon -EEXIST from insmod
From: Lucas De Marchi @ 2025-10-09 14:13 UTC (permalink / raw)
To: Petr Pavlu; +Cc: Phil Sutter, Christophe Leroy, linux-modules, Yi Chen
In-Reply-To: <ce7f293c-d9f9-4137-bcad-8cc492d34773@suse.com>
On Thu, Oct 09, 2025 at 03:47:42PM +0200, Petr Pavlu wrote:
>On 10/8/25 8:41 AM, Lucas De Marchi wrote:
>> On Tue, Aug 19, 2025 at 09:17:50AM -0500, Lucas De Marchi wrote:
>>> On Tue, Aug 19, 2025 at 10:52:16AM +0200, Petr Pavlu wrote:
>>>> On 8/18/25 11:34 AM, Phil Sutter wrote:
>>>>> On Sun, Aug 17, 2025 at 05:54:27PM +0200, Christophe Leroy wrote:
>>>>>> Le 17/08/2025 à 01:33, Phil Sutter a écrit :
>>>>>>> [Vous ne recevez pas souvent de courriers de phil@nwl.cc. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>>>>>>>
>>>>>>> Hi,
>>>>>>>
>>>>>>> I admittedly didn't fully analyze the cause, but on my system a call to:
>>>>>>>
>>>>>>> # insmod /lib/module/$(uname -r)/kernel/net/netfilter/nf_conntrack_ftp.ko
>>>>>>>
>>>>>>> fails with -EEXIST (due to a previous call to 'nfct add helper ftp inet
>>>>>>> tcp'). A call to:
>>>>>>>
>>>>>>> # modprobe nf_conntrack_ftp
>>>>>>>
>>>>>>> though returns 0 even though module loading fails. Is there a bug in
>>>>>>> modprobe error status handling?
>>>>>>>
>>>>>>
>>>>>> Read the man page : https://linux.die.net/man/8/modprobe
>>>>>>
>>>>>> In the man page I see:
>>>>>>
>>>>>> Normally, modprobe will succeed (and do nothing) if told to
>>>>>> insert a module which is already present or to remove a module which
>>>>>> isn't present.
>>>>>
>>>>> This is not a case of already inserted module, it is not loaded before
>>>>> the call to modprobe. It is the module_init callback
>>>>> nf_conntrack_ftp_init() which returns -EEXIST it received from
>>>>> nf_conntrack_helpers_register().
>>>
>>> is this a real failure condition or something benign like "if it's
>>> already registered, there's nothing to do"?
>>>
>>>>>
>>>>> Can't user space distinguish the two causes of -EEXIST? Or in other
>>>>> words, is use of -EEXIST in module_init callbacks problematic?
>>>>
>>>> Unfortunately, error return codes from (f)init_module cannot be reliably
>>>> depended upon. For instance, cpufreq drivers have similar behavior of
>>>> returning -EEXIST when another cpufreq driver is already registered.
>>>> Returning this code unexpectedly can then confuse kmod, as it interprets
>>>> -EEXIST to mean "the module is already loaded" [1].
>>>
>>> well, it's not that it can't be relied on. There's 1 exit code that is
>>> treated specially, EEXISTS, because that error is used by the module
>>> loading part, before the module_init call, to signify the module is
>>> already loaded.
>>>
>>>>
>>>> I have thought about this problem before. We might fix the main
>>>> problematic occurrences, but we can't really audit all the code that
>>>> module init functions can invoke. I then wonder if it would make sense
>>>> for the module loader to warn about any -EEXIST returned by a module's
>>>> init function and translate it to -EBUSY.
>>>
>>> If it's a failure condition then yes, -EBUSY looks appropriate.
>>
>> something like this:
>>
>>
>> diff --git a/kernel/module/main.c b/kernel/module/main.c
>> index c66b261849362..e5fb1a4ef3441 100644
>> --- a/kernel/module/main.c
>> +++ b/kernel/module/main.c
>> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
>> if (mod->init != NULL)
>> ret = do_one_initcall(mod->init);
>> if (ret < 0) {
>> + if (ret == -EEXIST) {
>> + pr_warn("%s: '%s'->init suspiciously returned %d: Overriding with %d\n",
>> + __func__, mod->name, -EEXIST, -EBUSY);
>> + ret = -EBUSY;
>> + }
>> goto fail_free_freeinit;
>> }
>> if (ret > 0) {
>
>Yes, that's what I had in mind. Could you please send this as a proper
>patch to the list?
>
>I only think we should include a hint to explain why this is a problem
>and simplify the message somewhat, something like:
>
>pr_warn("%s: init suspiciously returned -EEXIST (reserved for loaded modules), overriding with -EBUSY\n", mod->name);
>
>I realize you based the message on the later warning about the init
>function returning a >0 value but I think we should rather update that
>message as well. It should follow the usual style of
>"<mod-name>: <error-description>". I suggest simplifying it to:
>
>pr_warn("%s: init suspiciously returned %d, it should follow 0/-E convention\n", mod->name, ret);
will do and actually run some tests to make sure it's not only
build-tested.
Thanks,
Lucas De Marchi
>
>--
>Thanks,
>Petr
^ permalink raw reply
* Re: modprobe returns 0 upon -EEXIST from insmod
From: Petr Pavlu @ 2025-10-09 13:47 UTC (permalink / raw)
To: Lucas De Marchi; +Cc: Phil Sutter, Christophe Leroy, linux-modules, Yi Chen
In-Reply-To: <hupl3hqym5ru3fr27s3elg6vti4fjtphdwvvyxmuvekc2w5mna@kilmmcgobw6x>
On 10/8/25 8:41 AM, Lucas De Marchi wrote:
> On Tue, Aug 19, 2025 at 09:17:50AM -0500, Lucas De Marchi wrote:
>> On Tue, Aug 19, 2025 at 10:52:16AM +0200, Petr Pavlu wrote:
>>> On 8/18/25 11:34 AM, Phil Sutter wrote:
>>>> On Sun, Aug 17, 2025 at 05:54:27PM +0200, Christophe Leroy wrote:
>>>>> Le 17/08/2025 à 01:33, Phil Sutter a écrit :
>>>>>> [Vous ne recevez pas souvent de courriers de phil@nwl.cc. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>>>>>>
>>>>>> Hi,
>>>>>>
>>>>>> I admittedly didn't fully analyze the cause, but on my system a call to:
>>>>>>
>>>>>> # insmod /lib/module/$(uname -r)/kernel/net/netfilter/nf_conntrack_ftp.ko
>>>>>>
>>>>>> fails with -EEXIST (due to a previous call to 'nfct add helper ftp inet
>>>>>> tcp'). A call to:
>>>>>>
>>>>>> # modprobe nf_conntrack_ftp
>>>>>>
>>>>>> though returns 0 even though module loading fails. Is there a bug in
>>>>>> modprobe error status handling?
>>>>>>
>>>>>
>>>>> Read the man page : https://linux.die.net/man/8/modprobe
>>>>>
>>>>> In the man page I see:
>>>>>
>>>>> Normally, modprobe will succeed (and do nothing) if told to
>>>>> insert a module which is already present or to remove a module which
>>>>> isn't present.
>>>>
>>>> This is not a case of already inserted module, it is not loaded before
>>>> the call to modprobe. It is the module_init callback
>>>> nf_conntrack_ftp_init() which returns -EEXIST it received from
>>>> nf_conntrack_helpers_register().
>>
>> is this a real failure condition or something benign like "if it's
>> already registered, there's nothing to do"?
>>
>>>>
>>>> Can't user space distinguish the two causes of -EEXIST? Or in other
>>>> words, is use of -EEXIST in module_init callbacks problematic?
>>>
>>> Unfortunately, error return codes from (f)init_module cannot be reliably
>>> depended upon. For instance, cpufreq drivers have similar behavior of
>>> returning -EEXIST when another cpufreq driver is already registered.
>>> Returning this code unexpectedly can then confuse kmod, as it interprets
>>> -EEXIST to mean "the module is already loaded" [1].
>>
>> well, it's not that it can't be relied on. There's 1 exit code that is
>> treated specially, EEXISTS, because that error is used by the module
>> loading part, before the module_init call, to signify the module is
>> already loaded.
>>
>>>
>>> I have thought about this problem before. We might fix the main
>>> problematic occurrences, but we can't really audit all the code that
>>> module init functions can invoke. I then wonder if it would make sense
>>> for the module loader to warn about any -EEXIST returned by a module's
>>> init function and translate it to -EBUSY.
>>
>> If it's a failure condition then yes, -EBUSY looks appropriate.
>
> something like this:
>
>
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c66b261849362..e5fb1a4ef3441 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
> if (mod->init != NULL)
> ret = do_one_initcall(mod->init);
> if (ret < 0) {
> + if (ret == -EEXIST) {
> + pr_warn("%s: '%s'->init suspiciously returned %d: Overriding with %d\n",
> + __func__, mod->name, -EEXIST, -EBUSY);
> + ret = -EBUSY;
> + }
> goto fail_free_freeinit;
> }
> if (ret > 0) {
Yes, that's what I had in mind. Could you please send this as a proper
patch to the list?
I only think we should include a hint to explain why this is a problem
and simplify the message somewhat, something like:
pr_warn("%s: init suspiciously returned -EEXIST (reserved for loaded modules), overriding with -EBUSY\n", mod->name);
I realize you based the message on the later warning about the init
function returning a >0 value but I think we should rather update that
message as well. It should follow the usual style of
"<mod-name>: <error-description>". I suggest simplifying it to:
pr_warn("%s: init suspiciously returned %d, it should follow 0/-E convention\n", mod->name, ret);
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 10/10] module loader: enforce symbol import protection
From: Petr Pavlu @ 2025-10-08 15:35 UTC (permalink / raw)
To: Siddharth Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel
In-Reply-To: <20250829105418.3053274-11-sidnayyar@google.com>
On 8/29/25 12:54 PM, Siddharth Nayyar wrote:
> The module loader will reject unsigned modules from loading if such a
> module attempts to import a symbol which has the import protection bit
> set in the kflagstab entry for the symbol.
>
> Signed-off-by: Siddharth Nayyar <sidnayyar@google.com>
> ---
> [...]
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 4437c2a451ea..ece074a6ba7b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -380,6 +380,7 @@ static bool find_exported_symbol_in_section(const struct symsearch *syms,
> fsa->crc = symversion(syms->crcs, sym - syms->start);
> fsa->sym = sym;
> fsa->license = (sym_flags & KSYM_FLAG_GPL_ONLY) ? GPL_ONLY : NOT_GPL_ONLY;
> + fsa->is_protected = sym_flags & KSYM_FLAG_PROTECTED;
>
> return true;
> }
> @@ -1273,6 +1274,11 @@ static const struct kernel_symbol *resolve_symbol(struct module *mod,
> goto getname;
> }
>
> + if (fsa.is_protected && !mod->sig_ok) {
> + fsa.sym = ERR_PTR(-EACCES);
> + goto getname;
> + }
> +
> getname:
> /* We must make copy under the lock if we failed to get ref. */
> strscpy(ownername, module_name(fsa.owner), MODULE_NAME_LEN);
The is_protected check should be moved before the ref_module() call.
Adding a reference to another module should be always the last step,
after all symbol checks have been performed.
> @@ -1550,8 +1556,12 @@ static int simplify_symbols(struct module *mod, const struct load_info *info)
> break;
>
> ret = PTR_ERR(ksym) ?: -ENOENT;
> - pr_warn("%s: Unknown symbol %s (err %d)\n",
> - mod->name, name, ret);
> + if (ret == -EACCES)
> + pr_warn("%s: Protected symbol %s (err %d)\n",
> + mod->name, name, ret);
> + else
> + pr_warn("%s: Unknown symbol %s (err %d)\n",
> + mod->name, name, ret);
> break;
>
> default:
I suggest moving the error message about the symbol being protected down
into resolve_symbol(), at the point where this issue is detected. This
approach is generally used for other checks, such as the CRC or
namespace check. Additionally, I think it would make sense to change the
current "Unknown symbol" warning here to "Unresolved symbol" to be more
accurate.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 09/10] modpost: add symbol import protection flag to kflagstab
From: Petr Pavlu @ 2025-10-08 13:35 UTC (permalink / raw)
To: Siddharth Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel
In-Reply-To: <20250829105418.3053274-10-sidnayyar@google.com>
On 8/29/25 12:54 PM, Siddharth Nayyar wrote:
> When the unused exports whitelist is provided, the symbol protection bit
> is set for symbols not present in the unused exports whitelist.
>
> The flag will be used in the following commit to prevent unsigned
> modules from the using symbols other than those explicitly declared by
> the such modules ahead of time.
>
> Signed-off-by: Siddharth Nayyar <sidnayyar@google.com>
> ---
> [...]
> diff --git a/include/linux/module_symbol.h b/include/linux/module_symbol.h
> index 574609aced99..96fe3f4d7424 100644
> --- a/include/linux/module_symbol.h
> +++ b/include/linux/module_symbol.h
> @@ -3,8 +3,9 @@
> #define _LINUX_MODULE_SYMBOL_H
>
> /* Kernel symbol flags bitset. */
> -enum ksym_flags {
> +enum symbol_flags {
> KSYM_FLAG_GPL_ONLY = 1 << 0,
> + KSYM_FLAG_PROTECTED = 1 << 1,
> };
>
Nit: The ksym_flags enum is added in patch #1. If you prefer a different
name, you can change it in that patch.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 08/10] remove references to *_gpl sections in documentation
From: Petr Pavlu @ 2025-10-08 13:24 UTC (permalink / raw)
To: Siddharth Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel
In-Reply-To: <20250829105418.3053274-9-sidnayyar@google.com>
On 8/29/25 12:54 PM, Siddharth Nayyar wrote:
> Signed-off-by: Siddharth Nayyar <sidnayyar@google.com>
> ---
> Documentation/kbuild/modules.rst | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/kbuild/modules.rst b/Documentation/kbuild/modules.rst
> index d0703605bfa4..f2022fa2342f 100644
> --- a/Documentation/kbuild/modules.rst
> +++ b/Documentation/kbuild/modules.rst
> @@ -426,11 +426,11 @@ Symbols From the Kernel (vmlinux + modules)
> Version Information Formats
> ---------------------------
>
> - Exported symbols have information stored in __ksymtab or __ksymtab_gpl
> - sections. Symbol names and namespaces are stored in __ksymtab_strings,
> + Exported symbols have information stored in the __ksymtab section.
> + Symbol names and namespaces are stored in __ksymtab_strings section,
> using a format similar to the string table used for ELF. If
> CONFIG_MODVERSIONS is enabled, the CRCs corresponding to exported
> - symbols will be added to the __kcrctab or __kcrctab_gpl.
> + symbols will be added to the __kcrctab section.
>
> If CONFIG_BASIC_MODVERSIONS is enabled (default with
> CONFIG_MODVERSIONS), imported symbols will have their symbol name and
Nit: I realize this part of the document primarily discusses sections
related to modversions, but I think it would be good to briefly mention
also the existence of the __kflagstab section. The first sentence could
say:
Exported symbols have information stored in the __ksymtab and
__kflagstab sections.
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 06/10] module loader: remove references of *_gpl sections
From: Petr Pavlu @ 2025-10-08 13:22 UTC (permalink / raw)
To: Siddharth Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel
In-Reply-To: <20250829105418.3053274-7-sidnayyar@google.com>
On 8/29/25 12:54 PM, Siddharth Nayyar wrote:
> The *_gpl section are not being used populated by modpost anymore. Hence
> the module loader doesn't need to find and process these sections in
> modules.
>
> Signed-off-by: Siddharth Nayyar <sidnayyar@google.com>
> ---
> [...]
> @@ -2601,10 +2590,6 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> mod->syms = section_objs(info, "__ksymtab",
> sizeof(*mod->syms), &mod->num_syms);
> mod->crcs = section_addr(info, "__kcrctab");
> - mod->gpl_syms = section_objs(info, "__ksymtab_gpl",
> - sizeof(*mod->gpl_syms),
> - &mod->num_gpl_syms);
> - mod->gpl_crcs = section_addr(info, "__kcrctab_gpl");
> mod->flagstab = section_addr(info, "__kflagstab");
>
I suggest adding a check that the loaded module doesn't contain
a __ksymtab_gpl or __kcrctab_gpl section, similarly how the function
later checks if the old __obsparm section isn't present. Something like:
if (section_addr(info, "__ksymtab_gpl"))
pr_warn("%s: ignoring obsolete section __ksymtab_gpl\n", mod->name);
if (section_addr(info, "__kcrctab_gpl"))
pr_warn("%s: ignoring obsolete section __kcrctab_gpl\n", mod->name);
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 04/10] module loader: use kflagstab instead of *_gpl sections
From: Petr Pavlu @ 2025-10-08 13:19 UTC (permalink / raw)
To: Siddharth Nayyar
Cc: Nathan Chancellor, Luis Chamberlain, Sami Tolvanen,
Nicolas Schier, Arnd Bergmann, linux-kbuild, linux-arch,
linux-modules, linux-kernel
In-Reply-To: <20250829105418.3053274-5-sidnayyar@google.com>
On 8/29/25 12:54 PM, Siddharth Nayyar wrote:
> Read __kflagstab section for vmlinux and modules to determine whether
> kernel symbols are GPL only.
>
> Signed-off-by: Siddharth Nayyar <sidnayyar@google.com>
> ---
> [...]
> @@ -2607,6 +2605,7 @@ static int find_module_sections(struct module *mod, struct load_info *info)
> sizeof(*mod->gpl_syms),
> &mod->num_gpl_syms);
> mod->gpl_crcs = section_addr(info, "__kcrctab_gpl");
> + mod->flagstab = section_addr(info, "__kflagstab");
>
> #ifdef CONFIG_CONSTRUCTORS
> mod->ctors = section_objs(info, ".ctors",
The module loader should always at least get through the signature and
blacklist checks without crashing due to a corrupted ELF file. After
that point, the module content is to be trusted, but we try to error out
for most issues that would cause problems later on.
For __kflagstab, I believe it would be useful to check that the section
is present to prevent the code from potentially crashing due to a NULL
dereference deep in find_exported_symbol_in_section(). You can rename
check_export_symbol_versions() to check_export_symbol_sections() and add
the following:
if (mod->num_syms && !mod->flagstab) {
pr_err("%s: no flags for exported symbols\n", mod->name);
return -ENOEXEC;
}
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH 3/3] module: Add compile-time check for embedded NUL characters
From: Petr Pavlu @ 2025-10-08 9:55 UTC (permalink / raw)
To: Kees Cook
Cc: Luis Chamberlain, Rusty Russell, Daniel Gomez, Sami Tolvanen,
linux-modules, Malcolm Priestley, Mauro Carvalho Chehab,
Hans Verkuil, Uwe Kleine-König, linux-kernel, linux-media,
linux-hardening
In-Reply-To: <20251008035938.838263-3-kees@kernel.org>
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
^ permalink raw reply
* Re: modprobe returns 0 upon -EEXIST from insmod
From: Lucas De Marchi @ 2025-10-08 6:41 UTC (permalink / raw)
To: Petr Pavlu; +Cc: Phil Sutter, Christophe Leroy, linux-modules, Yi Chen
In-Reply-To: <i4ayzta5zgltyubg6bfr4mwqhl6goyh73lyc7j7m3vngvpooi3@boorlngxpi52>
On Tue, Aug 19, 2025 at 09:17:50AM -0500, Lucas De Marchi wrote:
>On Tue, Aug 19, 2025 at 10:52:16AM +0200, Petr Pavlu wrote:
>>On 8/18/25 11:34 AM, Phil Sutter wrote:
>>>On Sun, Aug 17, 2025 at 05:54:27PM +0200, Christophe Leroy wrote:
>>>>Le 17/08/2025 à 01:33, Phil Sutter a écrit :
>>>>>[Vous ne recevez pas souvent de courriers de phil@nwl.cc. D?couvrez pourquoi ceci est important ? https://aka.ms/LearnAboutSenderIdentification ]
>>>>>
>>>>>Hi,
>>>>>
>>>>>I admittedly didn't fully analyze the cause, but on my system a call to:
>>>>>
>>>>># insmod /lib/module/$(uname -r)/kernel/net/netfilter/nf_conntrack_ftp.ko
>>>>>
>>>>>fails with -EEXIST (due to a previous call to 'nfct add helper ftp inet
>>>>>tcp'). A call to:
>>>>>
>>>>># modprobe nf_conntrack_ftp
>>>>>
>>>>>though returns 0 even though module loading fails. Is there a bug in
>>>>>modprobe error status handling?
>>>>>
>>>>
>>>>Read the man page : https://linux.die.net/man/8/modprobe
>>>>
>>>>In the man page I see:
>>>>
>>>> Normally, modprobe will succeed (and do nothing) if told to
>>>>insert a module which is already present or to remove a module which
>>>>isn't present.
>>>
>>>This is not a case of already inserted module, it is not loaded before
>>>the call to modprobe. It is the module_init callback
>>>nf_conntrack_ftp_init() which returns -EEXIST it received from
>>>nf_conntrack_helpers_register().
>
>is this a real failure condition or something benign like "if it's
>already registered, there's nothing to do"?
>
>>>
>>>Can't user space distinguish the two causes of -EEXIST? Or in other
>>>words, is use of -EEXIST in module_init callbacks problematic?
>>
>>Unfortunately, error return codes from (f)init_module cannot be reliably
>>depended upon. For instance, cpufreq drivers have similar behavior of
>>returning -EEXIST when another cpufreq driver is already registered.
>>Returning this code unexpectedly can then confuse kmod, as it interprets
>>-EEXIST to mean "the module is already loaded" [1].
>
>well, it's not that it can't be relied on. There's 1 exit code that is
>treated specially, EEXISTS, because that error is used by the module
>loading part, before the module_init call, to signify the module is
>already loaded.
>
>>
>>I have thought about this problem before. We might fix the main
>>problematic occurrences, but we can't really audit all the code that
>>module init functions can invoke. I then wonder if it would make sense
>>for the module loader to warn about any -EEXIST returned by a module's
>>init function and translate it to -EBUSY.
>
>If it's a failure condition then yes, -EBUSY looks appropriate.
something like this:
diff --git a/kernel/module/main.c b/kernel/module/main.c
index c66b261849362..e5fb1a4ef3441 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -3038,6 +3038,11 @@ static noinline int do_init_module(struct module *mod)
if (mod->init != NULL)
ret = do_one_initcall(mod->init);
if (ret < 0) {
+ if (ret == -EEXIST) {
+ pr_warn("%s: '%s'->init suspiciously returned %d: Overriding with %d\n",
+ __func__, mod->name, -EEXIST, -EBUSY);
+ ret = -EBUSY;
+ }
goto fail_free_freeinit;
}
if (ret > 0) {
Lucas De Marchi
>
>Lucas De Marchi
>
>>
>>Ensuring the reliability of the 0 and -EEXIST return codes from
>>(f)init_module should help user space.
>>
>>[1] https://github.com/kmod-project/kmod/blob/695fd084a727cf76f51b129b67d5a4be1d6db32e/libkmod/libkmod-module.c#L1087
>>
>>-- Petr
^ permalink raw reply related
* Re: [PATCH 0/3] module: Add compile-time check for embedded NUL characters
From: Hans Verkuil @ 2025-10-08 6:27 UTC (permalink / raw)
To: Kees Cook, Luis Chamberlain
Cc: Malcolm Priestley, Mauro Carvalho Chehab, Uwe Kleine-König,
Rusty Russell, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
linux-kernel, linux-media, linux-modules, linux-hardening
In-Reply-To: <20251008033844.work.801-kees@kernel.org>
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(-)
>
^ permalink raw reply
* Re: [PATCH 2/3] media: radio: si470x: Fix DRIVER_AUTHOR macro definition
From: Hans Verkuil @ 2025-10-08 6:24 UTC (permalink / raw)
To: Kees Cook, Luis Chamberlain
Cc: Hans Verkuil, Mauro Carvalho Chehab, Uwe Kleine-König,
linux-media, Malcolm Priestley, Rusty Russell, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, linux-kernel, linux-modules,
linux-hardening
In-Reply-To: <20251008035938.838263-2-kees@kernel.org>
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"
^ permalink raw reply
* Re: [PATCH 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
From: Hans Verkuil @ 2025-10-08 6:24 UTC (permalink / raw)
To: Kees Cook, Luis Chamberlain
Cc: Malcolm Priestley, Mauro Carvalho Chehab, linux-media,
Hans Verkuil, Uwe Kleine-König, Rusty Russell, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, linux-kernel, linux-modules,
linux-hardening
In-Reply-To: <20251008035938.838263-1-kees@kernel.org>
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;
^ permalink raw reply
* [PATCH 3/3] module: Add compile-time check for embedded NUL characters
From: Kees Cook @ 2025-10-08 3:59 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Rusty Russell, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
linux-modules, Malcolm Priestley, Mauro Carvalho Chehab,
Hans Verkuil, Uwe Kleine-König, linux-kernel, linux-media,
linux-hardening
In-Reply-To: <20251008033844.work.801-kees@kernel.org>
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
^ permalink raw reply related
* [PATCH 2/3] media: radio: si470x: Fix DRIVER_AUTHOR macro definition
From: Kees Cook @ 2025-10-08 3:59 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Hans Verkuil, Mauro Carvalho Chehab,
Uwe Kleine-König, linux-media, Malcolm Priestley,
Rusty Russell, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
linux-kernel, linux-modules, linux-hardening
In-Reply-To: <20251008033844.work.801-kees@kernel.org>
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
^ permalink raw reply related
* [PATCH 1/3] media: dvb-usb-v2: lmedm04: Fix firmware macro definitions
From: Kees Cook @ 2025-10-08 3:59 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Malcolm Priestley, Mauro Carvalho Chehab, linux-media,
Hans Verkuil, Uwe Kleine-König, Rusty Russell, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, linux-kernel, linux-modules,
linux-hardening
In-Reply-To: <20251008033844.work.801-kees@kernel.org>
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
^ permalink raw reply related
* [PATCH 0/3] module: Add compile-time check for embedded NUL characters
From: Kees Cook @ 2025-10-08 3:59 UTC (permalink / raw)
To: Luis Chamberlain
Cc: Kees Cook, Malcolm Priestley, Mauro Carvalho Chehab, Hans Verkuil,
Uwe Kleine-König, Rusty Russell, Petr Pavlu, Daniel Gomez,
Sami Tolvanen, linux-kernel, linux-media, linux-modules,
linux-hardening
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
^ permalink raw reply
* Re: [PATCH v8 7/8] modpost: Create modalias for builtin modules
From: Alexey Gladkov @ 2025-10-07 10:15 UTC (permalink / raw)
To: Charles Mirabile
Cc: da.gomez, linux-kbuild, linux-kernel, linux-modules, masahiroy,
mcgrof, nathan, nicolas.schier, petr.pavlu, samitolvanen, sfr
In-Reply-To: <20251007011637.2512413-1-cmirabil@redhat.com>
On Mon, Oct 06, 2025 at 09:16:37PM -0400, Charles Mirabile wrote:
> On Thu, Sep 18, 2025 at 10:05:51AM +0200, Alexey Gladkov wrote:
> > For some modules, modalias is generated using the modpost utility and
> > the section is added to the module file.
> >
> > When a module is added inside vmlinux, modpost does not generate
> > modalias for such modules and the information is lost.
> >
> > As a result kmod (which uses modules.builtin.modinfo in userspace)
> > cannot determine that modalias is handled by a builtin kernel module.
> >
> > $ cat /sys/devices/pci0000:00/0000:00:14.0/modalias
> > pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
> >
> > $ modinfo xhci_pci
> > name: xhci_pci
> > filename: (builtin)
> > license: GPL
> > file: drivers/usb/host/xhci-pci
> > description: xHCI PCI Host Controller Driver
> >
> > Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> > modpost if the module is built separately.
> >
> > To fix this it is necessary to generate the same modalias for vmlinux as
> > for the individual modules. Fortunately '.vmlinux.export.o' is already
> > generated from which '.modinfo' can be extracted in the same way as for
> > vmlinux.o.
>
> Hi -
>
> This patch broke RISC-V builds for me. During the final objcopy where the new
> symbols are supposed to be stripped, an error occurs producing lots of error
> messages similar to this one:
>
> riscv64-linux-gnu-objcopy: not stripping symbol `__mod_device_table__...'
> because it is named in a relocation
>
> It does not occur using defconfig, but I was able to bisect my way to this
> commit and then reduce my config delta w.r.t defconfig until I landed on:
>
> cat > .config <<'EOF'
> CONFIG_RELOCATABLE=y
> CONFIG_KASAN=y
> EOF
> ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make olddefconfig
> ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make -j $(nproc)
> ...
> LD vmlinux.unstripped
> NM System.map
> SORTTAB vmlinux.unstripped
> CHKREL vmlinux.unstripped
> OBJCOPY vmlinux
> OBJCOPY modules.builtin.modinfo
> GEN modules.builtin
> riscv64-linux-gnu-objcopy: not stripping symbol `<long symbol name>'
> because it is named in a relocation
> <repeats with different symbol names about a dozen times>
> make[3]: *** [scripts/Makefile.vmlinux:97: vmlinux] Error 1
> make[3]: *** Deleting file 'vmlinux'
> make[2]: *** [Makefile:1242: vmlinux] Error 2
> make[1]: *** [/tmp/linux/Makefile:369: __build_one_by_one] Error 2
> make: *** [Makefile:248: __sub-make] Error 2
>
> I confirmed that reverting this commit fixes the issue.
Hm. Indeed. I haven't found a good solution yet, but you can use the
following patch to unlock compilation. It won't solve the problem, it will
only hide it.
--- a/scripts/Makefile.vmlinux
+++ b/scripts/Makefile.vmlinux
@@ -84,7 +84,7 @@ endif
remove-section-y := .modinfo
remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
-remove-symbols := -w --strip-symbol='__mod_device_table__*'
+remove-symbols := -w --strip-unneeded-symbol='__mod_device_table__*'
# To avoid warnings: "empty loadable segment detected at ..." from GNU objcopy,
# it is necessary to remove the PT_LOAD flag from the segment.
> >
> > Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> > Signed-off-by: Alexey Gladkov <legion@kernel.org>
> > Tested-by: Stephen Rothwell <sfr@canb.auug.org.au>
> > ---
> > include/linux/module.h | 4 ----
> > scripts/Makefile.vmlinux | 4 +++-
> > scripts/mksysmap | 3 +++
> > scripts/mod/file2alias.c | 19 ++++++++++++++++++-
> > scripts/mod/modpost.c | 15 +++++++++++++++
> > scripts/mod/modpost.h | 2 ++
> > 6 files changed, 41 insertions(+), 6 deletions(-)
> >
> > diff --git a/include/linux/module.h b/include/linux/module.h
> > index e31ee29fac6b7..e135cc79aceea 100644
> > --- a/include/linux/module.h
> > +++ b/include/linux/module.h
> > @@ -256,14 +256,10 @@ struct module_kobject *lookup_or_create_module_kobject(const char *name);
> > __PASTE(type, \
> > __PASTE(__, name)))))
> >
> > -#ifdef MODULE
> > /* Creates an alias so file2alias.c can find device table. */
> > #define MODULE_DEVICE_TABLE(type, name) \
> > static typeof(name) __mod_device_table(type, name) \
> > __attribute__ ((used, alias(__stringify(name))))
> > -#else /* !MODULE */
> > -#define MODULE_DEVICE_TABLE(type, name)
> > -#endif
> >
> > /* Version of form [<epoch>:]<version>[-<extra-version>].
> > * Or for CVS/RCS ID version, everything but the number is stripped.
> > diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> > index ce79461714979..1e5e37aadcd05 100644
> > --- a/scripts/Makefile.vmlinux
> > +++ b/scripts/Makefile.vmlinux
> > @@ -89,11 +89,13 @@ endif
> > remove-section-y := .modinfo
> > remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
> >
> > +remove-symbols := -w --strip-symbol='__mod_device_table__*'
> > +
> > # To avoid warnings: "empty loadable segment detected at ..." from GNU objcopy,
> > # it is necessary to remove the PT_LOAD flag from the segment.
> > quiet_cmd_strip_relocs = OBJCOPY $@
> > cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< $@; \
> > - $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $@
> > + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $(remove-symbols) $@
> >
> > targets += vmlinux
> > vmlinux: vmlinux.unstripped FORCE
> > diff --git a/scripts/mksysmap b/scripts/mksysmap
> > index a607a0059d119..c4531eacde202 100755
> > --- a/scripts/mksysmap
> > +++ b/scripts/mksysmap
> > @@ -59,6 +59,9 @@
> > # EXPORT_SYMBOL (namespace)
> > / __kstrtabns_/d
> >
> > +# MODULE_DEVICE_TABLE (symbol name)
> > +/ __mod_device_table__/d
> > +
> > # ---------------------------------------------------------------------------
> > # Ignored suffixes
> > # (do not forget '$' after each pattern)
> > diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> > index 1260bc2287fba..7da9735e7ab3e 100644
> > --- a/scripts/mod/file2alias.c
> > +++ b/scripts/mod/file2alias.c
> > @@ -1477,7 +1477,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> > void *symval;
> > char *zeros = NULL;
> > const char *type, *name, *modname;
> > - size_t typelen;
> > + size_t typelen, modnamelen;
> > static const char *prefix = "__mod_device_table__";
> >
> > /* We're looking for a section relative symbol */
> > @@ -1500,6 +1500,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> > type = strstr(modname, "__");
> > if (!type)
> > return;
> > + modnamelen = type - modname;
> > type += strlen("__");
> >
> > name = strstr(type, "__");
> > @@ -1526,5 +1527,21 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> > }
> > }
> >
> > + if (mod->is_vmlinux) {
> > + struct module_alias *alias;
> > +
> > + /*
> > + * If this is vmlinux, record the name of the builtin module.
> > + * Traverse the linked list in the reverse order, and set the
> > + * builtin_modname unless it has already been set in the
> > + * previous call.
> > + */
> > + list_for_each_entry_reverse(alias, &mod->aliases, node) {
> > + if (alias->builtin_modname)
> > + break;
> > + alias->builtin_modname = xstrndup(modname, modnamelen);
> > + }
> > + }
> > +
> > free(zeros);
> > }
> > diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> > index 5ca7c268294eb..47c8aa2a69392 100644
> > --- a/scripts/mod/modpost.c
> > +++ b/scripts/mod/modpost.c
> > @@ -2067,11 +2067,26 @@ static void write_if_changed(struct buffer *b, const char *fname)
> > static void write_vmlinux_export_c_file(struct module *mod)
> > {
> > struct buffer buf = { };
> > + struct module_alias *alias, *next;
> >
> > buf_printf(&buf,
> > "#include <linux/export-internal.h>\n");
> >
> > add_exported_symbols(&buf, mod);
> > +
> > + buf_printf(&buf,
> > + "#include <linux/module.h>\n"
> > + "#undef __MODULE_INFO_PREFIX\n"
> > + "#define __MODULE_INFO_PREFIX\n");
> > +
> > + list_for_each_entry_safe(alias, next, &mod->aliases, node) {
> > + buf_printf(&buf, "MODULE_INFO(%s.alias, \"%s\");\n",
> > + alias->builtin_modname, alias->str);
> > + list_del(&alias->node);
> > + free(alias->builtin_modname);
> > + free(alias);
> > + }
> > +
> > write_if_changed(&buf, ".vmlinux.export.c");
> > free(buf.p);
> > }
> > diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> > index 9133e4c3803f0..2aecb8f25c87e 100644
> > --- a/scripts/mod/modpost.h
> > +++ b/scripts/mod/modpost.h
> > @@ -99,10 +99,12 @@ buf_write(struct buffer *buf, const char *s, int len);
> > * struct module_alias - auto-generated MODULE_ALIAS()
> > *
> > * @node: linked to module::aliases
> > + * @modname: name of the builtin module (only for vmlinux)
> > * @str: a string for MODULE_ALIAS()
> > */
> > struct module_alias {
> > struct list_head node;
> > + char *builtin_modname;
> > char str[];
> > };
> >
> > --
> > 2.51.0
> >
>
--
Rgrds, legion
^ permalink raw reply
* Re: [PATCH v8 7/8] modpost: Create modalias for builtin modules
From: Charles Mirabile @ 2025-10-07 1:16 UTC (permalink / raw)
To: legion
Cc: da.gomez, linux-kbuild, linux-kernel, linux-modules, masahiroy,
mcgrof, nathan, nicolas.schier, petr.pavlu, samitolvanen, sfr
In-Reply-To: <28d4da3b0e3fc8474142746bcf469e03752c3208.1758182101.git.legion@kernel.org>
On Thu, Sep 18, 2025 at 10:05:51AM +0200, Alexey Gladkov wrote:
> For some modules, modalias is generated using the modpost utility and
> the section is added to the module file.
>
> When a module is added inside vmlinux, modpost does not generate
> modalias for such modules and the information is lost.
>
> As a result kmod (which uses modules.builtin.modinfo in userspace)
> cannot determine that modalias is handled by a builtin kernel module.
>
> $ cat /sys/devices/pci0000:00/0000:00:14.0/modalias
> pci:v00008086d0000A36Dsv00001043sd00008694bc0Csc03i30
>
> $ modinfo xhci_pci
> name: xhci_pci
> filename: (builtin)
> license: GPL
> file: drivers/usb/host/xhci-pci
> description: xHCI PCI Host Controller Driver
>
> Missing modalias "pci:v*d*sv*sd*bc0Csc03i30*" which will be generated by
> modpost if the module is built separately.
>
> To fix this it is necessary to generate the same modalias for vmlinux as
> for the individual modules. Fortunately '.vmlinux.export.o' is already
> generated from which '.modinfo' can be extracted in the same way as for
> vmlinux.o.
Hi -
This patch broke RISC-V builds for me. During the final objcopy where the new
symbols are supposed to be stripped, an error occurs producing lots of error
messages similar to this one:
riscv64-linux-gnu-objcopy: not stripping symbol `__mod_device_table__...'
because it is named in a relocation
It does not occur using defconfig, but I was able to bisect my way to this
commit and then reduce my config delta w.r.t defconfig until I landed on:
cat > .config <<'EOF'
CONFIG_RELOCATABLE=y
CONFIG_KASAN=y
EOF
ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make olddefconfig
ARCH=riscv CROSS_COMPILE=riscv64-linux-gnu- make -j $(nproc)
...
LD vmlinux.unstripped
NM System.map
SORTTAB vmlinux.unstripped
CHKREL vmlinux.unstripped
OBJCOPY vmlinux
OBJCOPY modules.builtin.modinfo
GEN modules.builtin
riscv64-linux-gnu-objcopy: not stripping symbol `<long symbol name>'
because it is named in a relocation
<repeats with different symbol names about a dozen times>
make[3]: *** [scripts/Makefile.vmlinux:97: vmlinux] Error 1
make[3]: *** Deleting file 'vmlinux'
make[2]: *** [Makefile:1242: vmlinux] Error 2
make[1]: *** [/tmp/linux/Makefile:369: __build_one_by_one] Error 2
make: *** [Makefile:248: __sub-make] Error 2
I confirmed that reverting this commit fixes the issue.
>
> Signed-off-by: Masahiro Yamada <masahiroy@kernel.org>
> Signed-off-by: Alexey Gladkov <legion@kernel.org>
> Tested-by: Stephen Rothwell <sfr@canb.auug.org.au>
> ---
> include/linux/module.h | 4 ----
> scripts/Makefile.vmlinux | 4 +++-
> scripts/mksysmap | 3 +++
> scripts/mod/file2alias.c | 19 ++++++++++++++++++-
> scripts/mod/modpost.c | 15 +++++++++++++++
> scripts/mod/modpost.h | 2 ++
> 6 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index e31ee29fac6b7..e135cc79aceea 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -256,14 +256,10 @@ struct module_kobject *lookup_or_create_module_kobject(const char *name);
> __PASTE(type, \
> __PASTE(__, name)))))
>
> -#ifdef MODULE
> /* Creates an alias so file2alias.c can find device table. */
> #define MODULE_DEVICE_TABLE(type, name) \
> static typeof(name) __mod_device_table(type, name) \
> __attribute__ ((used, alias(__stringify(name))))
> -#else /* !MODULE */
> -#define MODULE_DEVICE_TABLE(type, name)
> -#endif
>
> /* Version of form [<epoch>:]<version>[-<extra-version>].
> * Or for CVS/RCS ID version, everything but the number is stripped.
> diff --git a/scripts/Makefile.vmlinux b/scripts/Makefile.vmlinux
> index ce79461714979..1e5e37aadcd05 100644
> --- a/scripts/Makefile.vmlinux
> +++ b/scripts/Makefile.vmlinux
> @@ -89,11 +89,13 @@ endif
> remove-section-y := .modinfo
> remove-section-$(CONFIG_ARCH_VMLINUX_NEEDS_RELOCS) += '.rel*'
>
> +remove-symbols := -w --strip-symbol='__mod_device_table__*'
> +
> # To avoid warnings: "empty loadable segment detected at ..." from GNU objcopy,
> # it is necessary to remove the PT_LOAD flag from the segment.
> quiet_cmd_strip_relocs = OBJCOPY $@
> cmd_strip_relocs = $(OBJCOPY) $(patsubst %,--set-section-flags %=noload,$(remove-section-y)) $< $@; \
> - $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $@
> + $(OBJCOPY) $(addprefix --remove-section=,$(remove-section-y)) $(remove-symbols) $@
>
> targets += vmlinux
> vmlinux: vmlinux.unstripped FORCE
> diff --git a/scripts/mksysmap b/scripts/mksysmap
> index a607a0059d119..c4531eacde202 100755
> --- a/scripts/mksysmap
> +++ b/scripts/mksysmap
> @@ -59,6 +59,9 @@
> # EXPORT_SYMBOL (namespace)
> / __kstrtabns_/d
>
> +# MODULE_DEVICE_TABLE (symbol name)
> +/ __mod_device_table__/d
> +
> # ---------------------------------------------------------------------------
> # Ignored suffixes
> # (do not forget '$' after each pattern)
> diff --git a/scripts/mod/file2alias.c b/scripts/mod/file2alias.c
> index 1260bc2287fba..7da9735e7ab3e 100644
> --- a/scripts/mod/file2alias.c
> +++ b/scripts/mod/file2alias.c
> @@ -1477,7 +1477,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> void *symval;
> char *zeros = NULL;
> const char *type, *name, *modname;
> - size_t typelen;
> + size_t typelen, modnamelen;
> static const char *prefix = "__mod_device_table__";
>
> /* We're looking for a section relative symbol */
> @@ -1500,6 +1500,7 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> type = strstr(modname, "__");
> if (!type)
> return;
> + modnamelen = type - modname;
> type += strlen("__");
>
> name = strstr(type, "__");
> @@ -1526,5 +1527,21 @@ void handle_moddevtable(struct module *mod, struct elf_info *info,
> }
> }
>
> + if (mod->is_vmlinux) {
> + struct module_alias *alias;
> +
> + /*
> + * If this is vmlinux, record the name of the builtin module.
> + * Traverse the linked list in the reverse order, and set the
> + * builtin_modname unless it has already been set in the
> + * previous call.
> + */
> + list_for_each_entry_reverse(alias, &mod->aliases, node) {
> + if (alias->builtin_modname)
> + break;
> + alias->builtin_modname = xstrndup(modname, modnamelen);
> + }
> + }
> +
> free(zeros);
> }
> diff --git a/scripts/mod/modpost.c b/scripts/mod/modpost.c
> index 5ca7c268294eb..47c8aa2a69392 100644
> --- a/scripts/mod/modpost.c
> +++ b/scripts/mod/modpost.c
> @@ -2067,11 +2067,26 @@ static void write_if_changed(struct buffer *b, const char *fname)
> static void write_vmlinux_export_c_file(struct module *mod)
> {
> struct buffer buf = { };
> + struct module_alias *alias, *next;
>
> buf_printf(&buf,
> "#include <linux/export-internal.h>\n");
>
> add_exported_symbols(&buf, mod);
> +
> + buf_printf(&buf,
> + "#include <linux/module.h>\n"
> + "#undef __MODULE_INFO_PREFIX\n"
> + "#define __MODULE_INFO_PREFIX\n");
> +
> + list_for_each_entry_safe(alias, next, &mod->aliases, node) {
> + buf_printf(&buf, "MODULE_INFO(%s.alias, \"%s\");\n",
> + alias->builtin_modname, alias->str);
> + list_del(&alias->node);
> + free(alias->builtin_modname);
> + free(alias);
> + }
> +
> write_if_changed(&buf, ".vmlinux.export.c");
> free(buf.p);
> }
> diff --git a/scripts/mod/modpost.h b/scripts/mod/modpost.h
> index 9133e4c3803f0..2aecb8f25c87e 100644
> --- a/scripts/mod/modpost.h
> +++ b/scripts/mod/modpost.h
> @@ -99,10 +99,12 @@ buf_write(struct buffer *buf, const char *s, int len);
> * struct module_alias - auto-generated MODULE_ALIAS()
> *
> * @node: linked to module::aliases
> + * @modname: name of the builtin module (only for vmlinux)
> * @str: a string for MODULE_ALIAS()
> */
> struct module_alias {
> struct list_head node;
> + char *builtin_modname;
> char str[];
> };
>
> --
> 2.51.0
>
^ permalink raw reply
* Re: [PATCH 1/4] PCI: Support FIXUP quirks in modules
From: Brian Norris @ 2025-10-06 22:58 UTC (permalink / raw)
To: Petr Pavlu
Cc: Bjorn Helgaas, Luis Chamberlain, Daniel Gomez, linux-pci,
David Gow, Rae Moar, linux-kselftest, linux-kernel, linux-modules,
Johannes Berg, Sami Tolvanen, Richard Weinberger, Wei Liu,
Brendan Higgins, kunit-dev, Anton Ivanov, linux-um
In-Reply-To: <2071b071-874c-4f85-8500-033c73dfaaab@suse.com>
Hi Petr,
On Wed, Sep 24, 2025 at 09:48:47AM +0200, Petr Pavlu wrote:
> On 9/23/25 7:42 PM, Brian Norris wrote:
> > Hi Petr,
> >
> > On Tue, Sep 23, 2025 at 02:55:34PM +0200, Petr Pavlu wrote:
> >> On 9/13/25 12:59 AM, Brian Norris wrote:
> >>> @@ -259,6 +315,12 @@ void pci_fixup_device(enum pci_fixup_pass pass, struct pci_dev *dev)
> >>> return;
> >>> }
> >>> pci_do_fixups(dev, start, end);
> >>> +
> >>> + struct pci_fixup_arg arg = {
> >>> + .dev = dev,
> >>> + .pass = pass,
> >>> + };
> >>> + module_for_each_mod(pci_module_fixup, &arg);
> >>
> >> The function module_for_each_mod() walks not only modules that are LIVE,
> >> but also those in the COMING and GOING states. This means that this code
> >> can potentially execute a PCI fixup from a module before its init
> >> function is invoked, and similarly, a fixup can be executed after the
> >> exit function has already run. Is this intentional?
> >
> > Thanks for the callout. I didn't really give this part much thought
> > previously.
> >
> > Per the comments, COMING means "Full formed, running module_init". I
> > believe that is a good thing, actually; specifically for controller
> > drivers, module_init() might be probing the controller and enumerating
> > child PCI devices to which we should apply these FIXUPs. That is a key
> > case to support.
> >
> > GOING is not clearly defined in the header comments, but it seems like
> > it's a relatively narrow window between determining there are no module
> > refcounts (and transition to GOING) and starting to really tear it down
> > (transitioning to UNFORMED before any significant teardown).
> > module_exit() runs in the GOING phase.
> >
> > I think it does not make sense to execute FIXUPs on a GOING module; I'll
> > make that change.
>
> Note that when walking the modules list using module_for_each_mod(),
> the delete_module() operation can concurrently transition a module to
> MODULE_STATE_GOING. If you are thinking about simply having
> pci_module_fixup() check that mod->state isn't MODULE_STATE_GOING,
> I believe this won't quite work.
Good point. I think this at least suggests that this should hook into
some blocking point in the module-load sequence, such as the notifiers
or even module_init() as you suggest below.
> > Re-quoting one piece:
> >> This means that this code
> >> can potentially execute a PCI fixup from a module before its init
> >> function is invoked,
> >
> > IIUC, this part is not true? A module is put into COMING state before
> > its init function is invoked.
>
> When loading a module, the load_module() function calls
> complete_formation(), which puts the module into the COMING state. At
> this point, the new code in pci_fixup_device() can see the new module
> and potentially attempt to invoke its PCI fixups. However, such a module
> has still a bit of way to go before its init function is called from
> do_init_module(). The module hasn't yet had its arguments parsed, is not
> linked in sysfs, isn't fully registered with codetag support, and hasn't
> invoked its constructors (needed for gcov/kasan support).
It seems unlikely that sysfs, codetag, or arguments should matter much.
gcov and kasan might be nice to have though.
> I don't know enough about PCI fixups and what is allowable in them, but
> I suspect it would be better to ensure that no fixup can be invoked from
> the module during this period.
I don't know of general rules, but they generally do pretty minimal work
to adjust various fields in and around 'struct pci_dev', to account for
broken IDs. Sometimes they need to read a few PCI registers. They may
even tweak PM-related features. It varies based
on what kind of "quriky" devices need to be handled, but it's usually
pretty straightforward and well-contained -- not relying on any kind of
global state, or even all that much specific to the module in question
besides constant IDs.
(You can peruse drivers/pci/quirks.c or the various other files that use
DECLARE_PCI_FIXUP_*() macros, if you're curious.)
> If the above makes sense, I think using module_for_each_mod() might not
> be the right approach. Alternative options include registering a module
> notifier or having modules explicitly register their PCI fixups in their
> init function.
I agree module_for_each_mod() is probably not the right choice, but I'm
not sure what the right choice is.
register_module_notifier() + keying off MODULE_STATE_COMING before
pulling in the '.pci_fixup*' list seems attractive, but it still comes
before gcov/kasan.
It seems like "first thing in module_init()" would be the right choice,
but I don't know of a great way to do that. I could insert PCI-related
calls directly into do_init_module() / delete_module(), but that doesn't
seem very elegant. I could also mess with the module_{init,exit}()
macros, but that seems a bit strange too.
I'm open to suggestions. Or else maybe I'll just go with
register_module_notifier(), and accept that there may some small
downsides still.
Thanks,
Brian
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox