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

On Tue, Aug 4, 2026 at 8:16=E2=80=AFPM <[email protected]> wrote:
>
> Thank you for your contribution! Sashiko AI review found 3 potential issu=
e(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_fr=
ame()` causes an out-of-bounds write of `skb_shared_info` into the next phy=
sical page.
> - [High] Silently dropping packet fragments in `xdp_convert_zc_to_xdp_fra=
me()` 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 <[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;
> >
> > -     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 i=
s
> passed here, only the linear data size is calculated. The fragments appea=
r
> 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?
> }
>
> --
> Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/6b2afef5d1738763c=
[email protected]?part=3D1

Thanks for the review.

The reported issues are not introduced by this patch. This patch is limited
to the reproduced devmap broadcast clone path in xdpf_clone(), where the cl=
one
is copied into a single page and can later be converted to an skb without
reserved skb_shared_info tailroom.

I agree that xdp_convert_zc_to_xdp_frame() looks like a similar pre-existin=
g
area worth auditing separately: it also clones into a PAGE_ORDER0 xdp_frame
and sets frame_sz to PAGE_SIZE. The SKB_WITH_OVERHEAD() bound and the
metadata/headroom handling there need separate validation because that path=
 is
for XSK zero-copy conversion, not the devmap broadcast clone path fixed her=
e.

For the fragment point, I would like to verify the reachable path first. If=
 a
fragmented XSK zero-copy xdp_buff can reach xdp_convert_zc_to_xdp_frame(),
then preserving or rejecting frags should be handled in a follow-up fix.

So I would prefer to keep this patch focused on the confirmed xdpf_clone()
bug, and handle xdp_convert_zc_to_xdp_frame() as a separate follow-up after
checking reachability and reproducing it.