linux-kernel - Re: [PATCH v2 00/15] Make the user mode driver code a better citizen

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]

Date:   Thu, 02 Jul 2020 11:02:13 -0500
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp>
Cc:     Alexei Starovoitov <alexei.starovoitov@...il.com>,
        linux-kernel@...r.kernel.org, David Miller <davem@...emloft.net>,
        Greg Kroah-Hartman <greg@...ah.com>,
        Kees Cook <keescook@...omium.org>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexei Starovoitov <ast@...nel.org>,
        Al Viro <viro@...iv.linux.org.uk>, bpf <bpf@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Daniel Borkmann <daniel@...earbox.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Gary Lin <GLin@...e.com>, Bruno Meneguele <bmeneg@...hat.com>,
        LSM List <linux-security-module@...r.kernel.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        Luis Chamberlain <mcgrof@...nel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: [PATCH v2 00/15] Make the user mode driver code a better citizen

Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp> writes:

> On 2020/07/02 22:08, Eric W. Biederman wrote:
>> Tetsuo Handa <penguin-kernel@...ove.sakura.ne.jp> writes:
>> 
>>> On 2020/06/30 21:29, Eric W. Biederman wrote:
>>>> Hmm.  The wake up happens just of tgid->wait_pidfd happens just before
>>>> release_task is called so there is a race.  As it is possible to wake
>>>> up and then go back to sleep before pid_has_task becomes false.
>>>
>>> What is the reason we want to wait until pid_has_task() becomes false?
>>>
>>> - wait_event(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID));
>>> + while (!wait_event_timeout(tgid->wait_pidfd, !pid_has_task(tgid, PIDTYPE_TGID), 1));
>> 
>> So that it is safe to call bpfilter_umh_cleanup.  The previous code
>> performed the wait by having a callback in do_exit.
>
> But bpfilter_umh_cleanup() does only
>
> 	fput(info->pipe_to_umh);
> 	fput(info->pipe_from_umh);
> 	put_pid(info->tgid);
> 	info->tgid = NULL;
>
> which is (I think) already safe regardless of the usermode process because
> bpfilter_umh_cleanup() merely closes one side of two pipes used between
> two processes and forgets about the usermode process.

It is not safe.

Baring bugs there is only one use of shtudown_umh that matters.  The one
in fini_umh.  The use of the file by the mm must be finished before
umd_unload_blob.  AKA unmount.  Which completely frees the filesystem.

>> It might be possible to call bpf_umh_cleanup early but I have not done
>> that analysis.
>> 
>> To perform the test correctly what I have right now is:
>
> Waiting for the termination of a SIGKILLed usermode process is not
> such simple.

The waiting is that simple.

You are correct it might not be a quick process.

A good general principle is to start with something simple and correct
for what it does, and then to make it more complicated when real world
cases show up, and it can be understood what the real challenges are.

I am not going to merge known broken code but I am also not going to
overcomplicate it.

Dealing with very rare and pathological cases that are not handled or
considered today is out of scope for my patchset.

Eric