Re: [PATCH net 1/6] xsk: require at least 16 bytes of TX metadata

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Incomplete Fix / Out-of-Bounds Memory Read in TX Metadata
- [High] UAPI ABI Incompatibility between 32-bit and 64-bit architectures
--

--- Patch [1]: [PATCH net 1/6] xsk: require at least 16 bytes of TX metadata ---
commit 8f587b21f2b30dd32c9e63e0817476297538dfb0
Author: Stanislav Fomichev <sdf.kernel@gmail.com>
xsk: require at least 16 bytes of TX metadata

AF_XDP accepts a TX metadata length as small as eight bytes. This commit
requires a minimum of 16 bytes of TX metadata to preserve compatibility with
applications that do not use launch-time metadata while addressing
out-of-bounds reads.

Link: https://lore.kernel.org/netdev/20260720155210.34229-1-blbllhy@gmail.com/

> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 58da2f4f4397a..cd8643360eb3a 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -210,6 +210,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
>  	if (mr->flags & XDP_UMEM_TX_METADATA_LEN) {
>  		if (mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
>  			return -EINVAL;
> +		if (mr->tx_metadata_len < 16)
> +			return -EINVAL;

[Severity: Critical]
This is a pre-existing issue, but is a 16-byte minimum sufficient when the
launch_time flag is requested?

If a userspace application sets XDP_TXMD_FLAGS_LAUNCH_TIME in the metadata
flags and provides a tx_metadata_len of 16, it appears we can still read out
of bounds. In include/net/xdp_sock.h:xsk_tx_metadata_request(), the code
reads the 8-byte launch_time field at offset 16 when the flag is set:

include/net/xdp_sock.h:xsk_tx_metadata_request() {
    ...
    ops->tmo_request_launch_time(meta->request.launch_time, priv);
    ...
}

If the descriptor addr is placed at the very end of the registered umem (for
example, desc->addr = umem_size - 1 with len = 1), reading 8 bytes at offset
16 extends beyond the umem bounds. Since xp_aligned_validate_desc() validates
based on the chunk, does this allow reading into potentially unmapped memory
if the launch_time flag is set?

[Severity: High]
This isn't a bug introduced by this patch, but does this tx_metadata_len
validation correctly handle 32-bit applications running on a 64-bit kernel?

Looking at struct xsk_tx_metadata in include/uapi/linux/if_xdp.h, there is no
explicit padding between csum_offset and launch_time:

include/uapi/linux/if_xdp.h:struct xsk_tx_metadata {
    __u64 flags;
    union {
        struct {
            __u16 csum_start;
            __u16 csum_offset;
            __u64 launch_time;
        } request;
        ...
    };
};

On 32-bit x86, __u64 has a 4-byte alignment, which places launch_time at
offset 12 and makes the total structure size 20 bytes. On a 64-bit kernel,
__u64 is 8-byte aligned, placing launch_time at offset 16 with a 24-byte
structure size.

If a 32-bit application registers a 20-byte length, it would be rejected by
the tx_metadata_len % 8 check above. If it registers 24 bytes but populates
launch_time at its native offset 12, would the 64-bit kernel read unexpected
data from offset 16?

>  		umem->tx_metadata_len = mr->tx_metadata_len;
>  	}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727161959.885642-1-sdf@fomichev.me?part=1