* Re: [PATCH 0/2] module: restrict module auto-loading to privileged users
From: Kees Cook @ 2026-06-10 20:23 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Michal Gorlas, Jonathan Corbet, Shuah Khan, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260605183646.GC2939956@google.com>
On Fri, Jun 05, 2026 at 06:36:46PM +0000, Sami Tolvanen wrote:
> On Fri, May 15, 2026 at 07:20:18PM +0200, Michal Gorlas wrote:
> > Add option to restrict the module auto-loading to CAP_SYS_ADMIN.
> > This is heavily inspired by CONFIG_GRKERNSEC_MODHARDEN of the latest
> > available Grsecurity patches [1]. Instead of checking whether the
> > callers' UID is 0, check whether the calling process has CAP_SYS_ADMIN.
> > The reasoning here is that many modules are autoloaded by systemd
> > services which are running as privileged users, but do not have UID 0.
> > While systemd-udevd runs as root, systemd-network (which often
> > auto-loads a module) for example runs as system user (UID range 6 to
> > 999).
> >
> > When enabled, reduces attack surface where unprivileged users can trigger
> > vulnerable module to be auto-loaded, to then exploit it. Recent LPEs
> > (CopyFail [3], DirtyFrag [4]) for example, would have been mitigated
> > with this option enabled as long as the vulnerable modules are not built-in
> > (or already loaded at the point of running the exploit).
>
> This sounds potentially useful as an optional feature. Kees, you've
> looked at grsec features in the past, do you have any thoughts about
> this?
This doesn't really look like GRKERNSEC_MODHARDEN to me? In that
feature, the credentials of the usermode helper are passed down so that
udev or whatever can examine them and make choices (instead of seeing
the uid-0 usermode helper credentials).
This looks like it is just doing a request-time policy check, but that's
already covered by the security_kernel_module_request() call immediately
before the proposed module_autoload_restrict check.
Also note that module loading is _already_ controlled by CAP_SYS_MODULE,
not uid 0 nor CAP_SYS_ADMIN.
Sashiko has similar feedback, and some other notes too:
https://sashiko.dev/#/patchset/20260515-autoload_restrict-v1-0-40b7c03ddd04%409elements.com
I'm not clear what problem this patch is trying to solve?
-Kees
--
Kees Cook
^ permalink raw reply
* Re: [PATCH v2] module: use strscpy() to copy module names in stats and dup tracking
From: Petr Pavlu @ 2026-06-09 14:50 UTC (permalink / raw)
To: Naveen Kumar Chaudhary
Cc: mcgrof, da.gomez, samitolvanen, atomlin, linux-modules
In-Reply-To: <slebbzn7p6c4ybokolvey2sgvrooit66zjewwggqac2755ujgp@ptanmtdclmw5>
On 6/4/26 7:45 PM, Naveen Kumar Chaudhary wrote:
> Both try_add_failed_module() and kmod_dup_request_exists_wait() use
> memcpy() with strlen() to copy module names into fixed-size
> char[MODULE_NAME_LEN] buffers. Neither performs a bounds check on the
> copy. Current callers always pass names originating from
> mod->name (itself char[MODULE_NAME_LEN]), so this is not exploitable
> today. However both functions accept a plain const char * with no
> documented length contract, making them latent buffer overflows if a
> future caller passes a longer string.
>
> Replace memcpy() with strscpy() in both sites, which bounds the copy
> to MODULE_NAME_LEN and always NUL-terminates.
>
> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
Reviewed-by: Petr Pavlu <petr.pavlu@suse.com>
--
Thanks,
Petr
^ permalink raw reply
* Re: [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
From: Danilo Krummrich @ 2026-06-09 10:29 UTC (permalink / raw)
To: Suzuki K Poulose
Cc: Shashank Balaji, James Clark, Alexander Shishkin,
Greg Kroah-Hartman, Rafael J . Wysocki, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Jonathan Corbet, Shuah Khan,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Mike Leach, Leo Yan, Thierry Reding,
Jonathan Hunter, Rahul Bukte, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules, linux-tegra, Sumit Gupta
In-Reply-To: <1c8e441a-6b33-465a-88f9-9552f346ae18@arm.com>
On Tue Jun 9, 2026 at 11:08 AM CEST, Suzuki K Poulose wrote:
> On 08/06/2026 23:24, Danilo Krummrich wrote:
>> On Mon, 18 May 2026 19:19:56 +0900, Shashank Balaji wrote:
>>> [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
>>
>> Applied, thanks!
>>
>> Branch: driver-core-testing
>> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git
>>
>> [1/4] soc/tegra: cbb: Move driver registration from pure_initcall to core_initcall
>> commit: cd6e95e7ab29
>> [2/4] kernel: param: initialize module_kset in a pure_initcall
>> commit: c82dfce47833
>> [3/4] coresight: pass THIS_MODULE implicitly through a macro
>> commit: efc22b3f89a3
>> [4/4] driver core: platform: set mod_name in driver registration
>> commit: a7a7dc5c46a0
>>
>> The patches will appear in the next linux-next integration (typically within 24
>> hours on weekdays).
>>
>> The patches are in the driver-core-testing branch and will be promoted to
>> driver-core-next after validation.
>
> Apologies, I missed your emails. I am fine with those, happy to fixup
> anything if the linux-next screams.
Thanks for confirming! I did a test merge with linux-next and an allmodconfig
arm64 build before picking it up, so it should be fine.
Thanks,
Danilo
^ permalink raw reply
* Re: [PATCH 2/2] module: restrict autoload to CAP_SYS_ADMIN if CONFIG_MODULE_RESTRICT_AUTOLOAD
From: Michal Gorlas @ 2026-06-09 10:19 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260605183002.GB2939956@google.com>
On Fri Jun 5, 2026 at 8:30 PM CEST, Sami Tolvanen wrote:
> On Fri, May 15, 2026 at 07:20:20PM +0200, Michal Gorlas wrote:
>> Restrict module auto-loading to CAP_SYS_ADMIN if
>> CONFIG_MODULE_RESTRICT_AUTOLOAD is enabled, cmdline parameter
>> modrestrict=true, or kernel.modrestrict=1 is set with sysctl.
>>
>> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
>> ---
>> kernel/module/internal.h | 1 +
>> kernel/module/kmod.c | 5 +++++
>> kernel/module/main.c | 11 +++++++++++
>> 3 files changed, 17 insertions(+)
>>
>> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
>> index 061161cc79d9..496d8703f0c6 100644
>> --- a/kernel/module/internal.h
>> +++ b/kernel/module/internal.h
>> @@ -46,6 +46,7 @@ struct kernel_symbol {
>>
>> extern struct mutex module_mutex;
>> extern struct list_head modules;
>> +extern bool module_autoload_restrict;
>>
>> extern const struct module_attribute *const modinfo_attrs[];
>> extern const size_t modinfo_attrs_count;
>> diff --git a/kernel/module/kmod.c b/kernel/module/kmod.c
>> index a25dccdf7aa7..58b28c23f571 100644
>> --- a/kernel/module/kmod.c
>> +++ b/kernel/module/kmod.c
>> @@ -156,6 +156,11 @@ int __request_module(bool wait, const char *fmt, ...)
>> if (ret)
>> return ret;
>>
>> + if (module_autoload_restrict && !capable(CAP_SYS_ADMIN)) {
>> + pr_alert("denied attempt to auto-load module %s\n", module_name);
>
> Is pr_alert appropriate here or can this be a warning? Also, use the _ratelimited
> variant like the pre-existing warning in this function.
pr_alert was here in the grsec version (thus I assumed it makes sense
here), but agree, pr_warn_ratelimited makes more sense.
Best,
Michal
^ permalink raw reply
* Re: [PATCH 1/2] module: add CONFIG_MODULE_RESTRICT_AUTOLOAD
From: Michal Gorlas @ 2026-06-09 10:07 UTC (permalink / raw)
To: Sami Tolvanen
Cc: Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260605182517.GA2939956@google.com>
On Fri Jun 5, 2026 at 8:25 PM CEST, Sami Tolvanen wrote:
> On Fri, May 15, 2026 at 07:20:19PM +0200, Michal Gorlas wrote:
>> Add CONFIG_MODULE_RESTRICT_AUTOLOAD and modrestrict parameter
>> documentation.
>>
>> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
>> ---
>> Documentation/admin-guide/kernel-parameters.txt | 5 +++++
>> kernel/module/Kconfig | 15 +++++++++++++++
>> 2 files changed, 20 insertions(+)
>>
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 03a550630644..1013104f0943 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -4185,6 +4185,11 @@ Kernel parameters
>> For details see:
>> Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
>>
>> + modrestrict=<bool>
>> + Control the restriction of module auto-loading to
>> + CAP_SYS_ADMIN. If no <bool> value is specified, this
>> + is set to the value of CONFIG_MODULE_RESTRICT_AUTOLOAD.
>
> Doesn't this default to true if no bool value is specified? It only uses
> the config if modrestrict is not passed to the kernel at all.
Right. Will adjust the description here.
>
>> <module>.async_probe[=<bool>] [KNL]
>> If no <bool> value is specified or if the value
>> specified is not a valid <bool>, enable asynchronous
>> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
>> index 43b1bb01fd27..c9e01bb848c0 100644
>> --- a/kernel/module/Kconfig
>> +++ b/kernel/module/Kconfig
>> @@ -337,6 +337,21 @@ config MODULE_SIG_HASH
>>
>> endif # MODULE_SIG || IMA_APPRAISE_MODSIG
>>
>> +config MODULE_RESTRICT_AUTOLOAD
>> + bool "Restrict module auto-loading to privileged users"
>> + default n
>
> You don't need to specify default n here.
>
> Also, I think you can just squash the two patches. There's no benefit
> in splitting the config/documentation into a separate patch.
Alright, will squash them in v2.
Best,
Michal
^ permalink raw reply
* Re: [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
From: Suzuki K Poulose @ 2026-06-09 9:08 UTC (permalink / raw)
To: Danilo Krummrich, Shashank Balaji
Cc: James Clark, Alexander Shishkin, Greg Kroah-Hartman,
Rafael J . Wysocki, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Jonathan Corbet, Shuah Khan, Luis Chamberlain,
Petr Pavlu, Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Mike Leach,
Leo Yan, Thierry Reding, Jonathan Hunter, Rahul Bukte,
linux-kernel, coresight, linux-arm-kernel, driver-core,
rust-for-linux, linux-doc, Daniel Palmer, Tim Bird, linux-modules,
linux-tegra, Sumit Gupta
In-Reply-To: <20260608222448.1353773-1-dakr@kernel.org>
On 08/06/2026 23:24, Danilo Krummrich wrote:
> On Mon, 18 May 2026 19:19:56 +0900, Shashank Balaji wrote:
>> [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
>
> Applied, thanks!
>
> Branch: driver-core-testing
> Tree: git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git
>
> [1/4] soc/tegra: cbb: Move driver registration from pure_initcall to core_initcall
> commit: cd6e95e7ab29
> [2/4] kernel: param: initialize module_kset in a pure_initcall
> commit: c82dfce47833
> [3/4] coresight: pass THIS_MODULE implicitly through a macro
> commit: efc22b3f89a3
> [4/4] driver core: platform: set mod_name in driver registration
> commit: a7a7dc5c46a0
>
> The patches will appear in the next linux-next integration (typically within 24
> hours on weekdays).
>
> The patches are in the driver-core-testing branch and will be promoted to
> driver-core-next after validation.
Apologies, I missed your emails. I am fine with those, happy to fixup
anything if the linux-next screams.
Cheers
Suzuki
^ permalink raw reply
* Re: [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
From: Danilo Krummrich @ 2026-06-08 22:24 UTC (permalink / raw)
To: Shashank Balaji
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Greg Kroah-Hartman, Rafael J . Wysocki, Danilo Krummrich,
Miguel Ojeda, Boqun Feng, Gary Guo, Björn Roy Baron,
Benno Lossin, Andreas Hindborg, Alice Ryhl, Trevor Gross,
Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen, Aaron Tomlin, Mike Leach, Leo Yan,
Thierry Reding, Jonathan Hunter, Rahul Bukte, linux-kernel,
coresight, linux-arm-kernel, driver-core, rust-for-linux,
linux-doc, Daniel Palmer, Tim Bird, linux-modules, linux-tegra,
Sumit Gupta
In-Reply-To: <20260518-acpi_mod_name-v5-0-705ccc430885@sony.com>
On Mon, 18 May 2026 19:19:56 +0900, Shashank Balaji wrote:
> [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
Applied, thanks!
Branch: driver-core-testing
Tree: git://git.kernel.org/pub/scm/linux/kernel/git/driver-core/driver-core.git
[1/4] soc/tegra: cbb: Move driver registration from pure_initcall to core_initcall
commit: cd6e95e7ab29
[2/4] kernel: param: initialize module_kset in a pure_initcall
commit: c82dfce47833
[3/4] coresight: pass THIS_MODULE implicitly through a macro
commit: efc22b3f89a3
[4/4] driver core: platform: set mod_name in driver registration
commit: a7a7dc5c46a0
The patches will appear in the next linux-next integration (typically within 24
hours on weekdays).
The patches are in the driver-core-testing branch and will be promoted to
driver-core-next after validation.
^ permalink raw reply
* Re: [PATCH v5 0/4] Enable sysfs module symlink for more built-in drivers
From: Danilo Krummrich @ 2026-06-08 22:16 UTC (permalink / raw)
To: Shashank Balaji
Cc: Suzuki K Poulose, James Clark, Alexander Shishkin,
Greg Kroah-Hartman, Rafael J. Wysocki, Miguel Ojeda, Boqun Feng,
Gary Guo, Björn Roy Baron, Benno Lossin, Andreas Hindborg,
Alice Ryhl, Trevor Gross, Jonathan Corbet, Shuah Khan,
Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Mike Leach, Leo Yan, Thierry Reding,
Jonathan Hunter, Rahul Bukte, linux-kernel, coresight,
linux-arm-kernel, driver-core, rust-for-linux, linux-doc,
Daniel Palmer, Tim Bird, linux-modules, linux-tegra, Sumit Gupta
In-Reply-To: <20260518-acpi_mod_name-v5-0-705ccc430885@sony.com>
On Mon May 18, 2026 at 12:19 PM CEST, Shashank Balaji wrote:
> Shashank Balaji (4):
> soc/tegra: cbb: Move driver registration from pure_initcall to core_initcall
> kernel: param: initialize module_kset in a pure_initcall
> coresight: pass THIS_MODULE implicitly through a macro
> driver core: platform: set mod_name in driver registration
Picking this up now, so it can still make it for 7.2-rc1 and get some time in
linux-next.
Suzuki, since I haven't heard back I figured it should be fine to also pick the
coresight change as it is purely mechanic and driver-core motivated, but please
let me know if you have any concerns.
Thanks,
Danilo
^ permalink raw reply
* Re: [PATCH 1/2] xtensa/simdisk: Avoid referring to module::args
From: Max Filippov @ 2026-06-07 20:44 UTC (permalink / raw)
To: Petr Pavlu
Cc: Chris Zankel, Luis Chamberlain, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Matthew Wood, linux-modules, linux-kernel
In-Reply-To: <20260604110455.1608038-2-petr.pavlu@suse.com>
On Thu, Jun 4, 2026 at 4:05 AM Petr Pavlu <petr.pavlu@suse.com> wrote:
>
> When simdisk support is built as a loadable module,
> simdisk_param_set_filename() receives a pointer into module::args and
> stores each filename pointer as is.
>
> In preparation for removing module::args, update the simdisk.filename
> parameter code to copy the provided string. This is somewhat complicated by
> the fact that simdisk support can also be built-in, in which case the
> parameters are parsed during early boot before slab is available. In that
> case, the command line itself is preserved for the lifetime of the kernel,
> so continue storing the incoming pointer directly.
>
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
> arch/xtensa/platforms/iss/simdisk.c | 38 +++++++++++++++++++++++++----
> 1 file changed, 33 insertions(+), 5 deletions(-)
Reviewed-by: Max Filippov <jcmvbkbc@gmail.com>
--
Thanks.
-- Max
^ permalink raw reply
* [PATCH v2] module: procfs: fix signed integer overflow in module_total_size()
From: Naveen Kumar Chaudhary @ 2026-06-07 4:18 UTC (permalink / raw)
To: samitolvanen; +Cc: mcgrof, petr.pavlu, da.gomez, atomlin, linux-modules
In-Reply-To: <20260605205215.GA3054112@google.com>
module_total_size() returns unsigned int but uses a signed int
accumulator. While the result is numerically correct, the type
mismatch is misleading.
Change the accumulator to unsigned int to match the return type.
Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---
Changes in v2:
- Modify the commit message
Thanks Sami for the catch. I have corrected the commit message.
Regards,
Naveen
kernel/module/procfs.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/module/procfs.c b/kernel/module/procfs.c
index 0a4841e88adb..90712aa9dd13 100644
--- a/kernel/module/procfs.c
+++ b/kernel/module/procfs.c
@@ -64,7 +64,7 @@ static void m_stop(struct seq_file *m, void *p)
static unsigned int module_total_size(struct module *mod)
{
- int size = 0;
+ unsigned int size = 0;
for_each_mod_mem_type(type)
size += mod->mem[type].size;
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] rust: module_param: use pr_warn_once! for null pointer warning
From: Miguel Ojeda @ 2026-06-06 20:15 UTC (permalink / raw)
To: Maurice Hieronymus
Cc: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich, linux-modules, linux-kernel,
rust-for-linux
In-Reply-To: <20260606-module-param-pr-warn-once-v1-1-96d9a93e8a1b@mailbox.org>
On Sat, Jun 6, 2026 at 8:24 PM Maurice Hieronymus <mhi@mailbox.org> wrote:
>
> This resolves the existing TODO that asked for pr_warn_once! once
> it became available.
>
> Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
Thanks for the patch!
This was already sent and applied, please see commit:
ac2f40107cf1 ("rust: module_param: use `pr_warn_once!` for null
pointer warning")
in linux-next.
Cheers,
Miguel
^ permalink raw reply
* [PATCH] rust: module_param: use pr_warn_once! for null pointer warning
From: Maurice Hieronymus @ 2026-06-06 18:24 UTC (permalink / raw)
To: Luis Chamberlain, Petr Pavlu, Daniel Gomez, Sami Tolvanen,
Aaron Tomlin, Miguel Ojeda, Boqun Feng, Gary Guo,
Björn Roy Baron, Benno Lossin, Andreas Hindborg, Alice Ryhl,
Trevor Gross, Danilo Krummrich
Cc: linux-modules, linux-kernel, rust-for-linux, Maurice Hieronymus
This resolves the existing TODO that asked for pr_warn_once! once
it became available.
Signed-off-by: Maurice Hieronymus <mhi@mailbox.org>
---
rust/kernel/module_param.rs | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/rust/kernel/module_param.rs b/rust/kernel/module_param.rs
index 6a8a7a875643..dd6d663a0a3c 100644
--- a/rust/kernel/module_param.rs
+++ b/rust/kernel/module_param.rs
@@ -62,8 +62,7 @@ pub trait ModuleParam: Sized + Copy {
// NOTE: If we start supporting arguments without values, val _is_ allowed
// to be null here.
if val.is_null() {
- // TODO: Use pr_warn_once available.
- crate::pr_warn!("Null pointer passed to `module_param::set_param`");
+ crate::pr_warn_once!("Null pointer passed to `module_param::set_param`");
return EINVAL.to_errno();
}
---
base-commit: 838a0871cb3cf5988560d17afaaf57592bdb486b
change-id: 20260606-module-param-pr-warn-once-ed2f548ea741
Best regards,
--
Maurice Hieronymus <mhi@mailbox.org>
^ permalink raw reply related
* Re: [PATCH] module: procfs: fix signed integer overflow in module_total_size()
From: Sami Tolvanen @ 2026-06-05 20:52 UTC (permalink / raw)
To: Naveen Kumar Chaudhary
Cc: mcgrof, petr.pavlu, da.gomez, atomlin, linux-modules,
linux-kernel
In-Reply-To: <2k7x7245kygzpa5wzssebsrt5ivpykvvoeiljb5at2yiwgp4fy@fqkcethe4hyx>
On Wed, Jun 03, 2026 at 09:54:11PM +0530, Naveen Kumar Chaudhary wrote:
> module_total_size() accumulates unsigned section sizes into a signed int
> before returning as unsigned int. If the total exceeds INT_MAX, this is
> signed integer overflow.
This doesn't sound accurate to me. The compiler performs an implicit
type conversion, but there's no signed integer overflow:
https://godbolt.org/z/hzGrYMsPW
> Change the accumulator to unsigned int to match the return type.
>
> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> ---
> kernel/module/procfs.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/module/procfs.c b/kernel/module/procfs.c
> index 0a4841e88adb..90712aa9dd13 100644
> --- a/kernel/module/procfs.c
> +++ b/kernel/module/procfs.c
> @@ -64,7 +64,7 @@ static void m_stop(struct seq_file *m, void *p)
>
> static unsigned int module_total_size(struct module *mod)
> {
> - int size = 0;
> + unsigned int size = 0;
While there's no behavioral difference, using unsigned int seems like
good hygiene. With the commit message corrected:
Reviewed-by: Sami Tolvanen <samitolvanen@google.com>
Sami
^ permalink raw reply
* Re: [PATCH 0/2] module: restrict module auto-loading to privileged users
From: Sami Tolvanen @ 2026-06-05 18:36 UTC (permalink / raw)
To: Michal Gorlas, Kees Cook
Cc: Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260515-autoload_restrict-v1-0-40b7c03ddd04@9elements.com>
On Fri, May 15, 2026 at 07:20:18PM +0200, Michal Gorlas wrote:
> Add option to restrict the module auto-loading to CAP_SYS_ADMIN.
> This is heavily inspired by CONFIG_GRKERNSEC_MODHARDEN of the latest
> available Grsecurity patches [1]. Instead of checking whether the
> callers' UID is 0, check whether the calling process has CAP_SYS_ADMIN.
> The reasoning here is that many modules are autoloaded by systemd
> services which are running as privileged users, but do not have UID 0.
> While systemd-udevd runs as root, systemd-network (which often
> auto-loads a module) for example runs as system user (UID range 6 to
> 999).
>
> When enabled, reduces attack surface where unprivileged users can trigger
> vulnerable module to be auto-loaded, to then exploit it. Recent LPEs
> (CopyFail [3], DirtyFrag [4]) for example, would have been mitigated
> with this option enabled as long as the vulnerable modules are not built-in
> (or already loaded at the point of running the exploit).
This sounds potentially useful as an optional feature. Kees, you've
looked at grsec features in the past, do you have any thoughts about
this?
Sami
^ permalink raw reply
* Re: [PATCH 2/2] module: restrict autoload to CAP_SYS_ADMIN if CONFIG_MODULE_RESTRICT_AUTOLOAD
From: Sami Tolvanen @ 2026-06-05 18:30 UTC (permalink / raw)
To: Michal Gorlas
Cc: Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260515-autoload_restrict-v1-2-40b7c03ddd04@9elements.com>
On Fri, May 15, 2026 at 07:20:20PM +0200, Michal Gorlas wrote:
> Restrict module auto-loading to CAP_SYS_ADMIN if
> CONFIG_MODULE_RESTRICT_AUTOLOAD is enabled, cmdline parameter
> modrestrict=true, or kernel.modrestrict=1 is set with sysctl.
>
> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
> ---
> kernel/module/internal.h | 1 +
> kernel/module/kmod.c | 5 +++++
> kernel/module/main.c | 11 +++++++++++
> 3 files changed, 17 insertions(+)
>
> diff --git a/kernel/module/internal.h b/kernel/module/internal.h
> index 061161cc79d9..496d8703f0c6 100644
> --- a/kernel/module/internal.h
> +++ b/kernel/module/internal.h
> @@ -46,6 +46,7 @@ struct kernel_symbol {
>
> extern struct mutex module_mutex;
> extern struct list_head modules;
> +extern bool module_autoload_restrict;
>
> extern const struct module_attribute *const modinfo_attrs[];
> extern const size_t modinfo_attrs_count;
> diff --git a/kernel/module/kmod.c b/kernel/module/kmod.c
> index a25dccdf7aa7..58b28c23f571 100644
> --- a/kernel/module/kmod.c
> +++ b/kernel/module/kmod.c
> @@ -156,6 +156,11 @@ int __request_module(bool wait, const char *fmt, ...)
> if (ret)
> return ret;
>
> + if (module_autoload_restrict && !capable(CAP_SYS_ADMIN)) {
> + pr_alert("denied attempt to auto-load module %s\n", module_name);
Is pr_alert appropriate here or can this be a warning? Also, use the _ratelimited
variant like the pre-existing warning in this function.
Sami
^ permalink raw reply
* Re: [PATCH 1/2] module: add CONFIG_MODULE_RESTRICT_AUTOLOAD
From: Sami Tolvanen @ 2026-06-05 18:25 UTC (permalink / raw)
To: Michal Gorlas
Cc: Jonathan Corbet, Shuah Khan, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Aaron Tomlin, linux-doc, linux-kernel,
linux-modules
In-Reply-To: <20260515-autoload_restrict-v1-1-40b7c03ddd04@9elements.com>
On Fri, May 15, 2026 at 07:20:19PM +0200, Michal Gorlas wrote:
> Add CONFIG_MODULE_RESTRICT_AUTOLOAD and modrestrict parameter
> documentation.
>
> Signed-off-by: Michal Gorlas <michal.gorlas@9elements.com>
> ---
> Documentation/admin-guide/kernel-parameters.txt | 5 +++++
> kernel/module/Kconfig | 15 +++++++++++++++
> 2 files changed, 20 insertions(+)
>
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 03a550630644..1013104f0943 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4185,6 +4185,11 @@ Kernel parameters
> For details see:
> Documentation/admin-guide/hw-vuln/processor_mmio_stale_data.rst
>
> + modrestrict=<bool>
> + Control the restriction of module auto-loading to
> + CAP_SYS_ADMIN. If no <bool> value is specified, this
> + is set to the value of CONFIG_MODULE_RESTRICT_AUTOLOAD.
Doesn't this default to true if no bool value is specified? It only uses
the config if modrestrict is not passed to the kernel at all.
> <module>.async_probe[=<bool>] [KNL]
> If no <bool> value is specified or if the value
> specified is not a valid <bool>, enable asynchronous
> diff --git a/kernel/module/Kconfig b/kernel/module/Kconfig
> index 43b1bb01fd27..c9e01bb848c0 100644
> --- a/kernel/module/Kconfig
> +++ b/kernel/module/Kconfig
> @@ -337,6 +337,21 @@ config MODULE_SIG_HASH
>
> endif # MODULE_SIG || IMA_APPRAISE_MODSIG
>
> +config MODULE_RESTRICT_AUTOLOAD
> + bool "Restrict module auto-loading to privileged users"
> + default n
You don't need to specify default n here.
Also, I think you can just squash the two patches. There's no benefit
in splitting the config/documentation into a separate patch.
Sami
^ permalink raw reply
* Re: [PATCH v2] module: use strscpy() to copy module names in stats and dup tracking
From: sashiko-bot @ 2026-06-04 18:19 UTC (permalink / raw)
To: Naveen Kumar Chaudhary; +Cc: linux-modules
In-Reply-To: <slebbzn7p6c4ybokolvey2sgvrooit66zjewwggqac2755ujgp@ptanmtdclmw5>
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Medium] Incomplete fix: Truncating overlong module names breaks deduplication and causes list bloat/memory leak.
Pre-existing issues:
- [High] Race condition in duplicate module load tracking causing Use-After-Free.
--
commit 609988fdef836043f78467817ec8e3cba34b5daf
Author: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
module: use strscpy() to copy module names in stats and dup tracking
Both try_add_failed_module() and kmod_dup_request_exists_wait() use
memcpy() with strlen() to copy module names into fixed-size buffers.
Neither performs a bounds check on the copy. This patch replaces
memcpy() with strscpy() to ensure the copy is bounded and NUL-terminated.
> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba0..8b6f8f177de4f 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
> @@ -129,7 +129,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
> if (!new_kmod_req)
> return false;
>
> - memcpy(new_kmod_req->name, module_name, strlen(module_name));
> + strscpy(new_kmod_req->name, module_name);
[Severity: High]
This is a pre-existing issue, but looking at kmod_dup_request_exists_wait(),
is there a race condition that could lead to a use-after-free?
In kmod_dup_request_exists_wait(), the kmod_req pointer is retrieved while
holding kmod_dup_mutex, but the lock is dropped before waiting on the
completion:
kernel/module/dups.c:kmod_dup_request_exists_wait() {
...
mutex_unlock(&kmod_dup_mutex);
...
ret = wait_for_completion_state(&kmod_req->first_req_done, TASK_KILLABLE);
if (ret) {
*dup_ret = ret;
return true;
}
...
*dup_ret = kmod_req->dup_ret;
...
}
There doesn't appear to be a reference count or RCU read-side critical section
protecting kmod_req during this wait.
The completion is signaled by kmod_dup_request_complete(), which then
schedules kmod_dup_request_delete() via delayed work after 60 seconds.
If a duplicate request arrives exactly 60 seconds later, it could drop the
mutex just before the delayed work executes and frees the request:
kernel/module/dups.c:kmod_dup_request_delete() {
...
mutex_lock(&kmod_dup_mutex);
list_del_rcu(&kmod_req->list);
synchronize_rcu();
mutex_unlock(&kmod_dup_mutex);
kfree(kmod_req);
}
When the duplicate thread resumes and accesses kmod_req->dup_ret, would it
dereference the freed memory?
[ ... ]
> diff --git a/kernel/module/stats.c b/kernel/module/stats.c
> index 3a9672f93a8ef..78b59bc05387f 100644
> --- a/kernel/module/stats.c
> +++ b/kernel/module/stats.c
> @@ -253,7 +253,7 @@ int try_add_failed_module(const char *name, enum fail_dup_mod_reason reason)
> mod_fail = kzalloc_obj(*mod_fail);
> if (!mod_fail)
> return -ENOMEM;
> - memcpy(mod_fail->name, name, strlen(name));
> + strscpy(mod_fail->name, name);
[Severity: Medium]
Does truncating overlong module names here break deduplication?
If try_add_failed_module() is called with an overlong name (which can
originate from the .modinfo ELF section), strscpy() safely truncates the
string stored in mod_fail->name.
However, the deduplication lookup uses strcmp(), which compares the truncated
stored name against the untruncated original string:
kernel/module/stats.c:try_add_failed_module() {
...
list_for_each_entry_rcu(mod_fail, &dup_failed_modules, list, lockdep_is_held(&module_mutex)) {
if (!strcmp(mod_fail->name, name)) {
atomic_long_inc(&mod_fail->count);
...
}
This comparison will always fail for overlong names.
Would every attempt to load a malformed module allocate a new mod_fail struct
and add it to dup_failed_modules, potentially allowing an attacker with
CAP_SYS_MODULE to trigger unbounded list bloat and a memory leak?
> __set_bit(reason, &mod_fail->dup_fail_mask);
> atomic_long_inc(&mod_fail->count);
> list_add_rcu(&mod_fail->list, &dup_failed_modules);
--
Sashiko AI review · https://sashiko.dev/#/patchset/slebbzn7p6c4ybokolvey2sgvrooit66zjewwggqac2755ujgp@ptanmtdclmw5?part=1
^ permalink raw reply
* [PATCH v2] module: use strscpy() to copy module names in stats and dup tracking
From: Naveen Kumar Chaudhary @ 2026-06-04 17:45 UTC (permalink / raw)
To: petr.pavlu; +Cc: mcgrof, da.gomez, samitolvanen, atomlin, linux-modules
In-Reply-To: <59c7b5d1-643b-4ca5-94ad-eaf82a66fc9e@suse.com>
Both try_add_failed_module() and kmod_dup_request_exists_wait() use
memcpy() with strlen() to copy module names into fixed-size
char[MODULE_NAME_LEN] buffers. Neither performs a bounds check on the
copy. Current callers always pass names originating from
mod->name (itself char[MODULE_NAME_LEN]), so this is not exploitable
today. However both functions accept a plain const char * with no
documented length contract, making them latent buffer overflows if a
future caller passes a longer string.
Replace memcpy() with strscpy() in both sites, which bounds the copy
to MODULE_NAME_LEN and always NUL-terminates.
Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
---
v1 -> v2:
- Dropped third argument to strscpy
- Merged the other patch https://lore.kernel.org/linux-modules/jmm7r4r3k3qt767tl7lojglosgc3umhc63cdp2fckdkgb3fzki@3fgvxgvzo5ex/
Thanks Petr for the reviews. Taken care of the suggestions.
Regards,
Naveen
kernel/module/dups.c | 2 +-
kernel/module/stats.c | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/kernel/module/dups.c b/kernel/module/dups.c
index 1d720a5311ba..33bddfb57317 100644
--- a/kernel/module/dups.c
+++ b/kernel/module/dups.c
@@ -129,7 +129,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
if (!new_kmod_req)
return false;
- memcpy(new_kmod_req->name, module_name, strlen(module_name));
+ strscpy(new_kmod_req->name, module_name);
INIT_WORK(&new_kmod_req->complete_work, kmod_dup_request_complete);
INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
init_completion(&new_kmod_req->first_req_done);
diff --git a/kernel/module/stats.c b/kernel/module/stats.c
index 3a9672f93a8e..08724baca773 100644
--- a/kernel/module/stats.c
+++ b/kernel/module/stats.c
@@ -253,7 +253,7 @@ int try_add_failed_module(const char *name, enum fail_dup_mod_reason reason)
mod_fail = kzalloc_obj(*mod_fail);
if (!mod_fail)
return -ENOMEM;
- memcpy(mod_fail->name, name, strlen(name));
+ strscpy(mod_fail->name, name);
__set_bit(reason, &mod_fail->dup_fail_mask);
atomic_long_inc(&mod_fail->count);
list_add_rcu(&mod_fail->list, &dup_failed_modules);
--
2.43.0
^ permalink raw reply related
* Re: [PATCH] module: stats: add lockdep annotation for dup_failed_modules list traversal
From: Naveen Kumar Chaudhary @ 2026-06-04 17:19 UTC (permalink / raw)
To: Petr Pavlu
Cc: mcgrof, da.gomez, samitolvanen, atomlin, linux-modules,
linux-kernel
In-Reply-To: <2d797a44-bdd4-4b37-8bdd-2e2052c6dd38@suse.com>
Thanks Petr. Noted.
On Thu 04 Jun 10:40 AM, Petr Pavlu wrote:
> On 6/3/26 6:22 PM, Naveen Kumar Chaudhary wrote:
> > read_file_mod_stats() traverses dup_failed_modules with
> > list_for_each_entry_rcu() while holding module_mutex, but does not pass
> > the lockdep condition. This triggers a false-positive "RCU-list traversed
> > in non-reader section" warning with CONFIG_PROVE_RCU_LIST=y.
> >
> > The warning can be reproduced by:
> > 1. Racing two loads of the same module to populate dup_failed_modules:
> > insmod dummy.ko &
> > insmod dummy.ko &
> > 2. Reading the stats debugfs file:
> > cat /sys/kernel/debug/modules/stats
> >
> > =============================
> > WARNING: suspicious RCU usage
> > 7.1.0-rc5-gae12a56ba16a #1 Not tainted
> > -----------------------------
> > kernel/module/stats.c:385 RCU-list traversed in non-reader section!!
> >
> > other info that might help us debug this:
> >
> > rcu_scheduler_active = 2, debug_locks = 1
> > 1 lock held by cat/128:
> > #0: ffff80008288f7a8 (module_mutex){+.+.}-{4:4}, at: read_file_mod_stats+0x46c/0x5ec
> >
> > stack backtrace:
> > CPU: 1 UID: 0 PID: 128 Comm: cat Kdump: loaded Not tainted 7.1.0-rc5-gae12a56ba16a #1 PREEMPT
> > Hardware name: linux,dummy-virt (DT)
> > Call trace:
> > show_stack+0x18/0x24 (C)
> > __dump_stack+0x28/0x38
> > dump_stack_lvl+0x64/0x84
> > dump_stack+0x18/0x24
> > lockdep_rcu_suspicious+0x134/0x1d4
> > read_file_mod_stats+0x554/0x5ec
> > full_proxy_read+0xe0/0x1ac
> > vfs_read+0xd8/0x2b0
> > ksys_read+0x70/0xe4
> > __arm64_sys_read+0x1c/0x28
> > invoke_syscall+0x48/0xf8
> > el0_svc_common+0x8c/0xd8
> > do_el0_svc+0x1c/0x28
> > el0_svc+0x58/0x1d8
> > el0t_64_sync_handler+0x84/0x12c
> > el0t_64_sync+0x198/0x19c
> >
> > The traversal is protected by module_mutex, so pass
> > lockdep_is_held(&module_mutex) to inform the checker.
> >
> > Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> > ---
> > kernel/module/stats.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/kernel/module/stats.c b/kernel/module/stats.c
> > index 3a9672f93a8e..a62961acd8e3 100644
> > --- a/kernel/module/stats.c
> > +++ b/kernel/module/stats.c
> > @@ -382,7 +382,8 @@ static ssize_t read_file_mod_stats(struct file *file, char __user *user_buf,
> > mutex_lock(&module_mutex);
> >
> >
> > - list_for_each_entry_rcu(mod_fail, &dup_failed_modules, list) {
> > + list_for_each_entry_rcu(mod_fail, &dup_failed_modules, list,
> > + lockdep_is_held(&module_mutex)) {
> > if (WARN_ON_ONCE(++count_failed >= MAX_FAILED_MOD_PRINT))
> > goto out_unlock;
> > len += scnprintf(buf + len, size - len, "%25s\t%15lu\t%25s\n", mod_fail->name,
>
> I mentioned in my reply to your previous patch fixing the use of
> synchronize_rcu() in the module dups code [1] that the overall RCU usage
> in this code appears to be incorrect. I also noticed that Sashiko
> reported the same issue. I think it is not very productive to try to fix
> these specific RCU-related problems and instead the code should be
> properly reworked. It most likely should not be using RCU at all and
> kmod_dup_req should instead be reference-counted.
>
> [1] https://lore.kernel.org/linux-modules/ajydyxgaea27rhcopp5eauji24znotu65d2b4uw344yvmwcc6f@7l5re6f2xcuk/
>
> --
> Thanks,
> Petr
^ permalink raw reply
* Re: [PATCH 2/2] module: Remove unnecessary module::args
From: Aaron Tomlin @ 2026-06-04 13:25 UTC (permalink / raw)
To: Petr Pavlu
Cc: Chris Zankel, Max Filippov, Luis Chamberlain, Daniel Gomez,
Sami Tolvanen, Matthew Wood, linux-modules, linux-kernel
In-Reply-To: <20260604110455.1608038-3-petr.pavlu@suse.com>
On Thu, Jun 04, 2026 at 01:04:46PM +0200, Petr Pavlu wrote:
> Historically, various parameter-handling code kept pointers into
> module::args, most notably the charp support. However, in 2009,
> commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
> charp parameters to kstrdup() the input string as well. As a result,
> module::args now mostly wastes memory.
>
> The last users that still pointed into module::args have now been cleaned
> up, so remove this data.
>
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
> include/linux/module.h | 4 ----
> kernel/module/main.c | 15 ++++++++-------
> 2 files changed, 8 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 7566815fabbe..96cc98568eea 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -477,10 +477,6 @@ struct module {
> struct module_notes_attrs *notes_attrs;
> #endif
>
> - /* The command line arguments (may be mangled). People like
> - keeping pointers to this stuff */
> - char *args;
> -
> #ifdef CONFIG_SMP
> /* Per-cpu data. */
> void __percpu *percpu;
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 46dd8d25a605..528690ba160b 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1458,7 +1458,6 @@ static void free_module(struct module *mod)
>
> /* This may be empty, but that's OK */
> module_arch_freeing_init(mod);
> - kfree(mod->args);
> percpu_modfree(mod);
>
> free_mod_mem(mod);
> @@ -3425,7 +3424,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> struct module *mod;
> bool module_allocated = false;
> long err = 0;
> - char *after_dashes;
> + char *args = NULL, *after_dashes;
>
> /*
> * Do the signature check (if any) first. All that
> @@ -3523,9 +3522,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
> flush_module_icache(mod);
>
> /* Now copy in args */
> - mod->args = strndup_user(uargs, ~0UL >> 1);
> - if (IS_ERR(mod->args)) {
> - err = PTR_ERR(mod->args);
> + args = strndup_user(uargs, ~0UL >> 1);
> + if (IS_ERR(args)) {
> + err = PTR_ERR(args);
> goto free_arch_cleanup;
> }
>
> @@ -3546,7 +3545,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> mod->async_probe_requested = async_probe;
>
> /* Module is ready to execute: parsing args may do that. */
> - after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
> + after_dashes = parse_args(mod->name, args, mod->kp, mod->num_kp,
> -32768, 32767, mod,
> unknown_module_param_cb);
> if (IS_ERR(after_dashes)) {
> @@ -3556,6 +3555,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
> pr_warn("%s: parameters '%s' after `--' ignored\n",
> mod->name, after_dashes);
> }
> + kfree(args);
> + args = NULL;
>
> /* Link in to sysfs. */
> err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
> @@ -3597,7 +3598,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
> ddebug_cleanup:
> ftrace_release_mod(mod);
> synchronize_rcu();
> - kfree(mod->args);
> + kfree(args);
> free_arch_cleanup:
> module_arch_cleanup(mod);
> free_modinfo:
> --
> 2.54.0
>
LGTM.
Reviewed-by: Aaron Tomlin <atomlin@atomlin.com>
--
Aaron Tomlin
^ permalink raw reply
* Re: [PATCH 1/2] xtensa/simdisk: Avoid referring to module::args
From: sashiko-bot @ 2026-06-04 11:18 UTC (permalink / raw)
To: Petr Pavlu; +Cc: linux-modules
In-Reply-To: <20260604110455.1608038-2-petr.pavlu@suse.com>
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Concurrent access to `dev->filename` in `proc_read_simdisk` and `simdisk_detach` causes a Use-After-Free.
--
commit e0a519c0b452f39fc91f2ac38f46b81a32db738a
Author: Petr Pavlu <petr.pavlu@suse.com>
xtensa/simdisk: Avoid referring to module::args
This commit prepares for the removal of module::args by copying the filename
string provided as a parameter instead of storing a direct pointer when built
as a loadable module. It also adds a free function to clean up these copied
strings upon module unload.
> diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
> index 7c7a2aa749f82..a2ddb49c269dc 100644
> --- a/arch/xtensa/platforms/iss/simdisk.c
> +++ b/arch/xtensa/platforms/iss/simdisk.c
[ ... ]
> static const struct kernel_param_ops simdisk_param_ops_filename = {
> .set = simdisk_param_set_filename,
> + .free = simdisk_param_free_filename,
> };
> -module_param_cb(filename, &simdisk_param_ops_filename, &n_files, 0);
> +module_param_cb(filename, &simdisk_param_ops_filename, NULL, 0);
> MODULE_PARM_DESC(filename, "Backing storage filename.");
>
> static int simdisk_major = SIMDISK_MAJOR;
[Severity: High]
This isn't a bug introduced by this patch, but is it possible to trigger a
use-after-free here due to concurrent access to dev->filename?
The procfs entry for simdisk devices is created with 0644 permissions, allowing
any user to read it. In proc_read_simdisk(), dev->filename is fetched without
acquiring dev->lock:
arch/xtensa/platforms/iss/simdisk.c:proc_read_simdisk() {
...
const char *s = dev->filename;
if (s) {
ssize_t len = strlen(s);
char *temp = kmalloc(len + 2, GFP_KERNEL);
...
}
If an administrator concurrently writes to the same procfs entry, it triggers
proc_write_simdisk() -> simdisk_detach(), which acquires dev->lock and frees
the filename:
arch/xtensa/platforms/iss/simdisk.c:simdisk_detach() {
...
spin_lock(&dev->lock);
...
} else {
pr_info("SIMDISK: %s detached from %s\n",
dev->gd->disk_name, dev->filename);
dev->fd = -1;
kfree(dev->filename);
dev->filename = NULL;
}
...
}
If proc_read_simdisk() fetches the pointer right before simdisk_detach() frees
it, the subsequent strlen() and scnprintf() operations could dereference freed
memory. Does this need locking or RCU protection to prevent a potential oops
or local information leak?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260604110455.1608038-1-petr.pavlu@suse.com?part=1
^ permalink raw reply
* [PATCH 2/2] module: Remove unnecessary module::args
From: Petr Pavlu @ 2026-06-04 11:04 UTC (permalink / raw)
To: Chris Zankel, Max Filippov, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen
Cc: Aaron Tomlin, Matthew Wood, linux-modules, linux-kernel
In-Reply-To: <20260604110455.1608038-1-petr.pavlu@suse.com>
Historically, various parameter-handling code kept pointers into
module::args, most notably the charp support. However, in 2009,
commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
charp parameters to kstrdup() the input string as well. As a result,
module::args now mostly wastes memory.
The last users that still pointed into module::args have now been cleaned
up, so remove this data.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
include/linux/module.h | 4 ----
kernel/module/main.c | 15 ++++++++-------
2 files changed, 8 insertions(+), 11 deletions(-)
diff --git a/include/linux/module.h b/include/linux/module.h
index 7566815fabbe..96cc98568eea 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -477,10 +477,6 @@ struct module {
struct module_notes_attrs *notes_attrs;
#endif
- /* The command line arguments (may be mangled). People like
- keeping pointers to this stuff */
- char *args;
-
#ifdef CONFIG_SMP
/* Per-cpu data. */
void __percpu *percpu;
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 46dd8d25a605..528690ba160b 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -1458,7 +1458,6 @@ static void free_module(struct module *mod)
/* This may be empty, but that's OK */
module_arch_freeing_init(mod);
- kfree(mod->args);
percpu_modfree(mod);
free_mod_mem(mod);
@@ -3425,7 +3424,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
struct module *mod;
bool module_allocated = false;
long err = 0;
- char *after_dashes;
+ char *args = NULL, *after_dashes;
/*
* Do the signature check (if any) first. All that
@@ -3523,9 +3522,9 @@ static int load_module(struct load_info *info, const char __user *uargs,
flush_module_icache(mod);
/* Now copy in args */
- mod->args = strndup_user(uargs, ~0UL >> 1);
- if (IS_ERR(mod->args)) {
- err = PTR_ERR(mod->args);
+ args = strndup_user(uargs, ~0UL >> 1);
+ if (IS_ERR(args)) {
+ err = PTR_ERR(args);
goto free_arch_cleanup;
}
@@ -3546,7 +3545,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
mod->async_probe_requested = async_probe;
/* Module is ready to execute: parsing args may do that. */
- after_dashes = parse_args(mod->name, mod->args, mod->kp, mod->num_kp,
+ after_dashes = parse_args(mod->name, args, mod->kp, mod->num_kp,
-32768, 32767, mod,
unknown_module_param_cb);
if (IS_ERR(after_dashes)) {
@@ -3556,6 +3555,8 @@ static int load_module(struct load_info *info, const char __user *uargs,
pr_warn("%s: parameters '%s' after `--' ignored\n",
mod->name, after_dashes);
}
+ kfree(args);
+ args = NULL;
/* Link in to sysfs. */
err = mod_sysfs_setup(mod, info, mod->kp, mod->num_kp);
@@ -3597,7 +3598,7 @@ static int load_module(struct load_info *info, const char __user *uargs,
ddebug_cleanup:
ftrace_release_mod(mod);
synchronize_rcu();
- kfree(mod->args);
+ kfree(args);
free_arch_cleanup:
module_arch_cleanup(mod);
free_modinfo:
--
2.54.0
^ permalink raw reply related
* [PATCH 1/2] xtensa/simdisk: Avoid referring to module::args
From: Petr Pavlu @ 2026-06-04 11:04 UTC (permalink / raw)
To: Chris Zankel, Max Filippov, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen
Cc: Aaron Tomlin, Matthew Wood, linux-modules, linux-kernel
In-Reply-To: <20260604110455.1608038-1-petr.pavlu@suse.com>
When simdisk support is built as a loadable module,
simdisk_param_set_filename() receives a pointer into module::args and
stores each filename pointer as is.
In preparation for removing module::args, update the simdisk.filename
parameter code to copy the provided string. This is somewhat complicated by
the fact that simdisk support can also be built-in, in which case the
parameters are parsed during early boot before slab is available. In that
case, the command line itself is preserved for the lifetime of the kernel,
so continue storing the incoming pointer directly.
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
arch/xtensa/platforms/iss/simdisk.c | 38 +++++++++++++++++++++++++----
1 file changed, 33 insertions(+), 5 deletions(-)
diff --git a/arch/xtensa/platforms/iss/simdisk.c b/arch/xtensa/platforms/iss/simdisk.c
index 7c7a2aa749f8..a2ddb49c269d 100644
--- a/arch/xtensa/platforms/iss/simdisk.c
+++ b/arch/xtensa/platforms/iss/simdisk.c
@@ -41,7 +41,7 @@ module_param(simdisk_count, int, S_IRUGO);
MODULE_PARM_DESC(simdisk_count, "Number of simdisk units.");
static int n_files;
-static const char *filename[MAX_SIMDISK_COUNT] = {
+static char *filename[MAX_SIMDISK_COUNT] = {
#ifdef CONFIG_SIMDISK0_FILENAME
CONFIG_SIMDISK0_FILENAME,
#ifdef CONFIG_SIMDISK1_FILENAME
@@ -50,20 +50,48 @@ static const char *filename[MAX_SIMDISK_COUNT] = {
#endif
};
+/*
+ * The simdisk code can be built either into the kernel or as a loadable module.
+ * When built-in, CONFIG_SIMDISK{0,1}_FILENAME can be used to specify the
+ * initial simdisk filenames and additional filenames can be provided on the
+ * kernel command line. These arguments are parsed during early boot when slab
+ * is not yet available, but the command line itself is preserved for the
+ * lifetime of the kernel, so the incoming pointer is stored directly.
+ * When built as a loadable module, each value is copied with kstrdup() and all
+ * allocated memory is freed in simdisk_param_free_filename() when the module is
+ * unloaded.
+ */
static int simdisk_param_set_filename(const char *val,
const struct kernel_param *kp)
{
- if (n_files < ARRAY_SIZE(filename))
- filename[n_files++] = val;
- else
+ char *str;
+
+ if (n_files >= ARRAY_SIZE(filename))
return -EINVAL;
+
+#ifdef MODULE
+ str = kstrdup(val, GFP_KERNEL);
+ if (!str)
+ return -ENOMEM;
+#else
+ str = (char *)val;
+#endif
+
+ filename[n_files++] = str;
return 0;
}
+static void simdisk_param_free_filename(void *arg)
+{
+ for (int i = 0; i < n_files; i++)
+ kfree(filename[i]);
+}
+
static const struct kernel_param_ops simdisk_param_ops_filename = {
.set = simdisk_param_set_filename,
+ .free = simdisk_param_free_filename,
};
-module_param_cb(filename, &simdisk_param_ops_filename, &n_files, 0);
+module_param_cb(filename, &simdisk_param_ops_filename, NULL, 0);
MODULE_PARM_DESC(filename, "Backing storage filename.");
static int simdisk_major = SIMDISK_MAJOR;
--
2.54.0
^ permalink raw reply related
* [PATCH 0/2] module: Remove unnecessary module::args
From: Petr Pavlu @ 2026-06-04 11:04 UTC (permalink / raw)
To: Chris Zankel, Max Filippov, Luis Chamberlain, Petr Pavlu,
Daniel Gomez, Sami Tolvanen
Cc: Aaron Tomlin, Matthew Wood, linux-modules, linux-kernel
Historically, various parameter-handling code kept pointers into
module::args, most notably the charp support. However, in 2009,
commit e180a6b7759a ("param: fix charp parameters set via sysfs") changed
charp parameters to kstrdup() the input string as well. As a result,
module::args now mostly wastes memory.
Reviewing all kernel_param_ops and module_param_call instances shows that
the last code still relying on module::args remaining valid for the
module's lifetime is simdisk_param_ops_filename. Update it to use kstrdup()
and then remove module::args.
Petr Pavlu (2):
xtensa/simdisk: Avoid referring to module::args
module: Remove unnecessary module::args
arch/xtensa/platforms/iss/simdisk.c | 38 +++++++++++++++++++++++++----
include/linux/module.h | 4 ---
kernel/module/main.c | 15 ++++++------
3 files changed, 41 insertions(+), 16 deletions(-)
base-commit: e43ffb69e0438cddd72aaa30898b4dc446f664f8
--
2.54.0
^ permalink raw reply
* Re: [PATCH] module: dups: use strscpy() to copy module name in dup request
From: Petr Pavlu @ 2026-06-04 8:52 UTC (permalink / raw)
To: Naveen Kumar Chaudhary
Cc: mcgrof, da.gomez, samitolvanen, atomlin, linux-modules,
linux-kernel
In-Reply-To: <v546667lngoo6dxwb7gqzf7liya2opdfeevrtar7iipsbkipfc@uh5fuaofcrty>
On 6/3/26 6:25 PM, Naveen Kumar Chaudhary wrote:
> kmod_dup_request_exists_wait() uses memcpy() with strlen(module_name) to
> copy into new_kmod_req->name, a fixed-size char[MODULE_NAME_LEN] buffer.
> This does not bounds-check the copy and does not explicitly NUL-terminate.
> The buffer is zeroed from kzalloc_obj() so NUL-termination happens to
> work, but the pattern is fragile and lacks an explicit bounds check.
>
> Replace with strscpy() which bounds the copy and guarantees
> NUL-termination.
>
> Signed-off-by: Naveen Kumar Chaudhary <naveen.osdev@gmail.com>
> ---
> kernel/module/dups.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/kernel/module/dups.c b/kernel/module/dups.c
> index 1d720a5311ba..33bddfb57317 100644
> --- a/kernel/module/dups.c
> +++ b/kernel/module/dups.c
> @@ -129,7 +129,7 @@ bool kmod_dup_request_exists_wait(char *module_name, bool wait, int *dup_ret)
> if (!new_kmod_req)
> return false;
>
> - memcpy(new_kmod_req->name, module_name, strlen(module_name));
> + strscpy(new_kmod_req->name, module_name, MODULE_NAME_LEN);
> INIT_WORK(&new_kmod_req->complete_work, kmod_dup_request_complete);
> INIT_DELAYED_WORK(&new_kmod_req->delete_work, kmod_dup_request_delete);
> init_completion(&new_kmod_req->first_req_done);
This can be shortened to:
strscpy(new_kmod_req->name, module_name);
I also suggest merging this patch and the second patch [1], which cleans
up the same issue in the module stats code, into one.
[1] https://lore.kernel.org/linux-modules/jmm7r4r3k3qt767tl7lojglosgc3umhc63cdp2fckdkgb3fzki@3fgvxgvzo5ex/
--
Thanks,
Petr
^ 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