Re: [PATCH bpf 1/2] bpf, tcx: reject offloaded programs on attach

[sashiko-bot@kernel.org]

Sashiko AI review found 2 potential issue(s):
- [High] Incomplete fix: Offloaded programs can still be attached to software paths via BPF_LINK_UPDATE
- [High] Missing device-bound validation in TCX and Netkit attach paths
--

commit ea2bbf8a59b47b06842fc8c920b0debcc1bed553
Author: Jiayuan Chen <jiayuan.chen@linux.dev>

bpf, tcx: reject offloaded programs on attach
    
An offloaded prog's bpf_func is replaced by bpf_prog_warn_on_exec(),
since it's supposed to run on the NIC, not the host. But tcx doesn't
check this and happily attaches it to the software path, so the first
packet hits the WARN.

Closes: https://lore.kernel.org/bpf/64d8e2b5-a214-4f3c-b9e8-bcedbcb2c602@hust.edu.cn/

> diff --git a/kernel/bpf/tcx.c b/kernel/bpf/tcx.c
> index 02db0113b8e7c..86f4636d5a677 100644
> --- a/kernel/bpf/tcx.c
> +++ b/kernel/bpf/tcx.c
> @@ -16,6 +16,9 @@ int tcx_prog_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  	struct net_device *dev;
>  	int ret;
>  
> +	if (bpf_prog_is_offloaded(prog->aux))
> +		return -EINVAL;
> +

The commit message mentions using bpf_prog_is_offloaded() instead of
bpf_prog_is_dev_bound() so that dev-bound-only programs keep working for TCX
and Netkit.

XDP validates device matching in dev_xdp_attach():

    if (bpf_prog_is_dev_bound(new_prog->aux) &&
        !bpf_offload_dev_match(new_prog, dev)) {
        ...
    }

The kernel relies on attach-time validation to guarantee safety for hardware
metadata kfuncs, as documented in bpf_dev_bound_resolve_kfunc():

"We rely on bpf_dev_bound_match() check at attach to render this program
unusable"

If a program is device-bound, shouldn't we still verify it is being attached
to the correct device?

Without a similar bpf_offload_dev_match() check here in the TCX and Netkit
attach paths, could a user attach a program bound to device A onto device B?

Could this cause driver-specific kfuncs to execute on device B's SKBs while
blindly assuming device A's internal data layout, leading to out-of-bounds
memory accesses and kernel panics?

>  	rtnl_lock();
>  	dev = __dev_get_by_index(net, attr->target_ifindex);

[ ... ]

> @@ -315,6 +318,9 @@ int tcx_link_attach(const union bpf_attr *attr, struct bpf_prog *prog)
>  	struct tcx_link *tcx;
>  	int ret;
>  
> +	if (bpf_prog_is_offloaded(prog->aux))
> +		return -EINVAL;
> +

While this prevents offloaded programs from being attached initially, do
we need to protect the link update paths against this regression as well?

It appears tcx_link_update() and netkit_link_update() pass the new program
directly to bpf_mprog_attach() without validating its offload status:

kernel/bpf/tcx.c:tcx_link_update() {
    ...
    ret = bpf_mprog_attach(entry, &entry_new, nprog, oprog, NULL,
                           link->flags, 0, 0);
    ...
}

If a user creates a valid link with a standard program, could they use
BPF_LINK_UPDATE to replace it with an offloaded program?

Similarly, does bpf_xdp_link_update() lack the bpf_prog_is_offloaded() check
present in dev_xdp_attach(), allowing it to install an offloaded program into
an SKB or native mode XDP link?

If an offloaded program is attached via these update paths, its bpf_func is
replaced by bpf_prog_warn_on_exec().

Would executing this on the host datapath trigger the WARN(1) panic this
patch intended to fix, potentially allowing a local DoS if panic_on_warn
is set?

>  	rtnl_lock();
>  	dev = __dev_get_by_index(net, attr->link_create.target_ifindex);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260423033609.252464-1-jiayuan.chen@linux.dev?part=1