Re: [PATCH net v2] xsk: reject undersized tx_metadata at runtime validation

[Stanislav Fomichev <sdf.kernel@gmail.com>]

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;
 }