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

Bernd Edlinger
2020-03-10T20:19:18Z

On 3/10/20 8:06 PM, Eric W. Biederman wrote:
> Bernd Edlinger <bernd.edlinger@hotmail.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@openwall.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@openwall.com>
> Cc: Al Viro <viro@zeniv.linux.org.uk>
> Cc: <stable@kernel.org>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>
>
>
>> Signed-off-by: Bernd Edlinger <bernd.edlinger@hotmail.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;
>> }