Messages in this thread | | | Subject | Re: [PATCH 3/4] proc: io_accounting: Use new infrastructure to fix deadlocks in execve | From | Bernd Edlinger <> | Date | Wed, 11 Mar 2020 20:48:31 +0100 |
| |
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@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; >> } >> >> -- >> 1.9.1 >
| |