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

From: Bernd Edlinger
Date: Wed Mar 11 2020 - 15:49:02 EST


On 3/11/20 8:08 PM, Kees Cook wrote:
> On Tue, Mar 10, 2020 at 06:45:47PM +0100, Bernd Edlinger wrote:
>> 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.
>
> I'd like to see the rationale described better here for why it should be
> safe. I'm still not seeing why this is safe here, as we might check
> ptrace_may_access() with one cred and then iterate io accounting with a
> different credential...
>
> What am I missing?
>

The same here, even if execve is already started, the credentials
are not actually changed until the execve acquired the exec_update_mutex.

The data flow is from the task->cred => do_io_accounting,
if the data flow would be from do_io_accounting => task's no new privs
you would see an entirely different patch.

I am open for suggestions how to improve the description, or even
add a comment from time to time :)

Thanks
Bernd.

> -Kees
>
>>
>> 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;
>> }
>>
>> --
>> 1.9.1
>