[PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

Restore the read memory barrier in __ptrace_may_access() that was deleted
a couple years ago. Also add comments on this barrier and the one it pairs
with to explain why they're there (as far as I understand).

Fixes: bfedb589252c ("mm: Add a user_ns owner to mm_struct and fix ptrace permission checks")
Cc: stable@vger.kernel.org
Signed-off-by: Jann Horn <jannh@google.com>
---
(I have no clue whatsoever what the relevant tree for this is, but I
guess Oleg is the relevant maintainer?)

 kernel/cred.c   |  9 +++++++++
 kernel/ptrace.c | 10 ++++++++++
 2 files changed, 19 insertions(+)

diff --git a/kernel/cred.c b/kernel/cred.c
index 45d77284aed0..07e069d00696 100644
--- a/kernel/cred.c
+++ b/kernel/cred.c
@@ -450,6 +450,15 @@ int commit_creds(struct cred *new)
 		if (task->mm)
 			set_dumpable(task->mm, suid_dumpable);
 		task->pdeath_signal = 0;
+		/*
+		 * If a task drops privileges and becomes nondumpable,
+		 * the dumpability change must become visible before
+		 * the credential change; otherwise, a __ptrace_may_access()
+		 * racing with this change may be able to attach to a task it
+		 * shouldn't be able to attach to (as if the task had dropped
+		 * privileges without becoming nondumpable).
+		 * Pairs with a read barrier in __ptrace_may_access().
+		 */
 		smp_wmb();
 	}
 
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 5710d07e67cf..e54452c2954b 100644
--- 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();
 	mm = task->mm;
 	if (mm &&
 	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
-- 
2.22.0.rc1.257.g3120a18244-goog

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Eric W. Biederman]

Jann Horn <jannh@google.com> writes:

> Restore the read memory barrier in __ptrace_may_access() that was deleted
> a couple years ago. Also add comments on this barrier and the one it pairs
> with to explain why they're there (as far as I understand).

My bad.

When I made that change I could not figure out what that barrier was
for, and it did not appear necessary.

Do you happen to know of any real world problems?

Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>

If no one else would prefer to pick this up I will grab it.  I have
another bug fix I already queueing for 5.2-rcX.

Thank you,
Eric

>
> Fixes: bfedb589252c ("mm: Add a user_ns owner to mm_struct and fix ptrace permission checks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>
> ---
> (I have no clue whatsoever what the relevant tree for this is, but I
> guess Oleg is the relevant maintainer?)
>
>  kernel/cred.c   |  9 +++++++++
>  kernel/ptrace.c | 10 ++++++++++
>  2 files changed, 19 insertions(+)
>
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 45d77284aed0..07e069d00696 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -450,6 +450,15 @@ int commit_creds(struct cred *new)
>  		if (task->mm)
>  			set_dumpable(task->mm, suid_dumpable);
>  		task->pdeath_signal = 0;
> +		/*
> +		 * If a task drops privileges and becomes nondumpable,
> +		 * the dumpability change must become visible before
> +		 * the credential change; otherwise, a __ptrace_may_access()
> +		 * racing with this change may be able to attach to a task it
> +		 * shouldn't be able to attach to (as if the task had dropped
> +		 * privileges without becoming nondumpable).
> +		 * Pairs with a read barrier in __ptrace_may_access().
> +		 */
>  		smp_wmb();
>  	}
>  
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 5710d07e67cf..e54452c2954b 100644
> --- 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();
>  	mm = task->mm;
>  	if (mm &&
>  	    ((get_dumpable(mm) != SUID_DUMP_USER) &&

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Wed, May 29, 2019 at 5:59 PM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Jann Horn <jannh@google.com> writes:
>
> > Restore the read memory barrier in __ptrace_may_access() that was deleted
> > a couple years ago. Also add comments on this barrier and the one it pairs
> > with to explain why they're there (as far as I understand).
>
> My bad.
>
> When I made that change I could not figure out what that barrier was
> for, and it did not appear necessary.
>
> Do you happen to know of any real world problems?

No, this is just from reading the code and trying to make other
changes to it. (I'm trying to figure out how to finally deal with some
of the other issues in the ptrace access check, so I've been staring
at that code a lot over the last couple days.)

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Oleg Nesterov]

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)

>  	mm = task->mm;

while at it, could you also change this into mm = READ_ONCE(task->mm) ?

Oleg.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Wed, May 29, 2019 at 6:21 PM Oleg Nesterov <oleg@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).

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Eric W. Biederman]

Jann Horn <jannh@google.com> writes:

> 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).

Do you have a plan for dealing with old linux-threads style threads
where you have two processes that share the same mm, but have different
signal_structs.

I don't think it is required to share any other structures except
mm_struct when you share mm_struct.  Maybe sighand_struct.

Not to derail your idea.  Only needing to look at signal_struct sounds
very nice.  I just know we have some other somewhat bizarre cases the
kernel still supports.

Eric

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Thu, May 30, 2019 at 3:42 AM Eric W. Biederman <ebiederm@xmission.com> wrote:
> Jann Horn <jannh@google.com> writes:
>
> > 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).
>
> Do you have a plan for dealing with old linux-threads style threads
> where you have two processes that share the same mm, but have different
> signal_structs.

Oh, I hadn't realized that linux-threads exists and uses this feature...

> I don't think it is required to share any other structures except
> mm_struct when you share mm_struct.  Maybe sighand_struct.
>
> Not to derail your idea.  Only needing to look at signal_struct sounds
> very nice.  I just know we have some other somewhat bizarre cases the
> kernel still supports.

My line of thought was that conceptually, dumpability doesn't have
much to do with the mm_struct. Dumpability has the following purposes,
as far as I know:


1. Prevent the creation of core dumps with elevated privileges in
attacker-specified locations (if the core_pattern is a relative path).
This could happen in the following scenarios:
1a) setuid executable crashes with elevated privileges (dumpability is
reduced in setup_new_exec() on privileged execve() for this reason)
1b) a privileged process is deliberately sharing its fs_struct with an
attacker-controlled one
2. Prevent reading the memory of processes that are running
execute-only binaries [*] (dumpability is reduced in would_dump() on
execve() for this reason)
3. Prevent ptrace-attaching (and similar forms of access) against
formerly-privileged processes that have dropped UID/GID/caps
privileges, but still have some other form of privilege left (e.g.
file descriptors). Similarly, prevent reading process memory after a
privilege transition by triggering a core dump.

For numbers 1a and 2, it doesn't matter on which level the flag is -
during execve, the signal_struct and the new mm_struct are both not
shared, so the effect is the same.

For number 1b, you're probably not sharing the mm_struct, so either
way the privileged process needs to mark itself as nondumpable or
already be nondumpable for some reason. (I think I know a single
example where this actually happens, and that one's a setuid helper,
so it's nondumpable from the start anyway.)

For number 3, when the kernel automatically marks a process as
nondumpable during commit_creds(), I don't think it matters for
security on which level that change happens, whether it's on the task
level, the signal_struct level, or the mm_struct level, since you can
only attach to a task that has itself gone through the
privilege-dropping process - in other words, as long as the scope of
the dumpability flag includes the scope of the credentials (which is
per-task), it should be fine. I think the behavior actually makes more
sense here if it happens on the signal_struct level - for number 3, if
one process with shared mm drops privileges, that is irrelevant for
other processes sharing the mm, since they remain inaccessible until
they also go through a similar credential change.


For manual control of the dumpability through PR_SET_DUMPABLE, the
prctl(2) manpage says that dumpability is a property of "the process",
which is the same wording that is also used for per-task properties
and at least one per-thread-group property in there; so I was hoping
that I could get away with just fudging the semantics so that
PR_SET_DUMPABLE only affects the thread group. In case someone tells
me that I can't do that because it would break something, my backup
plan was to do something really ugly in PR_SET_DUMPABLE, similar to
what zap_threads() and __set_oom_adj() do, like this:

if (refcount_read(current->signal->sigcnt) != 1) {
  for_each_process(p) {
    if (READ_ONCE(p->mm) != current->mm)
      continue;
    task_lock(p);
    if (p->mm == current->mm)
      WRITE_ONCE(p->signal->dumpable, dumpable);
    task_unlock(p);
  }
}

But I'd really like to avoid that, because it makes the code messier,
slower, and in my opinion, less logical.


The reason why I want to make this change is that I think the current
fail-open behavior of __ptrace_may_access() for a process whose
mm_struct has gone away is dangerous; but I also don't want to just
make it fail-closed. So I have to shove the dumpability information
somewhere else (or muck around with the mm_struct's lifetime, but I'd
like to avoid that).


[*]: That doesn't really work though, does it? I don't think anything
prevents running an execute-only program in a task that already has a
ptracer attached to it. And for dynamically-linked binaries, I think
you can probably still create a new user namespace, do chroot() in
there, and then the kernel will resolve the absolute path to the ELF
loader inside the chroot(), and then your own fake ELF loader can dump
the binary. And I don't think the AT_SECURE flag gets set, so you can
use LD_PRELOAD or whatever.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Andrea Parri]

On Wed, May 29, 2019 at 07:38:46PM +0200, Jann Horn wrote:
> On Wed, May 29, 2019 at 6:21 PM Oleg Nesterov <oleg@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.

It is supposed to be used when you want an ACQUIRE but you only have a
control dependency (so you "augment the dependency" with this barrier).

FWIW, I do agree on the "dark magic"..., and I'd strongly recommend to
not use this barrier (or, at least, to use it with high suspicion).

  Andrea

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Peter Zijlstra]

On Thu, May 30, 2019 at 12:34:05PM +0200, Andrea Parri wrote:
> On Wed, May 29, 2019 at 07:38:46PM +0200, Jann Horn wrote:
> > On Wed, May 29, 2019 at 6:21 PM Oleg Nesterov <oleg@redhat.com> wrote:

> > > (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...

Yeah, it's sorta not documented on purpose. It's too easy to get wrong
and we've only used it inside a number of more convenient primitives as
an optimzation.

I suppose we could add it to the section on control dependencies; just
to scare more people :-)

> > 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.
> 
> It is supposed to be used when you want an ACQUIRE but you only have a
> control dependency (so you "augment the dependency" with this barrier).
> 
> FWIW, I do agree on the "dark magic"..., and I'd strongly recommend to
> not use this barrier (or, at least, to use it with high suspicion).

Right, so the purpose of the barrier is to upgrade a LOAD->STORE order
(as provided by the ctrl-dep) to a LOAD->{LOAD,STORE} order as would be
provided by load-acquire.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Oleg Nesterov]

On 05/29, Jann Horn wrote:
>
> > (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...

Yes. and if it was not clear I didn't try to seriously suggest to use this
barrier. I was just curious if it can be used or not in this particular case.

> Anyway, looking at it, I think smp_acquire__after_ctrl_dep() doesn't
> make sense here;

Well I still _think_ it should work, it provides the LOAD-LOAD ordering
and this is what we need.

But I can be easily wrong, and again, I wasn't serious.

Oleg.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Peter Zijlstra]

On Thu, May 30, 2019 at 02:05:31PM +0200, Oleg Nesterov wrote:
> > Anyway, looking at it, I think smp_acquire__after_ctrl_dep() doesn't
> > make sense here;
> 
> Well I still _think_ it should work, it provides the LOAD-LOAD ordering
> and this is what we need.

So it hard relies on being part of a control dependency, note how the
comment says that architectures that do not do load speculation can
override the smp_rmb() default with barrier() (and we used have an
architecture that made use of that, although it's been since removed).

IOW, it is an error to use smp_acquire__after_ctrl_dep() without an
(immediate) preceding branch.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Oleg Nesterov]

On 05/31, Peter Zijlstra wrote:
>
> On Thu, May 30, 2019 at 02:05:31PM +0200, Oleg Nesterov wrote:
> > > Anyway, looking at it, I think smp_acquire__after_ctrl_dep() doesn't
> > > make sense here;
> >
> > Well I still _think_ it should work, it provides the LOAD-LOAD ordering
> > and this is what we need.
>
> So it hard relies on being part of a control dependency,

Yes,

> IOW, it is an error to use smp_acquire__after_ctrl_dep() without an
> (immediate) preceding branch.

and it is still not clear to me if __ptrace_may_acess() has a control
dependency or not,

		if (uid_eq(caller_uid, tcred->euid) && ...)
			goto ok;
		retuurn;

	ok:
		// provide LOAD->LOAD
		smp_acquire__after_ctrl_dep();


again, again, I didn't suggest to change the patch, I was just curious
if it would be correct or not.

Oleg.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Wed, May 29, 2019 at 6:21 PM Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/29, Jann Horn wrote:
> > --- a/kernel/ptrace.c
> > +++ b/kernel/ptrace.c
> > @@ -324,6 +324,16 @@ static int __ptrace_may_access(struct task_struct *task, unsigned int mode)
[...]
> >       mm = task->mm;
>
> while at it, could you also change this into mm = READ_ONCE(task->mm) ?

Actually, that shouldn't be necessary. The caller of
__ptrace_may_access() holds the task_lock() on the task, and that
should prevent concurrent updates of ->mm. If concurrent updates of
->mm *were* possible, we'd probably be in deep trouble here (and by
that I mean use-after-free).

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Kees Cook]

On Wed, May 29, 2019 at 01:31:57PM +0200, Jann Horn wrote:
> Restore the read memory barrier in __ptrace_may_access() that was deleted
> a couple years ago. Also add comments on this barrier and the one it pairs
> with to explain why they're there (as far as I understand).
> 
> Fixes: bfedb589252c ("mm: Add a user_ns owner to mm_struct and fix ptrace permission checks")
> Cc: stable@vger.kernel.org
> Signed-off-by: Jann Horn <jannh@google.com>

Ah, thanks! Nice find.

Acked-by: Kees Cook <keescook@chromium.org>

(And, yeah, Eric, I say snag it if you've got stuff queued up...)

-Kees

> ---
> (I have no clue whatsoever what the relevant tree for this is, but I
> guess Oleg is the relevant maintainer?)
> 
>  kernel/cred.c   |  9 +++++++++
>  kernel/ptrace.c | 10 ++++++++++
>  2 files changed, 19 insertions(+)
> 
> diff --git a/kernel/cred.c b/kernel/cred.c
> index 45d77284aed0..07e069d00696 100644
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -450,6 +450,15 @@ int commit_creds(struct cred *new)
>  		if (task->mm)
>  			set_dumpable(task->mm, suid_dumpable);
>  		task->pdeath_signal = 0;
> +		/*
> +		 * If a task drops privileges and becomes nondumpable,
> +		 * the dumpability change must become visible before
> +		 * the credential change; otherwise, a __ptrace_may_access()
> +		 * racing with this change may be able to attach to a task it
> +		 * shouldn't be able to attach to (as if the task had dropped
> +		 * privileges without becoming nondumpable).
> +		 * Pairs with a read barrier in __ptrace_may_access().
> +		 */
>  		smp_wmb();
>  	}
>  
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 5710d07e67cf..e54452c2954b 100644
> --- 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();
>  	mm = task->mm;
>  	if (mm &&
>  	    ((get_dumpable(mm) != SUID_DUMP_USER) &&
> -- 
> 2.22.0.rc1.257.g3120a18244-goog
> 

-- 
Kees Cook

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Oleg Nesterov]

On 05/29, Jann Horn wrote:
>
> --- a/kernel/cred.c
> +++ b/kernel/cred.c
> @@ -450,6 +450,15 @@ int commit_creds(struct cred *new)
>  		if (task->mm)
>  			set_dumpable(task->mm, suid_dumpable);
>  		task->pdeath_signal = 0;
> +		/*
> +		 * If a task drops privileges and becomes nondumpable,
> +		 * the dumpability change must become visible before
> +		 * the credential change; otherwise, a __ptrace_may_access()
> +		 * racing with this change may be able to attach to a task it
> +		 * shouldn't be able to attach to (as if the task had dropped
> +		 * privileges without becoming nondumpable).
> +		 * Pairs with a read barrier in __ptrace_may_access().
> +		 */
>  		smp_wmb();

Hmm. Now that I tried to actually read this patch I do not understand this wmb().

commit_creds() does rcu_assign_pointer(real_cred) which implies smp_store_release(),
the dumpability change must be visible before ->real_cred is updated without any
additional barriers?

Oleg.

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Thu, May 30, 2019 at 2:35 PM Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/29, Jann Horn wrote:
> > --- a/kernel/cred.c
> > +++ b/kernel/cred.c
> > @@ -450,6 +450,15 @@ int commit_creds(struct cred *new)
> >               if (task->mm)
> >                       set_dumpable(task->mm, suid_dumpable);
> >               task->pdeath_signal = 0;
> > +             /*
> > +              * If a task drops privileges and becomes nondumpable,
> > +              * the dumpability change must become visible before
> > +              * the credential change; otherwise, a __ptrace_may_access()
> > +              * racing with this change may be able to attach to a task it
> > +              * shouldn't be able to attach to (as if the task had dropped
> > +              * privileges without becoming nondumpable).
> > +              * Pairs with a read barrier in __ptrace_may_access().
> > +              */
> >               smp_wmb();
>
> Hmm. Now that I tried to actually read this patch I do not understand this wmb().
>
> commit_creds() does rcu_assign_pointer(real_cred) which implies smp_store_release(),
> the dumpability change must be visible before ->real_cred is updated without any
> additional barriers?

Oh, yes, I think you're right.

So I guess I should make a v2 that still adds the smp_rmb() in
__ptrace_may_access(), but gets rid of the smp_wmb() in
commit_creds()? (With a comment above the rcu_assign_pointer() that
explains the ordering?)

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Oleg Nesterov]

On 05/31, Jann Horn wrote:
>
> So I guess I should make a v2 that still adds the smp_rmb() in
> __ptrace_may_access(), but gets rid of the smp_wmb() in
> commit_creds()? (With a comment above the rcu_assign_pointer() that
> explains the ordering?)

I am fine either way, whatever you like more.

If you prefer v1 (this patch), feel free to add

Acked-by: Oleg Nesterov <oleg@redhat.com>

Re: [PATCH] ptrace: restore smp_rmb() in __ptrace_may_access()

[Jann Horn]

On Fri, May 31, 2019 at 3:35 PM Oleg Nesterov <oleg@redhat.com> wrote:
> On 05/31, Jann Horn wrote:
> >
> > So I guess I should make a v2 that still adds the smp_rmb() in
> > __ptrace_may_access(), but gets rid of the smp_wmb() in
> > commit_creds()? (With a comment above the rcu_assign_pointer() that
> > explains the ordering?)
>
> I am fine either way, whatever you like more.
>
> If you prefer v1 (this patch), feel free to add
>
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Thanks! I think I actually like the current version more, since this
way, we have a nice simple pairing of smp_rmb() and smp_wmb().
Otherwise we end up mixing smp_rmb() with smp_store_release(), which I
find kind of weird. And to me, semantically, it also seems slightly
weird to use rcu_assign_pointer() as a barrier for previous writes
that are not pointed to by the pointer being assigned. Maybe I'm just
not familiar enough with the details of how memory ordering works to
feel comfortable with it...