Bernd Edlinger <bernd.edlinger@xxxxxxxxxx> writes: > 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. Jann Horn has convinced me we should just convert these to the exec_change_mutex today. Because while not 100% correct in theory, the only really interesting case is exec. So the code does something interesting and worth while, and mostly correct. The last thing I want to do is to cause an unnecessary regression. Eric
