[PATCH] sched/pid fix use-after free in task_tgid_vnr

[PATCH] sched/pid fix use-after free in task_tgid_vnr

[EunTaik Lee]

[-- Attachment #1: Type: text/plain, Size: 1413 bytes --]

There is a use-after-free case with below call stack.

pid_nr_ns+0x10/0x38
cgroup_pidlist_start+0x144/0x400
cgroup_seqfile_start+0x1c/0x24
kernfs_seq_start+0x54/0x90
seq_read+0x15c/0x3a8
kernfs_fop_read+0x38/0x160
__vfs_read+0x28/0xc8
vfs_read+0x84/0xfc

A task in the cg_list was dying and the group_leader's
task struct was already freed.

To avoid this task_tgid_vnr needs to take the
rcu_read_lock and check for pid_alive.

Signed-off-by: Eun Taik Lee <eun.taik.lee@samsung.com>
---
 include/linux/sched.h | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/include/linux/sched.h b/include/linux/sched.h
index e9c009d..ed567bc 100644
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2118,14 +2118,21 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
 }
 
 pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns);
+static inline int pid_alive(const struct task_struct *p);
 
 static inline pid_t task_tgid_vnr(struct task_struct *tsk)
 {
-	return pid_vnr(task_tgid(tsk));
+	pid_t pid = 0;
+
+	rcu_read_lock();
+	if (pid_alive(tsk))
+		pid = pid_vnr(task_tgid(tsk));
+	rcu_read_unlock();
+
+	return pid;
 }
 
 
-static inline int pid_alive(const struct task_struct *p);
 static inline pid_t task_ppid_nr_ns(const struct task_struct *tsk, struct pid_namespace *ns)
 {
 	pid_t pid = 0;
-- 
1.9.1

Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

[Oleg Nesterov]

On 12/09, EunTaik Lee wrote:
>
> There is a use-after-free case with below call stack.
>
> pid_nr_ns+0x10/0x38
> cgroup_pidlist_start+0x144/0x400
> cgroup_seqfile_start+0x1c/0x24
> kernfs_seq_start+0x54/0x90
> seq_read+0x15c/0x3a8
> kernfs_fop_read+0x38/0x160
> __vfs_read+0x28/0xc8
> vfs_read+0x84/0xfc

This reminds about perf_event_pid() which is equally buggy...

>  static inline pid_t task_tgid_vnr(struct task_struct *tsk)
>  {
> -	return pid_vnr(task_tgid(tsk));
> +	pid_t pid = 0;
> +
> +	rcu_read_lock();
> +	if (pid_alive(tsk))
> +		pid = pid_vnr(task_tgid(tsk));
> +	rcu_read_unlock();
> +
> +	return pid;
>  }

Eric, EunTaik, what do you think about the patch below?

I can't decide whether it is too ugly or not, but it would be nice
to avoid the code duplication.

Oleg.


--- x/include/linux/pid.h
+++ x/include/linux/pid.h
@@ -8,7 +8,8 @@ enum pid_type
 	PIDTYPE_PID,
 	PIDTYPE_PGID,
 	PIDTYPE_SID,
-	PIDTYPE_MAX
+	PIDTYPE_MAX,
+	PIDTYPE_TGID	/* do not use */
 };
 
 /*
--- x/kernel/pid.c
+++ x/kernel/pid.c
@@ -526,8 +526,11 @@ pid_t __task_pid_nr_ns(struct task_struc
 	if (!ns)
 		ns = task_active_pid_ns(current);
 	if (likely(pid_alive(task))) {
-		if (type != PIDTYPE_PID)
+		if (type != PIDTYPE_PID) {
+			if (type == PIDTYPE_TGID)
+				type = PIDTYPE_PID;
 			task = task->group_leader;
+		}
 		nr = pid_nr_ns(rcu_dereference(task->pids[type].pid), ns);
 	}
 	rcu_read_unlock();
@@ -538,7 +541,7 @@ EXPORT_SYMBOL(__task_pid_nr_ns);
 
 pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
 {
-	return pid_nr_ns(task_tgid(tsk), ns);
+	return __task_pid_nr_ns(tsk, PIDTYPE_TGID, ns);
 }
 EXPORT_SYMBOL(task_tgid_nr_ns);
 

Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 12/09, EunTaik Lee wrote:
>>
>> There is a use-after-free case with below call stack.
>>
>> pid_nr_ns+0x10/0x38
>> cgroup_pidlist_start+0x144/0x400
>> cgroup_seqfile_start+0x1c/0x24
>> kernfs_seq_start+0x54/0x90
>> seq_read+0x15c/0x3a8
>> kernfs_fop_read+0x38/0x160
>> __vfs_read+0x28/0xc8
>> vfs_read+0x84/0xfc

How is this a use after free.  The function pid_nr_ns should take a NULL pointer
as input and return 0?

Certainly if the addtion of pid_alive fixes it pid_vnr(task_tgid(tsk))
is fine.  Are we perhaps missing rcu locking?

Or is the problem simply that in task_tgid we are accessing
task->group_leader which may already be dead?  If so the fix needs to be
in task_tgid.

> This reminds about perf_event_pid() which is equally buggy...
>
>>  static inline pid_t task_tgid_vnr(struct task_struct *tsk)
>>  {
>> -	return pid_vnr(task_tgid(tsk));
>> +	pid_t pid = 0;
>> +
>> +	rcu_read_lock();
>> +	if (pid_alive(tsk))
>> +		pid = pid_vnr(task_tgid(tsk));
>> +	rcu_read_unlock();
>> +
>> +	return pid;
>>  }
>
> Eric, EunTaik, what do you think about the patch below?
>
> I can't decide whether it is too ugly or not, but it would be nice
> to avoid the code duplication.

I think it can be beaten into shape but I am not certain it addresses the
core issue.

>
> Oleg.
>
>
> --- x/include/linux/pid.h
> +++ x/include/linux/pid.h
> @@ -8,7 +8,8 @@ enum pid_type
>  	PIDTYPE_PID,
>  	PIDTYPE_PGID,
>  	PIDTYPE_SID,
> -	PIDTYPE_MAX
> +	PIDTYPE_MAX,
> +	PIDTYPE_TGID	/* do not use */


I would do:

/* __PIDTYPE_TGID is only valid to __task_pid_nr_ns */
#define __PIDTYPE_TGID PIDTYPE_MAX

Prefixing __PIDTYPE_TGID  with __ should help make it clear
this is a special use define.

I am also curious why pid_alive is the proper check to see if
task->group_leader is valid?  That feels like it could get us into
trouble later.

Especially as that is the real problem child here.

>  };
>  
>  /*
> --- x/kernel/pid.c
> +++ x/kernel/pid.c
> @@ -526,8 +526,11 @@ pid_t __task_pid_nr_ns(struct task_struc
>  	if (!ns)
>  		ns = task_active_pid_ns(current);
>  	if (likely(pid_alive(task))) {
> -		if (type != PIDTYPE_PID)
> +		if (type != PIDTYPE_PID) {
> +			if (type == PIDTYPE_TGID)
> +				type = PIDTYPE_PID;
>  			task = task->group_leader;
> +		}
>  		nr = pid_nr_ns(rcu_dereference(task->pids[type].pid), ns);
>  	}
>  	rcu_read_unlock();
> @@ -538,7 +541,7 @@ EXPORT_SYMBOL(__task_pid_nr_ns);
>  
>  pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
>  {
> -	return pid_nr_ns(task_tgid(tsk), ns);
> +	return __task_pid_nr_ns(tsk, PIDTYPE_TGID, ns);
>  }
>  EXPORT_SYMBOL(task_tgid_nr_ns);
>  

Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

[Oleg Nesterov]

On 12/10, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > On 12/09, EunTaik Lee wrote:
> >>
> >> There is a use-after-free case with below call stack.
> >>
> >> pid_nr_ns+0x10/0x38
> >> cgroup_pidlist_start+0x144/0x400
> >> cgroup_seqfile_start+0x1c/0x24
> >> kernfs_seq_start+0x54/0x90
> >> seq_read+0x15c/0x3a8
> >> kernfs_fop_read+0x38/0x160
> >> __vfs_read+0x28/0xc8
> >> vfs_read+0x84/0xfc
>
> How is this a use after free.  The function pid_nr_ns should take a NULL pointer
> as input and return 0?

No, the task (task_struct) itself can't go away, but task->group_leader
can point to nowhere.

> Certainly if the addtion of pid_alive fixes it pid_vnr(task_tgid(tsk))
> is fine.  Are we perhaps missing rcu locking?

rcu_read_lock() is not enough in this case, see below.

> Or is the problem simply that in task_tgid we are accessing
> task->group_leader which may already be dead?

Yes. Lets forget about the callchain above, I didn't even bother to verify
that it can actually hit the problem. Although I think EunTaik is very right,
css_task_iter_next() does get_task_struct() and drops css_set_lock, the task
can exit after that. Forget.

Just suppose that a task simply does

	pid = task_tgid_vnr(current);

after it has already called exit_notify(). And this is what perf_event_pid()
does, perhaps we have more buggy users.

In this case current->group_leader or parent/real_parent can point to the
exited/freed tasks. I already said this many times, ee really need to nullify
them in __unhash_process() but this needs a lot of (mostly simple) cleanups.

> If so the fix needs to be
> in task_tgid.

Yes, task_tgid() should probably return NULL in this case, but this connects
to "a lot of cleanups" above.

> > --- x/include/linux/pid.h
> > +++ x/include/linux/pid.h
> > @@ -8,7 +8,8 @@ enum pid_type
> >  	PIDTYPE_PID,
> >  	PIDTYPE_PGID,
> >  	PIDTYPE_SID,
> > -	PIDTYPE_MAX
> > +	PIDTYPE_MAX,
> > +	PIDTYPE_TGID	/* do not use */
>
>
> I would do:
>
> /* __PIDTYPE_TGID is only valid to __task_pid_nr_ns */
> #define __PIDTYPE_TGID PIDTYPE_MAX
>
> Prefixing __PIDTYPE_TGID  with __ should help make it clear
> this is a special use define.

OK, will do, thanks.

> I am also curious why pid_alive is the proper check to see if
> task->group_leader is valid?  That feels like it could get us into
> trouble later.

It is. pid_alive(task) == T means that this task was not removed from
rcu-protected-lists and thus task->group_leader (in particular) can't
go away. As long as you check pid_alive() and use ->group_leader in
the same rcu_read_lock section, of course.

To clarify, this is not because detach_pid(PIDTYPE_PID) is called
before list_del_rcu(), this doesn't matter. What does matter is that
if you see pid_alive() == T  (or task->sighand != NULL, or anything
else which means that release_task/__unhash_process was not called yet)
you know that this task, its leader/parent/real_parent/pids/etc can't
go away until rcu_read_unlock().

And this is why __task_pid_nr_ns() checks pid_alive() before it reads
->group_leader. Note that __task_pid_nr_ns(PIDTYPE_PID) does not need
this check, task->pids[PID] is nullified by detach_pid() and pid_nr_ns()
check pid != NULL.

However. I think it should be renamed. Or, better, we should add a new
helper to make this all more clear. Say,

	bool task_is_rcu_safe(task)
	{
		return task->sighand != NULL;
	}

then change __task_pid_nr_ns() to use this helper rather than pid_alive().
Because, once again, it is not that we need to ensure that
pids[PIDTYPE_PID].pid != NULL, we need to ensure that rcu_read_lock() can
actually protect this task and its leader/parent/etc. And of course, it
can have more users.

Oleg.

Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

[Eric W. Biederman]

Oleg Nesterov <oleg@redhat.com> writes:

> On 12/10, Eric W. Biederman wrote:
>>
>> Oleg Nesterov <oleg@redhat.com> writes:
>>
>> > On 12/09, EunTaik Lee wrote:
>> >>
>> >> There is a use-after-free case with below call stack.
>> >>
>> >> pid_nr_ns+0x10/0x38
>> >> cgroup_pidlist_start+0x144/0x400
>> >> cgroup_seqfile_start+0x1c/0x24
>> >> kernfs_seq_start+0x54/0x90
>> >> seq_read+0x15c/0x3a8
>> >> kernfs_fop_read+0x38/0x160
>> >> __vfs_read+0x28/0xc8
>> >> vfs_read+0x84/0xfc
>>
>> How is this a use after free.  The function pid_nr_ns should take a NULL pointer
>> as input and return 0?
>
> No, the task (task_struct) itself can't go away, but task->group_leader
> can point to nowhere.
>
>> Certainly if the addtion of pid_alive fixes it pid_vnr(task_tgid(tsk))
>> is fine.  Are we perhaps missing rcu locking?
>
> rcu_read_lock() is not enough in this case, see below.
>
>> Or is the problem simply that in task_tgid we are accessing
>> task->group_leader which may already be dead?
>
> Yes. Lets forget about the callchain above, I didn't even bother to verify
> that it can actually hit the problem. Although I think EunTaik is very right,
> css_task_iter_next() does get_task_struct() and drops css_set_lock, the task
> can exit after that. Forget.
>
> Just suppose that a task simply does
>
> 	pid = task_tgid_vnr(current);
>
> after it has already called exit_notify(). And this is what perf_event_pid()
> does, perhaps we have more buggy users.
>
> In this case current->group_leader or parent/real_parent can point to the
> exited/freed tasks. I already said this many times, ee really need to nullify
> them in __unhash_process() but this needs a lot of (mostly simple)
> cleanups.

Is there anything wrong with starting with the patch below?

diff --git a/kernel/exit.c b/kernel/exit.c
index 9d68c45ebbe3..03daeecc335d 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -200,6 +200,7 @@ void release_task(struct task_struct *p)
                if (zap_leader)
                        leader->exit_state = EXIT_DEAD;
        }
+       p->group_leader = NULL;
 
        write_unlock_irq(&tasklist_lock);
        release_thread(p);


That seems to cut to the heart of the matter.  Failures will be clearer,
as will be code that is introduced to handle the situation.   Then we
don't need pid_alive or any other magic just a simple:
	rcu_read_lock();
	leader = READ_ONCE(task->group_leader);
	if (leader) {
		/* Do stuff */
	}
	rcu_read_unlock();

>> If so the fix needs to be
>> in task_tgid.
>
> Yes, task_tgid() should probably return NULL in this case, but this connects
> to "a lot of cleanups" above.

But that is important because that is where things go wrong in the
specific case under discussion.  pid_nr_ns handles all of the other
cases, it is task_tgid that went wrong.

Eric

Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

[Oleg Nesterov]

On 12/13, Eric W. Biederman wrote:
>
> Oleg Nesterov <oleg@redhat.com> writes:
>
> > In this case current->group_leader or parent/real_parent can point to the
> > exited/freed tasks. I already said this many times, ee really need to nullify
> > them in __unhash_process() but this needs a lot of (mostly simple)
> > cleanups.
>
> Is there anything wrong with starting with the patch below?
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 9d68c45ebbe3..03daeecc335d 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -200,6 +200,7 @@ void release_task(struct task_struct *p)
>                 if (zap_leader)
>                         leader->exit_state = EXIT_DEAD;
>         }
> +       p->group_leader = NULL;

Yes, see above, this is what we should do. Except I think we should
change __unhash_process() to do this. The same for parent/real_parent.

> That seems to cut to the heart of the matter.

Exactly.

But we can not start with this change. Just for example,
task_session/task_pgrp/tgid can obviously crash, even if used properly,
they will need to check group_leader != NULL. __task_pid_nr_ns() should
be changed too. And more.

That is why I'd prefer to start with the simple fix I suggested before,
then do the cleanups.

And imo that change makes sense in any case to avoid the code duplication,
even if we change __unhash_process() to nullify group_leader/etc. Note that
we can also add __PIDTYPE_PPID and turn task_ppid_nr_ns() into another
trivial users of  __task_pid_nr_ns(). Just it will need to check task != NULL
instead if pid_alive(p).

So, will you agree with v2 below? s/PIDTYPE_TGID/__PIDTYPE_TGID/ as you
suggested, and move task_tgid_nr_ns() into sched.h close to other similar
helpers.

Oleg.


--- a/include/linux/pid.h
+++ b/include/linux/pid.h
@@ -8,7 +8,9 @@ enum pid_type
 	PIDTYPE_PID,
 	PIDTYPE_PGID,
 	PIDTYPE_SID,
-	PIDTYPE_MAX
+	PIDTYPE_MAX,
+	/* only valid to __task_pid_nr_ns() */
+	__PIDTYPE_TGID
 };
 
 /*
--- a/include/linux/sched.h
+++ b/include/linux/sched.h
@@ -2117,14 +2117,6 @@ static inline pid_t task_tgid_nr(struct task_struct *tsk)
 	return tsk->tgid;
 }
 
-pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns);
-
-static inline pid_t task_tgid_vnr(struct task_struct *tsk)
-{
-	return pid_vnr(task_tgid(tsk));
-}
-
-
 static inline int pid_alive(const struct task_struct *p);
 static inline pid_t task_ppid_nr_ns(const struct task_struct *tsk, struct pid_namespace *ns)
 {
@@ -2166,6 +2158,16 @@ static inline pid_t task_session_vnr(struct task_struct *tsk)
 	return __task_pid_nr_ns(tsk, PIDTYPE_SID, NULL);
 }
 
+static inline pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
+{
+	return __task_pid_nr_ns(tsk, __PIDTYPE_TGID, ns);
+}
+
+static inline pid_t task_tgid_vnr(struct task_struct *tsk)
+{
+	return __task_pid_nr_ns(tsk, __PIDTYPE_TGID, NULL);
+}
+
 /* obsolete, do not use */
 static inline pid_t task_pgrp_nr(struct task_struct *tsk)
 {
--- a/kernel/pid.c
+++ b/kernel/pid.c
@@ -526,8 +526,11 @@ pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 	if (!ns)
 		ns = task_active_pid_ns(current);
 	if (likely(pid_alive(task))) {
-		if (type != PIDTYPE_PID)
+		if (type != PIDTYPE_PID) {
+			if (type == __PIDTYPE_TGID)
+				type = PIDTYPE_PID;
 			task = task->group_leader;
+		}
 		nr = pid_nr_ns(rcu_dereference(task->pids[type].pid), ns);
 	}
 	rcu_read_unlock();
@@ -536,12 +539,6 @@ pid_t __task_pid_nr_ns(struct task_struct *task, enum pid_type type,
 }
 EXPORT_SYMBOL(__task_pid_nr_ns);
 
-pid_t task_tgid_nr_ns(struct task_struct *tsk, struct pid_namespace *ns)
-{
-	return pid_nr_ns(task_tgid(tsk), ns);
-}
-EXPORT_SYMBOL(task_tgid_nr_ns);
-
 struct pid_namespace *task_active_pid_ns(struct task_struct *tsk)
 {
 	return ns_of_pid(task_pid(tsk));