Re: [PATCH net 1/1] xdp: reject clones that overrun skb_shared_info tailroom

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing SKB_WITH_OVERHEAD check in `xdp_convert_zc_to_xdp_frame()` leads to in-page overwrite of packet data with `skb_shared_info`.
- [High] Incorrect `headroom` initialization in `xdp_convert_zc_to_xdp_frame()` causes an out-of-bounds write of `skb_shared_info` into the next physical page.
- [High] Silently dropping packet fragments in `xdp_convert_zc_to_xdp_frame()` causes truncation of multi-buffer packets.
--

--- Patch [1]: [PATCH net 1/1] xdp: reject clones that overrun skb_shared_info tailroom ---
commit 06453b67e02a5370da9d6643a102dd0628f4807c
Author: Zhiling Zou <zhilinz@nebusec.ai>

xdp: reject clones that overrun skb_shared_info tailroom

This commit updates xdpf_clone() to reject frames that do not leave
enough room for skb_shared_info. It changes the bounds check to use
SKB_WITH_OVERHEAD(PAGE_SIZE) to prevent build_skb_around() from
corrupting live packet bytes during XDP-to-SKB conversion.

> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9890a30584ba7..0194e69da339a 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -871,7 +871,7 @@ struct xdp_frame *xdpf_clone(struct xdp_frame *xdpf)
>  	headroom = xdpf->headroom + sizeof(*xdpf);
>  	totalsize = headroom + xdpf->len;
>  
> -	if (unlikely(totalsize > PAGE_SIZE))
> +	if (unlikely(totalsize > SKB_WITH_OVERHEAD(PAGE_SIZE)))
>  		return NULL;
>  	page = dev_alloc_page();
>  	if (!page)

Looking at the fix applied here, there appear to be similar pre-existing
issues in the same file within xdp_convert_zc_to_xdp_frame():

net/core/xdp.c:xdp_convert_zc_to_xdp_frame() {
	metasize = xdp_data_meta_unsupported(xdp) ? 0 :
		   xdp->data - xdp->data_meta;
	totsize = xdp->data_end - xdp->data + metasize;

[Severity: High]
This isn't a bug introduced by this patch, but does this calculation
result in silently dropped packet fragments?

If an AF_XDP multi-buffer zero-copy packet with XDP_FLAGS_HAS_FRAGS set is
passed here, only the linear data size is calculated. The fragments appear
to be ignored, and the original xdp_buff is freed back to the pool,
returning a truncated xdp_frame without propagating the fragments.

	if (sizeof(*xdpf) + totsize > PAGE_SIZE)
		return NULL;

[Severity: High]
This is a pre-existing issue, but should this bounds check also use
SKB_WITH_OVERHEAD(PAGE_SIZE) similar to the fix in xdpf_clone()?

If an AF_XDP zero-copy packet has a linear length between
SKB_WITH_OVERHEAD(PAGE_SIZE) and PAGE_SIZE, it will pass this check.
When __xdp_build_skb_from_frame() later converts this frame into an SKB,
build_skb_around() could overwrite the valid packet payload with
skb_shared_info.

	addr += sizeof(*xdpf);
	data_to_copy = metasize ? xdp->data_meta : xdp->data;
	memcpy(addr, data_to_copy, totsize);

	xdpf->data = addr + metasize;
	xdpf->len = totsize - metasize;
	xdpf->headroom = 0;

[Severity: High]
This isn't a bug introduced by this patch, but could setting headroom to 0
instead of metasize cause an out-of-bounds write?

If metasize is greater than 0, setting headroom to 0 shifts the computed
hard_start of the SKB buffer forward by metasize bytes. When
build_skb_around() is called on this buffer with a length of PAGE_SIZE,
the buffer logically extends past the allocated page boundary.

If fragments are later appended or the tailroom is used, could the stack
write directly into the next physical page?
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6b2afef5d1738763c6965e8e466eb16e43e4f956.1785757386.git.zhilinz@nebusec.ai?part=1