Re: [PATCH 3/4] proc: io_accounting: Use new infrastructure to fix deadlocks in execve

[Bernd Edlinger]

On 3/10/20 8:06 PM, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> 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@xxxxxxxxxxxx>
> 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@xxxxxxxxxxxx>
>     Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
>     Cc: <stable@xxxxxxxxxx>
>     Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
>     Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> 
> 
> 
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@xxxxxxxxxx>
>> ---
>>  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;
>>  }