linux-kernel - Re: [PATCH] module: pr_debug when there is no version info

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <86062810-ff6b-4181-83b7-dfe443ff4012@gmail.com>
Date: Tue, 22 Jul 2025 11:08:08 +0800
From: Wang Jinchao <wangjinchao600@...il.com>
To: Petr Pavlu <petr.pavlu@...e.com>
Cc: Luis Chamberlain <mcgrof@...nel.org>, Daniel Gomez <da.gomez@...nel.org>,
 Sami Tolvanen <samitolvanen@...gle.com>, linux-kernel@...r.kernel.org,
 linux-modules@...r.kernel.org
Subject: Re: [PATCH] module: pr_debug when there is no version info

On 7/21/25 22:40, Petr Pavlu wrote:
> On 7/21/25 6:52 AM, Wang Jinchao wrote:
>> When there is no version information, modprobe and insmod only
>> report "invalid format".
>> Print the actual cause to make it easier to diagnose the issue.
>> This helps developers quickly identify version-related module
>> loading failures.
>> Signed-off-by: Wang Jinchao <wangjinchao600@...il.com>
>> ---
>>   kernel/module/version.c | 4 +++-
>>   1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/kernel/module/version.c b/kernel/module/version.c
>> index 2beefeba82d9..bc28c697ff3a 100644
>> --- a/kernel/module/version.c
>> +++ b/kernel/module/version.c
>> @@ -42,8 +42,10 @@ int check_version(const struct load_info *info,
>>   	}
>>   
>>   	/* No versions at all?  modprobe --force does this. */
>> -	if (versindex == 0)
>> +	if (versindex == 0) {
>> +		pr_debug("No version info for module %s\n", info->name);
>>   		return try_to_force_load(mod, symname) == 0;
>> +	}
>>   
>>   	versions = (void *)sechdrs[versindex].sh_addr;
>>   	num_versions = sechdrs[versindex].sh_size
> 
> I think it would be better to instead improve the behavior of
> try_to_force_load(). The function should print the error reason prior to
> returning -ENOEXEC. This would also help its two other callers,
> check_modinfo() and check_export_symbol_versions().
> 
> Additionally, I suggest moving the check to ensure version information
> is available for imported symbols earlier in the loading process.
> A suitable place might be check_modstruct_version(). This way the check
> is performed only once per module.
> 
> The following is a prototype patch:
> 
> diff --git a/kernel/module/main.c b/kernel/module/main.c
> index c2c08007029d..c1ccd343e8c3 100644
> --- a/kernel/module/main.c
> +++ b/kernel/module/main.c
> @@ -1053,6 +1053,7 @@ int try_to_force_load(struct module *mod, const char *reason)
>   	add_taint_module(mod, TAINT_FORCED_MODULE, LOCKDEP_NOW_UNRELIABLE);
>   	return 0;
>   #else
> +	pr_err("%s: %s\n", mod->name, reason);
>   	return -ENOEXEC;
>   #endif
>   }
> diff --git a/kernel/module/version.c b/kernel/module/version.c
> index 2beefeba82d9..4d9ebf0834de 100644
> --- a/kernel/module/version.c
> +++ b/kernel/module/version.c
> @@ -41,9 +41,9 @@ int check_version(const struct load_info *info,
>   		return 1;
>   	}
>   
> -	/* No versions at all?  modprobe --force does this. */
> +	/* No versions? Ok, already tainted in check_modstruct_version(). */
>   	if (versindex == 0)
> -		return try_to_force_load(mod, symname) == 0;
> +		return 1;
>   
>   	versions = (void *)sechdrs[versindex].sh_addr;
>   	num_versions = sechdrs[versindex].sh_size
> @@ -90,6 +90,11 @@ int check_modstruct_version(const struct load_info *info,
>   		have_symbol = find_symbol(&fsa);
>   	BUG_ON(!have_symbol);
>   
> +	/* No versions at all?  modprobe --force does this. */
> +	if (!info->index.vers && !info->index.vers_ext_crc)
> +		return try_to_force_load(
> +			       mod, "no versions for imported symbols") == 0;
> +
>   	return check_version(info, "module_layout", mod, fsa.crc);
>   }
>   
> 
> As a side note, I'm confused why with CONFIG_MODULE_FORCE_LOAD=y, the
> code treats missing modversions for imported symbols as ok, even without
> MODULE_INIT_IGNORE_MODVERSIONS. This is at least consistent with the
> handling of missing vermagic, but it seems this behavior should be
> stricter.
> 
When debugging syzkaller, I noticed that the insmod command always 
reports errors. However, to get the exact information, I need to trace 
the kernel, so I casually submitted this patch.

Based on your response, I also feel that the meaning of force_load here 
is somewhat unclear. It would be better to create a mask or a clear list 
to indicate which fields can be forced and which cannot. Once this is 
clear, we can create a function named may_force_check().

In addition, check_modstruct_version also calls check_version to handle 
tainting. So there's a minor issue with the logic in your example patch.

-- 
Best regards,
Jinchao