Message-ID: <9b768f91-121a-4072-88b2-36cb48be3917@suse.com>
Date: Tue, 22 Jul 2025 10:25:09 +0200
From: Petr Pavlu <petr.pavlu@...e.com>
To: Wang Jinchao <wangjinchao600@...il.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/22/25 5:08 AM, Wang Jinchao wrote:
> 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().

I cannot find an explicit reason in the Git history why a missing
vermagic is treated as if the module was loaded with
MODULE_INIT_IGNORE_VERMAGIC, and similarly why missing modversions data
is treated as if the module was loaded with
MODULE_INIT_IGNORE_MODVERSIONS.

I would argue that a more sensible behavior would be to consider
a missing vermagic as an error and allow loading the module only if
MODULE_INIT_IGNORE_VERMAGIC is explicitly specified. And analogously for
missing modversions and MODULE_INIT_IGNORE_MODVERSIONS.

Nonetheless, if I understand correctly, this should be mostly separate
from your issue.

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

I'm not sure I follow here. My example lifts the try_to_force_load()
call from check_version() to check_modstruct_version(), and should still
result in tainting the kernel.

-- 
Thanks,
Petr

Powered by blists - more mailing lists