On 2/16/2024 5:12 AM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
>
> The following race is possible between bpf_timer_cancel_and_free
> and bpf_timer_cancel. It will lead a UAF on the timer->timer.
>
> bpf_timer_cancel();
> spin_lock();
> t = timer->time;
> spin_unlock();
>
> bpf_timer_cancel_and_free();
> spin_lock();
> t = timer->timer;
> timer->timer = NULL;
> spin_unlock();
> hrtimer_cancel(&t->timer);
> kfree(t);
>
> /* UAF on t */
> hrtimer_cancel(&t->timer);
>
> In bpf_timer_cancel_and_free, this patch frees the timer->timer
> after a rcu grace period. This requires a rcu_head addition
> to the "struct bpf_hrtimer". Another kfree(t) happens in bpf_timer_init,
> this does not need a kfree_rcu because it is still under the
> spin_lock and timer->timer has not been visible by others yet.
>
> In bpf_timer_cancel, rcu_read_lock() is added because this helper
> can be used in a non rcu critical section context (e.g. from
> a sleepable bpf prog). Other timer->timer usages in helpers.c
> have been audited, bpf_timer_cancel() is the only place where
> timer->timer is used outside of the spin_lock.
>
> Another solution considered is to mark a t->flag in bpf_timer_cancel
> and clear it after hrtimer_cancel() is done. In bpf_timer_cancel_and_free,
> it busy waits for the flag to be cleared before kfree(t). This patch
> goes with a straight forward solution and frees timer->timer after
> a rcu grace period.
>
> Fixes: b00628b1c7d5 ("bpf: Introduce bpf timers.")
> Suggested-by: Alexei Starovoitov <ast@xxxxxxxxxx>
> Signed-off-by: Martin KaFai Lau <martin.lau@xxxxxxxxxx>
Acked-by: Hou Tao <houtao1@xxxxxxxxxx>
