* Re: [PATCH net v2] xsk: reject undersized tx_metadata at runtime validation
2026-07-22 4:13 [PATCH net v2] xsk: reject undersized tx_metadata at runtime validation Cen Zhang (Microsoft)
@ 2026-07-22 17:55 ` Stanislav Fomichev
0 siblings, 0 replies; 2+ messages in thread
From: Stanislav Fomichev @ 2026-07-22 17:55 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/22, Cen Zhang (Microsoft) wrote:
> There is no metadata length check before accessing struct xsk_tx_metadata
> fields in both xdp_umem_reg() (registration) and
> xsk_buff_valid_tx_metadata() (runtime). Thus, an invalid small
> tx_metadata_len (e.g. 8 bytes) passes registration and later causes an
> out-of-bounds read when xsk_skb_metadata() accesses fields beyond the
> declared metadata region.
>
> 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)
>
> Fixing at registration time would break backward compatibility with older
> userspace applications, so apply a runtime length check in
> xsk_buff_valid_tx_metadata() that validates tx_metadata_len against the
> fields required by the enabled flags.
>
> Fixes: 341ac980eab9 ("xsk: Support tx_metadata_len")
> Reported-by: AutonomousCodeSecurity@microsoft.com
> Link: https://lore.kernel.org/all/20260720155210.34229-1-blbllhy@gmail.com/
> Suggested-by: Stanislav Fomichev <sdf@fomichev.me>
> Signed-off-by: Cen Zhang (Microsoft) <blbllhy@gmail.com>
> ---
> V2:
> - Change to runtime per-flag length check as suggested by Stanislav.
Hmm, the AI review points out another set of TOCTOU issues :-/ I wonder
whether want to go all in and rewrite the users to have a copy
of metadata? Something like the following below, build tested only,
any issues with that?
(xsk_tx_metadata_to_compl (!meta) check is still buggy? not sure)
diff --git a/drivers/net/ethernet/intel/igc/igc.h b/drivers/net/ethernet/intel/igc/igc.h
index 17f213cc93e4..94f2a32917d2 100644
--- a/drivers/net/ethernet/intel/igc/igc.h
+++ b/drivers/net/ethernet/intel/igc/igc.h
@@ -604,7 +604,7 @@ struct igc_xdp_buff {
struct igc_metadata_request {
struct igc_tx_buffer *tx_buffer;
- struct xsk_tx_metadata *meta;
+ struct xsk_tx_metadata meta;
struct igc_ring *tx_ring;
u32 cmd_type;
u16 used_desc;
diff --git a/drivers/net/ethernet/intel/igc/igc_main.c b/drivers/net/ethernet/intel/igc/igc_main.c
index e6e9441fc3d4..ec51a5b1051d 100644
--- a/drivers/net/ethernet/intel/igc/igc_main.c
+++ b/drivers/net/ethernet/intel/igc/igc_main.c
@@ -2961,7 +2961,7 @@ static void igc_xsk_request_timestamp(void *_priv)
* metadata area. It is the location to store the value of
* tx hardware timestamp.
*/
- xsk_tx_metadata_to_compl(meta_req->meta, &tstamp->xsk_meta);
+ xsk_tx_metadata_to_compl(&meta_req->meta, &tstamp->xsk_meta);
/* Set timestamp bit based on the _TSTAMP(_X) bit. */
tx_flags |= tstamp->flags;
@@ -3059,7 +3059,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
*/
while (budget >= 4 && xsk_tx_peek_desc(pool, &xdp_desc)) {
struct igc_metadata_request meta_req;
- struct xsk_tx_metadata *meta = NULL;
+ struct xsk_tx_metadata meta = {};
struct igc_tx_buffer *bi;
u32 olinfo_status;
dma_addr_t dma;
@@ -3071,7 +3071,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
olinfo_status = xdp_desc.len << IGC_ADVTXD_PAYLEN_SHIFT;
dma = xsk_buff_raw_get_dma(pool, xdp_desc.addr);
- meta = xsk_buff_get_metadata(pool, xdp_desc.addr);
+ xsk_buff_read_metadata(pool, xdp_desc.addr, &meta);
xsk_buff_raw_dma_sync_for_device(pool, dma, xdp_desc.len);
bi = &ring->tx_buffer_info[ntu];
@@ -3079,7 +3079,7 @@ static void igc_xdp_xmit_zc(struct igc_ring *ring)
meta_req.tx_buffer = bi;
meta_req.meta = meta;
meta_req.used_desc = 0;
- xsk_tx_metadata_request(meta, &igc_xsk_tx_metadata_ops,
+ xsk_tx_metadata_request(&meta, &igc_xsk_tx_metadata_ops,
&meta_req);
/* xsk_tx_metadata_request() may have updated next_to_use */
diff --git a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
index 8aeab4b21035..1721a0a9220a 100644
--- a/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
+++ b/drivers/net/ethernet/mellanox/mlx5/core/en/xsk/tx.c
@@ -66,7 +66,7 @@ static void mlx5e_xsk_tx_post_err(struct mlx5e_xdpsq *sq,
bool mlx5e_xsk_tx(struct mlx5e_xdpsq *sq, unsigned int budget)
{
struct xsk_buff_pool *pool = sq->xsk_pool;
- struct xsk_tx_metadata *meta = NULL;
+ struct xsk_tx_metadata meta = {};
union mlx5e_xdp_info xdpi;
bool work_done = true;
bool flush = false;
@@ -99,13 +99,13 @@ bool mlx5e_xsk_tx(struct mlx5e_xdpsq *sq, unsigned int budget)
xdptxd.dma_addr = xsk_buff_raw_get_dma(pool, desc.addr);
xdptxd.data = xsk_buff_raw_get_data(pool, desc.addr);
xdptxd.len = desc.len;
- meta = xsk_buff_get_metadata(pool, desc.addr);
+ xsk_buff_read_metadata(pool, desc.addr, &meta);
xsk_buff_raw_dma_sync_for_device(pool, xdptxd.dma_addr, xdptxd.len);
ret = INDIRECT_CALL_2(sq->xmit_xdp_frame, mlx5e_xmit_xdp_frame_mpwqe,
mlx5e_xmit_xdp_frame, sq, &xdptxd,
- check_result, meta);
+ check_result, &meta);
if (unlikely(!ret)) {
if (sq->mpwqe.wqe)
mlx5e_xdp_mpwqe_complete(sq);
@@ -116,7 +116,7 @@ bool mlx5e_xsk_tx(struct mlx5e_xdpsq *sq, unsigned int budget)
if (xp_tx_metadata_enabled(sq->xsk_pool)) {
struct xsk_tx_metadata_compl compl;
- xsk_tx_metadata_to_compl(meta, &compl);
+ xsk_tx_metadata_to_compl(&meta, &compl);
XSK_TX_COMPL_FITS(void *);
mlx5e_xdpi_fifo_push(&sq->db.xdpi_fifo,
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 562d20830b94..217134559d41 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -2707,7 +2707,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
for (; budget > 0; budget--) {
struct stmmac_metadata_request meta_req;
- struct xsk_tx_metadata *meta = NULL;
+ struct xsk_tx_metadata meta = {};
dma_addr_t dma_addr;
bool set_ic;
@@ -2732,7 +2732,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
tx_desc = stmmac_get_tx_desc(priv, tx_q, entry);
dma_addr = xsk_buff_raw_get_dma(pool, xdp_desc.addr);
- meta = xsk_buff_get_metadata(pool, xdp_desc.addr);
+ xsk_buff_read_metadata(pool, xdp_desc.addr, &meta);
xsk_buff_raw_dma_sync_for_device(pool, dma_addr, xdp_desc.len);
/* To return XDP buffer to XSK pool, we simple call
@@ -2761,7 +2761,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
meta_req.set_ic = &set_ic;
meta_req.tbs = tx_q->tbs;
meta_req.edesc = &tx_q->dma_entx[entry];
- xsk_tx_metadata_request(meta, &stmmac_xsk_tx_metadata_ops,
+ xsk_tx_metadata_request(&meta, &stmmac_xsk_tx_metadata_ops,
&meta_req);
if (set_ic) {
tx_q->tx_count_frames = 0;
@@ -2775,7 +2775,7 @@ static bool stmmac_xdp_xmit_zc(struct stmmac_priv *priv, u32 queue, u32 budget)
stmmac_enable_dma_transmission(priv, priv->ioaddr, queue);
- xsk_tx_metadata_to_compl(meta,
+ xsk_tx_metadata_to_compl(&meta,
&tx_q->tx_skbuff_dma[entry].xsk_meta);
tx_q->cur_tx = STMMAC_NEXT_ENTRY(tx_q->cur_tx, priv->dma_conf.dma_tx_size);
diff --git a/include/net/libeth/xsk.h b/include/net/libeth/xsk.h
index 82b5d21aae87..e2fa6bf6b1b3 100644
--- a/include/net/libeth/xsk.h
+++ b/include/net/libeth/xsk.h
@@ -205,7 +205,7 @@ __libeth_xsk_xmit_fill_buf_md(const struct xdp_desc *xdesc,
BUILD_BUG_ON(!__builtin_constant_p(tmo == libeth_xsktmo));
tmo = tmo == libeth_xsktmo ? &__libeth_xsktmo : tmo;
- xsk_tx_metadata_request(ctx.meta, tmo, &desc);
+ xsk_tx_metadata_request(&ctx.meta, tmo, &desc);
return desc;
}
diff --git a/include/net/xdp_sock.h b/include/net/xdp_sock.h
index ebac60a3d8a1..eb2645a88934 100644
--- a/include/net/xdp_sock.h
+++ b/include/net/xdp_sock.h
@@ -163,7 +163,7 @@ static inline void xsk_tx_metadata_request(const struct xsk_tx_metadata *meta,
const struct xsk_tx_metadata_ops *ops,
void *priv)
{
- if (!meta)
+ if (!meta || !meta->flags)
return;
if (ops->tmo_request_launch_time)
diff --git a/include/net/xdp_sock_drv.h b/include/net/xdp_sock_drv.h
index 46797645a0c2..925c25bad57c 100644
--- a/include/net/xdp_sock_drv.h
+++ b/include/net/xdp_sock_drv.h
@@ -259,31 +259,42 @@ xsk_buff_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr)
XDP_TXMD_FLAGS_LAUNCH_TIME | \
0)
-static inline bool
-xsk_buff_valid_tx_metadata(const struct xsk_tx_metadata *meta)
+static inline int
+xsk_buff_read_tx_metadata(const struct xsk_buff_pool *pool,
+ void *data,
+ struct xsk_tx_metadata *meta)
{
- return !(meta->flags & ~XDP_TXMD_FLAGS_VALID);
-}
-
-static inline struct xsk_tx_metadata *
-__xsk_buff_get_metadata(const struct xsk_buff_pool *pool, void *data)
-{
- struct xsk_tx_metadata *meta;
+ struct xsk_tx_metadata *p;
+ u64 flags = 0;
if (!pool->tx_metadata_len)
- return NULL;
+ return 0;
+
+ /* covers flags, XDP_TXMD_FLAGS_CHECKSUM & XDP_TXMD_FLAGS_TIMESTAMP */
+ if (unlikely(pool->tx_metadata_len < 16))
+ return -EINVAL;
+
+ p = data - pool->tx_metadata_len;
+ flags = READ_ONCE(p->flags);
+
+ if (flags & ~XDP_TXMD_FLAGS_VALID)
+ return -EINVAL;
- meta = data - pool->tx_metadata_len;
- if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
- return NULL; /* no way to signal the error to the user */
+ if (flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
+ if (pool->tx_metadata_len < offsetofend(struct xsk_tx_metadata, request.launch_time))
+ return -EINVAL;
- return meta;
+ memcpy(meta, p, min(pool->tx_metadata_len, sizeof(struct xsk_tx_metadata)));
+ meta->flags = flags;
+ return 0;
}
-static inline struct xsk_tx_metadata *
-xsk_buff_get_metadata(struct xsk_buff_pool *pool, u64 addr)
+static inline void xsk_buff_read_metadata(const struct xsk_buff_pool *pool,
+ u64 addr,
+ struct xsk_tx_metadata *meta)
{
- return __xsk_buff_get_metadata(pool, xp_raw_get_data(pool, addr));
+ if (xsk_buff_read_tx_metadata(pool, xp_raw_get_data(pool, addr), meta) < 0)
+ meta->flags = 0;
}
static inline void xsk_buff_dma_sync_for_cpu(struct xdp_buff *xdp)
@@ -469,21 +480,18 @@ 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 int
+xsk_buff_read_tx_metadata(const struct xsk_buff_pool *pool,
+ void *data,
+ struct xsk_tx_metadata *meta)
{
return false;
}
-static inline struct xsk_tx_metadata *
-__xsk_buff_get_metadata(const struct xsk_buff_pool *pool, void *data)
-{
- return NULL;
-}
-
-static inline struct xsk_tx_metadata *
-xsk_buff_get_metadata(struct xsk_buff_pool *pool, u64 addr)
+static inline void xsk_buff_read_metadata(const struct xsk_buff_pool *pool,
+ u64 addr,
+ struct xsk_tx_metadata *meta)
{
- return NULL;
}
static inline void xsk_buff_dma_sync_for_cpu(struct xdp_buff *xdp)
diff --git a/include/net/xsk_buff_pool.h b/include/net/xsk_buff_pool.h
index ccb3b350001f..71992b9e680d 100644
--- a/include/net/xsk_buff_pool.h
+++ b/include/net/xsk_buff_pool.h
@@ -141,12 +141,12 @@ void xp_dma_unmap(struct xsk_buff_pool *pool, unsigned long attrs);
struct xdp_buff *xp_alloc(struct xsk_buff_pool *pool);
u32 xp_alloc_batch(struct xsk_buff_pool *pool, struct xdp_buff **xdp, u32 max);
bool xp_can_alloc(struct xsk_buff_pool *pool, u32 count);
-void *xp_raw_get_data(struct xsk_buff_pool *pool, u64 addr);
+void *xp_raw_get_data(const struct xsk_buff_pool *pool, u64 addr);
dma_addr_t xp_raw_get_dma(struct xsk_buff_pool *pool, u64 addr);
struct xdp_desc_ctx {
dma_addr_t dma;
- struct xsk_tx_metadata *meta;
+ struct xsk_tx_metadata meta;
};
struct xdp_desc_ctx xp_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr);
diff --git a/net/xdp/xsk.c b/net/xdp/xsk.c
index b970f30ea9b9..dbe4ddae30e8 100644
--- a/net/xdp/xsk.c
+++ b/net/xdp/xsk.c
@@ -802,19 +802,17 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
struct xdp_desc *desc, struct xsk_buff_pool *pool,
u32 hr)
{
- struct xsk_tx_metadata *meta = NULL;
+ struct xsk_tx_metadata meta = {};
u16 csum_start, csum_offset;
+ int ret;
- if (unlikely(pool->tx_metadata_len == 0))
- return -EINVAL;
-
- meta = buffer - pool->tx_metadata_len;
- if (unlikely(!xsk_buff_valid_tx_metadata(meta)))
- return -EINVAL;
+ ret = xsk_buff_read_tx_metadata(pool, buffer, &meta);
+ if (ret < 0)
+ return ret;
- if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
- csum_start = READ_ONCE(meta->request.csum_start);
- csum_offset = READ_ONCE(meta->request.csum_offset);
+ if (meta.flags & XDP_TXMD_FLAGS_CHECKSUM) {
+ csum_start = READ_ONCE(meta.request.csum_start);
+ csum_offset = READ_ONCE(meta.request.csum_offset);
if (unlikely(csum_start + csum_offset +
sizeof(__sum16) > desc->len))
@@ -833,9 +831,9 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,
}
}
- if (meta->flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
- skb->skb_mstamp_ns = meta->request.launch_time;
- xsk_tx_metadata_to_compl(meta, &skb_shinfo(skb)->xsk_meta);
+ if (meta.flags & XDP_TXMD_FLAGS_LAUNCH_TIME)
+ skb->skb_mstamp_ns = meta.request.launch_time;
+ xsk_tx_metadata_to_compl(&meta, &skb_shinfo(skb)->xsk_meta);
return 0;
}
diff --git a/net/xdp/xsk_buff_pool.c b/net/xdp/xsk_buff_pool.c
index 1f28a9641571..c9f05835e433 100644
--- a/net/xdp/xsk_buff_pool.c
+++ b/net/xdp/xsk_buff_pool.c
@@ -735,7 +735,7 @@ static void *__xp_raw_get_data(const struct xsk_buff_pool *pool, u64 addr)
return pool->addrs + addr;
}
-void *xp_raw_get_data(struct xsk_buff_pool *pool, u64 addr)
+void *xp_raw_get_data(const struct xsk_buff_pool *pool, u64 addr)
{
return __xp_raw_get_data(pool, __xp_raw_get_addr(pool, addr));
}
@@ -773,7 +773,7 @@ struct xdp_desc_ctx xp_raw_get_ctx(const struct xsk_buff_pool *pool, u64 addr)
addr = __xp_raw_get_addr(pool, addr);
ret.dma = __xp_raw_get_dma(pool, addr);
- ret.meta = __xsk_buff_get_metadata(pool, __xp_raw_get_data(pool, addr));
+ xsk_buff_read_metadata(pool, addr, &ret.meta);
return ret;
}
^ permalink raw reply related [flat|nested] 2+ messages in thread