Re: [PATCH net-next v1 03/11] xdp: Copy buffers with restricted lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] xdp: missing xdp_data_meta_unsupported() check in
  xdp_build_skb_from_zc()
- [High] xdp: skb_shared_info overwrite in xdp_build_skb_from_zc() via
  aligned memcpy
It also noticed 2 related pre-existing issues this patch does not
introduce:
- [Critical] bpf: double free and memory leak in xdp_do_redirect() [1]
- [Critical] xdp: double-free in xdp_return_buff() [2]
--

commit b80d3a81c03c102ec5d4e46044e260ec432b9137
Author: Björn Töpel <bjorn@kernel.org>

xdp: Copy buffers with restricted lifetime

This commit adds the MP_CAP_RETAIN memory provider capability and copies XDP
buffers from providers without MP_CAP_RETAIN when building an skb, converting
to an xdp_frame, or redirecting outside XSKMAP.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 80380f89adf90..f93839b493b25 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4774,12 +4774,28 @@ int xdp_do_redirect(struct net_device *dev, struct xdp_buff *xdp,
>  {
>  	struct bpf_redirect_info *ri = bpf_net_ctx_get_ri();
>  	enum bpf_map_type map_type = ri->map_type;
> +	struct xdp_frame *xdpf;
> +	int err;
>  
>  	if (map_type == BPF_MAP_TYPE_XSKMAP)
>  		return __xdp_do_redirect_xsk(ri, dev, xdp, xdp_prog);
>  
> -	return __xdp_do_redirect_frame(ri, dev, xdp_convert_buff_to_frame(xdp),
> -				       xdp_prog);
> +	if (likely(!xdp_buff_needs_copy(xdp)))
> +		return __xdp_do_redirect_frame(ri, dev,
> +					       xdp_convert_buff_to_frame(xdp),
> +					       xdp_prog);
> +
> +	/* Release the source only after the target took the copy. */
> +	xdpf = xdp_copy_buff_to_frame(xdp);
> +	err = __xdp_do_redirect_frame(ri, dev, xdpf, xdp_prog);

[Severity: Critical]
This problem wasn't introduced by this patch, but can redirecting a
MEM_TYPE_XSK_BUFF_POOL buffer still double free xdp and leak xdpf [1]
when __xdp_do_redirect_frame() fails?

For MEM_TYPE_XSK_BUFF_POOL buffers, xdp_buff_needs_copy(xdp) is false, so
xdp_do_redirect() takes the xdp_convert_buff_to_frame() ->
xdp_convert_zc_to_xdp_frame() path, which frees xdp via xsk_buff_free() before
__xdp_do_redirect_frame() runs.  If __xdp_do_redirect_frame() returns an
error, the allocated xdpf is leaked and the caller frees xdp a second time on
its error path.

> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 1d679e8fd6493..0cf46d492d91b 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -559,10 +561,41 @@ void xdp_return_buff(struct xdp_buff *xdp)
>  			     xdp->rxq->mem.type, true, xdp);
>  
>  out:
> -	__xdp_return(virt_to_netmem(xdp->data), xdp->rxq->mem.type, true, xdp);
> +	__xdp_return(xdp_buff_head_netmem(xdp), xdp->rxq->mem.type, true, xdp);
>  }

[Severity: Critical]
This problem wasn't introduced by this patch, but does xdp_return_buff() call
xsk_buff_free(xdp) once per fragment [2] on multi-fragment
MEM_TYPE_XSK_BUFF_POOL buffers?

When xdp->rxq->mem.type is MEM_TYPE_XSK_BUFF_POOL, __xdp_return() ignores the
netmem argument and calls xsk_buff_free(xdp), which frees the head buffer and
all attached fragments at once.  Looping over sinfo->nr_frags before the final
__xdp_return() frees the same xdp_buff 1 + nr_frags times, corrupting the XSK
buffer pool.

[ ... ]

> @@ -638,6 +689,9 @@ struct sk_buff *xdp_build_skb_from_buff(const struct xdp_buff *xdp)
>  	u32 nr_frags = 0;
>  	int metalen;
>  
> +	if (unlikely(xdp_buff_needs_copy(xdp)))
> +		return xdp_build_skb_from_zc(xdp);

[Severity: High]
Does delegating to xdp_build_skb_from_zc() drop the first byte of the packet
when xdp_data_meta_unsupported(xdp) is true?

When a driver passes meta_valid == false to xdp_prepare_buff(), or calls
xdp_set_data_meta_invalid(), xdp->data_meta is set to xdp->data + 1.  Unlike
xdp_copy_buff_to_frame(), xdp_build_skb_from_zc() does not check
xdp_data_meta_unsupported(xdp):

net/core/xdp.c:xdp_build_skb_from_zc() {
	const struct xdp_rxq_info *rxq = xdp->rxq;
	u32 len = xdp->data_end - xdp->data_meta;
	...
	skb_reserve(skb, xdp->data_meta - xdp->data_hard_start);

	memcpy(__skb_put(skb, len), xdp->data_meta, LARGEST_ALIGN(len));

	metalen = xdp->data - xdp->data_meta;
	if (metalen > 0) {
		skb_metadata_set(skb, metalen);
		__skb_pull(skb, metalen);
	}
}

With xdp->data_meta == xdp->data + 1, this copies from xdp->data + 1, sets
skb->len one byte short, and skips __skb_pull() because metalen is -1,
corrupting the packet header on XDP_PASS.

[ ... ]

> @@ -781,7 +835,7 @@ struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
>  		goto out;
>  	}
>  
> -	xsk_buff_free(xdp);
> +	xdp_release_copied(xdp);

[Severity: High]
Can the LARGEST_ALIGN(len) memcpy in xdp_build_skb_from_zc() overwrite
skb_shinfo(skb) when copying a buffer that reaches xdp_data_hard_end(xdp)?

In xdp_build_skb_from_zc(), napi_build_skb(data, truesize) initializes
skb_shinfo(skb) at data + truesize - SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)), and then:

	skb_reserve(skb, xdp->data_meta - xdp->data_hard_start);

	memcpy(__skb_put(skb, len), xdp->data_meta, LARGEST_ALIGN(len));

When truesize equals xdp->frame_sz, skb->end is at the same offset from data
as xdp_data_hard_end(xdp) is from xdp->data_hard_start.  If an XDP program
adjusts xdp->data or xdp->data_meta to an offset that is not 16-byte aligned
and the head buffer is full up to xdp_data_hard_end(xdp), LARGEST_ALIGN(len)
extends up to 15 bytes past skb->end and overwrites skb_shinfo(skb) (including
nr_frags and frag_list) with bytes from xdp->data_end.

[1] https://sashiko.dev/#/bug/linux-c51b4a6d-8f07-471a-858e-04053743c6d2
[2] https://sashiko.dev/#/bug/linux-ad1513d6-3423-4560-95ce-a92871813ba4

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110102039.1002605-1-bjorn@kernel.org?part=3