Date:   Tue, 10 Mar 2020 21:19:05 +0100
From:   Bernd Edlinger <bernd.edlinger@...mail.de>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     Christian Brauner <christian.brauner@...ntu.com>,
        Kees Cook <keescook@...omium.org>,
        Jann Horn <jannh@...gle.com>, Jonathan Corbet <corbet@....net>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Oleg Nesterov <oleg@...hat.com>,
        Frederic Weisbecker <frederic@...nel.org>,
        Andrei Vagin <avagin@...il.com>,
        Ingo Molnar <mingo@...nel.org>,
        "Peter Zijlstra (Intel)" <peterz@...radead.org>,
        Yuyang Du <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>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Shakeel Butt <shakeelb@...gle.com>,
        Jason Gunthorpe <jgg@...pe.ca>,
        Christian Kellner <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 3/4] proc: io_accounting: Use new infrastructure to fix
 deadlocks in execve

On 3/10/20 8:06 PM, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@...mail.de> writes:
> 
>> This changes do_io_accounting to use the new exec_update_mutex
>> instead of cred_guard_mutex.
>>
>> This fixes possible deadlocks when the trace is accessing
>> /proc/$pid/io for instance.
>>
>> This should be safe, as the credentials are only used for reading.
> 
> This is an improvement.
> 
> We probably want to do this just as an incremental step in making things
> better but perhaps I am blind but I am not finding the reason for
> guarding this with the cred_guard_mutex to be at all persuasive.
> 
> I think moving the ptrace_may_access check down to after the
> unlock_task_sighand would be just as effective at addressing the
> concerns raised in the original commit.  I think the task_lock provides
> all of the barrier we need to make it safe to move the ptrace_may_access
> checks safe.
> 
> The reason I say this is I don't see exec changing ->ioac.  Just
> performing some I/O which would update the io accounting statistics.
> 

Maybe the suid executable is starting up and doing io or not,
and what the program does immediately at startup is a secret,
that we want to keep secret but evil eve want to find out.
eve is using /proc/alice/io to do that.

It is a bit constructed, but seems like a security concern.
when we keep the exec_update_mutex while collecting the data, we
cannot see any io of the new process when the new credentials
don't allow that.


Bernd.

> Can anyone see if I am wrong?
> 
> Eric
> 
> 
> commit 293eb1e7772b25a93647c798c7b89bf26c2da2e0
> Author: Vasiliy Kulikov <segoon@...nwall.com>
> Date:   Tue Jul 26 16:08:38 2011 -0700
> 
>     proc: fix a race in do_io_accounting()
>     
>     If an inode's mode permits opening /proc/PID/io and the resulting file
>     descriptor is kept across execve() of a setuid or similar binary, the
>     ptrace_may_access() check tries to prevent using this fd against the
>     task with escalated privileges.
>     
>     Unfortunately, there is a race in the check against execve().  If
>     execve() is processed after the ptrace check, but before the actual io
>     information gathering, io statistics will be gathered from the
>     privileged process.  At least in theory this might lead to gathering
>     sensible information (like ssh/ftp password length) that wouldn't be
>     available otherwise.
>     
>     Holding task->signal->cred_guard_mutex while gathering the io
>     information should protect against the race.
>     
>     The order of locking is similar to the one inside of ptrace_attach():
>     first goes cred_guard_mutex, then lock_task_sighand().
>     
>     Signed-off-by: Vasiliy Kulikov <segoon@...nwall.com>
>     Cc: Al Viro <viro@...iv.linux.org.uk>
>     Cc: <stable@...nel.org>
>     Signed-off-by: Andrew Morton <akpm@...ux-foundation.org>
>     Signed-off-by: Linus Torvalds <torvalds@...ux-foundation.org>
> 
> 
> 
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@...mail.de>
>> ---
>>  fs/proc/base.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/proc/base.c b/fs/proc/base.c
>> index 4fdfe4f..529d0c6 100644
>> --- a/fs/proc/base.c
>> +++ b/fs/proc/base.c
>> @@ -2770,7 +2770,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
>>  	unsigned long flags;
>>  	int result;
>>  
>> -	result = mutex_lock_killable(&task->signal->cred_guard_mutex);
>> +	result = mutex_lock_killable(&task->signal->exec_update_mutex);
>>  	if (result)
>>  		return result;
>>  
>> @@ -2806,7 +2806,7 @@ static int do_io_accounting(struct task_struct *task, struct seq_file *m, int wh
>>  	result = 0;
>>  
>>  out_unlock:
>> -	mutex_unlock(&task->signal->cred_guard_mutex);
>> +	mutex_unlock(&task->signal->exec_update_mutex);
>>  	return result;
>>  }

Powered by blists - more mailing lists