RE: [PATCH v3] xsk: fix memory corruptions in net/core/xdp.c

[Fijalkowski, Maciej]

> 
> From: Clément Lecigne <clecigne@xxxxxxxxxx>
> 
> Commit 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> introduced a vulnerability in the handling of XDP_PASS for AF_XDP zero-copy
> frames.
> 
> Note: Currently, this specific AF_XDP zero-copy conversion path is only
> reachable from the drivers/net/ethernet/intel/ice and
> drivers/net/ethernet/intel/idpf drivers.
> 
> When building an skb, xdp_build_skb_from_zc() uses the chunk size
> (xdp->frame_sz) for the allocation. However, napi_build_skb() automatically
> reserves space at the end of the allocation for the skb_shared_info
> structure.
> 
> Most high performance UMEM applications use 4K chunks, where the
> corruption cannot happen. However, if the UMEM is configured with 2KB
> chunks (a very common configuration to maximize packet density in memory),
> a standard 1500 MTU packet will trigger the corruption because the required
> space exceeds the 2048 byte chunk size:
> 
> Headroom (256) + Packet (1514) + skb_shared_info (320) = 2090 bytes
> 
> Because 2090 bytes > 2048 bytes and __skb_put() does not perform bounds
> checking, the memcpy() writes past the available linear data area and
> corrupts the skb_shared_info structure. This can lead to arbitrary code
> execution if pointers like destructor_arg are overwritten.
> 
> Additionally, in xdp_copy_frags_from_zc(), the allocation size is set
> strictly to the fragment size (len), but the subsequent memcpy() uses
> LARGEST_ALIGN(len). This mismatch results in an out-of-bounds write of
> up to 7 bytes, which triggers KASAN warnings and is unsafe despite typical
> page pool allocator padding.
> 
> Fix the skb allocation in xdp_build_skb_from_zc() by dynamically
> calculating the exact truesize required using SKB_HEAD_ALIGN() to
> properly account for the headroom, the LARGEST_ALIGN(len), and the
> skb_shared_info overhead.
> 
> Fix the out-of-bounds write in xdp_copy_frags_from_zc() by rounding up
> the allocation request using LARGEST_ALIGN(len) to match the copy
> operation.
> 
> Fixes: 560d958c6c68 ("xsk: add generic XSk &xdp_buff -> skb conversion")
> CC: Alexander Lobakin <aleksander.lobakin@xxxxxxxxx>
> CC: Eric Dumazet <edumazet@xxxxxxxxxx>
> Signed-off-by: Clément Lecigne <clecigne@xxxxxxxxxx>

Hi Clement,

Do you have a reproducer for mentioned issue or is it only a fix from
theoretical POV?

To be clear, we were addressing headroom issues in this series:
https://lore.kernel.org/bpf/20260402154958.562179-1-maciej.fijalkowski@xxxxxxxxx/

so I wanted to ask if you are able to have this malformed setup for
2k chunk size. That series should not allow for that.

I think this is the second time someone is trying to fix this area of code,
so it is not a nack or something, let us fix this, but I wanted to have
us on the same page.

Thanks,
Maciej

> ---
> Changes since v2:
>  - Used LARGEST_ALIGN to calculate the len to account for the aligned
> memcpy.
>  - Fixed the commit message to include the idpf driver.
> 
> Changes since v1:
>  - Used SKB_HEAD_ALIGN to properly calculate the required allocation size
>    including the skb_shared_info overhead.
>  - Re-ordered variable declarations.
> 
> ---
> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9890a30584ba..7e39f17ad407 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
> @@ -698,8 +698,8 @@ static noinline bool xdp_copy_frags_from_zc(struct
> sk_buff *skb,
> 
>  	for (u32 i = 0; i < nr_frags; i++) {
>  		const skb_frag_t *frag = &xinfo->frags[i];
> -		u32 len = skb_frag_size(frag);
> -		u32 offset, truesize = len;
> +		u32 offset, len = skb_frag_size(frag);
> +		u32 truesize = LARGEST_ALIGN(len);
>  		struct page *page;
> 
>  		page = page_pool_dev_alloc(pp, &offset, &truesize);
> @@ -738,9 +738,10 @@ static noinline bool xdp_copy_frags_from_zc(struct
> sk_buff *skb,
>   */
>  struct sk_buff *xdp_build_skb_from_zc(struct xdp_buff *xdp)
>  {
> +	u32 headroom = xdp->data_meta - xdp->data_hard_start;
>  	const struct xdp_rxq_info *rxq = xdp->rxq;
> -	u32 len = xdp->data_end - xdp->data_meta;
> -	u32 truesize = xdp->frame_sz;
> +	u32 len = LARGEST_ALIGN(xdp->data_end - xdp->data_meta);
> +	u32 truesize = SKB_HEAD_ALIGN(headroom + len);
>  	struct sk_buff *skb = NULL;
>  	struct page_pool *pp;
>  	int metalen;
> @@ -762,7 +763,7 @@ struct sk_buff *xdp_build_skb_from_zc(struct
> xdp_buff *xdp)
>  	}
> 
>  	skb_mark_for_recycle(skb);
> -	skb_reserve(skb, xdp->data_meta - xdp->data_hard_start);
> +	skb_reserve(skb, headroom);
> 
>  	memcpy(__skb_put(skb, len), xdp->data_meta,
> LARGEST_ALIGN(len));
>