On Wed, May 29, 2019 at 6:21 PM Oleg Nesterov <o...@redhat.com> wrote: > On 05/29, Jann Horn wrote: > > (I have no clue whatsoever what the relevant tree for this is, but I > > guess Oleg is the relevant maintainer?) > > we usually route ptrace changes via -mm tree, plus I lost my account on korg. > > > --- a/kernel/ptrace.c > > +++ b/kernel/ptrace.c > > @@ -324,6 +324,16 @@ static int __ptrace_may_access(struct task_struct > > *task, unsigned int mode) > > return -EPERM; > > ok: > > rcu_read_unlock(); > > + /* > > + * If a task drops privileges and becomes nondumpable (through a > > syscall > > + * like setresuid()) while we are trying to access it, we must ensure > > + * that the dumpability is read after the credentials; otherwise, > > + * we may be able to attach to a task that we shouldn't be able to > > + * attach to (as if the task had dropped privileges without becoming > > + * nondumpable). > > + * Pairs with a write barrier in commit_creds(). > > + */ > > + smp_rmb(); > > (I am wondering if smp_acquire__after_ctrl_dep() could be used instead, just > to > make this code look more confusing)
Uuh, I had no idea that that barrier type exists. The helper isn't even explicitly mentioned in Documentation/memory-barriers.rst. I don't really want to use dark magic in the middle of ptrace access logic... Anyway, looking at it, I think smp_acquire__after_ctrl_dep() doesn't make sense here; quoting the documentation: "A load-load control dependency requires a full read memory barrier, not simply a data dependency barrier to make it work correctly". IIUC smp_acquire__after_ctrl_dep() is for cases in which you would otherwise need a full memory barrier - smp_mb() - and you want to be able to reduce it to a read barrier. > > mm = task->mm; > > while at it, could you also change this into mm = READ_ONCE(task->mm) ? I'm actually trying to get rid of the ->mm access in __ptrace_may_access() entirely by moving the dumpability and the user_ns into the signal_struct, but I don't have patches for that ready (yet).
