Re: [PATCH AUTOSEL 6.18] xsk: validate MTU against usable frame size on bind

[sashiko-bot@kernel.org]

Sashiko AI review found 4 potential issue(s):
- [Critical] The patch references undefined functions `__xsk_pool_get_rx_frame_size()` and `xsk_pool_get_tailroom()`, which will cause a build failure.
- [High] Secondary sockets binding to a shared UMEM drop the `XDP_USE_SG` flag, causing spurious MTU validation failures.
- [High] The `XDP_UMEM_SG_FLAG` is set on the persistent UMEM object before the new MTU validation and is not reverted on failure, leading to socket state corruption.
- [High] An integer underflow in the calculation of `frame_size` bypasses the MTU validation, allowing out-of-bounds DMA writes.
--

commit f3a660b520a18a5f378e6506df30590629252c09
Author: Maciej Fijalkowski <maciej.fijalkowski@intel.com>

    xsk: validate MTU against usable frame size on bind

AF_XDP bind currently accepts zero-copy pool configurations without
verifying that the device MTU fits into the usable frame space provided
by the UMEM chunk. 2k chunk size might not provide enough space for
standard 1500 MTU, so catch such settings at bind time.

Link: https://patch.msgid.link/20260402154958.562179-5-maciej.fijalkowski@intel.com

> diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
> index aa9788f20d0db..5641de2ed60d1 100644
> --- a/net/xdp/xsk_buff_pool.c
> +++ b/net/xdp/xsk_buff_pool.c
> @@ -165,8 +167,12 @@ static void xp_disable_drv_zc(struct xsk_buff_pool *pool)
>  int xp_assign_dev(struct xsk_buff_pool *pool,
>  		  struct net_device *netdev, u16 queue_id, u16 flags)
>  {
> +	u32 needed = netdev->mtu + ETH_PAD_LEN;
> +	u32 segs = netdev->xdp_zc_max_segs;
> +	bool mbuf = flags & XDP_USE_SG;

Secondary sockets binding to a shared UMEM omit the XDP_USE_SG flag in
xp_assign_dev_shared(). Does this cause mbuf to evaluate to false and
force segs = 1, leading to spurious MTU validation failures for secondary
sockets on devices with jumbo frames?

>  	bool force_zc, force_copy;
>  	struct netdev_bpf bpf;
> +	u32 frame_size;
>  	int err = 0;
>  
>  	ASSERT_RTNL();

[ ... ]

> @@ -186,7 +192,7 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  	if (err)
>  		return err;
>  
> -	if (flags & XDP_USE_SG)
> +	if (mbuf)
>  		pool->umem->flags |= XDP_UMEM_SG_FLAG;

If the MTU validation fails later in this function, the XDP_UMEM_SG_FLAG
will remain set on the persistent UMEM object. Can this cause a subsequent
bind on the same UMEM without XDP_USE_SG to bypass SG device setup but
still be placed in multi-buffer mode, leading to driver inconsistency?

[ ... ]

> @@ -208,8 +214,24 @@ int xp_assign_dev(struct xsk_buff_pool *pool,
>  		goto err_unreg_pool;
>  	}
>  
> -	if (netdev->xdp_zc_max_segs == 1 && (flags & XDP_USE_SG)) {
> -		err = -EOPNOTSUPP;
> +	if (mbuf) {
> +		if (segs == 1) {
> +			err = -EOPNOTSUPP;
> +			goto err_unreg_pool;
> +		}
> +	} else {
> +		segs = 1;
> +	}
> +
> +	/* open-code xsk_pool_get_rx_frame_size() as pool->dev is not
> +	 * set yet at this point; we are before getting down to driver
> +	 */
> +	frame_size = __xsk_pool_get_rx_frame_size(pool) -
> +		     xsk_pool_get_tailroom(mbuf);

Are __xsk_pool_get_rx_frame_size() and xsk_pool_get_tailroom() defined
in the kernel tree? It looks like this might cause an implicit function
declaration error and break the build. The comment mentions open-coding
these functions, but they appear to be called directly.

Also, if the XDP UMEM is configured with a large headroom, the available
rx frame size could be smaller than the tailroom. Would this subtraction
underflow the u32 frame_size, leading to a huge positive value that bypasses
the MTU validation and allows out-of-bounds DMA writes?

> +	frame_size = ALIGN_DOWN(frame_size, 128);
> +
> +	if (needed > frame_size * segs) {
> +		err = -EINVAL;
>  		goto err_unreg_pool;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260420132314.1023554-312-sashal@kernel.org?part=1