linux-kernel - Re: [PATCH] sched/pid fix use-after free in task_tgid

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Sat, 10 Dec 2016 11:21:59 +1300
From:   ebiederm@...ssion.com (Eric W. Biederman)
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     EunTaik Lee <eun.taik.lee@...sung.com>,
        "mingo\@redhat.com" <mingo@...hat.com>,
        "peterz\@infradead.org" <peterz@...radead.org>,
        "linux-kernel\@vger.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr

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