* Re: [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata
2026-07-20 15:52 [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata Cen Zhang (Microsoft)
@ 2026-07-20 20:24 ` Stanislav Fomichev
2026-07-22 4:11 ` Cen Zhang (Microsoft)
2026-07-21 15:52 ` sashiko-bot
1 sibling, 1 reply; 4+ messages in thread
From: Stanislav Fomichev @ 2026-07-20 20:24 UTC (permalink / raw)
To: Cen Zhang (Microsoft)
Cc: magnus.karlsson, maciej.fijalkowski, davem, edumazet, kuba,
pabeni, sdf, horms, netdev, bpf, linux-kernel,
AutonomousCodeSecurity, tgopinath, kys
On 07/20, Cen Zhang (Microsoft) wrote:
> xdp_umem_reg() validates tx_metadata_len for upper bound (<256) and
> alignment (%8) but not a lower bound. xsk_skb_metadata() computes
> meta = buffer - pool->tx_metadata_len then unconditionally accesses
> the full 24-byte struct xsk_tx_metadata, so any value less than
> sizeof(struct xsk_tx_metadata) allows an out-of-bounds read.
>
> KASAN reports this as:
>
> BUG: KASAN: vmalloc-out-of-bounds in xsk_skb_metadata+0x4b2/0x500
> Read of size 8 at addr ffffc90000f11000 by task exploit/148
>
> xsk_skb_metadata (net/xdp/xsk.c:837)
> xsk_build_skb (net/xdp/xsk.c)
> __xsk_generic_xmit (net/xdp/xsk.c)
> xsk_sendmsg (net/xdp/xsk.c)
>
> Add a lower-bound check in xdp_umem_reg() to reject tx_metadata_len
> values that cannot cover the full metadata struct.
>
> Fixes: 341ac980eab9 ("xsk: Support tx_metadata_len")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> net/xdp/xdp_umem.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 58da2f4f4397..d16ad9d8f919 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -208,7 +208,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> return -EINVAL;
>
> if (mr->flags & XDP_UMEM_TX_METADATA_LEN) {
> - if (mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
> + if (mr->tx_metadata_len < sizeof(struct xsk_tx_metadata) ||
> + mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
> return -EINVAL;
> umem->tx_metadata_len = mr->tx_metadata_len;
> }
> --
> 2.53.0
>
This will make adding new tx metadata types harder (and will require all
userspace to be updated whenever we do so), will the following be
a bit nicer? (but, obviously, paying more per-packet at runtime)
(untested)
diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index 46797645a0c2..b55b878949d5 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -260,9 +260,21 @@ xsk_buff_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr)
0)
static inline bool
-xsk_buff_valid_tx_metadata(const struct xsk_tx_metadata *meta)
+xsk_buff_valid_tx_metadata(const struct xsk_buff_pool *pool,
+ const struct xsk_tx_metadata *meta)
{
- return !(meta->flags & ~XDP_TXMD_FLAGS_VALID);
+ /* covers flags, XDP_TXMD_FLAGS_CHECKSUM & XDP_TXMD_FLAGS_TIMESTAMP */
+ if (unlikely(pool->tx_metadata_len < 16))
+ return false;
+
+ if (unlikely(meta->flags & ~XDP_TXMD_FLAGS_VALID))
+ return false;
+
+ if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
+ if (unlikely(pool->tx_metadata_len < offsetofend(struct xsk_tx_metadata, launch_time)))
+ return false;
+
+ return true;
}
static inline struct xsk_tx_metadata *
@@ -274,7 +286,7 @@ __xsk_buff_get_metadata(const struct xsk_buff_pool *pool, void *data)
return NULL;
meta = data - pool->tx_metadata_len;
- if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
+ if (unlikely(!xsk_buff_valid_tx_metadata(pool, meta)))
return NULL; /* no way to signal the error to the user */
return meta;
@@ -469,7 +481,8 @@ xsk_buff_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr)
return (struct xdp_desc_ctx){ };
}
-static inline bool xsk_buff_valid_tx_metadata(struct xsk_tx_metadata *meta)
+static inline bool xsk_buff_valid_tx_metadata(const struct xsk_buff_pool *pool,
+ struct xsk_tx_metadata *meta)
{
return false;
}
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index b970f30ea9b9..75b2c97e41e4 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -809,7 +809,7 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
return -EINVAL;
meta = buffer - pool->tx_metadata_len;
- if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
+ if (unlikely(!xsk_buff_valid_tx_metadata(pool, meta)))
return -EINVAL;
if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
^ permalink raw reply related [flat|nested] 4+ messages in thread* Re: [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata
2026-07-20 15:52 [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata Cen Zhang (Microsoft)
2026-07-20 20:24 ` Stanislav Fomichev
@ 2026-07-21 15:52 ` sashiko-bot
1 sibling, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-07-21 15:52 UTC (permalink / raw)
To: Cen Zhang (Microsoft); +Cc: bpf
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [Critical] UAPI Backward Compatibility Breakage
Pre-existing issues:
- [High] UAPI ABI Compatibility Mismatch in struct xsk_tx_metadata
--
--- Patch [1]: [PATCH net] xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata ---
commit a89522120661c494f685c71309affa610fc7ca08
Author: Cen Zhang (Microsoft) <blbllhy@gmail.com>
xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata
xdp_umem_reg() validates tx_metadata_len for upper bound (<256) and
alignment (%8) but not a lower bound. xsk_skb_metadata() computes
meta = buffer - pool->tx_metadata_len then unconditionally accesses
the full 24-byte struct xsk_tx_metadata, so any value less than
sizeof(struct xsk_tx_metadata) allows an out-of-bounds read. This adds
a lower-bound check in xdp_umem_reg() to reject tx_metadata_len
values that cannot cover the full metadata struct.
> diff --git a/net/xdp/xdp_umem.c b/net/xdp/xdp_umem.c
> index 58da2f4f4397a..d16ad9d8f9197 100644
> --- a/net/xdp/xdp_umem.c
> +++ b/net/xdp/xdp_umem.c
> @@ -208,7 +208,8 @@ static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> return -EINVAL;
>
> if (mr->flags & XDP_UMEM_TX_METADATA_LEN) {
> - if (mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
> + if (mr->tx_metadata_len < sizeof(struct xsk_tx_metadata) ||
> + mr->tx_metadata_len >= 256 || mr->tx_metadata_len % 8)
[Severity: Critical]
Does this strict size check in xdp_umem_reg() break backward compatibility
with older userspace applications?
Older applications compiled against earlier kernel headers (where
struct xsk_tx_metadata was 16 bytes, prior to the addition of the
launch_time field) will legitimately request a tx_metadata_len of 16.
This check evaluates to 16 < 24 and unconditionally rejects these
valid requests with -EINVAL, causing registration to fail.
To safely fix the underlying KASAN out-of-bounds read, should the kernel
instead dynamically validate that pool->tx_metadata_len is large enough to
cover the fields actually requested by meta->flags at runtime in
xsk_skb_metadata(), rather than enforcing a global minimum size here?
> return -EINVAL;
> umem->tx_metadata_len = mr->tx_metadata_len;
> }
[Severity: High]
This is a pre-existing issue, but does struct xsk_tx_metadata have an ABI
compatibility mismatch between 32-bit and 64-bit architectures?
Because __u64 has a 4-byte alignment requirement on 32-bit x86 but an 8-byte
alignment requirement on 64-bit x86, the compiler introduces 4 bytes of
implicit padding on 64-bit architectures before the launch_time field.
If a 32-bit userspace program runs on a 64-bit kernel in compat mode, it
writes launch_time to offset 12, but the 64-bit kernel reads it from offset
16 in xsk_skb_metadata(), resulting in garbage values being used for packet
transmission scheduling.
include/uapi/linux/if_xdp.h:struct xsk_tx_metadata {
...
struct {
...
__u16 csum_offset;
/* XDP_TXMD_FLAGS_LAUNCH_TIME */
/* Launch time in nanosecond against the PTP HW Clock */
__u64 launch_time;
} request;
}
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260720155210.34229-1-blbllhy@gmail.com?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread