linux-kernel - Re: [PATCH v3 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 for Android: free password hash cracker in your pocket

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

Date:   Wed, 22 Apr 2020 16:55:34 +0800
From:   Tiezhu Yang <yangtiezhu@...ngson.cn>
To:     Jessica Yu <jeyu@...nel.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Shuah Khan <shuah@...nel.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        linux-kselftest@...r.kernel.org, linux-kernel@...r.kernel.org,
        Markus Elfring <Markus.Elfring@....de>,
        Xuefeng Li <lixuefeng@...ngson.cn>,
        Al Viro <viro@...iv.linux.org.uk>
Subject: Re: [PATCH v3 3/4] kmod: Return directly if module name is empty in
 request_module()

On 04/21/2020 10:49 PM, Jessica Yu wrote:
> +++ Tiezhu Yang [21/04/20 11:07 +0800]:
>> On 04/21/2020 02:19 AM, Luis Chamberlain wrote:
>>> On Mon, Apr 20, 2020 at 08:33:54PM +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.
>>>>
>>>> Call trace:
>>>>
>>>> request_module()
>>>>       |
>>>>       |
>>>> __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);
>>>>
>>>> sub_info->retval is 256 after call kernel_wait4(), the function
>>>> call_usermodehelper_exec() returns sub_info->retval which is 256,
>>>> then call_modprobe() and __request_module() returns 256.
>>>>
>>>> Signed-off-by: Tiezhu Yang <yangtiezhu@...ngson.cn>
>>> Thanks for looking into this. I still cannot find where
>>> userspace it returns 256. Can you? If I run modprobe without
>>> an argument I see 1 returned.
>>>
>>> At least kmod [0] has a series of cmd helper structs, the one for 
>>> modprobe
>>> seems to be kmod_cmd_compat_modprobe, and I can see -1 returned which
>>> can be converted to 255. It can also return EXIT_FAILURE or 
>>> EXIT_SUCCESS
>>> and /usr/include/stdlib.h defines these as 1 and 0 respectively.
>
> I'm also seeing modprobe return 1 as exit status when I run it without
> arguments. I don't think the 256 value is coming from modprobe though,
> see below -
>
>>> https://git.kernel.org/pub/scm/utils/kernel/kmod/kmod.git/
>>>
>>>   Luis
>>
>> Here is my understanding:
>>
>> When build and execute the following application, we can see the exit 
>> status is 256.
>>
>> $ ./system
>> modprobe: FATAL: Module  not found in directory 
>> /lib/modules/4.18.0-147.5.1.el8_1.x86_64
>> exit status = 256
>>
>> $ ./execl
>> modprobe: FATAL: Module  not found in directory 
>> /lib/modules/4.18.0-147.5.1.el8_1.x86_64
>> exit status = 256
>
> I am going to guess this has something to do with how system() and
> waitpid() (and the wait family of syscalls in general) encode the exit
> status in their return values. According to their man pages, you need
> to use the appropriate WIF* macros to get the actual exit code of the
> child process.
>
> From system(3):
>
>    the return value is a "wait status" that can be examined using the
>    macros described in waitpid(2).  (i.e., WIFEXITED(),
>    WEXITSTATUS(), and so on)
>
> From waitpid(2):
>
>     If  wstatus  is  not  NULL,  wait()  and  waitpid() store status
>     information in the int to which it points.  This integer can be
>     inspected with the following macros (which take the integer
>     itself as an argument, not a pointer to it, as is done in wait()
>     and waitpid()!):
>
>       WEXITSTATUS(wstatus)
>              returns the exit status of the child.  This consists of
>              the least significant 8 bits of the status argument that
>              the child specified in a call to exit(3) or _exit(2) or
>              as the argument for a return statement in main(). This
>              macro should be employed only if WIFEXITED returned
>              true.
>
> In your test code, you are reading &status directly. To obtain the
> exit status, you need to use WEXITSTATUS(status), or right shift the
> value by 8 bits. That gives you 1, which was the original exit code
> given by modprobe. That's why you see an exit code of 1 when running
> modprobe directly and you see 256 when using system() and waitpid()
> and don't use the WIF* macros.
>
> As for why __request_module() returns 256, I am guessing this would
> come from kernel_wait4(), but I did not dive into the call path to
> verify this yet.

+Cc Al Viro <viro@...iv.linux.org.uk>

Hi Al,

When module name is empty, __request_module() returns 256.
What do you think about this case and patch?
Thank you very much for your attention.

patch v3:
https://lore.kernel.org/patchwork/patch/1227274/

patch v4 (update the commit message):
https://lore.kernel.org/patchwork/patch/1227981/

>
> Jessica