[bpf,1/2] bpf: Fix racing between bpf_timer_cancel_and_free and bpf_timer_cancel

Commit Message

Martin KaFai Lau Feb. 15, 2024, 9:12 p.m. UTC

From: Martin KaFai Lau <[e-mail]>

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 <[e-mail]>
Signed-off-by: Martin KaFai Lau <[e-mail]>
---
 kernel/bpf/helpers.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Hou Tao Feb. 18, 2024, 7:13 a.m. UTC | #1

On 2/16/2024 5:12 AM, Martin KaFai Lau wrote:
> From: Martin KaFai Lau <[e-mail]>
>
> 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 <[e-mail]>
> Signed-off-by: Martin KaFai Lau <[e-mail]>

Acked-by: Hou Tao <[e-mail]>

[e-mail] Feb. 19, 2024, 11:30 a.m. UTC | #2

Hello:

This series was applied to bpf/bpf.git (master)
by Daniel Borkmann <[e-mail]>:

On Thu, 15 Feb 2024 13:12:17 -0800 you wrote:
> From: Martin KaFai Lau <[e-mail]>
> 
> 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();
> 
> [...]

Here is the summary with links:
  - [bpf,1/2] bpf: Fix racing between bpf_timer_cancel_and_free and bpf_timer_cancel
    https://git.kernel.org/bpf/bpf/c/0281b919e175
  - [bpf,2/2] selftests/bpf: Test racing between bpf_timer_cancel_and_free and bpf_timer_cancel
    https://git.kernel.org/bpf/bpf/c/3f00e4a9c96f

You are awesome, thank you!

Patch

diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c
index be72824f32b2..d19cd863d294 100644
--- a/kernel/bpf/helpers.c
+++ b/kernel/bpf/helpers.c
@@ -1101,6 +1101,7 @@  struct bpf_hrtimer {
 	struct bpf_prog *prog;
 	void __rcu *callback_fn;
 	void *value;
+	struct rcu_head rcu;
 };
 
 /* the actual struct hidden inside uapi struct bpf_timer */
@@ -1332,6 +1333,7 @@  BPF_CALL_1(bpf_timer_cancel, struct bpf_timer_kern *, timer)
 
 	if (in_nmi())
 		return -EOPNOTSUPP;
+	rcu_read_lock();
 	__bpf_spin_lock_irqsave(&timer->lock);
 	t = timer->timer;
 	if (!t) {
@@ -1353,6 +1355,7 @@  BPF_CALL_1(bpf_timer_cancel, struct bpf_timer_kern *, timer)
 	 * if it was running.
 	 */
 	ret = ret ?: hrtimer_cancel(&t->timer);
+	rcu_read_unlock();
 	return ret;
 }
 
@@ -1407,7 +1410,7 @@  void bpf_timer_cancel_and_free(void *val)
 	 */
 	if (this_cpu_read(hrtimer_running) != t)
 		hrtimer_cancel(&t->timer);
-	kfree(t);
+	kfree_rcu(t, rcu);
 }
 
 BPF_CALL_2(bpf_kptr_xchg, void *, map_value, void *, ptr)