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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch incorrectly rounds up the SKB logical length (`len`), appending uninitialized garbage bytes to the packet and corrupting its size.
--

commit 2b99f24545da4e2ce6d6317b4cc495604fb468a5
Author: Clément Lecigne <clecigne@google.com>

xsk: fix memory corruptions in net/core/xdp.c

This commit fixes a buffer overflow in xdp_build_skb_from_zc() when using
2KB UMEM chunks by accurately calculating the required truesize, including
SKB headroom and skb_shared_info overhead. It also fixes an out-of-bounds
write in xdp_copy_frags_from_zc() by aligning the allocation request size
with the memcpy operation.

> diff --git a/net/core/xdp.c b/net/core/xdp.c
> index 9890a30584ba7..7e39f17ad407d 100644
> --- a/net/core/xdp.c
> +++ b/net/core/xdp.c
[ ... ]
> @@ -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);

[Severity: High]
Does redefining len with LARGEST_ALIGN() discard the exact packet payload
length needed for correct SKB construction?

> +	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));

[Severity: High]
Will passing the improperly aligned len to __skb_put() incorrectly inflate
the logical length of the constructed SKB? 

If len is rounded up to the nearest LARGEST_ALIGN boundary, __skb_put()
will increase skb->len and skb->tail beyond the actual valid payload length.

Could this cause the trailing bytes to contain uninitialized data from UMEM,
leading to a potential info leak or data corruption when the SKB is
forwarded?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629072300.1664622-1-clecigne@google.com?part=1