Oleg Nesterov <o...@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);
>
