Commit 0281b919 authored by Martin KaFai Lau's avatar Martin KaFai Lau Committed by Daniel Borkmann
Browse files

bpf: Fix racing between bpf_timer_cancel_and_free and bpf_timer_cancel



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: b00628b1 ("bpf: Introduce bpf timers.")
Suggested-by: default avatarAlexei Starovoitov <ast@kernel.org>
Signed-off-by: default avatarMartin KaFai Lau <martin.lau@kernel.org>
Signed-off-by: default avatarDaniel Borkmann <daniel@iogearbox.net>
Acked-by: default avatarHou Tao <houtao1@huawei.com>
Link: https://lore.kernel.org/bpf/20240215211218.990808-1-martin.lau@linux.dev
parent 54d46c9f
Loading
Loading
Loading
Loading
+4 −1
Original line number Diff line number Diff line
@@ -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)