Re: [PATCH bpf v2] bpf: devmap: reject fragmented frames in clone-based broadcasts

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Generic XDP devmap broadcast executes eBPF programs on cloned SKBs without unsharing the data buffer, causing silent data corruption across destinations.
--

commit aa9501e9eb38c9d016ad75d6259b7ed07d668fb0
Author: Zhao Zhang <zzhan461@ucr.edu>
Subject: bpf: devmap: reject fragmented frames in clone-based broadcasts

This commit rejects fragmented native and generic XDP frames in clone-based 
broadcasts. By adding checks in the generic and native clone paths, it 
prevents potential out-of-bounds access.

> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index cc0a43ebab6b9..5b9eac5342a90 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c

[ ... ]

> @@ -726,6 +730,9 @@ static int dev_map_redirect_clone(struct bpf_dtab_netdev *dst,
>  	struct sk_buff *nskb;
>  	int err;
>  
> +	if (unlikely(skb_is_nonlinear(skb)))
> +		return -EOPNOTSUPP;
> +
>  	nskb = skb_clone(skb, GFP_ATOMIC);

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to use skb_clone 
here without unsharing the buffer before executing the eBPF program?

Since nskb is passed to dev_map_generic_redirect, which eventually calls 
dev_map_bpf_prog_run_skb to execute the destination's eBPF program:

dev_map_bpf_prog_run_skb()
    ...
    act = bpf_prog_run_generic_xdp(skb, &xdp, dst->xdp_prog);

Does this grant the XDP program direct write access to the cloned SKB 
payload? If the data buffer is shared and never unshared, could modifications 
made by the eBPF program for one destination silently corrupt the packet 
data for the original SKB and all other clones in the broadcast loop?

>  	if (!nskb)
>  		return -ENOMEM;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/21c2d153dd25603d359069a02bf06779b51f6423.1780385378.git.zzhan461@ucr.edu?part=1