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

[Stanislav Fomichev <sdf.kernel@gmail.com>]

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"?