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

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_fram=
e()` causes an out-of-bounds write of `skb_shared_info` into the next physi=
cal 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_i=
nfo tailroom ---
commit 06453b67e02a5370da9d6643a102dd0628f4807c
Author: Zhiling Zou <[email protected]>

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 =3D xdpf->headroom + sizeof(*xdpf);
>  	totalsize =3D headroom + xdpf->len;
> =20
> -	if (unlikely(totalsize > PAGE_SIZE))
> +	if (unlikely(totalsize > SKB_WITH_OVERHEAD(PAGE_SIZE)))
>  		return NULL;
>  	page =3D 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 =3D xdp_data_meta_unsupported(xdp) ? 0 :
		   xdp->data - xdp->data_meta;
	totsize =3D 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 +=3D sizeof(*xdpf);
	data_to_copy =3D metasize ? xdp->data_meta : xdp->data;
	memcpy(addr, data_to_copy, totsize);

	xdpf->data =3D addr + metasize;
	xdpf->len =3D totsize - metasize;
	xdpf->headroom =3D 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?
}

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/6b2afef5d1738763c69=
[email protected]?part=3D1