Re: [PATCH] bpf: net_sched: reject bpf_throw() in qdisc teardown callbacks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Low] bpf: net_sched: duplicated st_ops check and lookup in bpf_qdisc_kfunc_filter()
- [Low] bpf: net_sched: raw bug-report boilerplate in commit message
--

commit 4987df9cdbc82b0c8a5dd77111f15b5660ca8808
Author: shaos <shawnq1@163.com>

bpf: net_sched: reject bpf_throw() in qdisc teardown callbacks

This commit updates bpf_qdisc_kfunc_filter() to reject calls to bpf_throw()
from BPF qdisc reset and destroy callbacks so that the generated watchdog
cancellation epilogue is not bypassed during qdisc teardown.

>     bpf: net_sched: reject bpf_throw() in qdisc teardown callbacks
>     
>     Hello,
>     
>     A BPF qdisc's generated reset/destroy epilogue cancels its watchdog
>     timer.  bpf_throw() unwinds directly to the exception callback and
>     skips that epilogue.  If the timer was armed, qdisc deletion frees the
>     qdisc while the embedded timer remains queued.  Later timer-queue
>     activity accesses the freed qdisc, producing a KASAN slab-use-after-free
>     and a kernel crash.
>     
>     The reproducer registers a BPF qdisc whose dequeue callback arms the
>     watchdog and whose reset/destroy callbacks throw.  Two fresh KASAN boots
>     reported use-after-free in rb_insert_color on an object allocated by
>     qdisc_alloc() and later freed by qdisc teardown.  An otherwise identical
>     no-throw control completed deletion cleanly.  The required BPF and
>     network-administration capabilities make this a privileged local kernel
>     crash; no privilege escalation or code execution is demonstrated.
>     
>     HEAD commit:    c03c1a14507dfaf8b856c2608f9ac216021fc222
>     Git tree:       30fd2ef82912c26cf3a58ad0ba6afc375fa851ac
>     Source version: Linux 7.3
>     Compiler:       gcc 11.4.0, GNU ld 2.38, clang 20 (BPF PoC)
>     
>     The console log, full KASAN kernel config, and BPF C reproducer are
>     available from one fixed-revision Gist:
>     
>     Console output:
>     https://gist.githubusercontent.com/shaos26/9908a8ddac69e303cb5dcc709958537f/raw/3fa043e43683907bcccacdc80a71b150920049da/console-output.txt
>     Kernel config:
>     https://gist.githubusercontent.com/shaos26/9908a8ddac69e303cb5dcc709958537f/raw/3fa043e43683907bcccacdc80a71b150920049da/kernel-config.txt
>     PoC source:
>     https://gist.githubusercontent.com/shaos26/9908a8ddac69e303cb5dcc709958537f/raw/3fa043e43683907bcccacdc80a71b150920049da/poc.c
>     
>     Run this as root in a disposable VM booted with the target kernel,
>     CONFIG_DEBUG_INFO_BTF=y, bpftool, iproute2, clang 20, and libbpf-dev.
>     Build each variant against that kernel's BTF; test them on separate
>     fresh boots.  With libbpf headers installed under /usr/include:
>     
>         bpftool btf dump file /sys/kernel/btf/vmlinux format c > vmlinux.h
>         CLANG=/usr/lib/llvm-20/bin/clang
>         LIBBPF_INC=/usr/include
>         "$CLANG" -target bpf -D__TARGET_ARCH_x86 -O2 -g -Wall -Werror \
>           -Wno-missing-declarations -I. -I"$LIBBPF_INC" \
>           -DVARIANT_THROW=1 -c poc.c -o qdisc-throw.bpf.o
>         "$CLANG" -target bpf -D__TARGET_ARCH_x86 -O2 -g -Wall -Werror \
>           -Wno-missing-declarations -I. -I"$LIBBPF_INC" \
>           -DVARIANT_THROW=0 -c poc.c -o qdisc-control.bpf.o
>     
>     Mount bpffs if it is not already mounted, register the matching object,
>     attach its qdisc to loopback, send one packet to arm the watchdog, and
>     delete the qdisc:
>     
>         mkdir -p /sys/fs/bpf
>         mount -t bpf bpffs /sys/fs/bpf 2>/dev/null || true
>         bpftool struct_ops register qdisc-throw.bpf.o /sys/fs/bpf
>         ip link set lo up
>         tc qdisc add dev lo root handle 1: agent31_uaf
>         ping -c 1 -W 1 127.0.0.1
>         tc qdisc del dev lo root
>         sleep 10
>     
>     For the separate control boot, register qdisc-control.bpf.o and use
>     qdisc kind agent31_ctl in the same sequence.
>     
>     On the vulnerable kernel, the two throw runs reproduced the KASAN UAF;
>     the no-throw control survived.  With the patch below, the verifier
>     rejects bpf_throw() from reset/destroy with -EACCES, before qdisc
>     registration.  The patched control object registered, attached, armed
>     and deleted normally, then survived the watchdog interval.
>     
>     Fixes: 7a2dafda950b ("bpf: net_sched: Add a qdisc watchdog timer")
>     Signed-off-by: shaos <shawnq1@163.com>

[Severity: Low]
This isn't a bug, but the commit message includes a conversational greeting
("Hello,"), bug-report metadata headers (HEAD commit, Git tree, Source
version, Compiler), external Gist URLs, and VM build/run shell instructions.

Could the commit message be trimmed to a self-contained description of the
problem and the fix without the bug-report boilerplate and external links?

> diff --git a/net/sched/bpf_qdisc.c b/net/sched/bpf_qdisc.c
> index 5691c13781a8b..20c0b892d2cb1 100644
> --- a/net/sched/bpf_qdisc.c
> +++ b/net/sched/bpf_qdisc.c
> @@ -12,6 +12,7 @@
>  #define QDISC_MOFF_IDX(moff)	(moff / sizeof(void (*)(void)))
>  
>  static struct bpf_struct_ops bpf_Qdisc_ops;
> +static const struct bpf_verifier_ops bpf_qdisc_verifier_ops;
>  
>  struct bpf_sched_data {
>  	struct qdisc_watchdog watchdog;

[ ... ]

> @@ -305,20 +307,31 @@ enum qdisc_ops_kf_flags {
>  	QDISC_OPS_KF_COMMON		= 0,
>  	QDISC_OPS_KF_ENQUEUE		= 1 << 0,
>  	QDISC_OPS_KF_DEQUEUE		= 1 << 1,
> +	QDISC_OPS_KF_TEARDOWN		= 1 << 2,
>  };
>  
>  static const u32 qdisc_ops_context_flags[] = {
>  	[QDISC_OP_IDX(enqueue)]		= QDISC_OPS_KF_ENQUEUE,
>  	[QDISC_OP_IDX(dequeue)]		= QDISC_OPS_KF_DEQUEUE,
>  	[QDISC_OP_IDX(init)]		= QDISC_OPS_KF_COMMON,
> -	[QDISC_OP_IDX(reset)]		= QDISC_OPS_KF_COMMON,
> -	[QDISC_OP_IDX(destroy)]		= QDISC_OPS_KF_COMMON,
> +	[QDISC_OP_IDX(reset)]		= QDISC_OPS_KF_TEARDOWN,
> +	[QDISC_OP_IDX(destroy)]		= QDISC_OPS_KF_TEARDOWN,
>  };
>  
>  static int bpf_qdisc_kfunc_filter(const struct bpf_prog *prog, u32 kfunc_id)
>  {
>  	u32 moff, flags;
>  
> +	/* bpf_throw() bypasses the generated watchdog-cancellation epilogue. */
> +	if (kfunc_id == bpf_qdisc_throw_ids[0] &&
> +	    prog->aux->st_ops &&
> +	    prog->aux->st_ops->verifier_ops == &bpf_qdisc_verifier_ops) {
> +		moff = prog->aux->attach_st_ops_member_off;
> +		flags = qdisc_ops_context_flags[QDISC_MOFF_IDX(moff)];
> +		if (flags & QDISC_OPS_KF_TEARDOWN)
> +			return -EACCES;
> +	}

[Severity: Low]
This isn't a bug, but bpf_Qdisc_ops is already forward-declared in
net/sched/bpf_qdisc.c, and the existing code right below in
bpf_qdisc_kfunc_filter() already checks prog->aux->st_ops against
&bpf_Qdisc_ops and computes moff and flags:

	if (!btf_id_set8_contains(&qdisc_kfunc_ids, kfunc_id))
		return 0;

	if (prog->aux->st_ops != &bpf_Qdisc_ops)
		return -EACCES;

	moff = prog->aux->attach_st_ops_member_off;
	flags = qdisc_ops_context_flags[QDISC_MOFF_IDX(moff)];

Could bpf_qdisc_kfunc_filter() check prog->aux->st_ops == &bpf_Qdisc_ops
instead of adding a forward declaration for bpf_qdisc_verifier_ops, and share
the moff and flags lookup rather than duplicating it?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6AEA1AAD.2E7977.08739@m16.mail.163.com?part=1