Messages in this thread | | | From | Marco Elver <> | Date | Wed, 23 Nov 2022 12:12:20 +0100 | Subject | Re: [syzbot] KASAN: use-after-free Read in task_work_run (2) |
| |
On Thu, 27 Oct 2022 at 05:03, Hillf Danton <hdanton@sina.com> wrote: > > On 26 Oct 2022 11:29:35 -0700 > > syzbot has found a reproducer for the following issue on: > > > > HEAD commit: 88619e77b33d net: stmmac: rk3588: Allow multiple gmac cont.. > > git tree: bpf > > console output: https://syzkaller.appspot.com/x/log.txt?x=1646d6f2880000 > > kernel config: https://syzkaller.appspot.com/x/.config?x=a66c6c673fb555e8 > > dashboard link: https://syzkaller.appspot.com/bug?extid=9228d6098455bb209ec8 > > compiler: gcc (Debian 10.2.1-6) 10.2.1 20210110, GNU ld (GNU Binutils for Debian) 2.35.2 > > syz repro: https://syzkaller.appspot.com/x/repro.syz?x=12bc425e880000 > > C reproducer: https://syzkaller.appspot.com/x/repro.c?x=1126516e880000 > > Grab another hold on event upon adding task work in bid to fix uaf. > > #syz test https://git.kernel.org/pub/scm/linux/kernel/git/bpf/bpf.git 88619e77b33d > > --- x/kernel/events/core.c > +++ c/kernel/events/core.c > @@ -2291,6 +2291,7 @@ event_sched_out(struct perf_event *event > !event->pending_work) { > event->pending_work = 1; > dec = false; > + atomic_long_inc(&event->refcount); > task_work_add(current, &event->pending_task, TWA_RESUME); > } > if (dec) > @@ -6561,6 +6562,8 @@ static void perf_pending_task(struct cal > struct perf_event *event = container_of(head, struct perf_event, pending_task); > int rctx; > > + if (event->state == PERF_EVENT_STATE_DEAD) > + goto out; > /* > * If we 'fail' here, that's OK, it means recursion is already disabled > * and we won't recurse 'further'. > @@ -6577,6 +6580,8 @@ static void perf_pending_task(struct cal > if (rctx >= 0) > perf_swevent_put_recursion_context(rctx); > preempt_enable_notrace(); > +out: > + put_event(event); > } > > #ifdef CONFIG_GUEST_PERF_EVENTS
I'm not convinced this is what we want - while we could prolong the lifetime of an event, but if we're concurrently killing the event somewhere, we might as well cancel the task work (and potentially just skip a pending SIGTRAP). Your change most likely results in similar behaviour due to the DEAD check, although it prolongs the event's lifetime unnecessarily.
| |