linux-kernel - Re: [PATCH net-next] modules: allow modprobe load regular elf binaries

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: <d83869b4-260e-179d-d469-83b8d8068399@fb.com>
Date:   Thu, 8 Mar 2018 15:07:01 -0800
From:   Alexei Starovoitov <ast@...com>
To:     "Luis R. Rodriguez" <mcgrof@...nel.org>,
        Alexei Starovoitov <ast@...nel.org>
CC:     <davem@...emloft.net>, <daniel@...earbox.net>,
        <torvalds@...ux-foundation.org>, <gregkh@...uxfoundation.org>,
        <netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <kernel-team@...com>, <linux-api@...r.kernel.org>,
        Jessica Yu <jeyu@...nel.org>,
        Kees Cook <keescook@...omium.org>,
        "Rafael J. Wysocki" <rjw@...ysocki.net>,
        Mimi Zohar <zohar@...ux.vnet.ibm.com>,
        Jiri Kosina <jikos@...nel.org>
Subject: Re: [PATCH net-next] modules: allow modprobe load regular elf
 binaries

On 3/7/18 5:23 PM, Luis R. Rodriguez wrote:
>
> request_module() has its own world though too. How often in your proof of
> concept is request_module() called? How many times do you envision it being
> called?

once.

>> +static int run_umh(struct file *file)
>> +{
>> +	struct subprocess_info *sub_info = call_usermodehelper_setup_file(file);
>> +
>> +	if (!sub_info)
>> +		return -ENOMEM;
>> +	return call_usermodehelper_exec(sub_info, UMH_WAIT_EXEC);
>> +}
>
> run_umh() calls the program and waits. Note that while we are running a UMH we
> can't suspend. What if they take forever, who is hosing them down with an
> equivalent:
>
> 	schedule();
> 	try_to_freeze();
>
> Say they are buggy and never return, will they stall suspend, have you tried?
>
> call_usermodehelper_exec() uses helper_lock() which in turn is used for
> umh's accounting for number of running umh's. One of the sad obscure uses
> for this is to deal with suspend/resume. Refer to __usermodehelper* calls
> on kernel/power/process.c
>
> Note how you use UMH_WAIT_EXEC too, so this is all synchronous.

looks like you misread this code and the rest of your concerns
with suspend/resume are not applicable any more.

#define UMH_NO_WAIT     0       /* don't wait at all */
#define UMH_WAIT_EXEC   1       /* wait for the exec, but not the process */
#define UMH_WAIT_PROC   2       /* wait for the process to complete */
#define UMH_KILLABLE    4       /* wait for EXEC/PROC killable */

bpftiler.ko is started once and runs non-stop from there on.
Unless it crashes, but it's a different discussion.

>> +	if (info->hdr->e_type == ET_EXEC) {
>> +#ifdef CONFIG_MODULE_SIG
>> +		if (!info->sig_ok) {
>> +			pr_notice_once("umh %s verification failed: signature and/or required key missing - tainting kernel\n",
>> +				       info->file->f_path.dentry->d_name.name);
>> +			add_taint(TAINT_UNSIGNED_MODULE, LOCKDEP_STILL_OK);
>> +		}
>> +#endif
>
> So I guess this check is done *after* run_umh() then, what about the enforce mode,
> don't we want to reject loading at all in any circumstance?

already answered this twice in the thread.