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 >
