Re: [PATCH net v2 5/8] xsk: avoid skb leak in XDP_TX_METADATA case

On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
>
> Sashiko AI review found 2 potential issue(s):
> - [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in `xsk_skb_metadata` allows unprivileged users to bypass bounds checks and cause out-of-bounds memory accesses.
> - [High] Software checksumming calculation fails completely because `skb_checksum_help()` is invoked on an incomplete and non-linear SKB.
> --
>
> commit aef1b3344bd952f39b1181e046ab7d9ce77fbc8a
> Author: Jason Xing <[email protected]>
>
> xsk: avoid skb leak in XDP_TX_METADATA case
>
> This commit explicitly adds a call to kfree_skb() (via
> xsk_drop_untrans_skb) when xsk_skb_metadata() returns an error, preventing
> an skb leak if metadata processing fails.
>
> Link: https://lore.kernel.org/all/[email protected]/
>
> > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > index 55b448abb8178..fd5432fb807e1 100644
> > --- a/net/xdp/xsk.c
> > +++ b/net/xdp/xsk.c
> > @@ -829,8 +829,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,
> >               xsk_skb_init_misc(skb, xs, desc->addr);
> >               if (desc->options & XDP_TX_METADATA) {
> >                       err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
>
> While looking at this metadata handling, I noticed a couple of pre-existing
> regressions.
>
> Will skb_checksum_help() always fail when called here?
>
> xsk_skb_metadata() is called right after allocating the SKB but before any
> payload fragments are appended via skb_fill_page_desc() further down in
> xsk_build_skb_zerocopy(). This means skb->len and skb_headlen(skb) are 0.
>
> If software checksumming is enabled, xsk_skb_metadata() calls
> skb_checksum_help(skb). Since skb_headlen(skb) is 0, won't the check
> offset >= skb_headlen(skb) in skb_checksum_help() always evaluate to true,
> triggering a warning and causing the packet to be dropped?

Patch 1 has already dealt with this issue.

>
> Also, can a user-space program modify the checksum metadata concurrently?
>
> In xsk_skb_metadata(), the metadata is read directly from the UMEM buffer,
> which is shared memory mapped into user-space:
>
> net/xdp/xsk.c:xsk_skb_metadata() {
>     ...
>         if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
>                 if (unlikely(meta->request.csum_start +
>                              meta->request.csum_offset +
>                              sizeof(__sum16) > desc->len))
>                         return -EINVAL;
>
>                 skb->csum_start = hr + meta->request.csum_start;
>                 skb->csum_offset = meta->request.csum_offset;
>     ...
> }
>
> The bounds check does not use READ_ONCE(). Could a user-space application
> modify meta->request.csum_start and meta->request.csum_offset between the
> check and the assignment? If so, it might bypass the bounds check and specify
> arbitrarily large offsets, causing out-of-bounds writes when the hardware
> inserts the checksum.

Yes, you're right about this pre-existing behavior but currently I
don't have any interest in expanding the series. Maybe I will
construct it as a follow-up patch.

>
> > -                     if (unlikely(err))
> > +                     if (unlikely(err)) {
> > +                             xsk_drop_untrans_skb(skb);
> >                               return ERR_PTR(err);
> > +                     }
> >               }
> >       } else {
> >               struct xsk_addrs *xsk_addr;
>
> --
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5