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