Re: [PATCH 6.6.y] bpf: Defer work in bpf_timer_cancel_and_free

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] bpf: UAF in bpf_timer_cb due to deferred map element cancellation
--

commit 7fae89f817868d5f5aa9bd764292b2373d1f4677
Author: Kumar Kartikeya Dwivedi <memxor@gmail.com>

bpf: Defer work in bpf_timer_cancel_and_free

This commit addresses a deadlock and use-after-free scenario in
bpf_timer_cancel_and_free() by deferring the cancellation of hrtimers to a
workqueue when running within a timer callback.

Link: https://lore.kernel.org/r/20240709185440.1104957-3-memxor@gmail.com

> diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
> --- a/kernel/bpf/helpers.c
> +++ b/kernel/bpf/helpers.c
[ ... ]
> @@ -1484,14 +1504,8 @@ void bpf_timer_cancel_and_free(void *val)
[ ... ]
>  	 *  CPU 1 (timer1_cb)			CPU 2 (timer2_cb)
>  	 *  bpf_timer_cancel_and_free(timer2)	bpf_timer_cancel_and_free(timer1)
>  	 *
>  	 * To avoid these issues, punt to workqueue context when we are in a
>  	 * timer callback.
>  	 */
> -	if (this_cpu_read(hrtimer_running) != t)
> -		hrtimer_cancel(&t->timer);
> -	kfree_rcu(t, cb.rcu);
> +	if (this_cpu_read(hrtimer_running))
> +		queue_work(system_unbound_wq, &t->cb.delete_work);

[Severity: High]
Does punting the cancellation to a workqueue and returning immediately break
the synchronous cancellation guarantee?

If a BPF program running in a timer callback deletes an element containing a
second timer from a preallocated hash map (or updates an array map),
bpf_timer_cancel_and_free() defers hrtimer_cancel() to the workqueue and
returns immediately.

This allows the caller to immediately recycle the map element (pushing it
to a freelist for preallocated maps, or overwriting it in array maps)
without waiting for the timer callback to finish. If the second timer's
callback is currently executing on another CPU, it will continue to read
and write the map element data, which now belongs to a different key or
has been overwritten.

Can this lead to a use-after-free and data corruption in BPF map elements?

> +	else
> +		bpf_timer_delete_work(&t->cb.delete_work);
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260929030524.86560-1-artem@trailofbits.com?part=1