linux-kernel - Re: [PATCH 3/4] kmod: Return directly if module name is empty in request

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] [day] [month] [year] [list]

Date:   Mon, 20 Apr 2020 12:11:21 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     Luis Chamberlain <mcgrof@...nel.org>, Jessica Yu <jeyu@...nel.org>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Shuah Khan <shuah@...nel.org>,
        "open list:KERNEL SELFTEST FRAMEWORK" 
        <linux-kselftest@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Xuefeng Li <lixuefeng@...ngson.cn>
Subject: Re: [PATCH 3/4] kmod: Return directly if module name is empty in
 request_module()

On 04/20/2020 12:08 PM, Tiezhu Yang wrote:
> On 04/18/2020 03:19 PM, Luis Chamberlain wrote:
>> On Sat, Apr 18, 2020 at 01:58:45PM +0800, Tiezhu Yang wrote:
>>> On 04/18/2020 01:48 PM, Luis Chamberlain wrote:
>>>> On Fri, Apr 17, 2020 at 11:45 PM Luis Chamberlain 
>>>> <mcgrof@...nel.org> wrote:
>>>>> On Sat, Apr 18, 2020 at 01:19:59PM +0800, Tiezhu Yang wrote:
>>>>>> If module name is empty, it is better to return directly at the 
>>>>>> beginning
>>>>>> of request_module() without doing the needless call_modprobe() 
>>>>>> operation.
>>>>>>
>>>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
>>>>>> ---
>>>>>>    kernel/kmod.c | 5 +++++
>>>>>>    1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/kernel/kmod.c b/kernel/kmod.c
>>>>>> index 3cd075c..5851444 100644
>>>>>> --- a/kernel/kmod.c
>>>>>> +++ b/kernel/kmod.c
>>>>>> @@ -28,6 +28,8 @@
>>>>>>
>>>>>>    #include <trace/events/module.h>
>>>>>>
>>>>>> +#define MODULE_NOT_FOUND 256
>>>>>> +
>>>>>>    /*
>>>>>>     * Assuming:
>>>>>>     *
>>>>>> @@ -144,6 +146,9 @@ int __request_module(bool wait, const char 
>>>>>> *fmt, ...)
>>>>>>         if (ret >= MODULE_NAME_LEN)
>>>>>>                 return -ENAMETOOLONG;
>>>>>>
>>>>>> +     if (strlen(module_name) == 0)
>>>>>> +             return MODULE_NOT_FOUND;
>>>>> I'd rather we just use something standard like -EINVAL.
>>>>> What do we return if its not found? Then use that value.
>>>> Also, are we testing for this condition yet? If not can we add one?
>>> Yes, kmod_test_0001_driver() in tools/testing/selftests/kmod/kmod.sh 
>>> tests
>>> this case and expects result MODULE_NOT_FOUND which is 256.
>> OK I see now I had put:
>>
>> errno_name_to_val()
>> {
>>          case "$1" in
>>     # kmod calls modprobe and upon of a module not found
>>     # modprobe returns just 1... However in the
>>     # kernel we *sometimes* see 256...
>>     MODULE_NOT_FOUND)
>>         echo 256;;
>>
>> I found that through testing, however there was nothing set in stone,
>> nothing documented. While you are at it, can you find the places where
>> this is returned in the kernel code? We should clear this up and
>> se things straight. We cannot change what we gave userspace already
>> though.
>
> Call Trace:
>
> __request_module()
>       |
>       |
> call_modprobe()
>       |
>       |
> call_usermodehelper_exec()   -- retval = sub_info->retval;
>       |
>       |
> call_usermodehelper_exec_work()
>       |
>       |
> call_usermodehelper_exec_sync()   -- sub_info->retval = ret;
>       |
>       | --> call_usermodehelper_exec_async() --> do_execve()
>       |
> kernel_wait4(pid, (int __user *)&ret, 0, NULL);
>
> __request_module() returns the exist status of child process, if 
> module name

Sorry for the typo: exist status --> exit status

> is empty or non-exist, sub_info->retval is 256 after call kernel_wait4().
>
> Should I add this analysis to the commit message?
>
> Thanks,
> Tiezhu Yang
>
>>
>>    Luis
>