Re: [PATCH net v2] xsk: fix NULL pointer dereference in __xsk_rcv()

[Maciej Fijalkowski <maciej.fijalkowski@intel.com>]

On Wed, Jul 29, 2026 at 08:37:54AM +0800, Jason Xing wrote:
> On Tue, Jul 28, 2026 at 7:33 PM Maciej Fijalkowski
> <maciej.fijalkowski@intel.com> wrote:
> >
> > On Tue, Jul 28, 2026 at 08:07:48AM +0800, Jason Xing wrote:
> > > Hi Maciej,
> > >
> > > On Mon, Jul 27, 2026 at 8:09 PM Maciej Fijalkowski
> > > <maciej.fijalkowski@intel.com> wrote:
> > > >
> > > > On Fri, Jul 24, 2026 at 11:42:46PM -0400, Cen Zhang (Microsoft) wrote:
> > > > > In the __xsk_rcv() multi-buffer path, xsk_buff_alloc() is called in a
> > > > > loop without checking its return value. xsk_buff_can_alloc() only
> > > > > counts fill queue entries without validating their addresses, so it
> > > > > can succeed while xsk_buff_alloc() rejects all remaining entries and
> > > > > returns NULL.
> > > > >
> > > > >   Oops: general protection fault, probably for non-canonical address
> > > > >    0xdffffc0000000000
> > > > >   KASAN: null-ptr-deref in range
> > > > >    [0x0000000000000000-0x0000000000000007]
> > > > >   RIP: 0010:__xsk_rcv+0x426/0xc20 (net/xdp/xsk.c:350)
> > > > >   Call Trace:
> > > > >    xsk_generic_rcv+0x26d/0x5f0
> > > > >    xdp_do_generic_redirect+0x3c5/0xcf0
> > > > >    do_xdp_generic+0x92f/0xe70
> > > > >    __netif_receive_skb_core.constprop.0+0xf7e/0x2b30
> > > > >
> > > > > Fix this with a two-stage transaction. First allocate and stage all
> > > > > buffers required for the packet, recycling all staged buffers with
> > > > > xsk_buff_free() if any allocation fails. Only after this stage
> > > > > succeeds, copy the data, reserve the RX descriptors, and release the
> > > > > buffers in an error-free loop.
> > > > >
> > > > > Fixes: 804627751b42 ("xsk: add support for AF_XDP multi-buffer on Rx path")
> > > > > Reported-by: AutonomousCodeSecurity@microsoft.com
> > > > > Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> > >
> > > Reviewed-by: Jason Xing <kerneljasonxing@gmail.com>
> > >
> > > > > ---
> > > > > v2:
> > > > >  - Allocate all packet buffers before reserving RX descriptors.
> > > > >  - Recycle partially allocated buffers instead of only cancelling the
> > > > >   RX producer reservations.
> > > > > Link: https://lore.kernel.org/netdev/20260724164719.99563-1-blbllhy@gmail.com
> > > > >
> > > > >  net/xdp/xsk.c | 29 ++++++++++++++++++++++++++---
> > > > >  1 file changed, 26 insertions(+), 3 deletions(-)
> > > > >
> > > > > diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> > > > > index f906d51b6699..383fc2b1de48 100644
> > > > > --- a/net/xdp/xsk.c
> > > > > +++ b/net/xdp/xsk.c
> > > > > @@ -298,9 +298,11 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
> > > > >       u32 frame_size = __xsk_pool_get_rx_frame_size(xs->pool);
> > > > >       void *copy_from = xsk_copy_xdp_start(xdp), *copy_to;
> > > > >       u32 from_len, meta_len, rem, num_desc;
> > > > > -     struct xdp_buff_xsk *xskb;
> > > > > +     struct xdp_buff_xsk *xskb, *tmp;
> > > > >       struct xdp_buff *xsk_xdp;
> > > > > +     LIST_HEAD(xsk_buffs);
> > > > >       skb_frag_t *frag;
> > > > > +     u32 i;
> > > > >
> > > > >       from_len = xdp->data_end - copy_from;
> > > > >       meta_len = xdp->data - copy_from;
> > > > > @@ -343,23 +345,44 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
> > > > >               frag =  &sinfo->frags[0];
> > > > >       }
> > > > >
> > > > > +     for (i = 0; i < num_desc; i++) {
> > > > > +             xsk_xdp = xsk_buff_alloc(xs->pool);
> > > > > +             if (!xsk_xdp)
> > > > > +                     goto err_alloc;
> > > > > +
> > > > > +             xskb = container_of(xsk_xdp, struct xdp_buff_xsk, xdp);
> > > > > +             if (unlikely(!list_empty(&xskb->list_node)))
> > > > > +                     goto err_alloc;
> > > > > +             list_add_tail(&xskb->list_node, &xsk_buffs);
> > > >
> > > > could we use existing xsk_buff_add_frag() ?
> > > > then I presume xsk_buff_free() would understand list and walk through
> > > > xdp_buff's and free it ?
> > >
> > > Good suggestion, but xsk_buff_add_frag() will add more irrelevant
> > > stuff like nr_frags, xdp_frags_size, xdp_frags_truesize... They are
> > > all happening in the softirq context. Refactoring the helper would
> > > bring more work here.
> >
> > Fair enough, how about we meet in the halfway?
> >
> > Don't use xxx_add_frag but reuse pool's list. Reason I'm pushing for it is
> > xsk_buff_free() has been thought to consume multi-buffer xskb's, so all
> > the list-walking would be hidden and error path would only consist of a
> > single free() call.
> 
> It does make sense (reusing/developing common helpers is always a good
> way to go). Side note: maybe Cen needs to handle
> xdp_buff_set_frags_flag() to allow xsk_buff_free() freeing a list of
> nodes.

Yeah exactly this needs to be done on a 'head' xskb.

> 
> >
> > then the do/while loop while iterating could be using xsk_buff_add_frag()
> 
> But I don't get it. Sorry. Are you saying we still need to call
> xsk_buff_add_frag()?

Sorry i meant xsk_buff_get_frag() which would be peeeling off xskbs from
pool's xskb_list.

> 
> Thanks,
> Jason
> 
> >
> > >
> > > Honestly, I like the local array which seems simpler/cleaner to understand :)
> > >
> > > Thanks,
> > > Jason
> > >
> > > >
> > > > I believe we could reuse pool's xskb_list instead of fabricating the
> > > > on-stack variant here.
> > > >
> > > > > +     }
> > > > > +
> > > > >       do {
> > > > >               u32 to_len = frame_size + meta_len;
> > > > >               u32 copied;
> > > > >
> > > > > -             xsk_xdp = xsk_buff_alloc(xs->pool);
> > > > > +             xskb = list_first_entry(&xsk_buffs, struct xdp_buff_xsk,
> > > > > +                                     list_node);
> > > > > +             list_del_init(&xskb->list_node);
> > > > > +             xsk_xdp = &xskb->xdp;
> > > > >               copy_to = xsk_xdp->data - meta_len;
> > > > >
> > > > >               copied = xsk_copy_xdp(copy_to, &copy_from, to_len, &from_len, &frag, rem);
> > > > >               rem -= copied;
> > > > >
> > > > > -             xskb = container_of(xsk_xdp, struct xdp_buff_xsk, xdp);
> > > > >               __xsk_rcv_zc_safe(xs, xskb, copied - meta_len,
> > > > >                                 rem ? XDP_PKT_CONTD : 0);
> > > > >               meta_len = 0;
> > > > >       } while (rem);
> > > > >
> > > > >       return 0;
> > > > > +
> > > > > +err_alloc:
> > > > > +     list_for_each_entry_safe(xskb, tmp, &xsk_buffs, list_node) {
> > > > > +             list_del_init(&xskb->list_node);
> > > > > +             xsk_buff_free(&xskb->xdp);
> > > > > +     }
> > > > > +     xs->rx_dropped++;
> > > > > +     return -ENOMEM;
> > > > >  }
> > > > >
> > > > >  static bool xsk_tx_writeable(struct xdp_sock *xs)
> > > > > --
> > > > > 2.53.0
> > > > >
> > > >