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