Re: [PATCH bpf v3 1/2] bpf: Run generic devmap egress prog on private skb

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] A maliciously crafted or buggy BPF devmap egress program can cause a double free of an skb by returning an action code that implicitly casts to a negative integer.
- [High] skb_copy() zeroes out skb->mac_len, causing generic XDP to operate on the wrong skb->data offset and potentially read garbage from uninitialized headroom.
--

commit b95ebb55a25ce44657b1e9ee2add3b1177f06853
Author: Sun Jian <sun.jian.kdev@gmail.com>

bpf: Run generic devmap egress prog on private skb

This commit introduces a fix to prevent multiple destinations from sharing
the same packet data in generic devmap egress programs by copying cloned
skbs before running the program.

> diff --git a/kernel/bpf/devmap.c b/kernel/bpf/devmap.c
> index cc0a43ebab6b9..14506834345aa 100644
> --- a/kernel/bpf/devmap.c
> +++ b/kernel/bpf/devmap.c
> @@ -512,8 +512,10 @@ static inline int __xdp_enqueue(struct net_device *dev, struct xdp_frame *xdpf,
>  	return 0;
>  }
>  
> -static u32 dev_map_bpf_prog_run_skb(struct sk_buff *skb, struct bpf_dtab_netdev *dst)
> +static int dev_map_bpf_prog_run_skb(struct sk_buff **pskb,
> +				    struct bpf_dtab_netdev *dst)

[Severity: Critical]
The return type here was changed from u32 to int. Because xdp action codes
returned by bpf programs are unconstrained 32-bit unsigned integers, what
happens if a program returns a value with the highest bit set? 

Could this arbitrary unsigned action code be implicitly cast to a negative
integer, causing callers to mistakenly interpret it as a kernel error?

>  {
> +	struct sk_buff *skb = *pskb;
>  	struct xdp_txq_info txq = { .dev = dst->dev };
>  	struct xdp_buff xdp;

[ ... ]

> @@ -521,6 +523,18 @@ static u32 dev_map_bpf_prog_run_skb(struct sk_buff *skb, struct bpf_dtab_netdev
>  	if (!dst->xdp_prog)
>  		return XDP_PASS;
>  
> +	if (skb_cloned(skb)) {
> +		struct sk_buff *nskb;
> +
> +		nskb = skb_copy(skb, GFP_ATOMIC);

[Severity: High]
Does skb_copy preserve the mac_len field?

Because skb_copy allocates a new skb and zeroes out the structure up to the
headers, mac_len is left as 0.

> +		if (!nskb)
> +			return -ENOMEM;
> +
> +		consume_skb(skb);
> +		skb = nskb;
> +		*pskb = nskb;
> +	}
> +
>  	__skb_pull(skb, skb->mac_len);

[Severity: High]
Following up on the skb_copy question above, if mac_len is 0, will this
pull 0 bytes instead of the expected header length?

If so, the data pointer would remain at the mac header instead of the
network header. If an egress program later modifies the ethernet header
and triggers the mac update path, could this push the data pointer
backwards into uninitialized headroom, leading to protocol corruption
or leaked kernel memory?

[ ... ]

> @@ -710,7 +724,10 @@ int dev_map_generic_redirect(struct bpf_dtab_netdev *dst, struct sk_buff *skb,
>  	 * return 0 even if packet is dropped. Helper below takes care of
>  	 * freeing skb.
>  	 */
> -	if (dev_map_bpf_prog_run_skb(skb, dst) != XDP_PASS)
> +	err = dev_map_bpf_prog_run_skb(&skb, dst);
> +	if (err < 0)
> +		return err;

[Severity: Critical]
If dev_map_bpf_prog_run_skb returns a negative integer due to an implicitly
cast unsigned action code as mentioned above, will it hit this error path?

If dev_map_bpf_prog_run_skb already dropped the packet via kfree_skb, will
propagating this negative value cause higher level callers to execute their
own error paths and free the identical skb a second time?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260611043317.512843-1-sun.jian.kdev@gmail.com?part=1