xsk: reject tx_metadata_len smaller than struct xsk_tx_metadata
Subsystems (4)
PATCH 1/1 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.
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.
--- net/xdp/xdp_umem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
net/xdp/xdp_umem.c +2 -1
@@ -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; }
Signature · 1 line
2.53.0
Quoted 43 lines · Cen Zhang (Microsoft)
xdp_umem_reg()validatestx_metadata_lenfor upper bound (<256) and alignment (%8) but not a lower bound.xsk_skb_metadata()computes meta = buffer -pool->tx_metadata_lenthen unconditionally accesses the full 24-byte structxsk_tx_metadata, so any value less thansizeof(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 rejecttx_metadata_lenvalues that cannot cover the full metadata struct.Fixes 341ac980eab9 ("xsk: Support tx_metadata_len")Reported-by [email protected]Signed-off-by Cen Zhang (Microsoft) <[email protected]>--- net/xdp/xdp_umem.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)net/xdp/xdp_umem.c +2
-1@@ -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; }Signature · 1 line
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)
include/net/xdp_sock_drv.h +17 -4
@@ -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; }
net/xdp/xsk.c +1 -1
@@ -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 ) {
Thanks for the suggestion. I've tested the proposed approach and will send v2 shortly.
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
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) <[email protected]>
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.
net/xdp/xdp_umem.c static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr) · line 212
Quoted 11 lines
net/xdp/xdp_umem.c +2
-1@@ -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?
net/xdp/xdp_umem.c static int xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr) · line 215
Quoted 3 lines
net/xdp/xdp_umem.c +0
-0return -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;
}Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1