Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1539220 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] sched/pid fix use-after free in task_tgid_vnr EunTaik Lee <eun.taik.lee@samsung.com> - 2016-12-09 10:40 +0100
    Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr Oleg Nesterov <oleg@redhat.com> - 2016-12-09 18:30 +0100
      Re: [PATCH] sched/pid fix use-after free in task_tgid_vnr ebiederm@xmission.com (Eric W. Biederman) - 2016-12-09 23:30 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1539569

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

[toc] | [prev] | [next] | [standalone]

#1539712

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web