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

From: Oleg Nesterov
Date: Thu Feb 22 2018 - 12:40:51 EST


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