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

[Oleg Nesterov <oleg@redhat.com>]

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