[PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Oleg Nesterov]

selinux_setprocattr() does ptrace_parent(p) under task_lock(p),
but task_struct->alloc_lock doesn't pin ->parent or ->ptrace,
this looks confusing and triggers the "suspicious RCU usage"
warning because ptrace_parent() does rcu_dereference_check().

And in theory this is wrong, spin_lock()->preempt_disable()
doesn't necessarily imply rcu_read_lock() we need to access
the ->parent.

The patch also checks pid_alive(p) before ptrace_parent(p) to
ensure that this task can't be dead even before rcu_read_lock(),
in this case its ->parent points to nowhere. This is not really
needed "in practice", task->ptrace must be already cleared in
this case but we should not rely on this.

Note: perhaps we should simply kill ptrace_parent(), it buys
almost nothing and it is obviously racy. Or perhaps we should
change it to ensure it can't wrongly return the natural parent
if it races with ptrace_detach.

Reported-by: Evan McNabb <emcnabb@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 security/selinux/hooks.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 794c3ca..2adfd7a 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5503,11 +5503,14 @@ static int selinux_setprocattr(struct task_struct *p,
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and fail. */
 		ptsid = 0;
-		task_lock(p);
-		tracer = ptrace_parent(p);
-		if (tracer)
-			ptsid = task_sid(tracer);
-		task_unlock(p);
+		tracer = NULL;
+		rcu_read_lock();
+		if (pid_alive(p)) {
+			tracer = ptrace_parent(p);
+			if (tracer)
+				ptsid = task_sid(tracer);
+		}
+		rcu_read_unlock();
 
 		if (tracer) {
 			error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS,
-- 
1.5.5.1

Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Paul Moore]

On Thursday, December 05, 2013 05:59:53 PM Oleg Nesterov wrote:
> selinux_setprocattr() does ptrace_parent(p) under task_lock(p),
> but task_struct->alloc_lock doesn't pin ->parent or ->ptrace,
> this looks confusing and triggers the "suspicious RCU usage"
> warning because ptrace_parent() does rcu_dereference_check().
> 
> And in theory this is wrong, spin_lock()->preempt_disable()
> doesn't necessarily imply rcu_read_lock() we need to access
> the ->parent.
> 
> The patch also checks pid_alive(p) before ptrace_parent(p) to
> ensure that this task can't be dead even before rcu_read_lock(),
> in this case its ->parent points to nowhere. This is not really
> needed "in practice", task->ptrace must be already cleared in
> this case but we should not rely on this.
> 
> Note: perhaps we should simply kill ptrace_parent(), it buys
> almost nothing and it is obviously racy. Or perhaps we should
> change it to ensure it can't wrongly return the natural parent
> if it races with ptrace_detach.

Can you elaborate on "kill ptrace_parent()"?  If the process is being traced 
we do need to fetch the tracer's task_struct for use in the SELinux access 
check at this bottom of the diff below.  If you have something better in mind 
than ptrace_parent() it would be helpful to share that ...
 
> Reported-by: Evan McNabb <emcnabb@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  security/selinux/hooks.c |   13 ++++++++-----
>  1 files changed, 8 insertions(+), 5 deletions(-)
> 
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 794c3ca..2adfd7a 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5503,11 +5503,14 @@ static int selinux_setprocattr(struct task_struct
> *p, /* Check for ptracing, and update the task SID if ok.
>  		   Otherwise, leave SID unchanged and fail. */
>  		ptsid = 0;
> -		task_lock(p);
> -		tracer = ptrace_parent(p);
> -		if (tracer)
> -			ptsid = task_sid(tracer);
> -		task_unlock(p);
> +		tracer = NULL;
> +		rcu_read_lock();
> +		if (pid_alive(p)) {
> +			tracer = ptrace_parent(p);
> +			if (tracer)
> +				ptsid = task_sid(tracer);
> +		}
> +		rcu_read_unlock();
> 
>  		if (tracer) {
>  			error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS,

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Oleg Nesterov]

On 12/05, Paul Moore wrote:
>
> On Thursday, December 05, 2013 05:59:53 PM Oleg Nesterov wrote:
> >
> > Note: perhaps we should simply kill ptrace_parent(), it buys
> > almost nothing and it is obviously racy. Or perhaps we should
> > change it to ensure it can't wrongly return the natural parent
> > if it races with ptrace_detach.
>
> Can you elaborate on "kill ptrace_parent()"?  If the process is being traced
> we do need to fetch the tracer's task_struct for use in the SELinux access
> check at this bottom of the diff below.  If you have something better in mind
> than ptrace_parent() it would be helpful to share that ...

Sorry for confusion.

I meant that the code like

	tracer = ptrace_parent(p);
	if (tracer)
		do_something(tracer);

doesn't look better than just

	if (p->ptrace)
		do_something(p->parent);

but this is subjective of course.

And perhaps I am wrong. Because otoh the usage of ->ptrace should be
avoided outside of the core kernel code.

Mostly it annoys me because it is racy, without tasklist_lock it can
return ->real_parent (which never traced its child) if it races with
attach or detach, and I do not see a simple fix.

Oleg.

Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Paul Moore]

On Fri, Dec 6, 2013 at 9:47 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 12/05, Paul Moore wrote:
>> On Thursday, December 05, 2013 05:59:53 PM Oleg Nesterov wrote:
>> >
>> > Note: perhaps we should simply kill ptrace_parent(), it buys
>> > almost nothing and it is obviously racy. Or perhaps we should
>> > change it to ensure it can't wrongly return the natural parent
>> > if it races with ptrace_detach.
>>
>> Can you elaborate on "kill ptrace_parent()"?  If the process is being traced
>> we do need to fetch the tracer's task_struct for use in the SELinux access
>> check at this bottom of the diff below.  If you have something better in mind
>> than ptrace_parent() it would be helpful to share that ...
>
> Sorry for confusion.
>
> I meant that the code like
>
>         tracer = ptrace_parent(p);
>         if (tracer)
>                 do_something(tracer);
>
> doesn't look better than just
>
>         if (p->ptrace)
>                 do_something(p->parent);
>
> but this is subjective of course.

First, my apologizes for such a late reply, I got stuck working on
some other bugs and this fell on the back burner for a bit.

I understand your point, but I still think there is some value in
keeping the call to ptrace_parent() rather than fetching the ptrace
pointer on our own.  Not only does it insulate the SELinux code from
any future changes in the task_struct (unlikely, but still ...) it
also helps act as a warning if something significant changes, e.g.
ptrace_parent() goes away for some reason.  As you say, it's largely
subjective, but I still see some objective value in sticking with the
ptrace_parent() call for the time being.

However, that said, I think we should try and do something about the
"suspicious RCU usage" you mentioned in your original posting.  Adding
the RCU read locks as you do in your patch seems reasonable to me, but
I'm curious about the removal of the task lock; shouldn't week keep
the task lock in place?

> And perhaps I am wrong. Because otoh the usage of ->ptrace should be
> avoided outside of the core kernel code.

Not to muddy things up, but one could argue that this particular
LSM/SELinux hook should be regarded as part of the "core" kernel code.
 However, I'm not sure that the distinction is really important here.

> Mostly it annoys me because it is racy, without tasklist_lock it can
> return ->real_parent (which never traced its child) if it races with
> attach or detach, and I do not see a simple fix.

Neither do I, and I'm fairly certain I haven't looked at this as long
as others have.

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Oleg Nesterov]

On 12/14, Paul Moore wrote:
>
> I understand your point, but I still think there is some value in
> keeping the call to ptrace_parent() rather than fetching the ptrace
> pointer on our own.

Yes, agreed, I changed my mind ;)

> However, that said, I think we should try and do something about the
> "suspicious RCU usage" you mentioned in your original posting.

Yes, this was the only motivation for this patch.

> but
> I'm curious about the removal of the task lock; shouldn't week keep
> the task lock in place?

Why? It protects nothing in this case, afaics. Unless of course it
protects cred->security somehow, but it doesn't look as if.

Probably task_lock() is here because PTRACE_ATTACH used the same lock,
but this was changed by 4b105cbbaf7c0 in 2009 (ptrace_attach() still
takes it for __ptrace_may_access() but this is another story).

However (iirc) PTRACE_DETACH never took this lock, so this was always
racy and task_lock() is simply misleading and confusing, at least
currently.

So I think the patch is fine, but I decided to send v2 without pid_alive().
If we are going to keep ptrace_parent(), it would be better to add the
comment into ptrace_parent() to explain that ->ptrace != 0 guarantees that
this task is not unhashed.

IOW, I also changed my mind about this part

	The patch also checks pid_alive(p) before ptrace_parent(p) to
	ensure that this task can't be dead even before rcu_read_lock(),
	in this case its ->parent points to nowhere. This is not really
	needed "in practice", task->ptrace must be already cleared in
	this case but we should not rely on this.

in the changelog.

> > And perhaps I am wrong. Because otoh the usage of ->ptrace should be
> > avoided outside of the core kernel code.
>
> Not to muddy things up, but one could argue that this particular
> LSM/SELinux hook should be regarded as part of the "core" kernel code.
>  However, I'm not sure that the distinction is really important here.

Yes, yes, sorry for confusion. I meant, the core kernel code which works
with ptrace/exit/fork/etc.

Oleg.

[PATCH v2] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Oleg Nesterov]

selinux_setprocattr() does ptrace_parent(p) under task_lock(p),
but task_struct->alloc_lock doesn't pin ->parent or ->ptrace,
this looks confusing and triggers the "suspicious RCU usage"
warning because ptrace_parent() does rcu_dereference_check().

And in theory this is wrong, spin_lock()->preempt_disable()
doesn't necessarily imply rcu_read_lock() we need to access
the ->parent.

Reported-by: Evan McNabb <emcnabb@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 security/selinux/hooks.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -5503,11 +5503,11 @@ static int selinux_setprocattr(struct ta
 		/* Check for ptracing, and update the task SID if ok.
 		   Otherwise, leave SID unchanged and fail. */
 		ptsid = 0;
-		task_lock(p);
+		rcu_read_lock();
 		tracer = ptrace_parent(p);
 		if (tracer)
 			ptsid = task_sid(tracer);
-		task_unlock(p);
+		rcu_read_unlock();
 
 		if (tracer) {
 			error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS,

Re: [PATCH v2] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Paul Moore]

On Saturday, December 14, 2013 05:33:17 PM Oleg Nesterov wrote:
> selinux_setprocattr() does ptrace_parent(p) under task_lock(p),
> but task_struct->alloc_lock doesn't pin ->parent or ->ptrace,
> this looks confusing and triggers the "suspicious RCU usage"
> warning because ptrace_parent() does rcu_dereference_check().
> 
> And in theory this is wrong, spin_lock()->preempt_disable()
> doesn't necessarily imply rcu_read_lock() we need to access
> the ->parent.
> 
> Reported-by: Evan McNabb <emcnabb@redhat.com>
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
>  security/selinux/hooks.c |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

Applied, thanks.

> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -5503,11 +5503,11 @@ static int selinux_setprocattr(struct ta
>  		/* Check for ptracing, and update the task SID if ok.
>  		   Otherwise, leave SID unchanged and fail. */
>  		ptsid = 0;
> -		task_lock(p);
> +		rcu_read_lock();
>  		tracer = ptrace_parent(p);
>  		if (tracer)
>  			ptsid = task_sid(tracer);
> -		task_unlock(p);
> +		rcu_read_unlock();
> 
>  		if (tracer) {
>  			error = avc_has_perm(ptsid, sid, SECCLASS_PROCESS,

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: selinux_setprocattr()->ptrace_parent() needs rcu_read_lock()

[Paul Moore]

On Saturday, December 14, 2013 05:32:56 PM Oleg Nesterov wrote:
> On 12/14, Paul Moore wrote:
> > ... I'm curious about the removal of the task lock; shouldn't we keep
> > the task lock in place?
> 
> Why? It protects nothing in this case, afaics. Unless of course it
> protects cred->security somehow, but it doesn't look as if.
> 
> Probably task_lock() is here because PTRACE_ATTACH used the same lock,
> but this was changed by 4b105cbbaf7c0 in 2009 (ptrace_attach() still
> takes it for __ptrace_may_access() but this is another story).
> 
> However (iirc) PTRACE_DETACH never took this lock, so this was always
> racy and task_lock() is simply misleading and confusing, at least
> currently.

Okay, you convinced me.

> So I think the patch is fine, but I decided to send v2 without pid_alive().
> If we are going to keep ptrace_parent(), it would be better to add the
> comment into ptrace_parent() to explain that ->ptrace != 0 guarantees that
> this task is not unhashed.
> 
> IOW, I also changed my mind about this part
> 
> 	The patch also checks pid_alive(p) before ptrace_parent(p) to
> 	ensure that this task can't be dead even before rcu_read_lock(),
> 	in this case its ->parent points to nowhere. This is not really
> 	needed "in practice", task->ptrace must be already cleared in
> 	this case but we should not rely on this.
> 
> in the changelog.

Seems reasonable to me.  I'll let you work on the ptrace_parent() side of 
things and I'll go ahead and merge your v2 patch; it looks fine to me and 
regardless of what happens with ptrace_parent() we should dump the task_lock() 
and add a RCU lock in its place.

Thanks for your help.

-- 
paul moore
www.paul-moore.com