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

On 4/24/26 11:37 AM, [email protected] wrote:
> 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 <[email protected]>
>
> 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/[email protected]/
>
>> 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?


On bpf_offload_dev_match(): SCHED_CLS doesn't allow dev-bound-only 
(rejected in bpf_prog_dev_bound_init()),

so bpf_prog_is_offloaded() already covers every case: A program offloaded
to device A attached to device B is rejected unconditionally.

>>   	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?
You're right about the update path — will fix in v2.
> 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?

The same hole exists on the XDP side (bpf_xdp_link_update() bypasses 
dev_xdp_attach()'s offload check); I'll send a separate patch for that.


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