Re: [PATCH] perf/bpf: Fix data race in __perf_event_overflow

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless TOCTOU race in perf_event_alloc() causes NULL pointer dereference or Use-After-Free during BPF program inheritance.
- [Medium] Incomplete fix for data races and missing memory barriers in BPF attach/detach paths.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race drops valid perf events during concurrent BPF detach.
--

--- Patch [1]: [PATCH] perf/bpf: Fix data race in __perf_event_overflow ---
commit f63a2be5ceb79a2a8bb0263ba6723f7bf4ae763d
Author: Deepanshu Kartikey <[email protected]>

perf/bpf: Fix data race in __perf_event_overflow

This patch addresses a data race where __perf_event_overflow() concurrently
reads event->prog without lock protection while perf_event_detach_bpf_prog()
writes it to NULL. It fixes the race by introducing READ_ONCE() and
WRITE_ONCE() in the respective read and write paths.

Reported-by: [email protected]
Closes: https://syzkaller.appspot.com/bug?extid=651d2774bd1d8395595f
Fixes: f11f10bfa1ca ("perf/bpf: Call BPF handler directly, not through overflow machinery")
Signed-off-by: Deepanshu Kartikey <[email protected]>

> diff --git a/kernel/events/core.c b/kernel/events/core.c
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c

[ ... ]

> @@ -10766,7 +10767,8 @@ static int __perf_event_overflow(struct perf_event *event,
>  	if (event->attr.aux_pause)
>  		perf_event_aux_pause(event->aux_event, true);
>  
> -	if (event->prog && event->prog->type == BPF_PROG_TYPE_PERF_EVENT &&
> +	prog = READ_ONCE(event->prog);
> +	if (prog && prog->type == BPF_PROG_TYPE_PERF_EVENT &&
>  	    !bpf_overflow_handler(event, data, regs))
>  		goto out;

[Severity: Medium]
This isn't a bug introduced by this patch, but does the independent second
read in bpf_overflow_handler() create a time-of-check to time-of-use race?

If a concurrent detach sets event->prog to NULL between the READ_ONCE() here
in __perf_event_overflow() and the second READ_ONCE() inside
bpf_overflow_handler():

bpf_overflow_handler() {
    prog = READ_ONCE(event->prog);
    if (prog) {
        ...
    }
    return 0;
}

Wouldn't bpf_overflow_handler() evaluate prog as NULL and return 0? The
caller __perf_event_overflow() evaluates !0 as true and executes goto out,
which unconditionally drops the perf sample.

Should the already-validated prog pointer be passed directly to
bpf_overflow_handler()?


> diff --git a/kernel/trace/bpf_trace.c b/kernel/trace/bpf_trace.c
> --- a/kernel/trace/bpf_trace.c
> +++ b/kernel/trace/bpf_trace.c
> @@ -2018,7 +2018,7 @@ void perf_event_detach_bpf_prog(struct perf_event *event)
>  
>  put:
>  	prog = event->prog;
> -	event->prog = NULL;
> +	WRITE_ONCE(event->prog, NULL);
>  
>  unlock:

[Severity: Medium]
This is a pre-existing issue, but does this incomplete fix leave other data
races in the attach and detach paths?

Other functions like perf_event_set_bpf_handler(),
perf_event_free_bpf_handler(), and perf_event_attach_bpf_prog() still
perform plain writes to event->prog without WRITE_ONCE().

Additionally, does this need release and acquire semantics? If the writes to
event->prog and event->bpf_cookie in the attach paths are reordered by the
CPU, could the NMI handler read a valid event->prog but a stale or
uninitialized event->bpf_cookie?

[Severity: High]
This is also a pre-existing issue, but does the lockless access in
perf_event_alloc() create a use-after-free or NULL pointer dereference
during BPF program inheritance?

During fork, perf_event_alloc() inherits the BPF program:

perf_event_alloc() {
    ...
    if (parent_event->prog) {
        struct bpf_prog *prog = parent_event->prog;
        bpf_prog_inc(prog);
        ...
    }
}

Since this accesses parent_event->prog locklessly, can a concurrent
bpf_perf_link_release() clear the pointer? If the compiler reloads the
pointer after the check, bpf_prog_inc() would dereference NULL.

Even if it is optimized to a single read, perf_event_alloc() is not inside
an RCU read-side critical section. Could a concurrent detach finish its RCU
grace period and free the BPF program, causing bpf_prog_inc() to write to
freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1