Re: [syzbot] KASAN: use-after-free Read in task_work_run (2)

[Marco Elver <elver@google.com>]

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.