uprobes/perf: KASAN: use-after-free in uprobe_perf_close

uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Prashant Bhole]

Hi,
I encountered following BUG caught by KASAN with recent kernels when 
trying out [BCC project] bcc/testing/python/test_usdt2.py
Tried with v4.12, it was reproducible.

--- KASAN log ---
BUG: KASAN: use-after-free in uprobe_perf_close+0x118/0x1a0
Read of size 4 at addr ffff8800bb2db4cc by task test_usdt2.py/1265

CPU: 2 PID: 1265 Comm: test_usdt2.py Not tainted 
4.16.0-rc2-next-20180220+ #38
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 
1.10.2-1.fc26 04/01/2014
Call Trace:
  dump_stack+0x5c/0x80
  print_address_description+0x73/0x290
  kasan_report+0x257/0x380
  ? uprobe_perf_close+0x118/0x1a0
  uprobe_perf_close+0x118/0x1a0
  perf_uprobe_destroy+0x54/0x90
  _free_event+0x1a5/0x5c0
  perf_event_release_kernel+0x35e/0x620
  ? put_event+0x20/0x20
  perf_release+0x1c/0x20
  __fput+0x182/0x360
  task_work_run+0x9c/0xc0
  exit_to_usermode_loop+0xc2/0xd0
  do_syscall_64+0x244/0x250
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2
[...]
Allocated by task 1265:
  kasan_kmalloc+0xa0/0xd0
  kmem_cache_alloc_node+0x123/0x210
  copy_process.part.32+0xb9d/0x3050
  _do_fork+0x178/0x630
  do_syscall_64+0xe7/0x250
  entry_SYSCALL_64_after_hwframe+0x3d/0xa2

Freed by task 1265:
  __kasan_slab_free+0x135/0x180
  kmem_cache_free+0xaf/0x230
  rcu_process_callbacks+0x559/0xd90
  __do_softirq+0x125/0x3a2

The buggy address belongs to the object at ffff8800bb2db480
  which belongs to the cache task_struct of size 12928
-----------------


After debugging, found that uprobe_perf_close() is called after task has 
been terminated and uprobe_perf_close() tries to access task_struct of 
the terminated process.

As fix I came up with following changes. Basically it gets a refcount on 
task_struct in uprobe_perf_open() and releases in uprobe_perf_close(). 
If this is a correct fix, I will submit a proper patch.


Signed-off-by: Prashant Bhole <bhole_prashant_q7@lab.ntt.co.jp>
---
  kernel/trace/trace_uprobe.c | 9 +++++++--
  1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/kernel/trace/trace_uprobe.c b/kernel/trace/trace_uprobe.c
index 2014f4351ae0..b81e0a88136a 100644
--- a/kernel/trace/trace_uprobe.c
+++ b/kernel/trace/trace_uprobe.c
@@ -1039,6 +1039,7 @@ uprobe_filter_event(struct trace_uprobe *tu, 
struct perf_event *event)

  static int uprobe_perf_close(struct trace_uprobe *tu, struct 
perf_event *event)
  {
+    int err = 0;
      bool done;

      write_lock(&tu->filter.rwlock);
@@ -1054,9 +1055,12 @@ static int uprobe_perf_close(struct trace_uprobe 
*tu, struct perf_event *event)
      write_unlock(&tu->filter.rwlock);

      if (!done)
-        return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
+        err =  uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);

-    return 0;
+    if (event->hw.target)
+        put_task_struct(event->hw.target);
+
+    return err;
  }

  static int uprobe_perf_open(struct trace_uprobe *tu, struct perf_event 
*event)
@@ -1077,6 +1081,7 @@ static int uprobe_perf_open(struct trace_uprobe 
*tu, struct perf_event *event)
          done = tu->filter.nr_systemwide ||
              event->parent || event->attr.enable_on_exec ||
              uprobe_filter_event(tu, event);
+        get_task_struct(event->hw.target);
          list_add(&event->hw.tp_list, &tu->filter.perf_events);
      } else {
          done = tu->filter.nr_systemwide;
-- 
2.14.3


-Prashant

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Oleg Nesterov]

On 02/22, Prashant Bhole wrote:
>
> Hi,
> I encountered following BUG caught by KASAN with recent kernels when trying
> out [BCC project] bcc/testing/python/test_usdt2.py
> Tried with v4.12, it was reproducible.
>
> --- KASAN log ---
> BUG: KASAN: use-after-free in uprobe_perf_close+0x118/0x1a0
> Read of size 4 at addr ffff8800bb2db4cc by task test_usdt2.py/1265
>
> CPU: 2 PID: 1265 Comm: test_usdt2.py Not tainted 4.16.0-rc2-next-20180220+
> #38
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1.fc26
> 04/01/2014
> Call Trace:
>  dump_stack+0x5c/0x80
>  print_address_description+0x73/0x290
>  kasan_report+0x257/0x380
>  ? uprobe_perf_close+0x118/0x1a0
>  uprobe_perf_close+0x118/0x1a0

...

> Freed by task 1265:
>  __kasan_slab_free+0x135/0x180
>  kmem_cache_free+0xaf/0x230
>  rcu_process_callbacks+0x559/0xd90
>  __do_softirq+0x125/0x3a2

Hmm. this looks strange, I do not see no __put_task_struct/free_task in this
trace... OK, nevermind.

> After debugging, found that uprobe_perf_close() is called after task has
> been terminated and uprobe_perf_close() tries to access task_struct of the
> terminated process.

Oh. You can't imagine how much I forgot this code ;) I will recheck, but at
first glance you are right. We can't rely on _free_event()->put_ctx() which
does put_task_struct() after event->destroy(), the exiting task does
put_task_struct(current) itself and sets child_ctx->task = TASK_TOMBSTONE in
perf_event_exit_task_context().

In short, nothing protects event->hw.target. But uprobe_perf_open() should be
safe, perf_init_event() is called when the caller has the additional reference.

I am wondering if this was wrong from the very beginning or it was broken later,
but I won't even try to check.

>  static int uprobe_perf_close(struct trace_uprobe *tu, struct perf_event
> *event)
>  {
> +    int err = 0;
>      bool done;
> 
>      write_lock(&tu->filter.rwlock);
> @@ -1054,9 +1055,12 @@ static int uprobe_perf_close(struct trace_uprobe *tu,
> struct perf_event *event)
>      write_unlock(&tu->filter.rwlock);
> 
>      if (!done)
> -        return uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> +        err =  uprobe_apply(tu->inode, tu->offset, &tu->consumer, false);
> 
> -    return 0;
> +    if (event->hw.target)
> +        put_task_struct(event->hw.target);
> +
> +    return err;
>  }

No need to delay put_task_struct(), you can call it right after "done = ..."
in the "if (event->hw.target)" block and simplify this change.

However. Probably this is the simplest fix, but it doesn't look nice. We do not
really need task_struct, we need its ->mm. PF_EXITING check can be removed, this
is a minor optimization.

perhaps we can add _something_ like

	struct mm_struct *uprobe_event_mm(event)
	{
		// needs rcu_read_lock/READ_ONCE/etc
		if (event->ctx &&
		    event->ctx->task && event->ctx->task != TASK_TOMBSTONE)
			return event->ctx->task->mm;

		return NULL;
	}

and use it instead of event->hw.target->mm ... Not sure.


And. What about other users of event->hw.target? Say, task_bp_pinned(). It doesn't
dereference this pointer, How can we trust the result of "iter->hw.target == tsk"
if hw.target can be freed and then re-alloced?


This all makes me think that we should change (fix) kernel/events/core.c...

Oleg.

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Peter Zijlstra]

On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:
> On 02/22, Prashant Bhole wrote:

> > After debugging, found that uprobe_perf_close() is called after task has
> > been terminated and uprobe_perf_close() tries to access task_struct of the
> > terminated process.
> 
> Oh. You can't imagine how much I forgot this code ;) I will recheck, but at
> first glance you are right. We can't rely on _free_event()->put_ctx() which
> does put_task_struct() after event->destroy(), the exiting task does
> put_task_struct(current) itself and sets child_ctx->task = TASK_TOMBSTONE in
> perf_event_exit_task_context().
> 
> In short, nothing protects event->hw.target. But uprobe_perf_open() should be
> safe, perf_init_event() is called when the caller has the additional reference.
> 
> I am wondering if this was wrong from the very beginning or it was broken later,
> but I won't even try to check.

b2fe8ba674e8 ("uprobes/perf: Avoid uprobe_apply() whenever possible")

Seems to have added that PF_EXITING test that dereferences the target
pointer.

> And. What about other users of event->hw.target? Say, task_bp_pinned(). It doesn't
> dereference this pointer, How can we trust the result of "iter->hw.target == tsk"
> if hw.target can be freed and then re-alloced?

I _think_ that one is ok because it looks at available slots for the
task at init time, at that point the target task must exist.

> This all makes me think that we should change (fix) kernel/events/core.c...

That's going to be mighty dodgy though, holding a reference on the task
will avoid the task from dying which will avoid the events from being
destroyed which will avoid the task from dying which will... if you get
my drift :-)

And I suspect the proposed patch already suffers that problem.
pmu::destroy really should not be looking at that pointer I'm afraid.

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Peter Zijlstra]

On Thu, Feb 22, 2018 at 06:04:27PM +0100, Peter Zijlstra wrote:
> On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:

> > This all makes me think that we should change (fix) kernel/events/core.c...
> 
> That's going to be mighty dodgy though, holding a reference on the task
> will avoid the task from dying which will avoid the events from being
> destroyed which will avoid the task from dying which will... if you get
> my drift :-)

Hmm, it might not be all that bad.. I need to re-read some of that code.

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Oleg Nesterov]

On 02/22, Peter Zijlstra wrote:
>
> On Thu, Feb 22, 2018 at 06:04:27PM +0100, Peter Zijlstra wrote:
> > On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:
>
> > > This all makes me think that we should change (fix) kernel/events/core.c...
> >
> > That's going to be mighty dodgy though, holding a reference on the task
> > will avoid the task from dying which will avoid the events from being
> > destroyed which will avoid the task from dying which will... if you get
> > my drift :-)
>
> Hmm, it might not be all that bad.. I need to re-read some of that code.

I was thinking about the change below below. I do not think this patch is actually
correct/complete, but it seems to me that if perf_event_exit_task_context() does
put_task_struct(current) then put_ctx()->put_task_struct() should go away, every
user of ctx->task should check TASK_TOMBSTONE anyway?

Oleg.

--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -1165,8 +1165,6 @@ static void put_ctx(struct perf_event_context *ctx)
 	if (atomic_dec_and_test(&ctx->refcount)) {
 		if (ctx->parent_ctx)
 			put_ctx(ctx->parent_ctx);
-		if (ctx->task && ctx->task != TASK_TOMBSTONE)
-			put_task_struct(ctx->task);
 		call_rcu(&ctx->rcu_head, free_ctx);
 	}
 }
@@ -3731,10 +3729,9 @@ alloc_perf_context(struct pmu *pmu, struct task_struct *task)
 		return NULL;
 
 	__perf_event_init_context(ctx);
-	if (task) {
+	if (task)
 		ctx->task = task;
-		get_task_struct(task);
-	}
+
 	ctx->pmu = pmu;
 
 	return ctx;
@@ -4109,6 +4106,8 @@ static void _free_event(struct perf_event *event)
 
 	if (event->ctx)
 		put_ctx(event->ctx);
+	if (event->hw.target)
+		put_task_struct(event->hw.target);
 
 	exclusive_event_destroy(event);
 	module_put(event->pmu->module);
@@ -9475,6 +9474,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 		 * and we cannot use the ctx information because we need the
 		 * pmu before we get a ctx.
 		 */
+		get_task_struct(task);
 		event->hw.target = task;
 	}
 
@@ -9590,6 +9590,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
 		perf_detach_cgroup(event);
 	if (event->ns)
 		put_pid_ns(event->ns);
+	if (task)
+		put_task_struct(task);
 	kfree(event);
 
 	return ERR_PTR(err);
@@ -10572,7 +10574,6 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
 	RCU_INIT_POINTER(child->perf_event_ctxp[ctxn], NULL);
 	put_ctx(child_ctx); /* cannot be last */
 	WRITE_ONCE(child_ctx->task, TASK_TOMBSTONE);
-	put_task_struct(current); /* cannot be last */
 
 	clone_ctx = unclone_ctx(child_ctx);
 	raw_spin_unlock_irq(&child_ctx->lock);

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Oleg Nesterov]

On 02/22, Peter Zijlstra wrote:
>
> On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:
> > On 02/22, Prashant Bhole wrote:
>
> > > After debugging, found that uprobe_perf_close() is called after task has
> > > been terminated and uprobe_perf_close() tries to access task_struct of the
> > > terminated process.
> >
> > Oh. You can't imagine how much I forgot this code ;) I will recheck, but at
> > first glance you are right. We can't rely on _free_event()->put_ctx() which
> > does put_task_struct() after event->destroy(), the exiting task does
> > put_task_struct(current) itself and sets child_ctx->task = TASK_TOMBSTONE in
> > perf_event_exit_task_context().
> >
> > In short, nothing protects event->hw.target. But uprobe_perf_open() should be
> > safe, perf_init_event() is called when the caller has the additional reference.
> >
> > I am wondering if this was wrong from the very beginning or it was broken later,
> > but I won't even try to check.
>
> b2fe8ba674e8 ("uprobes/perf: Avoid uprobe_apply() whenever possible")
>
> Seems to have added that PF_EXITING test that dereferences the target
> pointer.

Hehe ;) no, I think we should blame another commit 63b6da39bb38e8f1a1ef3180d32a39d6
("perf: Fix perf_event_exit_task() race").

I can be easily wrong, but after perf_event_exit_task_context()->put_task_struct()
added by this commit nothing protects event->hw.target.

And just in case, we can simply remove that PF_EXITING test in uprobe_perf_close(),
this is a minor optimization. But __uprobe_perf_filter() needs a stable ->target too.

Oleg.

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Prashant Bhole]

On 2/23/2018 2:40 AM, Oleg Nesterov wrote:
> On 02/22, Peter Zijlstra wrote:
>>
>> On Thu, Feb 22, 2018 at 06:04:27PM +0100, Peter Zijlstra wrote:
>>> On Thu, Feb 22, 2018 at 05:37:15PM +0100, Oleg Nesterov wrote:
>>
>>>> This all makes me think that we should change (fix) kernel/events/core.c...
>>>
>>> That's going to be mighty dodgy though, holding a reference on the task
>>> will avoid the task from dying which will avoid the events from being
>>> destroyed which will avoid the task from dying which will... if you get
>>> my drift :-)
>>
>> Hmm, it might not be all that bad.. I need to re-read some of that code.
> 
> I was thinking about the change below below. I do not think this patch is actually
> correct/complete, but it seems to me that if perf_event_exit_task_context() does
> put_task_struct(current) then put_ctx()->put_task_struct() should go away, every
> user of ctx->task should check TASK_TOMBSTONE anyway?
> 
> Oleg.
> 
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -1165,8 +1165,6 @@ static void put_ctx(struct perf_event_context *ctx)
>   	if (atomic_dec_and_test(&ctx->refcount)) {
>   		if (ctx->parent_ctx)
>   			put_ctx(ctx->parent_ctx);
> -		if (ctx->task && ctx->task != TASK_TOMBSTONE)
> -			put_task_struct(ctx->task);
>   		call_rcu(&ctx->rcu_head, free_ctx);
>   	}
>   }
> @@ -3731,10 +3729,9 @@ alloc_perf_context(struct pmu *pmu, struct task_struct *task)
>   		return NULL;
>   
>   	__perf_event_init_context(ctx);
> -	if (task) {
> +	if (task)
>   		ctx->task = task;
> -		get_task_struct(task);
> -	}
> +
>   	ctx->pmu = pmu;
>   
>   	return ctx;
> @@ -4109,6 +4106,8 @@ static void _free_event(struct perf_event *event)
>   
>   	if (event->ctx)
>   		put_ctx(event->ctx);
> +	if (event->hw.target)
> +		put_task_struct(event->hw.target);
>   
>   	exclusive_event_destroy(event);
>   	module_put(event->pmu->module);
> @@ -9475,6 +9474,7 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   		 * and we cannot use the ctx information because we need the
>   		 * pmu before we get a ctx.
>   		 */
> +		get_task_struct(task);
>   		event->hw.target = task;
>   	}
>   
> @@ -9590,6 +9590,8 @@ perf_event_alloc(struct perf_event_attr *attr, int cpu,
>   		perf_detach_cgroup(event);
>   	if (event->ns)
>   		put_pid_ns(event->ns);
> +	if (task)
> +		put_task_struct(task);
>   	kfree(event);
>   
>   	return ERR_PTR(err);
> @@ -10572,7 +10574,6 @@ static void perf_event_exit_task_context(struct task_struct *child, int ctxn)
>   	RCU_INIT_POINTER(child->perf_event_ctxp[ctxn], NULL);
>   	put_ctx(child_ctx); /* cannot be last */
>   	WRITE_ONCE(child_ctx->task, TASK_TOMBSTONE);
> -	put_task_struct(current); /* cannot be last */
>   
>   	clone_ctx = unclone_ctx(child_ctx);
>   	raw_spin_unlock_irq(&child_ctx->lock);
> 
Sorry for late reply. I tried these changes. It didn't fix the problem. 
With these changes, the use-after-free access of task_struct occurs at 
_free_event() for the last remaining event.

In your changes, I tried keeping get/put_task_struct() in 
perf_alloc_context()/put_ctx() intact and The problem did not occur. 
Changes are mentioned below.

-Prashant

diff --git a/kernel/events/core.c b/kernel/events/core.c
index c98cce4ceebd..65889d2b5ae2 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -4109,6 +4109,8 @@ static void _free_event(struct perf_event *event)

  	if (event->ctx)
  		put_ctx(event->ctx);
+	if (event->hw.target)
+		put_task_struct(event->hw.target);

  	exclusive_event_destroy(event);
  	module_put(event->pmu->module);
@@ -9593,6 +9595,7 @@ perf_event_alloc(struct perf_event_attr *attr, int 
cpu,
  		 * and we cannot use the ctx information because we need the
  		 * pmu before we get a ctx.
  		 */
+		get_task_struct(task);
  		event->hw.target = task;
  	}

@@ -9708,6 +9711,8 @@ perf_event_alloc(struct perf_event_attr *attr, int 
cpu,
  		perf_detach_cgroup(event);
  	if (event->ns)
  		put_pid_ns(event->ns);
+	if (task)
+		put_task_struct(task);
  	kfree(event);

  	return ERR_PTR(err);

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Peter Zijlstra]

On Tue, Mar 06, 2018 at 06:49:10PM +0900, Prashant Bhole wrote:
> Sorry for late reply. I tried these changes. It didn't fix the problem. With

He, sorry for completely forgetting about this one :/

> these changes, the use-after-free access of task_struct occurs at
> _free_event() for the last remaining event.
> 
> In your changes, I tried keeping get/put_task_struct() in
> perf_alloc_context()/put_ctx() intact and The problem did not occur. Changes
> are mentioned below.

Yes, I think you're right in that this is the cleanest solution; it adds
reference counting to the exact pointer we're using.

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index c98cce4ceebd..65889d2b5ae2 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -4109,6 +4109,8 @@ static void _free_event(struct perf_event *event)
> 
>  	if (event->ctx)
>  		put_ctx(event->ctx);
> +	if (event->hw.target)
> +		put_task_struct(event->hw.target);
> 
>  	exclusive_event_destroy(event);
>  	module_put(event->pmu->module);
> @@ -9593,6 +9595,7 @@ perf_event_alloc(struct perf_event_attr *attr, int
> cpu,
>  		 * and we cannot use the ctx information because we need the
>  		 * pmu before we get a ctx.
>  		 */
> +		get_task_struct(task);
>  		event->hw.target = task;
>  	}
> 
> @@ -9708,6 +9711,8 @@ perf_event_alloc(struct perf_event_attr *attr, int
> cpu,
>  		perf_detach_cgroup(event);
>  	if (event->ns)
>  		put_pid_ns(event->ns);
> +	if (task)

Should this not too be 'event->hw.target', for consistency and clarity?

> +		put_task_struct(task);
>  	kfree(event);
> 
>  	return ERR_PTR(err);

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Prashant Bhole]

On 4/9/2018 4:38 PM, Peter Zijlstra wrote:
> On Tue, Mar 06, 2018 at 06:49:10PM +0900, Prashant Bhole wrote:
>> Sorry for late reply. I tried these changes. It didn't fix the problem. With
> 
> He, sorry for completely forgetting about this one :/
> 
>> these changes, the use-after-free access of task_struct occurs at
>> _free_event() for the last remaining event.
>>
>> In your changes, I tried keeping get/put_task_struct() in
>> perf_alloc_context()/put_ctx() intact and The problem did not occur. Changes
>> are mentioned below.
> 
> Yes, I think you're right in that this is the cleanest solution; it adds
> reference counting to the exact pointer we're using.
> 
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index c98cce4ceebd..65889d2b5ae2 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -4109,6 +4109,8 @@ static void _free_event(struct perf_event *event)
>>
>>   	if (event->ctx)
>>   		put_ctx(event->ctx);
>> +	if (event->hw.target)
>> +		put_task_struct(event->hw.target);
>>
>>   	exclusive_event_destroy(event);
>>   	module_put(event->pmu->module);
>> @@ -9593,6 +9595,7 @@ perf_event_alloc(struct perf_event_attr *attr, int
>> cpu,
>>   		 * and we cannot use the ctx information because we need the
>>   		 * pmu before we get a ctx.
>>   		 */
>> +		get_task_struct(task);
>>   		event->hw.target = task;
>>   	}
>>
>> @@ -9708,6 +9711,8 @@ perf_event_alloc(struct perf_event_attr *attr, int
>> cpu,
>>   		perf_detach_cgroup(event);
>>   	if (event->ns)
>>   		put_pid_ns(event->ns);
>> +	if (task)
> 
> Should this not too be 'event->hw.target', for consistency and clarity?


Yes, I am sending a patch with this change. Thanks.

-Prashant

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Oleg Nesterov]

On 04/09, Peter Zijlstra wrote:
>
> On Tue, Mar 06, 2018 at 06:49:10PM +0900, Prashant Bhole wrote:
> > Sorry for late reply. I tried these changes. It didn't fix the problem. With
>
> He, sorry for completely forgetting about this one :/

me too, sorry Prashant,

> > these changes, the use-after-free access of task_struct occurs at
> > _free_event() for the last remaining event.

Heh, I didn't even try to compile the "patch" I sent, I am not surprised it is
not correct. But unless I forget again, I'll try to make the working version.

> > In your changes, I tried keeping get/put_task_struct() in
> > perf_alloc_context()/put_ctx() intact and The problem did not occur. Change
> > are mentioned below.
>
> Yes, I think you're right in that this is the cleanest solution; it add
> reference counting to the exact pointer we're using.

OK, agreed, lets make the minimal fix for now.

But I still think that we should (try to) remove put_task_struct() from put_ctx().

Quite possibly I missed something, but I think it only adds some confusion. Once
again, even if ctx can't go away you can't use ctx->task without TASK_TOMBSTONE
check, exactly because this task can exit. So why perf_event_context should add
another reference?

Oleg.

Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close

[Peter Zijlstra]

On Mon, Apr 09, 2018 at 12:40:10PM +0200, Oleg Nesterov wrote:
> But I still think that we should (try to) remove put_task_struct() from put_ctx().
> 
> Quite possibly I missed something, but I think it only adds some confusion. Once
> again, even if ctx can't go away you can't use ctx->task without TASK_TOMBSTONE
> check, exactly because this task can exit. So why perf_event_context should add
> another reference?

Ah, I see what you mean. Yes that might be possible.