[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.