linux-kernel - Re: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with ptrace

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: <AM6PR03MB51700B30078BDCB6C6A4E0CAE4CA0@AM6PR03MB5170.eurprd03.prod.outlook.com>
Date:   Sun, 29 Mar 2020 06:31:11 +0200
From:   Bernd Edlinger <bernd.edlinger@...mail.de>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        Kirill Tkhai <ktkhai@...tuozzo.com>,
        Christian Brauner <christian.brauner@...ntu.com>,
        Kees Cook <keescook@...omium.org>,
        "jannh@...gle.com" <jannh@...gle.com>,
        Jonathan Corbet <corbet@....net>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        "adobriyan@...il.com" <adobriyan@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Oleg Nesterov <oleg@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        "avagin@...il.com" <avagin@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        "duyuyang@...il.com" <duyuyang@...il.com>,
        David Hildenbrand <david@...hat.com>,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Anshuman Khandual <anshuman.khandual@....com>,
        David Howells <dhowells@...hat.com>,
        James Morris <jamorris@...ux.microsoft.com>,
        Shakeel Butt <shakeelb@...gle.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        "christian@...lner.me" <christian@...lner.me>,
        Andrea Arcangeli <aarcange@...hat.com>,
        Aleksa Sarai <cyphar@...har.com>,
        "Dmitry V. Levin" <ldv@...linux.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
        "linux-mm@...ck.org" <linux-mm@...ck.org>,
        "stable@...r.kernel.org" <stable@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>
Subject: Re: [PATCH v6 15/16] exec: Fix dead-lock in de_thread with
 ptrace_attach

On 3/25/20 3:27 PM, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@...mail.de> writes:
> 
>> This removes the last users of cred_guard_mutex
>> and replaces it with a new mutex exec_guard_mutex,
>> and a boolean unsafe_execve_in_progress.
>>
>> This addresses the case when at least one of the
>> sibling threads is traced, and therefore the trace
>> process may dead-lock in ptrace_attach, but de_thread
>> will need to wait for the tracer to continue execution.
>>
>> The solution is to detect this situation and make
>> ptrace_attach and similar functions return -EAGAIN,
>> but only in a situation where a dead-lock is imminent.
>>
>> This means this is an API change, but only when the
>> process is traced while execve happens in a
>> multi-threaded application.
>>
>> See tools/testing/selftests/ptrace/vmaccess.c
>> for a test case that gets fixed by this change.
> 
> Hmm.  The logic with unsafe_execve_in_progress is interesting.
> I think I see what you are aiming for.
> 
> So far as you have hit what you are aiming for I think this is
> a safe change as the only cases that will change are the cases
> that would deadlock today.
> 
> At a minimum the code is subtle and I don't see big fat
> warning comments that subtle code needs to keep people
> from using it wrong.
> 

Okay, I can add big fat warning comments, yeah.

> Further while the change below to proc_pid_attr_write looks
> like it is being treated the same as ptrace_attach.  When in
> fact proc_pid_attr_write needs the no_new_privs and ptrace_attach
> protection the same as exec.  As the updated cred won't be used in an
> ongoing exec, exec does not need protection from proc_pid_attr_write,
> other than deadlock protection.
> 

Not sure I understand this comment correct.
You refer to this block here:

> @@ -2680,14 +2680,17 @@ static ssize_t proc_pid_attr_write(struct file * file, const char __user * buf,
>         }
> 
>         /* Guard against adverse ptrace interaction */
> -       rv = mutex_lock_interruptible(&current->signal->cred_guard_mutex);
> +       rv = mutex_lock_interruptible(&current->signal->exec_guard_mutex);
>         if (rv < 0)
>                 goto out_free;
> 
> -       rv = security_setprocattr(PROC_I(inode)->op.lsm,
> -                                 file->f_path.dentry->d_name.name, page,
> -                                 count);
> -       mutex_unlock(&current->signal->cred_guard_mutex);
> +       if (unlikely(current->signal->unsafe_execve_in_progress))
> +               rv = -EAGAIN;
> +       else
> +               rv = security_setprocattr(PROC_I(inode)->op.lsm,
> +                                         file->f_path.dentry->d_name.name,
> +                                         page, count);
> +       mutex_unlock(&current->signal->exec_guard_mutex);
>  out_free:
>         kfree(page);

I think the logic is correct, but instead if an if-then-else,
I need the big-fat-warning-comment followed by if-unsafe-goto-mutex-unlock
kind of thing, so it looks more like the other places, right?


> Having the relevant lock be per task_struct lock would probably be a
> better way to avoid deadlock with a concurrent proc_pid_attr_write.
> 

Please elaborate your idea a bit.

> 
> So I am going to pass on these last two patches for now, and apply the
> rest and get them into linux-next.
> 

No problem, I can update this patch and if you like take it to your tree,
otherwise it is of course not the most important issue in the world ;-)


Thanks
Bernd.