linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN
@ 2025-06-30 14:32 Petr Pavlu
  2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
                   ` (5 more replies)
  0 siblings, 6 replies; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

Fix a few minor problems related to MODULE_NAME_LEN and
MAX_PARAM_PREFIX_LEN, and clean up their usage.

Petr Pavlu (5):
  module: Prevent silent truncation of module name in delete_module(2)
  module: Remove unnecessary +1 from last_unloaded_module::name size
  module: Restore the moduleparam prefix length check
  tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
  module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN

 include/linux/module.h      |  2 +-
 include/linux/moduleparam.h | 15 +++++++++------
 kernel/module/main.c        | 12 +++++++-----
 kernel/trace/trace.c        |  2 +-
 4 files changed, 18 insertions(+), 13 deletions(-)


base-commit: d0b3b7b22dfa1f4b515fd3a295b3fd958f9e81af
-- 
2.49.0


^ permalink raw reply	[flat|nested] 15+ messages in thread

* [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
  2025-07-16 19:47   ` Daniel Gomez
  2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

Passing a module name longer than MODULE_NAME_LEN to the delete_module
syscall results in its silent truncation. This really isn't much of
a problem in practice, but it could theoretically lead to the removal of an
incorrect module. It is more sensible to return ENAMETOOLONG or ENOENT in
such a case.

Update the syscall to return ENOENT, as documented in the delete_module(2)
man page to mean "No module by that name exists." This is appropriate
because a module with a name longer than MODULE_NAME_LEN cannot be loaded
in the first place.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/main.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 413ac6ea3702..933a9854cb7d 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -751,14 +751,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
 	struct module *mod;
 	char name[MODULE_NAME_LEN];
 	char buf[MODULE_FLAGS_BUF_SIZE];
-	int ret, forced = 0;
+	int ret, len, forced = 0;
 
 	if (!capable(CAP_SYS_MODULE) || modules_disabled)
 		return -EPERM;
 
-	if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
-		return -EFAULT;
-	name[MODULE_NAME_LEN-1] = '\0';
+	len = strncpy_from_user(name, name_user, MODULE_NAME_LEN);
+	if (len == 0 || len == MODULE_NAME_LEN)
+		return -ENOENT;
+	if (len < 0)
+		return len;
 
 	audit_log_kern_module(name);
 
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
  2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
  2025-07-16 19:49   ` Daniel Gomez
  2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

The variable last_unloaded_module::name tracks the name of the last
unloaded module. It is a string copy of module::name, which is
MODULE_NAME_LEN bytes in size and includes the NUL terminator. Therefore,
the size of last_unloaded_module::name can also be just MODULE_NAME_LEN,
without the need for an extra byte.

Fixes: e14af7eeb47e ("debug: track and print last unloaded module in the oops trace")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 kernel/module/main.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/module/main.c b/kernel/module/main.c
index 933a9854cb7d..04173543639c 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -580,7 +580,7 @@ MODINFO_ATTR(version);
 MODINFO_ATTR(srcversion);
 
 static struct {
-	char name[MODULE_NAME_LEN + 1];
+	char name[MODULE_NAME_LEN];
 	char taints[MODULE_FLAGS_BUF_SIZE];
 } last_unloaded_module;
 
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 3/5] module: Restore the moduleparam prefix length check
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
  2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
  2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
  2025-07-17 19:23   ` Daniel Gomez
  2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
                   ` (2 subsequent siblings)
  5 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

The moduleparam code allows modules to provide their own definition of
MODULE_PARAM_PREFIX, instead of using the default KBUILD_MODNAME ".".

Commit 730b69d22525 ("module: check kernel param length at compile time,
not runtime") added a check to ensure the prefix doesn't exceed
MODULE_NAME_LEN, as this is what param_sysfs_builtin() expects.

Later, commit 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking
for sysfs perms.") removed this check, but there is no indication this was
intentional.

Since the check is still useful for param_sysfs_builtin() to function
properly, reintroduce it in __module_param_call(), but in a modernized form
using static_assert().

While here, clean up the __module_param_call() comments. In particular,
remove the comment "Default value instead of permissions?", which comes
from commit 9774a1f54f17 ("[PATCH] Compile-time check re world-writeable
module params"). This comment was related to the test variable
__param_perm_check_##name, which was removed in the previously mentioned
commit 58f86cc89c33.

Fixes: 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.")
Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 include/linux/moduleparam.h | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index bfb85fd13e1f..110e9d09de24 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -282,10 +282,9 @@ struct kparam_array
 #define __moduleparam_const const
 #endif
 
-/* This is the fundamental function for registering boot/module
-   parameters. */
+/* This is the fundamental function for registering boot/module parameters. */
 #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
-	/* Default value instead of permissions? */			\
+	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used __section("__param")					\
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
                   ` (2 preceding siblings ...)
  2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
  2025-07-28  6:48   ` Daniel Gomez
  2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
  2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez
  5 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel, Steven Rostedt, Masami Hiramatsu

Use the MODULE_NAME_LEN definition in module_exists() to obtain the maximum
size of a module name, instead of using MAX_PARAM_PREFIX_LEN. The values
are the same but MODULE_NAME_LEN is more appropriate in this context.
MAX_PARAM_PREFIX_LEN was added in commit 730b69d22525 ("module: check
kernel param length at compile time, not runtime") only to break a circular
dependency between module.h and moduleparam.h, and should mostly be limited
to use in moduleparam.h.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
Cc: Steven Rostedt <rostedt@goodmis.org>
Cc: Masami Hiramatsu <mhiramat@kernel.org>
---

As a side note, I suspect the function module_exists() would be better
replaced with !!find_module() + RCU locking, but that is a separate issue.

 kernel/trace/trace.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
index 95ae7c4e5835..b9da0c4661a0 100644
--- a/kernel/trace/trace.c
+++ b/kernel/trace/trace.c
@@ -10373,7 +10373,7 @@ bool module_exists(const char *module)
 {
 	/* All modules have the symbol __this_module */
 	static const char this_mod[] = "__this_module";
-	char modname[MAX_PARAM_PREFIX_LEN + sizeof(this_mod) + 2];
+	char modname[MODULE_NAME_LEN + sizeof(this_mod) + 2];
 	unsigned long val;
 	int n;
 
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
                   ` (3 preceding siblings ...)
  2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
@ 2025-06-30 14:32 ` Petr Pavlu
  2025-07-28  6:48   ` Daniel Gomez
  2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez
  5 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-06-30 14:32 UTC (permalink / raw)
  To: Luis Chamberlain, Petr Pavlu, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

The maximum module name length (MODULE_NAME_LEN) is somewhat confusingly
defined in terms of the maximum parameter prefix length
(MAX_PARAM_PREFIX_LEN), when in fact the dependency is in the opposite
direction.

This split originates from commit 730b69d22525 ("module: check kernel param
length at compile time, not runtime"). The code needed to use
MODULE_NAME_LEN in moduleparam.h, but because module.h requires
moduleparam.h, this created a circular dependency. It was resolved by
introducing MAX_PARAM_PREFIX_LEN in moduleparam.h and defining
MODULE_NAME_LEN in module.h in terms of MAX_PARAM_PREFIX_LEN.

Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN for clarity. This matches
the similar approach of defining MODULE_INFO in module.h and __MODULE_INFO
in moduleparam.h.

Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
---
 include/linux/module.h      |  2 +-
 include/linux/moduleparam.h | 12 ++++++++----
 2 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index 5faa1fb1f4b4..0f1dde3996d9 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -32,7 +32,7 @@
 #include <linux/percpu.h>
 #include <asm/module.h>
 
-#define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
+#define MODULE_NAME_LEN __MODULE_NAME_LEN
 
 struct modversion_info {
 	unsigned long crc;
diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
index 110e9d09de24..a04a2bc4f51e 100644
--- a/include/linux/moduleparam.h
+++ b/include/linux/moduleparam.h
@@ -6,6 +6,13 @@
 #include <linux/stringify.h>
 #include <linux/kernel.h>
 
+/*
+ * The maximum module name length, including the NUL byte.
+ * Chosen so that structs with an unsigned long line up, specifically
+ * modversion_info.
+ */
+#define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
+
 /* You can override this manually, but generally this should match the
    module name. */
 #ifdef MODULE
@@ -17,9 +24,6 @@
 #define __MODULE_INFO_PREFIX KBUILD_MODNAME "."
 #endif
 
-/* Chosen so that structs with an unsigned long line up. */
-#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
-
 #define __MODULE_INFO(tag, name, info)					  \
 	static const char __UNIQUE_ID(name)[]				  \
 		__used __section(".modinfo") __aligned(1)		  \
@@ -284,7 +288,7 @@ struct kparam_array
 
 /* This is the fundamental function for registering boot/module parameters. */
 #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
-	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\
+	static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN);	\
 	static const char __param_str_##name[] = prefix #name;		\
 	static struct kernel_param __moduleparam_const __param_##name	\
 	__used __section("__param")					\
-- 
2.49.0


^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2)
  2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
@ 2025-07-16 19:47   ` Daniel Gomez
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-16 19:47 UTC (permalink / raw)
  To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

On 30/06/2025 16.32, Petr Pavlu wrote:
> Passing a module name longer than MODULE_NAME_LEN to the delete_module
> syscall results in its silent truncation. This really isn't much of
> a problem in practice, but it could theoretically lead to the removal of an
> incorrect module. It is more sensible to return ENAMETOOLONG or ENOENT in
> such a case.
> 
> Update the syscall to return ENOENT, as documented in the delete_module(2)
> man page to mean "No module by that name exists." This is appropriate
> because a module with a name

Including the NUL byte...

> longer than MODULE_NAME_LEN cannot be loaded
> in the first place.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/main.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 413ac6ea3702..933a9854cb7d 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -751,14 +751,16 @@ SYSCALL_DEFINE2(delete_module, const char __user *, name_user,
>  	struct module *mod;
>  	char name[MODULE_NAME_LEN];
>  	char buf[MODULE_FLAGS_BUF_SIZE];
> -	int ret, forced = 0;
> +	int ret, len, forced = 0;
>  
>  	if (!capable(CAP_SYS_MODULE) || modules_disabled)
>  		return -EPERM;
>  
> -	if (strncpy_from_user(name, name_user, MODULE_NAME_LEN-1) < 0)
> -		return -EFAULT;
> -	name[MODULE_NAME_LEN-1] = '\0';
> +	len = strncpy_from_user(name, name_user, MODULE_NAME_LEN);
> +	if (len == 0 || len == MODULE_NAME_LEN)
> +		return -ENOENT;
> +	if (len < 0)
> +		return len;

This looks correct to me. The new code not only returns the correct errors
indicated in delete_module(2) but also checks for the case user passes an
empty string and the case where NUL char is not found when copying (with len
== MODULE_NAME_LEN) as well as it's using the correct length (MODULE_NAME_LEN)
for copying.

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
  2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
@ 2025-07-16 19:49   ` Daniel Gomez
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-16 19:49 UTC (permalink / raw)
  To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

On 30/06/2025 16.32, Petr Pavlu wrote:
> The variable last_unloaded_module::name tracks the name of the last
> unloaded module. It is a string copy of module::name, which is
> MODULE_NAME_LEN bytes in size and includes the NUL terminator. Therefore,
> the size of last_unloaded_module::name can also be just MODULE_NAME_LEN,
> without the need for an extra byte.
> 
> Fixes: e14af7eeb47e ("debug: track and print last unloaded module in the oops trace")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  kernel/module/main.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index 933a9854cb7d..04173543639c 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -580,7 +580,7 @@ MODINFO_ATTR(version);
>  MODINFO_ATTR(srcversion);
>  
>  static struct {
> -	char name[MODULE_NAME_LEN + 1];
> +	char name[MODULE_NAME_LEN];
>  	char taints[MODULE_FLAGS_BUF_SIZE];
>  } last_unloaded_module;
>  

LGTM,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
  2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
@ 2025-07-17 19:23   ` Daniel Gomez
  2025-07-21  9:21     ` Petr Pavlu
  0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2025-07-17 19:23 UTC (permalink / raw)
  To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

On 30/06/2025 16.32, Petr Pavlu wrote:
> The moduleparam code allows modules to provide their own definition of
> MODULE_PARAM_PREFIX, instead of using the default KBUILD_MODNAME ".".
> 
> Commit 730b69d22525 ("module: check kernel param length at compile time,
> not runtime") added a check to ensure the prefix doesn't exceed
> MODULE_NAME_LEN, as this is what param_sysfs_builtin() expects.
> 
> Later, commit 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking
> for sysfs perms.") removed this check, but there is no indication this was
> intentional.
> 
> Since the check is still useful for param_sysfs_builtin() to function
> properly, reintroduce it in __module_param_call(), but in a modernized form
> using static_assert().
> 
> While here, clean up the __module_param_call() comments. In particular,
> remove the comment "Default value instead of permissions?", which comes
> from commit 9774a1f54f17 ("[PATCH] Compile-time check re world-writeable
> module params"). This comment was related to the test variable
> __param_perm_check_##name, which was removed in the previously mentioned
> commit 58f86cc89c33.
> 
> Fixes: 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.")
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  include/linux/moduleparam.h | 5 ++---
>  1 file changed, 2 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index bfb85fd13e1f..110e9d09de24 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -282,10 +282,9 @@ struct kparam_array
>  #define __moduleparam_const const
>  #endif
>  
> -/* This is the fundamental function for registering boot/module
> -   parameters. */
> +/* This is the fundamental function for registering boot/module parameters. */
>  #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
> -	/* Default value instead of permissions? */			\
> +	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\

Can you clarify if -1 to remove the dot from prefix?

Final code 
	static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN);	\

with __MODULE_NAME_LEN being:

#define __MODULE_NAME_LEN (64 - sizeof(unsigned long))


>  	static const char __param_str_##name[] = prefix #name;		\
>  	static struct kernel_param __moduleparam_const __param_##name	\
>  	__used __section("__param")					\

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
  2025-07-17 19:23   ` Daniel Gomez
@ 2025-07-21  9:21     ` Petr Pavlu
  2025-07-28  6:43       ` Daniel Gomez
  0 siblings, 1 reply; 15+ messages in thread
From: Petr Pavlu @ 2025-07-21  9:21 UTC (permalink / raw)
  To: Daniel Gomez
  Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
	linux-kernel

On 7/17/25 9:23 PM, Daniel Gomez wrote:
> On 30/06/2025 16.32, Petr Pavlu wrote:
>> The moduleparam code allows modules to provide their own definition of
>> MODULE_PARAM_PREFIX, instead of using the default KBUILD_MODNAME ".".
>>
>> Commit 730b69d22525 ("module: check kernel param length at compile time,
>> not runtime") added a check to ensure the prefix doesn't exceed
>> MODULE_NAME_LEN, as this is what param_sysfs_builtin() expects.
>>
>> Later, commit 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking
>> for sysfs perms.") removed this check, but there is no indication this was
>> intentional.
>>
>> Since the check is still useful for param_sysfs_builtin() to function
>> properly, reintroduce it in __module_param_call(), but in a modernized form
>> using static_assert().
>>
>> While here, clean up the __module_param_call() comments. In particular,
>> remove the comment "Default value instead of permissions?", which comes
>> from commit 9774a1f54f17 ("[PATCH] Compile-time check re world-writeable
>> module params"). This comment was related to the test variable
>> __param_perm_check_##name, which was removed in the previously mentioned
>> commit 58f86cc89c33.
>>
>> Fixes: 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.")
>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>> ---
>>  include/linux/moduleparam.h | 5 ++---
>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>
>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>> index bfb85fd13e1f..110e9d09de24 100644
>> --- a/include/linux/moduleparam.h
>> +++ b/include/linux/moduleparam.h
>> @@ -282,10 +282,9 @@ struct kparam_array
>>  #define __moduleparam_const const
>>  #endif
>>  
>> -/* This is the fundamental function for registering boot/module
>> -   parameters. */
>> +/* This is the fundamental function for registering boot/module parameters. */
>>  #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
>> -	/* Default value instead of permissions? */			\
>> +	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\
> 
> Can you clarify if -1 to remove the dot from prefix?
> 
> Final code 
> 	static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN);	\
> 
> with __MODULE_NAME_LEN being:
> 
> #define __MODULE_NAME_LEN (64 - sizeof(unsigned long))

Correct, -1 is to account for the dot at the end of the prefix.

I actually also wanted to assert that the prefix ends with a dot, but
unfortunately prefix[sizeof(prefix)-2] (with prefix being a string
literal) is not a constant expression in C.

-- 
Thanks,
Petr

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 3/5] module: Restore the moduleparam prefix length check
  2025-07-21  9:21     ` Petr Pavlu
@ 2025-07-28  6:43       ` Daniel Gomez
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28  6:43 UTC (permalink / raw)
  To: Petr Pavlu
  Cc: Luis Chamberlain, Sami Tolvanen, Daniel Gomez, linux-modules,
	linux-kernel

On 21/07/2025 11.21, Petr Pavlu wrote:
> On 7/17/25 9:23 PM, Daniel Gomez wrote:
>> On 30/06/2025 16.32, Petr Pavlu wrote:
>>> The moduleparam code allows modules to provide their own definition of
>>> MODULE_PARAM_PREFIX, instead of using the default KBUILD_MODNAME ".".
>>>
>>> Commit 730b69d22525 ("module: check kernel param length at compile time,
>>> not runtime") added a check to ensure the prefix doesn't exceed
>>> MODULE_NAME_LEN, as this is what param_sysfs_builtin() expects.
>>>
>>> Later, commit 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking
>>> for sysfs perms.") removed this check, but there is no indication this was
>>> intentional.
>>>
>>> Since the check is still useful for param_sysfs_builtin() to function
>>> properly, reintroduce it in __module_param_call(), but in a modernized form
>>> using static_assert().
>>>
>>> While here, clean up the __module_param_call() comments. In particular,
>>> remove the comment "Default value instead of permissions?", which comes
>>> from commit 9774a1f54f17 ("[PATCH] Compile-time check re world-writeable
>>> module params"). This comment was related to the test variable
>>> __param_perm_check_##name, which was removed in the previously mentioned
>>> commit 58f86cc89c33.
>>>
>>> Fixes: 58f86cc89c33 ("VERIFY_OCTAL_PERMISSIONS: stricter checking for sysfs perms.")
>>> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
>>> ---
>>>  include/linux/moduleparam.h | 5 ++---
>>>  1 file changed, 2 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
>>> index bfb85fd13e1f..110e9d09de24 100644
>>> --- a/include/linux/moduleparam.h
>>> +++ b/include/linux/moduleparam.h
>>> @@ -282,10 +282,9 @@ struct kparam_array
>>>  #define __moduleparam_const const
>>>  #endif
>>>  
>>> -/* This is the fundamental function for registering boot/module
>>> -   parameters. */
>>> +/* This is the fundamental function for registering boot/module parameters. */
>>>  #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
>>> -	/* Default value instead of permissions? */			\
>>> +	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\
>>
>> Can you clarify if -1 to remove the dot from prefix?
>>
>> Final code 
>> 	static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN);	\
>>
>> with __MODULE_NAME_LEN being:
>>
>> #define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
> 
> Correct, -1 is to account for the dot at the end of the prefix.

LGTM,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

> 
> I actually also wanted to assert that the prefix ends with a dot, but
> unfortunately prefix[sizeof(prefix)-2] (with prefix being a string
> literal) is not a constant expression in C.
> 

But even if that would be possible, there are some calls that do not have
a prefix with dot. For example,

#define core_param(name, var, type, perm)				\
	param_check_##type(name, &(var));				\
	__module_param_call("", name, &param_ops_##type, &var, perm, -1, 0)

So, you'd have to handle both cases. I mean, where __module_param_call(<prefix>
is used with either MODULE_PARAM_PREFIX or an empty string "".

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
  2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
@ 2025-07-28  6:48   ` Daniel Gomez
  2025-07-28 14:34     ` Steven Rostedt
  0 siblings, 1 reply; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28  6:48 UTC (permalink / raw)
  To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez,
	Steven Rostedt, Masami Hiramatsu
  Cc: linux-modules, linux-kernel

On 30/06/2025 16.32, Petr Pavlu wrote:
> Use the MODULE_NAME_LEN definition in module_exists() to obtain the maximum
> size of a module name, instead of using MAX_PARAM_PREFIX_LEN. The values
> are the same but MODULE_NAME_LEN is more appropriate in this context.
> MAX_PARAM_PREFIX_LEN was added in commit 730b69d22525 ("module: check
> kernel param length at compile time, not runtime") only to break a circular
> dependency between module.h and moduleparam.h, and should mostly be limited
> to use in moduleparam.h.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> Cc: Steven Rostedt <rostedt@goodmis.org>
> Cc: Masami Hiramatsu <mhiramat@kernel.org>

Steven, Masami,

I'm planning to merge these series into modules-next. I think and Ack/Review
would be great from you. Otherwise, let me know if you'd rather take this patch
through tracing instead (in case it looks good from your side).

> ---
> 
> As a side note, I suspect the function module_exists() would be better
> replaced with !!find_module() + RCU locking, but that is a separate issue.
> 
>  kernel/trace/trace.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c
> index 95ae7c4e5835..b9da0c4661a0 100644
> --- a/kernel/trace/trace.c
> +++ b/kernel/trace/trace.c
> @@ -10373,7 +10373,7 @@ bool module_exists(const char *module)
>  {
>  	/* All modules have the symbol __this_module */
>  	static const char this_mod[] = "__this_module";
> -	char modname[MAX_PARAM_PREFIX_LEN + sizeof(this_mod) + 2];
> +	char modname[MODULE_NAME_LEN + sizeof(this_mod) + 2];
>  	unsigned long val;
>  	int n;
>  

LGTM,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
  2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
@ 2025-07-28  6:48   ` Daniel Gomez
  0 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-28  6:48 UTC (permalink / raw)
  To: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez
  Cc: linux-modules, linux-kernel

On 30/06/2025 16.32, Petr Pavlu wrote:
> The maximum module name length (MODULE_NAME_LEN) is somewhat confusingly
> defined in terms of the maximum parameter prefix length
> (MAX_PARAM_PREFIX_LEN), when in fact the dependency is in the opposite
> direction.
> 
> This split originates from commit 730b69d22525 ("module: check kernel param
> length at compile time, not runtime"). The code needed to use
> MODULE_NAME_LEN in moduleparam.h, but because module.h requires
> moduleparam.h, this created a circular dependency. It was resolved by
> introducing MAX_PARAM_PREFIX_LEN in moduleparam.h and defining
> MODULE_NAME_LEN in module.h in terms of MAX_PARAM_PREFIX_LEN.
> 
> Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN for clarity. This matches
> the similar approach of defining MODULE_INFO in module.h and __MODULE_INFO
> in moduleparam.h.
> 
> Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> ---
>  include/linux/module.h      |  2 +-
>  include/linux/moduleparam.h | 12 ++++++++----
>  2 files changed, 9 insertions(+), 5 deletions(-)
> 
> diff --git a/include/linux/module.h b/include/linux/module.h
> index 5faa1fb1f4b4..0f1dde3996d9 100644
> --- a/include/linux/module.h
> +++ b/include/linux/module.h
> @@ -32,7 +32,7 @@
>  #include <linux/percpu.h>
>  #include <asm/module.h>
>  
> -#define MODULE_NAME_LEN MAX_PARAM_PREFIX_LEN
> +#define MODULE_NAME_LEN __MODULE_NAME_LEN
>  
>  struct modversion_info {
>  	unsigned long crc;
> diff --git a/include/linux/moduleparam.h b/include/linux/moduleparam.h
> index 110e9d09de24..a04a2bc4f51e 100644
> --- a/include/linux/moduleparam.h
> +++ b/include/linux/moduleparam.h
> @@ -6,6 +6,13 @@
>  #include <linux/stringify.h>
>  #include <linux/kernel.h>
>  
> +/*
> + * The maximum module name length, including the NUL byte.
> + * Chosen so that structs with an unsigned long line up, specifically
> + * modversion_info.
> + */
> +#define __MODULE_NAME_LEN (64 - sizeof(unsigned long))
> +
>  /* You can override this manually, but generally this should match the
>     module name. */
>  #ifdef MODULE
> @@ -17,9 +24,6 @@
>  #define __MODULE_INFO_PREFIX KBUILD_MODNAME "."
>  #endif
>  
> -/* Chosen so that structs with an unsigned long line up. */
> -#define MAX_PARAM_PREFIX_LEN (64 - sizeof(unsigned long))
> -
>  #define __MODULE_INFO(tag, name, info)					  \
>  	static const char __UNIQUE_ID(name)[]				  \
>  		__used __section(".modinfo") __aligned(1)		  \
> @@ -284,7 +288,7 @@ struct kparam_array
>  
>  /* This is the fundamental function for registering boot/module parameters. */
>  #define __module_param_call(prefix, name, ops, arg, perm, level, flags)	\
> -	static_assert(sizeof(""prefix) - 1 <= MAX_PARAM_PREFIX_LEN);	\
> +	static_assert(sizeof(""prefix) - 1 <= __MODULE_NAME_LEN);	\
>  	static const char __param_str_##name[] = prefix #name;		\
>  	static struct kernel_param __moduleparam_const __param_##name	\
>  	__used __section("__param")					\


LGTM,

Reviewed-by: Daniel Gomez <da.gomez@samsung.com>

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
  2025-07-28  6:48   ` Daniel Gomez
@ 2025-07-28 14:34     ` Steven Rostedt
  0 siblings, 0 replies; 15+ messages in thread
From: Steven Rostedt @ 2025-07-28 14:34 UTC (permalink / raw)
  To: Daniel Gomez
  Cc: Petr Pavlu, Luis Chamberlain, Sami Tolvanen, Daniel Gomez,
	Masami Hiramatsu, linux-modules, linux-kernel

On Mon, 28 Jul 2025 08:48:01 +0200
Daniel Gomez <da.gomez@kernel.org> wrote:

> On 30/06/2025 16.32, Petr Pavlu wrote:
> > Use the MODULE_NAME_LEN definition in module_exists() to obtain the maximum
> > size of a module name, instead of using MAX_PARAM_PREFIX_LEN. The values
> > are the same but MODULE_NAME_LEN is more appropriate in this context.
> > MAX_PARAM_PREFIX_LEN was added in commit 730b69d22525 ("module: check
> > kernel param length at compile time, not runtime") only to break a circular
> > dependency between module.h and moduleparam.h, and should mostly be limited
> > to use in moduleparam.h.
> > 
> > Signed-off-by: Petr Pavlu <petr.pavlu@suse.com>
> > Cc: Steven Rostedt <rostedt@goodmis.org>
> > Cc: Masami Hiramatsu <mhiramat@kernel.org>  
> 
> Steven, Masami,
> 
> I'm planning to merge these series into modules-next. I think and Ack/Review
> would be great from you. Otherwise, let me know if you'd rather take this patch
> through tracing instead (in case it looks good from your side).

Acked-by: Steven Rostedt (Google) <rostedt@goodmis.org>

> 
> > ---
> > 
> > As a side note, I suspect the function module_exists() would be better
> > replaced with !!find_module() + RCU locking, but that is a separate issue.

Yeah, that is probably something that should be done too.

Thanks,

-- Steve


^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN
  2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
                   ` (4 preceding siblings ...)
  2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
@ 2025-07-31 12:03 ` Daniel Gomez
  5 siblings, 0 replies; 15+ messages in thread
From: Daniel Gomez @ 2025-07-31 12:03 UTC (permalink / raw)
  To: Luis Chamberlain, Sami Tolvanen, Petr Pavlu; +Cc: linux-modules, linux-kernel


On Mon, 30 Jun 2025 16:32:31 +0200, Petr Pavlu wrote:
> Fix a few minor problems related to MODULE_NAME_LEN and
> MAX_PARAM_PREFIX_LEN, and clean up their usage.
> 
> Petr Pavlu (5):
>   module: Prevent silent truncation of module name in delete_module(2)
>   module: Remove unnecessary +1 from last_unloaded_module::name size
>   module: Restore the moduleparam prefix length check
>   tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
>   module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
> 
> [...]

Applied, thanks!

[1/5] module: Prevent silent truncation of module name in delete_module(2)
      commit: a6323bd4e611567913e23df5b58f2d4e4da06789
[2/5] module: Remove unnecessary +1 from last_unloaded_module::name size
      commit: 6c171b2ccfe677ca97fc5334f853807959f26589
[3/5] module: Restore the moduleparam prefix length check
      commit: bdc877ba6b7ff1b6d2ebeff11e63da4a50a54854
[4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN
      commit: a7c54b2b41dd1f6ec780e7fbfb13f70c64c9731d
[5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN
      commit: 40a826bd6c82ae45cfd3a19cd2a60a10f56b74c0

Best regards,
-- 
Daniel Gomez <da.gomez@samsung.com>


^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2025-07-31 12:03 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-06-30 14:32 [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Petr Pavlu
2025-06-30 14:32 ` [PATCH 1/5] module: Prevent silent truncation of module name in delete_module(2) Petr Pavlu
2025-07-16 19:47   ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 2/5] module: Remove unnecessary +1 from last_unloaded_module::name size Petr Pavlu
2025-07-16 19:49   ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 3/5] module: Restore the moduleparam prefix length check Petr Pavlu
2025-07-17 19:23   ` Daniel Gomez
2025-07-21  9:21     ` Petr Pavlu
2025-07-28  6:43       ` Daniel Gomez
2025-06-30 14:32 ` [PATCH 4/5] tracing: Replace MAX_PARAM_PREFIX_LEN with MODULE_NAME_LEN Petr Pavlu
2025-07-28  6:48   ` Daniel Gomez
2025-07-28 14:34     ` Steven Rostedt
2025-06-30 14:32 ` [PATCH 5/5] module: Rename MAX_PARAM_PREFIX_LEN to __MODULE_NAME_LEN Petr Pavlu
2025-07-28  6:48   ` Daniel Gomez
2025-07-31 12:03 ` [PATCH 0/5] module: Fix minor problems related to MODULE_NAME_LEN Daniel Gomez

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).