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

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

[Cen Zhang (Microsoft)]

In __xsk_rcv() multi-buffer path, xsk_buff_alloc() is called in a
do-while loop without checking its return value for NULL. The pre-check
xsk_buff_can_alloc() only counts fill queue entries without validating
descriptor addresses, so it can pass while xsk_buff_alloc() rejects
all entries as invalid 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

Fixed by adding a NULL check after xsk_buff_alloc() and use
xskq_prod_cancel_n() to roll back any partially submitted RX ring
descriptors, ensuring no incomplete multi-buffer packet is delivered
to userspace.

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>
---
 net/xdp/xsk.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index b970f30ea9b9..76e0cdd43722 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -301,6 +301,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 	struct xdp_buff_xsk *xskb;
 	struct xdp_buff *xsk_xdp;
+	u32 nb_submitted = 0;
 	skb_frag_t *frag;
 
 	from_len = xdp->data_end - copy_from;
 	meta_len = xdp->data - copy_from;
@@ -348,6 +349,11 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 		u32 copied;
 
 		xsk_xdp = xsk_buff_alloc(xs->pool);
+		if (!xsk_xdp) {
+			xskq_prod_cancel_n(xs->rx, nb_submitted);
+			xs->rx_dropped++;
+			return -ENOMEM;
+		}
 		copy_to = xsk_xdp->data - meta_len;
 
 		copied = xsk_copy_xdp(copy_to, &copy_from, to_len, &from_len, &frag, rem);
@@ -356,6 +362,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
 		xskb = container_of(xsk_xdp, struct xdp_buff_xsk, xdp);
 		__xsk_rcv_zc_safe(xs, xskb, copied - meta_len,
 				  rem ? XDP_PKT_CONTD : 0);
+		nb_submitted++;
 		meta_len = 0;
 	} while (rem);
 
-- 
2.53.0

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

[Stanislav Fomichev]

On 07/24, Cen Zhang (Microsoft) wrote:
> In __xsk_rcv() multi-buffer path, xsk_buff_alloc() is called in a
> do-while loop without checking its return value for NULL. The pre-check
> xsk_buff_can_alloc() only counts fill queue entries without validating
> descriptor addresses, so it can pass while xsk_buff_alloc() rejects
> all entries as invalid and returns NULL.

Agreed, looks like a valid issue :-( (!ok branch in _xp_alloc) ..
 
>   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
> 
> Fixed by adding a NULL check after xsk_buff_alloc() and use
> xskq_prod_cancel_n() to roll back any partially submitted RX ring
> descriptors, ensuring no incomplete multi-buffer packet is delivered
> to userspace.
> 
> 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>
> ---
>  net/xdp/xsk.c | 7 +++++++
>  1 file changed, 7 insertions(+)
> 
> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index b970f30ea9b9..76e0cdd43722 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c
> @@ -301,6 +301,7 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  	struct xdp_buff_xsk *xskb;
>  	struct xdp_buff *xsk_xdp;
> +	u32 nb_submitted = 0;
>  	skb_frag_t *frag;
>  
>  	from_len = xdp->data_end - copy_from;
>  	meta_len = xdp->data - copy_from;
> @@ -348,6 +349,11 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  		u32 copied;
>  
>  		xsk_xdp = xsk_buff_alloc(xs->pool);
> +		if (!xsk_xdp) {

[..]

> +			xskq_prod_cancel_n(xs->rx, nb_submitted);

.. but I don't think doing xskq_prod_cancel_n is enough? The descriptors
from the fill ring have been consumed, and the prog ring stuff is
cancelled, which means they are "lost"?

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

[Cen Zhang (Microsoft)]

> ... which means they are "lost"?

You are right. The UMEM frames already consumed from the Fill Ring must
be recycled into free_list when the packet cannot be completed. 

Since the current loop calls __xsk_rcv_zc_safe(), and therefore
xp_release(), after every allocation, they cannot simply be recycled
after a later failure. I will rework v2 as a two-stage transaction:
first allocate and stage all required buffers, recycling all staged
buffers if any allocation fails; then run the error-free
copy/submit/release loop only after the allocation stage succeeds.

Thanks.

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Rolling back the RX ring via xskq_prod_cancel_n() leaks previously allocated UMEM buffers on multi-buffer packet allocation failure.
--

--- Patch [1]: [PATCH net] xsk: fix NULL pointer dereference in __xsk_rcv() ---
commit 53feb1519cd9a4933abf6d79d7b04cb89df34c4b
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>

xsk: fix NULL pointer dereference in __xsk_rcv()

This commit fixes a NULL pointer dereference in the multi-buffer receive path
of __xsk_rcv(). It introduces a check for when xsk_buff_alloc() fails and
uses xskq_prod_cancel_n() to roll back partially submitted RX ring descriptors
to ensure no incomplete multi-buffer packet is delivered to userspace.

> diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
> index b970f30ea9b9d..3aad3996bd803 100644
> --- a/net/xdp/xsk.c
> +++ b/net/xdp/xsk.c

[ ... ]

> @@ -348,6 +349,11 @@ static int __xsk_rcv(struct xdp_sock *xs, struct xdp_buff *xdp, u32 len)
>  		u32 copied;
>  
>  		xsk_xdp = xsk_buff_alloc(xs->pool);
> +		if (!xsk_xdp) {
> +			xskq_prod_cancel_n(xs->rx, nb_submitted);
> +			xs->rx_dropped++;
> +			return -ENOMEM;
> +		}

[Severity: High]
Does this error path leak the UMEM chunks from previous loop iterations?

If an earlier fragment was allocated and submitted to the RX ring via
__xsk_rcv_zc_safe() (which internally calls xp_release()), and a subsequent
allocation fails here, xskq_prod_cancel_n() rewinds the RX ring's local
producer pointer.

Because the previously reserved descriptors are cancelled without manually
retrieving and returning their UMEM chunks to the kernel's free list, will
those chunks be permanently lost, allowing a user to exhaust the shared
UMEM pools by intentionally feeding invalid descriptors?

>  		copy_to = xsk_xdp->data - meta_len;
>  
>  		copied = xsk_copy_xdp(copy_to, &copy_from, to_len, &from_len, &frag, rem);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724164719.99563-1-blbllhy@gmail.com?part=1