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

[Oleg Nesterov <oleg@redhat.com>]

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.