Messages in this thread Patch in this message | | | Date | Thu, 22 Feb 2018 18:40:42 +0100 | From | Oleg Nesterov <> | Subject | Re: uprobes/perf: KASAN: use-after-free in uprobe_perf_close |
| |
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);
| |