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