xsk: fix bugs around xsk skb allocation
Subsystems (4)
v2 cover xsk: fix bugs around xsk skb allocation
From: Jason Xing <[email protected]>
There are rare issues around xsk_build_skb(). Some of them were founded by Sashiko[1][2].
[1]: https://lore.kernel.org/all/[email protected]/
[2]: https://lore.kernel.org/all/[email protected]/
---
v2
1. add four patches spotted by sashiko to fix buggy pre-existing behavior
2. adjust the order of 8 patches.
Jason Xing (8):
xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
xsk: handle NULL dereference of the skb without frags issue
xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
xsk: avoid skb leak in XDP_TX_METADATA case
xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS
xsk: fix xsk_addrs slab leak on multi-buffer error path
xsk: fix u64 descriptor address truncation on 32-bit architectures
net/xdp/xsk.c | 88 +++++++++++++++++++++++++++++++++-------- net/xdp/xsk_buff_pool.c | 3 ++ 2 files changed, 75 insertions(+), 16 deletions(-)
Signature · 1 line
2.41.3
PATCH v2 1/8 xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
From: Jason Xing <[email protected]>
skb_checksum_help() is a common helper that writes the folded 16-bit checksum back via skb->data + csum_start + csum_offset, i.e. it relies on the skb's linear head and fails (with WARN_ONCE and -EINVAL) when skb_headlen() is 0.
AF_XDP generic xmit takes two very different paths depending on the netdev. Drivers that advertise IFF_TX_SKB_NO_LINEAR (e.g. virtio_net) skip the "copy payload into a linear head" step on purpose as a performance optimisation: xsk_build_skb_zerocopy() only attaches UMEM pages as frags and never calls skb_put(), so skb_headlen() stays 0 for the whole skb. For these skbs there is simply no linear area for skb_checksum_help() to write the csum into - the sw-csum fallback is structurally inapplicable.
The patch tries to catch this and reject the combination with error at setup time. Rejecting at bind() converts this silent per-packet failure into a synchronous, actionable -EOPNOTSUPP at setup time. HW csum and launch_time metadata on IFF_TX_SKB_NO_LINEAR drivers are unaffected because they do not call skb_checksum_help().
Without the patch, every descriptor carrying 'XDP_TX_METADATA | XDP_TXMD_FLAGS_CHECKSUM' produces:
1) a WARN_ONCE "offset (N) >= skb_headlen() (0)" from skb_checksum_help(),
2) sendmsg() returning -EINVAL without consuming the descriptor (invalid_descs is not incremented),
3) a wedged TX ring: __xsk_generic_xmit() does not advance the consumer on non-EOVERFLOW errors, so the next sendmsg() re-reads the same descriptor and re-hits the same WARN until the socket is closed.
--- net/xdp/xsk_buff_pool.c | 3 +++ 1 file changed, 3 insertions(+)
net/xdp/xsk_buff_pool.c +3 -0
@@ -169,6 +169,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;
Signature · 1 line
2.41.3
Re: [PATCH net v2 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 56 lines
From: Jason Xing <[email protected]>
skb_checksum_help()is a common helper that writes the folded 16-bit checksum back viaskb->data+csum_start+csum_offset, i.e. it relies on the skb's linear head and fails (withWARN_ONCEand-EINVAL) whenskb_headlen()is 0.
AF_XDPgeneric xmit takes two very different paths depending on the netdev. Drivers that advertiseIFF_TX_SKB_NO_LINEAR(e.g.virtio_net) skip the "copy payload into a linear head" step on purpose as a performance optimisation:xsk_build_skb_zerocopy()only attaches UMEM pages as frags and never callsskb_put(), soskb_headlen()stays 0 for the whole skb. For these skbs there is simply no linear area forskb_checksum_help()to write the csum into - the sw-csum fallback is structurally inapplicable.The patch tries to catch this and reject the combination with error at setup time. Rejecting at
bind()converts this silent per-packet failure into a synchronous, actionable-EOPNOTSUPPat setup time. HW csum andlaunch_timemetadata onIFF_TX_SKB_NO_LINEARdrivers are unaffected because they do not callskb_checksum_help().Without the patch, every descriptor carrying '
XDP_TX_METADATA|XDP_TXMD_FLAGS_CHECKSUM' produces:
1) aWARN_ONCE"offset (N) >=skb_headlen()(0)" fromskb_checksum_help(),
2)sendmsg()returning-EINVALwithout consuming the descriptor (invalid_descsis not incremented),
3) a wedged TX ring:__xsk_generic_xmit()does not advance the consumer on non-EOVERFLOW errors, so the nextsendmsg()re-reads the same descriptor and re-hits the same WARN until the socket is closed.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk_buff_pool.c | 3 +++ 1 file changed, 3 insertions(+)net/xdp/xsk_buff_pool.c +3
-0@@ -169,6 +169,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;Signature · 1 line
2.41.3
Wondering whether a better fixes tag is commit 11614723af26 ("xsk: Add option to calculate TX checksum in SW")?
Re: [PATCH net v2 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 54 lines · Stanislav Fomichev
Quoted 54 lines
From: Jason Xing <[email protected]>
skb_checksum_help()is a common helper that writes the folded 16-bit checksum back viaskb->data+csum_start+csum_offset, i.e. it relies on the skb's linear head and fails (withWARN_ONCEand-EINVAL) whenskb_headlen()is 0.
AF_XDPgeneric xmit takes two very different paths depending on the netdev. Drivers that advertiseIFF_TX_SKB_NO_LINEAR(e.g.virtio_net) skip the "copy payload into a linear head" step on purpose as a performance optimisation:xsk_build_skb_zerocopy()only attaches UMEM pages as frags and never callsskb_put(), soskb_headlen()stays 0 for the whole skb. For these skbs there is simply no linear area forskb_checksum_help()to write the csum into - the sw-csum fallback is structurally inapplicable.The patch tries to catch this and reject the combination with error at setup time. Rejecting at
bind()converts this silent per-packet failure into a synchronous, actionable-EOPNOTSUPPat setup time. HW csum andlaunch_timemetadata onIFF_TX_SKB_NO_LINEARdrivers are unaffected because they do not callskb_checksum_help().Without the patch, every descriptor carrying '
XDP_TX_METADATA|XDP_TXMD_FLAGS_CHECKSUM' produces:
1) aWARN_ONCE"offset (N) >=skb_headlen()(0)" fromskb_checksum_help(),
2)sendmsg()returning-EINVALwithout consuming the descriptor (invalid_descsis not incremented),
3) a wedged TX ring:__xsk_generic_xmit()does not advance the consumer on non-EOVERFLOW errors, so the nextsendmsg()re-reads the same descriptor and re-hits the same WARN until the socket is closed.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk_buff_pool.c | 3 +++ 1 file changed, 3 insertions(+)net/xdp/xsk_buff_pool.c +3
-0@@ -169,6 +169,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;
Quoted 8 lines
Quoted 3 lines
--
2.41.3Wondering whether a better fixes tag is commit
11614723af26("xsk: Add option to calculate TX checksum in SW")?
Thanks for the check. But not really. It is the commit 30c3055f9c0d that brings the csum support of IFF_TX_SKB_NO_LINEAR case where this issue can be triggered (because this mode no longer puts data into skb linear area).
Thanks,
Jason
Re: [PATCH net v2 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 62 lines · Jason Xing
Quoted 61 lines · Stanislav Fomichev
Quoted 55 lines
From: Jason Xing <[email protected]>
skb_checksum_help()is a common helper that writes the folded 16-bit checksum back viaskb->data+csum_start+csum_offset, i.e. it relies on the skb's linear head and fails (withWARN_ONCEand-EINVAL) whenskb_headlen()is 0.
AF_XDPgeneric xmit takes two very different paths depending on the netdev. Drivers that advertiseIFF_TX_SKB_NO_LINEAR(e.g.virtio_net) skip the "copy payload into a linear head" step on purpose as a performance optimisation:xsk_build_skb_zerocopy()only attaches UMEM pages as frags and never callsskb_put(), soskb_headlen()stays 0 for the whole skb. For these skbs there is simply no linear area forskb_checksum_help()to write the csum into - the sw-csum fallback is structurally inapplicable.The patch tries to catch this and reject the combination with error at setup time. Rejecting at
bind()converts this silent per-packet failure into a synchronous, actionable-EOPNOTSUPPat setup time. HW csum andlaunch_timemetadata onIFF_TX_SKB_NO_LINEARdrivers are unaffected because they do not callskb_checksum_help().Without the patch, every descriptor carrying '
XDP_TX_METADATA|XDP_TXMD_FLAGS_CHECKSUM' produces:
1) aWARN_ONCE"offset (N) >=skb_headlen()(0)" fromskb_checksum_help(),
2)sendmsg()returning-EINVALwithout consuming the descriptor (invalid_descsis not incremented),
3) a wedged TX ring:__xsk_generic_xmit()does not advance the consumer on non-EOVERFLOW errors, so the nextsendmsg()re-reads the same descriptor and re-hits the same WARN until the socket is closed.Fixes 30c3055f9c0d ("xsk: wrap generic metadata handling onto separate function")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk_buff_pool.c | 3 +++ 1 file changed, 3 insertions(+)net/xdp/xsk_buff_pool.c +3
-1@@ -169,6 +169,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ; --Quoted 2 lines
2.41.3
Wondering whether a better fixes tag is commit
11614723af26("xsk: Add option to calculate TX checksum in SW")?
Quoted 6 lines
Quoted 1 line
Thanks for the check. But not really. It is the commit
30c3055f9c0dthat brings the csum support ofIFF_TX_SKB_NO_LINEARcase where this issue can be triggered (because this mode no longer puts data into skb linear area).
Ah, right, good point, it's the IFF_TX_SKB_NO_LINEAR path.
[email protected]
AI reviewRe: [PATCH net v2 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
In reply to Jason XingSashiko AI review found 1 potential issue(s): - [High] The patch unconditionally rejects pool assignment for devices with IFF_TX_SKB_NO_LINEAR if tx_sw_csum is enabled, which incorrectly breaks…
Sashiko AI review found 1 potential issue(s):
- [High] The patch unconditionally rejects pool assignment for devices with IFF_TX_SKB_NO_LINEAR if tx_sw_csum is enabled, which incorrectly breaks native Zero-Copy (ZC) setups on those devices. --
commit 5beba1aaed8c200424f98af3c07472ab4da8d451
Author: Jason Xing <[email protected]>
xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
This commit rejects UMEM bindings for devices that advertise IFF_TX_SKB_NO_LINEAR when software checksumming (tx_sw_csum) is enabled. This prevents a warning and panic in skb_checksum_help() which relies on a linear skb head.
net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 180
Quoted 11 lines
net/xdp/xsk_buff_pool.c +3
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ; +if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +
Does this unconditionally reject the pool assignment even for native Zero-Copy (ZC) setups?
In native XDP_ZEROCOPY mode, AF_XDP descriptors are sent directly to the hardware. No sk_buff is allocated, so skb_checksum_help() is never called, making the tx_sw_csum flag irrelevant and safe to ignore.
By placing this check before determining if native ZC is available, does this break ZC mode on drivers like virtio_net for applications that pass the XDP_UMEM_TX_SW_CSUM flag as a fallback?
Should this check be deferred until we are certain we are using generic copy mode, such as when force_copy is true or after native ZC setup fails?
Quoted 3 lines
net/xdp/xsk_buff_pool.c +0
-0if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Re: [PATCH net v2 1/8] xsk: reject sw-csum UMEM binding to IFF_TX_SKB_NO_LINEAR devices
Quoted 25 lines · On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
Sashiko AI review found 1 potential issue(s):
- [High] The patch unconditionally rejects pool assignment for devices withIFF_TX_SKB_NO_LINEARiftx_sw_csumis enabled, which incorrectly breaks native Zero-Copy (ZC) setups on those devices. --commit
5beba1aaed8c200424f98af3c07472ab4da8d451
Author: Jason Xing <[email protected]>xsk: reject sw-csum UMEM binding to
IFF_TX_SKB_NO_LINEARdevicesThis commit rejects UMEM bindings for devices that advertise
IFF_TX_SKB_NO_LINEARwhen software checksumming (tx_sw_csum) is enabled. This prevents a warning and panic inskb_checksum_help()which relies on a linear skb head.Quoted 8 lines
net/xdp/xsk_buff_pool.c +0
-0@@ -175,6 +175,9 @@ int xp_assign_dev(struct xsk_buff_pool *pool,if (force_zc && force_copy)return -EINVAL ;
Quoted 6 lines
net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 174
Quoted 3 lines
net/xdp/xsk_buff_pool.c +3
-0+if (pool->tx_sw_csum && (netdev->priv_flags &IFF_TX_SKB_NO_LINEAR )) +return -EOPNOTSUPP ; +Does this unconditionally reject the pool assignment even for native Zero-Copy (ZC) setups?
It's weird. Nobody uses sw csum in zc mode.... If someone uses this and gets warned, he will surely root cause the reason and finally adjust the code in application.
Quoted 11 lines
In native
XDP_ZEROCOPYmode,AF_XDPdescriptors are sent directly to the hardware. Nosk_buffis allocated, soskb_checksum_help()is never called, making thetx_sw_csumflag irrelevant and safe to ignore.By placing this check before determining if native ZC is available, does this break ZC mode on drivers like
virtio_netfor applications that pass theXDP_UMEM_TX_SW_CSUMflag as a fallback?Should this check be deferred until we are certain we are using generic copy mode, such as when
force_copyis true or after native ZC setup fails?
It's more complex. I'll keep my code as-is unless someone screams at me to handle this trivial case.
Thanks,
Jason
Quoted 7 lines
net/xdp/xsk_buff_pool.c int xp_assign_dev(struct xsk_buff_pool *pool, · line 177
Quoted 4 lines
net/xdp/xsk_buff_pool.c +0
-0if (xsk_get_pool_from_qid (netdev, queue_id))return -EBUSY ;--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
PATCH v2 2/8 xsk: handle NULL dereference of the skb without frags issue
In reply to Jason XingFrom: Jason Xing <[email protected]>
When a first descriptor (xs->skb == NULL) triggers -EOVERFLOW in xsk_build_skb_zerocopy (e.g., MAX_SKB_FRAGS exceeded), the free_err EOVERFLOW handler unconditionally dereferences xs->skb via xsk_inc_num_desc(xs->skb) and xsk_drop_skb(xs->skb), causing a NULL pointer dereference.
In this series, the skb is already freed by kfree_skb() inside xsk_build_skb_zerocopy for the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.
Guard the existing xsk_inc_num_desc/xsk_drop_skb calls with an xs->skb check (for the continuation case), and add an else branch for the first-descriptor case that manually cancels the CQ slot and increments invalid_descs by one.
--- net/xdp/xsk.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)
net/xdp/xsk.c +8 -3
@@ -893,9 +893,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */ -xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }xskq_cons_release (xs->tx); }else {/* Let application retry */
Signature · 1 line
2.41.3
Re: [PATCH net v2 2/8] xsk: handle NULL dereference of the skb without frags issue
Quoted 49 lines
From: Jason Xing <[email protected]>
When a first descriptor (
xs->skb== NULL) triggers-EOVERFLOWinxsk_build_skb_zerocopy(e.g.,MAX_SKB_FRAGSexceeded), thefree_errEOVERFLOW handler unconditionally dereferencesxs->skbviaxsk_inc_num_desc(xs->skb)andxsk_drop_skb(xs->skb), causing a NULL pointer dereference.In this series, the skb is already freed by
kfree_skb()insidexsk_build_skb_zerocopyfor the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.Guard the existing xsk_inc_num_desc/xsk_drop_skb calls with an
xs->skbcheck (for the continuation case), and add an else branch for the first-descriptor case that manually cancels the CQ slot and incrementsinvalid_descsby one.Fixes cf24f5a5feea ("xsk: add support for AF_XDP multi-buffer on Tx path")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)net/xdp/xsk.c +8
-3@@ -893,9 +893,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */ -xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }xskq_cons_release (xs->tx); }else {/* Let application retry */ Signature · 1 line
2.41.3
[email protected]
AI reviewRe: [PATCH net v2 2/8] xsk: handle NULL dereference of the skb without frags issue
In reply to Jason XingSashiko AI review found 1 potential issue(s): - [High] Memory leak of newly allocated skb in `xsk_build_skb_zerocopy()` on error paths for the first descriptor. --
Sashiko AI review found 1 potential issue(s):
- [High] Memory leak of newly allocated skb in xsk_build_skb_zerocopy() on error paths for the first descriptor. --
commit c0083470d60454acdbd801635be8f00a9b79a9eb
Author: Jason Xing <[email protected]>
xsk: handle NULL dereference of the skb without frags issue
This commit addresses a NULL pointer dereference in the error handling path of xsk_build_skb by explicitly checking if xs->skb is NULL. For the first-descriptor case, it cancels the CQ slot and tracks the invalid descriptor instead of dropping the skb.
Quoted 4 lines
In this series, the skb is already freed by
kfree_skb()insidexsk_build_skb_zerocopyfor the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.
Is this description accurate regarding the skb being freed?
Looking at xsk_build_skb(), if xsk_build_skb_zerocopy() encounters an error and returns an error pointer, the local skb variable is set to NULL before jumping to the free_err label:
skb = xsk_build_skb_zerocopy(xs, desc);
if (IS_ERR(skb)) {
err = PTR_ERR(skb);
skb = NULL;
goto free_err;
}Since skb is NULL, the cleanup code at free_err is bypassed:
free_err:
if (skb && !skb_shinfo(skb)->nr_frags)
kfree_skb(skb);Does this code leak the newly allocated skb and its sk_wmem_alloc charge when xsk_build_skb_zerocopy() fails on the first descriptor?
I see this is addressed later in the series by commit aef1b3344bd95 ("xsk: avoid skb leak in XDP_TX_METADATA case") and commit 93315725e1616 ("xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS"), but the commit message here appears to incorrectly assume the free is already happening.
Quoted 4 lines
net/xdp/xsk.c +0
-0
[ ... ]
Quoted 5 lines
net/xdp/xsk.c +0
-1@@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);if (err == -EOVERFLOW ) { -/* Drop the packet */
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 983
Quoted 10 lines
net/xdp/xsk.c +8
-2-xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }
Because the skb wasn't freed as the commit message assumed, can a malicious application exploit this missing kfree_skb by repeatedly supplying abnormally large descriptors to exhaust socket buffer memory?
Quoted 3 lines
net/xdp/xsk.c +0
-0xskq_cons_release (xs->tx); }else {/* Let application retry */
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
Re: [PATCH net v2 2/8] xsk: handle NULL dereference of the skb without frags issue
Quoted 39 lines · On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
Sashiko AI review found 1 potential issue(s):
- [High] Memory leak of newly allocated skb inxsk_build_skb_zerocopy()on error paths for the first descriptor. --commit
c0083470d60454acdbd801635be8f00a9b79a9eb
Author: Jason Xing <[email protected]>xsk: handle NULL dereference of the skb without frags issue
This commit addresses a NULL pointer dereference in the error handling path of
xsk_build_skbby explicitly checking ifxs->skbis NULL. For the first-descriptor case, it cancels the CQ slot and tracks the invalid descriptor instead of dropping the skb.Quoted 4 lines
In this series, the skb is already freed by
kfree_skb()insidexsk_build_skb_zerocopyfor the first-descriptor case, so we only need to do the bookkeeping: cancel the one reserved CQ slot and account for the single invalid descriptor.Is this description accurate regarding the skb being freed?
Looking at
xsk_build_skb(), ifxsk_build_skb_zerocopy()encounters an error and returns an error pointer, the local skb variable is set to NULL before jumping to thefree_errlabel:skb = xsk_build_skb_zerocopy(xs, desc); if (IS_ERR(skb)) { err = PTR_ERR(skb); skb = NULL; goto free_err; }Since skb is NULL, the cleanup code at
free_erris bypassed:
free_err:if (skb && !skb_shinfo(skb)->nr_frags) kfree_skb(skb);
Quoted 8 lines
Does this code leak the newly allocated skb and its
sk_wmem_alloccharge whenxsk_build_skb_zerocopy()fails on the first descriptor?I see this is addressed later in the series by commit
aef1b3344bd95("xsk: avoid skb leak inXDP_TX_METADATAcase") and commit93315725e1616("xsk: free the skb when hitting the upper boundMAX_SKB_FRAGS"), but the commit message here appears to incorrectly assume the free is already happening.
They are already fixed in the series. And what I mentioned is "In the series,...".
Quoted 11 lines
Quoted 5 lines
net/xdp/xsk.c +0
-0[ ... ]
Quoted 3 lines
net/xdp/xsk.c +0
-0@@ -973,9 +973,14 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,kfree_skb (skb);
Quoted 16 lines
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 903
Quoted 12 lines
net/xdp/xsk.c +8
-3if (err == -EOVERFLOW ) { -/* Drop the packet */ -xsk_inc_num_desc (xs->skb); -xsk_drop_skb (xs->skb); +if (xs->skb) { +/* Drop the packet */ +xsk_inc_num_desc (xs->skb); +xsk_drop_skb (xs->skb); + }else { +xsk_cq_cancel_locked (xs->pool,1 ); + xs->tx->invalid_descs++; + }Because the skb wasn't freed as the commit message assumed, can a malicious application exploit this missing
kfree_skbby repeatedly supplying abnormally large descriptors to exhaust socket buffer memory?
It can happen but it doesn't belong to what I should here.
Quoted 7 lines
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 906
Quoted 4 lines
net/xdp/xsk.c +0
-0xskq_cons_release (xs->tx); }else {/* Let application retry */ --
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2
PATCH v2 3/8 xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
In reply to Jason XingFrom: Jason Xing <[email protected]>
When xsk_build_skb() processes multi-buffer packets in copy mode, the first descriptor stores data into the skb linear area without adding any frags, so nr_frags stays at 0. The caller then sets xs->skb = skb to accumulate subsequent descriptors.
If a continuation descriptor fails (e.g. alloc_page returns NULL with -EAGAIN), we jump to free_err where the condition:
if (skb && !skb_shinfo(skb)->nr_frags)
kfree_skb(skb);evaluates to true because nr_frags is still 0 (the first descriptor used the linear area, not frags). This frees the skb while xs->skb still points to it, creating a dangling pointer. On the next transmit attempt or socket close, xs->skb is dereferenced, causing a use-after-free or double-free.
Fix by adding a !xs->skb check to the condition, ensuring we only free skbs that were freshly allocated in this call (xs->skb is NULL) and never free an in-progress multi-buffer skb that the caller still references.
--- net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
net/xdp/xsk.c +1 -1
@@ -889,7 +889,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,return skb; free_err: -if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags)kfree_skb (skb);if (err == -EOVERFLOW ) {
Signature · 1 line
2.41.3
Re: [PATCH net v2 3/8] xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
Quoted 46 lines
From: Jason Xing <[email protected]>
When
xsk_build_skb()processes multi-buffer packets in copy mode, the first descriptor stores data into the skb linear area without adding any frags, sonr_fragsstays at 0. The caller then setsxs->skb= skb to accumulate subsequent descriptors.If a continuation descriptor fails (e.g.
alloc_pagereturns NULL with-EAGAIN), we jump tofree_errwhere the condition:if (skb && !skb_shinfo(skb)->nr_frags) kfree_skb(skb);evaluates to true because
nr_fragsis still 0 (the first descriptor used the linear area, not frags). This frees the skb whilexs->skbstill points to it, creating a dangling pointer. On the next transmit attempt or socket close,xs->skbis dereferenced, causing a use-after-free or double-free.Fix by adding a !xs->skb check to the condition, ensuring we only free skbs that were freshly allocated in this call (
xs->skbis NULL) and never free an in-progress multi-buffer skb that the caller still references.Fixes 6b9c129c2f93 ("xsk: remove @first_frag from xsk_build_skb()")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/xdp/xsk.c +1
-1@@ -889,7 +889,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,return skb; free_err: -if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags)kfree_skb (skb);if (err == -EOVERFLOW ) {Signature · 1 line
2.41.3
Now "!skb_shinfo(skb)->nr_frags" feels redundant? It's either "skb && !xs->skb" and we own the kfree. or "xs->skb != NULL" and we want xsk_drop_skb? Or am I missing something?
Re: [PATCH net v2 3/8] xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
Quoted 44 lines · Stanislav Fomichev
Quoted 44 lines
From: Jason Xing <[email protected]>
When
xsk_build_skb()processes multi-buffer packets in copy mode, the first descriptor stores data into the skb linear area without adding any frags, sonr_fragsstays at 0. The caller then setsxs->skb= skb to accumulate subsequent descriptors.If a continuation descriptor fails (e.g.
alloc_pagereturns NULL with-EAGAIN), we jump tofree_errwhere the condition:if (skb && !skb_shinfo(skb)->nr_frags) kfree_skb(skb);evaluates to true because
nr_fragsis still 0 (the first descriptor used the linear area, not frags). This frees the skb whilexs->skbstill points to it, creating a dangling pointer. On the next transmit attempt or socket close,xs->skbis dereferenced, causing a use-after-free or double-free.Fix by adding a !xs->skb check to the condition, ensuring we only free skbs that were freshly allocated in this call (
xs->skbis NULL) and never free an in-progress multi-buffer skb that the caller still references.Fixes 6b9c129c2f93 ("xsk: remove @first_frag from xsk_build_skb()")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)net/xdp/xsk.c +1
-1@@ -889,7 +889,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,return skb; free_err: -if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags)kfree_skb (skb);
Quoted 7 lines
Quoted 3 lines
net/xdp/xsk.c +0
-1if (err == -EOVERFLOW ) { --2.41.3
Now "!skb_shinfo(skb)->nr_frags" feels redundant? It's either "skb && !xs->skb" and we own the kfree. or "
xs->skb!= NULL" and we wantxsk_drop_skb? Or am I missing something?
Your feeling about being redundant is right. I'm removing it now:)
At this stage, the job of this if statement is to find out the first skb, so !xs->skb is a clear indicator as you said.
Thanks,
Jason
[email protected]
AI reviewRe: [PATCH net v2 3/8] xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
In reply to Jason XingSashiko AI review found 1 potential issue(s): - [Critical] Dangling `xs->skb` pointer on `-EOVERFLOW` error path leading to Use-After-Free. --
Sashiko AI review found 1 potential issue(s):
- [Critical] Dangling xs->skb pointer on -EOVERFLOW error path leading to Use-After-Free. --
commit 4ce0cecf5e8e18bd369c02242f70a42cc8fbdf03
Author: Jason Xing <[email protected]>
xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
This commit aims to fix a use-after-free in the xsk_build_skb() error handling path by ensuring we only free newly allocated skbs when a continuation descriptor fails. It prevents freeing an in-progress multi-buffer skb by adding a check for !xs->skb before kfree_skb(skb).
Quoted 4 lines
net/xdp/xsk.c +0
-0@@ -969,7 +969,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
[ ... ]
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 895
Quoted 6 lines
free_err:net/xdp/xsk.c +1
-1-if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags)kfree_skb (skb);if (err == -EOVERFLOW ) {
Does this patch leave a use-after-free on the -EOVERFLOW error path?
When xsk_build_skb() handles a continuation descriptor and encounters an -EOVERFLOW error, it jumps to free_err and enters this block. Looking at the subsequent code in xsk_build_skb():
if (err == -EOVERFLOW) {
if (xs->skb) {
/* Drop the packet */
xsk_inc_num_desc(xs->skb);
xsk_drop_skb(xs->skb);
} else {The xsk_drop_skb(xs->skb) call consumes and frees the socket buffer, but xs->skb is never set to NULL.
Because xsk_build_skb() returns ERR_PTR(-EOVERFLOW), the caller __xsk_generic_xmit() intercepts this error and continues its transmission loop without aborting.
On the next iteration, __xsk_generic_xmit() would pass the dangling xs->skb pointer back into xsk_build_skb(), which will dereference the freed memory when checking skb_shinfo(skb)->nr_frags.
Should xs->skb be cleared to NULL after calling xsk_drop_skb() here?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
Re: [PATCH net v2 3/8] xsk: fix use-after-free of xs->skb in xsk_build_skb() free_err path
Quoted 42 lines · On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
Sashiko AI review found 1 potential issue(s):
- [Critical] Danglingxs->skbpointer on-EOVERFLOWerror path leading to Use-After-Free. --commit
4ce0cecf5e8e18bd369c02242f70a42cc8fbdf03
Author: Jason Xing <[email protected]>xsk: fix use-after-free of
xs->skbinxsk_build_skb()free_errpathThis commit aims to fix a use-after-free in the
xsk_build_skb()error handling path by ensuring we only free newly allocated skbs when a continuation descriptor fails. It prevents freeing an in-progress multi-buffer skb by adding a check for !xs->skb beforekfree_skb(skb).Quoted 4 lines
net/xdp/xsk.c +0
-0@@ -969,7 +969,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,[ ... ]
net/xdp/xsk.c static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, · line 895
Quoted 6 lines
free_err:net/xdp/xsk.c +1
-1-if (skb && !skb_shinfo (skb)->nr_frags) +if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags)kfree_skb (skb);if (err == -EOVERFLOW ) {Does this patch leave a use-after-free on the
-EOVERFLOWerror path?When
xsk_build_skb()handles a continuation descriptor and encounters an-EOVERFLOWerror, it jumps tofree_errand enters this block. Looking at the subsequent code inxsk_build_skb():if (err == -EOVERFLOW) { if (xs->skb) { /* Drop the packet */ xsk_inc_num_desc(xs->skb); xsk_drop_skb(xs->skb); } else {
Quoted 2 lines
The
xsk_drop_skb(xs->skb)call consumes and frees the socket buffer, butxs->skbis never set to NULL.
This conclusion doesn't hold since this path (xsk_consume_skb(skb) -> xs->skb = NULL) handles this.
Quoted 13 lines
Because
xsk_build_skb()returnsERR_PTR(-EOVERFLOW), the caller__xsk_generic_xmit()intercepts this error and continues its transmission loop without aborting.On the next iteration,
__xsk_generic_xmit()would pass the danglingxs->skbpointer back intoxsk_build_skb(), which will dereference the freed memory when checking skb_shinfo(skb)->nr_frags.Should
xs->skbbe cleared to NULL after callingxsk_drop_skb()here?--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=3
PATCH v2 4/8 xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
In reply to Jason XingFrom: Jason Xing <[email protected]>
Once xsk_skb_init_misc() has been called on an skb, its destructor is set to xsk_destruct_skb(), which submits the descriptor address(es) to the completion queue and advances the CQ producer. If such an skb is subsequently freed via kfree_skb() along an error path - before the skb has ever been handed to the driver - the destructor still runs and submits a bogus, half-initialized address to the CQ.
Introduce a new common helper to fix the issue. That function will be used by the subsequent patches soon.
--- net/xdp/xsk.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)
net/xdp/xsk.c +7 -1
@@ -717,6 +717,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,return 0 ; } +static void xsk_drop_untrans_skb (struct sk_buff *skb) +{ + skb->destructor = sock_wfree; +kfree_skb (skb); +} +static struct sk_buff *xsk_build_skb_zerocopy (struct xdp_sock *xs,struct xdp_desc *desc) {
@@ -890,7 +896,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, free_err:if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags) -kfree_skb (skb); +xsk_drop_untrans_skb (skb);if (err == -EOVERFLOW ) {if (xs->skb) {
Signature · 1 line
2.41.3
Re: [PATCH net v2 4/8] xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
Quoted 48 lines
From: Jason Xing <[email protected]>
Once
xsk_skb_init_misc()has been called on an skb, its destructor is set toxsk_destruct_skb(), which submits the descriptor address(es) to the completion queue and advances the CQ producer. If such an skb is subsequently freed viakfree_skb()along an error path - before the skb has ever been handed to the driver - the destructor still runs and submits a bogus, half-initialized address to the CQ.Introduce a new common helper to fix the issue. That function will be used by the subsequent patches soon.
Fixes c30d084960cf ("xsk: avoid overwriting skb fields for multi-buffer traffic")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)net/xdp/xsk.c +7
-1@@ -717,6 +717,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,return 0 ; } +static void xsk_drop_untrans_skb (struct sk_buff *skb) +{ + skb->destructor = sock_wfree; +kfree_skb (skb); +} +static struct sk_buff *xsk_build_skb_zerocopy (struct xdp_sock *xs,struct xdp_desc *desc) {@@ -890,7 +896,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, free_err:if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags) -kfree_skb (skb); +xsk_drop_untrans_skb (skb);if (err == -EOVERFLOW ) {if (xs->skb) {Signature · 1 line
2.41.3
Have you considered the alternative where we postpone skb->destructor = xsk_destruct_skb to a later point? Will this be less messy than undoing that descriptor in a few curated places?
Re: [PATCH net v2 4/8] xsk: prevent CQ desync when freeing half-built skbs in xsk_build_skb()
Quoted 44 lines · Stanislav Fomichev
Quoted 44 lines
From: Jason Xing <[email protected]>
Once
xsk_skb_init_misc()has been called on an skb, its destructor is set toxsk_destruct_skb(), which submits the descriptor address(es) to the completion queue and advances the CQ producer. If such an skb is subsequently freed viakfree_skb()along an error path - before the skb has ever been handed to the driver - the destructor still runs and submits a bogus, half-initialized address to the CQ.Introduce a new common helper to fix the issue. That function will be used by the subsequent patches soon.
Fixes c30d084960cf ("xsk: avoid overwriting skb fields for multi-buffer traffic")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-)net/xdp/xsk.c +7
-1@@ -717,6 +717,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,return 0 ; } +static void xsk_drop_untrans_skb (struct sk_buff *skb) +{ + skb->destructor = sock_wfree; +kfree_skb (skb); +} +static struct sk_buff *xsk_build_skb_zerocopy (struct xdp_sock *xs,struct xdp_desc *desc) {@@ -890,7 +896,7 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs, free_err:if (skb && !xs->skb && !skb_shinfo (skb)->nr_frags) -kfree_skb (skb); +xsk_drop_untrans_skb (skb);
Quoted 9 lines
Quoted 5 lines
net/xdp/xsk.c +0
-1if (err == -EOVERFLOW ) {if (xs->skb) { --2.41.3
Have you considered the alternative where we postpone
skb->destructor = xsk_destruct_skbto a later point? Will this be less messy than undoing that descriptor in a few curated places?
Thanks. It does make sense. Adding the following code seems fine:
net/xdp/xsk.c +0 -0
@@ -820,7 +820,6 @@ static struct sk_buff
*xsk_build_skb_zerocopy(struct xdp_sock *xs,
skb_reserve(skb, hr);
- xsk_skb_init_misc(skb, xs, desc->addr);
if (desc->options & XDP_TX_METADATA) {
err = xsk_skb_metadata(skb, buffer, desc, pool, hr);
if (unlikely(err))
@@ -914,7 +913,6 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
if (unlikely(err))
goto free_err;- xsk_skb_init_misc(skb, xs, desc->addr);
if (desc->options & XDP_TX_METADATA) {
err = xsk_skb_metadata(skb, buffer, desc,
xs->pool, hr);
@@ -964,6 +962,8 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,
}
}+ if (!xs->skb)
+ xsk_skb_init_misc(skb, xs, desc->addr);
xsk_inc_num_desc(skb);
return skb;Thanks,
Jason
PATCH v2 5/8 xsk: avoid skb leak in XDP_TX_METADATA case
In reply to Jason XingFrom: Jason Xing <[email protected]>
Fix it by explicitly adding kfree_skb() before returning back to its caller.
How to reproduce it in virtio_net:
1. the current skb is the first one (which means no frag and xs->skb is NULL) and users enable metadata feature.
2. xsk_skb_metadata() returns a error code.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'.
4. there is no chance to free this skb anymore.
--- net/xdp/xsk.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
net/xdp/xsk.c +3 -1
@@ -749,8 +749,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,xsk_skb_init_misc (skb, xs, desc->addr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr); -if (unlikely (err)) +if (unlikely (err)) { +xsk_drop_untrans_skb (skb);return ERR_PTR (err); + } } }else {struct xsk_addrs *xsk_addr;
Signature · 1 line
2.41.3
[email protected]
AI reviewRe: [PATCH net v2 5/8] xsk: avoid skb leak in XDP_TX_METADATA case
Sashiko AI review found 2 potential issue(s): - [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in `xsk_skb_metadata` allows unprivileged users to bypass bounds checks and cause…
Sashiko AI review found 2 potential issue(s):
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability in xsk_skb_metadata allows unprivileged users to bypass bounds checks and cause out-of-bounds memory accesses.
- [High] Software checksumming calculation fails completely because skb_checksum_help() is invoked on an incomplete and non-linear SKB.
--
commit aef1b3344bd952f39b1181e046ab7d9ce77fbc8a
Author: Jason Xing <[email protected]>
xsk: avoid skb leak in XDP_TX_METADATA case
This commit explicitly adds a call to kfree_skb() (via xsk_drop_untrans_skb) when xsk_skb_metadata() returns an error, preventing an skb leak if metadata processing fails.
net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 831
Quoted 8 lines
net/xdp/xsk.c +0
-0@@ -829,8 +829,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,xsk_skb_init_misc (skb, xs, desc->addr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr);
While looking at this metadata handling, I noticed a couple of pre-existing regressions.
Will skb_checksum_help() always fail when called here?
xsk_skb_metadata() is called right after allocating the SKB but before any payload fragments are appended via skb_fill_page_desc() further down in xsk_build_skb_zerocopy(). This means skb->len and skb_headlen(skb) are 0.
If software checksumming is enabled, xsk_skb_metadata() calls skb_checksum_help(skb). Since skb_headlen(skb) is 0, won't the check offset >= skb_headlen(skb) in skb_checksum_help() always evaluate to true, triggering a warning and causing the packet to be dropped?
Also, can a user-space program modify the checksum metadata concurrently?
In xsk_skb_metadata(), the metadata is read directly from the UMEM buffer, which is shared memory mapped into user-space:
net/xdp/xsk.c:xsk_skb_metadata() {
...
if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) {
if (unlikely(meta->request.csum_start +
meta->request.csum_offset +
sizeof(__sum16) > desc->len))
return -EINVAL;
skb->csum_start = hr + meta->request.csum_start;
skb->csum_offset = meta->request.csum_offset;
...
}The bounds check does not use READ_ONCE(). Could a user-space application modify meta->request.csum_start and meta->request.csum_offset between the check and the assignment? If so, it might bypass the bounds check and specify arbitrarily large offsets, causing out-of-bounds writes when the hardware inserts the checksum.
Quoted 8 lines
net/xdp/xsk.c +3
-1-if (unlikely (err)) +if (unlikely (err)) { +xsk_drop_untrans_skb (skb);return ERR_PTR (err); + } } }else {struct xsk_addrs *xsk_addr;
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
Re: [PATCH net v2 5/8] xsk: avoid skb leak in XDP_TX_METADATA case
Quoted 31 lines · On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
Sashiko AI review found 2 potential issue(s):
- [Critical] Time-of-Check to Time-of-Use (TOCTOU) vulnerability inxsk_skb_metadataallows unprivileged users to bypass bounds checks and cause out-of-bounds memory accesses.
- [High] Software checksumming calculation fails completely becauseskb_checksum_help()is invoked on an incomplete and non-linear SKB.
--commit
aef1b3344bd952f39b1181e046ab7d9ce77fbc8a
Author: Jason Xing <[email protected]>xsk: avoid skb leak in
XDP_TX_METADATAcaseThis commit explicitly adds a call to
kfree_skb()(viaxsk_drop_untrans_skb) whenxsk_skb_metadata()returns an error, preventing an skb leak if metadata processing fails.net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 831
Quoted 8 lines
net/xdp/xsk.c +0
-0@@ -829,8 +829,10 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,xsk_skb_init_misc (skb, xs, desc->addr);if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr);While looking at this metadata handling, I noticed a couple of pre-existing regressions.
Will
skb_checksum_help()always fail when called here?
Quoted 8 lines
xsk_skb_metadata()is called right after allocating the SKB but before any payload fragments are appended viaskb_fill_page_desc()further down inxsk_build_skb_zerocopy(). This meansskb->lenandskb_headlen(skb)are 0.If software checksumming is enabled,
xsk_skb_metadata()callsskb_checksum_help(skb). Sinceskb_headlen(skb)is 0, won't the check offset >=skb_headlen(skb)inskb_checksum_help()always evaluate to true, triggering a warning and causing the packet to be dropped?
Patch 1 has already dealt with this issue.
Quoted 19 lines
Also, can a user-space program modify the checksum metadata concurrently?
In
xsk_skb_metadata(), the metadata is read directly from the UMEM buffer, which is shared memory mapped into user-space:net/xdp/xsk.c:xsk_skb_metadata() { ... if (meta->flags & XDP_TXMD_FLAGS_CHECKSUM) { if (unlikely(meta->request.csum_start + meta->request.csum_offset + sizeof(__sum16) > desc->len)) return -EINVAL; skb->csum_start = hr + meta->request.csum_start; skb->csum_offset = meta->request.csum_offset; ... }
Quoted 5 lines
The bounds check does not use
READ_ONCE(). Could a user-space application modifymeta->request.csum_startandmeta->request.csum_offsetbetween the check and the assignment? If so, it might bypass the bounds check and specify arbitrarily large offsets, causing out-of-bounds writes when the hardware inserts the checksum.
Yes, you're right about this pre-existing behavior but currently I don't have any interest in expanding the series. Maybe I will construct it as a follow-up patch.
Quoted 12 lines
net/xdp/xsk.c static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, · line 758
Quoted 9 lines
net/xdp/xsk.c +3
-1-if (unlikely (err)) +if (unlikely (err)) { +xsk_drop_untrans_skb (skb);return ERR_PTR (err); + } } }else {struct xsk_addrs *xsk_addr;--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
PATCH v2 6/8 xsk: free the skb when hitting the upper bound MAX_SKB_FRAGS
In reply to Jason XingFrom: Jason Xing <[email protected]>
Fix it by explicitly adding kfree_skb() before returning back to its caller.
How to reproduce it in virtio_net:
1. the current skb is the first one (which means xs->skb is NULL) and hit the limit MAX_SKB_FRAGS.
2. xsk_build_skb_zerocopy() returns -EOVERFLOW.
3. the caller xsk_build_skb() clears skb by using 'skb = NULL;'. This is why bug can be triggered.
4. there is no chance to free this skb anymore.
Note that if in this case the xs->skb is not NULL, xsk_build_skb() will call xsk_drop_skb(xs->skb) to do the right thing.
--- net/xdp/xsk.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
net/xdp/xsk.c +4 -1
@@ -784,8 +784,11 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs, addr = buffer - pool->addrs;for (copied =0 , i =skb_shinfo (skb)->nr_frags; copied < len; i++) { -if (unlikely (i >=MAX_SKB_FRAGS )) +if (unlikely (i >=MAX_SKB_FRAGS )) { +if (!xs->skb) +xsk_drop_untrans_skb (skb);return ERR_PTR (-EOVERFLOW ); + } page = pool->umem->pgs[addr >>PAGE_SHIFT ];get_page (page);
Signature · 1 line
2.41.3
PATCH v2 7/8 xsk: fix xsk_addrs slab leak on multi-buffer error path
In reply to Jason XingFrom: Jason Xing <[email protected]>
When xsk_build_skb() / xsk_build_skb_zerocopy() sees the first continuation descriptor, it promotes destructor_arg from an inlined address to a freshly allocated xsk_addrs (num_descs = 1). The counter is bumped to >= 2 only at the very end of a successful build (by calling xsk_inc_num_desc()).
If the build fails in between (e.g. alloc_page() returns NULL with -EAGAIN, or the MAX_SKB_FRAGS overflow hits), we jump to free_err, skip calling xsk_inc_num_desc() to increment num_descs and leave the half-built skb attached to xs->skb for the app to retry. The skb now has
1) destructor_arg = a real xsk_addrs pointer,
2) num_descs = 1
If the app never retries and just close()s the socket, xsk_release() calls xsk_drop_skb() -> xsk_consume_skb(), which decides whether to free xsk_addrs by testing num_descs > 1:
if (unlikely(num_descs > 1))
kmem_cache_free(xsk_tx_generic_cache, destructor_arg);
Because num_descs is exactly 1 the branch is skipped and the xsk_addrs object is leaked to the xsk_tx_generic_cache slab.
Fix it by directly testing if destructor_arg is still addr. Or else it is modified and used to store the newly allocated memory from xsk_tx_generic_cache regardless of increment of num_desc, which we need to handle.
--- net/xdp/xsk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
net/xdp/xsk.c +2 -2
@@ -605,7 +605,7 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,spin_lock_irqsave (&pool->cq_prod_lock, flags); idx =xskq_get_prod (pool->cq); -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;for (i =0 ; i < num_descs; i++) {
@@ -660,7 +660,7 @@ static void xsk_consume_skb(struct sk_buff *skb)u32 num_descs =xsk_get_num_desc (skb);struct xsk_addrs *xsk_addr; -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;kmem_cache_free (xsk_tx_generic_cache, xsk_addr); }
Signature · 1 line
2.41.3
Re: [PATCH net v2 7/8] xsk: fix xsk_addrs slab leak on multi-buffer error path
Quoted 62 lines · Jason Xing
From: Jason Xing <[email protected]>
When
xsk_build_skb()/xsk_build_skb_zerocopy()sees the first continuation descriptor, it promotesdestructor_argfrom an inlined address to a freshly allocatedxsk_addrs(num_descs= 1). The counter is bumped to >= 2 only at the very end of a successful build (by callingxsk_inc_num_desc()).If the build fails in between (e.g.
alloc_page()returns NULL with-EAGAIN, or theMAX_SKB_FRAGSoverflow hits), we jump tofree_err, skip callingxsk_inc_num_desc()to incrementnum_descsand leave the half-built skb attached toxs->skbfor the app to retry. The skb now has
1)destructor_arg= a realxsk_addrspointer,
2)num_descs= 1If the app never retries and just close()s the socket,
xsk_release()callsxsk_drop_skb()->xsk_consume_skb(), which decides whether to freexsk_addrsby testingnum_descs> 1:if (
unlikely(num_descs > 1))
kmem_cache_free(xsk_tx_generic_cache, destructor_arg);Because
num_descsis exactly 1 the branch is skipped and thexsk_addrsobject is leaked to thexsk_tx_generic_cacheslab.Fix it by directly testing if
destructor_argis still addr. Or else it is modified and used to store the newly allocated memory fromxsk_tx_generic_cacheregardless of increment ofnum_desc, which we need to handle.Fixes 0ebc27a4c67d ("xsk: avoid data corruption on cq descriptor number")Signed-off-by Jason Xing <[email protected]>--- net/xdp/xsk.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)net/xdp/xsk.c +2
-2@@ -605,7 +605,7 @@ static void xsk_cq_submit_addr_locked(struct xsk_buff_pool *pool,spin_lock_irqsave (&pool->cq_prod_lock, flags); idx =xskq_get_prod (pool->cq); -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;for (i =0 ; i < num_descs; i++) {@@ -660,7 +660,7 @@ static void xsk_consume_skb(struct sk_buff *skb)u32 num_descs =xsk_get_num_desc (skb);struct xsk_addrs *xsk_addr; -if (unlikely (num_descs >1 )) { +if (unlikely (!xsk_skb_destructor_is_addr (skb))) { xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg;kmem_cache_free (xsk_tx_generic_cache, xsk_addr); }Signature · 1 line
2.41.3
PATCH v2 8/8 xsk: fix u64 descriptor address truncation on 32-bit architectures
In reply to Jason XingFrom: Jason Xing <[email protected]>
In copy mode TX, xsk_skb_destructor_set_addr() stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via a uintptr_t cast:
skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);
On 32-bit architectures uintptr_t is 32 bits, so the upper 32 bits of the descriptor address are silently dropped. In XDP_ZEROCOPY unaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT = 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.
Fix this by handling the 32-bit case directly in xsk_skb_destructor_set_addr(): when !CONFIG_64BIT, allocate an xsk_addrs struct (the same path already used for multi-descriptor SKBs) to store the full u64 address.
Extend xsk_drop_untrans_skb() to free the xsk_addrs allocation on 32-bit when the skb is dropped before transmission. Note that here we don't use 0x1UL method to judge in this case.
Also extend xsk_skb_destructor_is_addr() to cover 32-bit case like above.
The overhead is one extra kmem_cache_zalloc per first descriptor on 32-bit only; 64-bit builds are completely unchanged.
--- net/xdp/xsk.c | 54 ++++++++++++++++++++++++++++++++++++++++++++------- 1 file changed, 47 insertions(+), 7 deletions(-)
net/xdp/xsk.c +47 -7
@@ -556,9 +556,23 @@ static int xsk_cq_reserve_locked(struct xsk_buff_pool *pool)return ret; } +/* +* On 64-bit, destructor_arg can store an inline address directly +* (tagged with bit 0 set). On 32-bit, all addresses go through an +* allocated xsk_addrs struct instead. In that case this function +* returns true only when destructor_arg is NULL (set_addr has not +* yet been called or has failed). +* +* For all callers: +* return true: no xsk_addrs struct to handle +* return false: destructor_arg points to an xsk_addrs struct +*/ static bool xsk_skb_destructor_is_addr (struct sk_buff *skb) { -return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +if (IS_ENABLED (CONFIG_64BIT )) +return (uintptr_t )skb_shinfo (skb)->destructor_arg &0x1UL ; +else +return !skb_shinfo (skb)->destructor_arg; }static u64 xsk_skb_destructor_get_addr (struct sk_buff *skb)
@@ -566,9 +580,21 @@ static u64 xsk_skb_destructor_get_addr(struct sk_buff *skb)return (u64)((uintptr_t )skb_shinfo (skb)->destructor_arg & ~0x1UL ); } -static void xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) +static int xsk_skb_destructor_set_addr (struct sk_buff *skb,u64 addr) { +if (!IS_ENABLED (CONFIG_64BIT )) { +struct xsk_addrs *xsk_addr; + + xsk_addr =kmem_cache_zalloc (xsk_tx_generic_cache,GFP_KERNEL ); +if (!xsk_addr) +return -ENOMEM ; + xsk_addr->addrs[0 ] = addr; +skb_shinfo (skb)->destructor_arg = (void *)xsk_addr; +return 0 ; + } +skb_shinfo (skb)->destructor_arg = (void *)((uintptr_t )addr |0x1UL ); +return 0 ; }static void xsk_inc_num_desc (struct sk_buff *skb)
@@ -644,14 +670,14 @@ void xsk_destruct_skb(struct sk_buff *skb)sock_wfree (skb); } -static void xsk_skb_init_misc (struct sk_buff *skb,struct xdp_sock *xs, -u64 addr) +static int xsk_skb_init_misc (struct sk_buff *skb,struct xdp_sock *xs, +u64 addr) { skb->dev = xs->dev; skb->priority =READ_ONCE (xs->sk.sk_priority); skb->mark =READ_ONCE (xs->sk.sk_mark); skb->destructor = xsk_destruct_skb; -xsk_skb_destructor_set_addr (skb, addr); +return xsk_skb_destructor_set_addr (skb, addr); }static void xsk_consume_skb (struct sk_buff *skb)
@@ -719,6 +745,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,static void xsk_drop_untrans_skb (struct sk_buff *skb) { +if (!IS_ENABLED (CONFIG_64BIT ) && !xsk_skb_destructor_is_addr (skb)) { +struct xsk_addrs *xsk_addr; + + xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +kmem_cache_free (xsk_tx_generic_cache, xsk_addr); + } skb->destructor = sock_wfree;kfree_skb (skb); }
@@ -746,7 +778,12 @@ static struct sk_buff *xsk_build_skb_zerocopy(struct xdp_sock *xs,skb_reserve (skb, hr); -xsk_skb_init_misc (skb, xs, desc->addr); + err =xsk_skb_init_misc (skb, xs, desc->addr); +if (unlikely (err)) { +xsk_drop_untrans_skb (skb); +return ERR_PTR (err); + } +if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, pool, hr);if (unlikely (err)) {
@@ -845,7 +882,10 @@ static struct sk_buff *xsk_build_skb(struct xdp_sock *xs,if (unlikely (err)) goto free_err; -xsk_skb_init_misc (skb, xs, desc->addr); + err =xsk_skb_init_misc (skb, xs, desc->addr); +if (unlikely (err)) + goto free_err; +if (desc->options &XDP_TX_METADATA ) { err =xsk_skb_metadata (skb, buffer, desc, xs->pool, hr);
Signature · 1 line
2.41.3
Re: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 15 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. InXDP_ZEROCOPYunaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.
Quoted 4 lines
Fix this by handling the 32-bit case directly in
xsk_skb_destructor_set_addr(): when !CONFIG_64BIT, allocate anxsk_addrsstruct (the same path already used for multi-descriptor SKBs) to store the full u64 address.
Is it easier to make XSK depends on 64BIT to avoid dealing with that? Does anybody seriously run af_xdp on 32 bit systems?
Re: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 17 lines · Stanislav Fomichev
Quoted 15 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. InXDP_ZEROCOPYunaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.
Quoted 6 lines
Quoted 4 lines
Fix this by handling the 32-bit case directly in
xsk_skb_destructor_set_addr(): when !CONFIG_64BIT, allocate anxsk_addrsstruct (the same path already used for multi-descriptor SKBs) to store the full u64 address.Is it easier to make XSK
depends on 64BITto avoid dealing with that? Does
Of course, it would be super easy. Actually the initial version looks like this. One line of coder is simply enough.
Quoted 1 line
anybody seriously run
af_xdpon 32 bit systems?
But my worry as you guess is if there exists a 32 bit system? I doubt it. That's why I put some effort into adding the compatibility code to cover the case. Good news is that it doesn't add any side effects of the 64 bit system since they are protected under IS_ENABLED condition.
Thanks,
Jason
Re: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 28 lines · Jason Xing
Quoted 17 lines · Stanislav Fomichev
Quoted 15 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. InXDP_ZEROCOPYunaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Quoted 6 lines
Quoted 4 lines
Fix this by handling the 32-bit case directly in
xsk_skb_destructor_set_addr(): when !CONFIG_64BIT, allocate anxsk_addrsstruct (the same path already used for multi-descriptor SKBs) to store the full u64 address.Is it easier to make XSK
depends on 64BITto avoid dealing with that? DoesOf course, it would be super easy. Actually the initial version looks like this. One line of coder is simply enough.
Quoted 6 lines
Quoted 1 line
anybody seriously run
af_xdpon 32 bit systems?But my worry as you guess is if there exists a 32 bit system? I doubt it. That's why I put some effort into adding the compatibility code to cover the case. Good news is that it doesn't add any side effects of the 64 bit system since they are protected under
IS_ENABLEDcondition.
If someone complains, we can follow up? af_xdp is all u64 everywhere, including uapi, I doubt someone is using it on 32 bit systems. We don't test this part on 32 bit systems on nipa either..
Re: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 32 lines · Stanislav Fomichev
Quoted 30 lines · Jason Xing
Quoted 17 lines · Stanislav Fomichev
Quoted 15 lines · Jason Xing
From: Jason Xing <[email protected]>
In copy mode TX,
xsk_skb_destructor_set_addr()stores the 64-bit descriptor address into skb_shinfo(skb)->destructor_arg (void *) via auintptr_tcast:skb_shinfo(skb)->destructor_arg = (void *)((uintptr_t)addr | 0x1UL);On 32-bit architectures
uintptr_tis 32 bits, so the upper 32 bits of the descriptor address are silently dropped. InXDP_ZEROCOPYunaligned mode the chunk offset is encoded in bits 48-63 of the descriptor address (XSK_UNALIGNED_BUF_OFFSET_SHIFT= 48), meaning the offset is lost entirely. The completion queue then returns a truncated address to userspace, making buffer recycling impossible.Quoted 6 lines
Quoted 4 lines
Fix this by handling the 32-bit case directly in
xsk_skb_destructor_set_addr(): when !CONFIG_64BIT, allocate anxsk_addrsstruct (the same path already used for multi-descriptor SKBs) to store the full u64 address.Is it easier to make XSK
depends on 64BITto avoid dealing with that? DoesOf course, it would be super easy. Actually the initial version looks like this. One line of coder is simply enough.
Quoted 1 line
anybody seriously run
af_xdpon 32 bit systems?
Quoted 8 lines
Quoted 4 lines
But my worry as you guess is if there exists a 32 bit system? I doubt it. That's why I put some effort into adding the compatibility code to cover the case. Good news is that it doesn't add any side effects of the 64 bit system since they are protected under
IS_ENABLEDcondition.If someone complains, we can follow up?
af_xdpis all u64 everywhere, including uapi, I doubt someone is using it on 32 bit systems. We don't test this part on 32 bit systems on nipa either..
Great! Then it saves me some time :)
Thanks,
Jason
[email protected]
AI reviewRe: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
In reply to Jason XingSashiko AI review found 1 potential issue(s): - [High] On 32-bit systems, xsk_drop_untrans_skb() will call kmem_cache_free() on an uninitialized destructor_arg pointer in early allocation failure…
Sashiko AI review found 1 potential issue(s):
- [High] On 32-bit systems, xsk_drop_untrans_skb() will call kmem_cache_free() on an uninitialized destructor_arg pointer in early allocation failure paths, causing slab corruption. --
commit cf643670755512ad76d19d6d0707e86a2a05c671
Author: Jason Xing <[email protected]>
xsk: fix u64 descriptor address truncation on 32-bit architectures
This commit fixes a truncation issue of the 64-bit descriptor address on 32-bit architectures by allocating an xsk_addrs struct to store the full address instead of packing it into the destructor_arg pointer. It also updates the related cleanup and initialization functions to handle the newly allocated struct.
Quoted 4 lines
net/xdp/xsk.c +0
-0
[ ... ]
net/xdp/xsk.c static int xsk_skb_metadata(struct sk_buff *skb, void *buffer, · line 833
Quoted 10 lines
net/xdp/xsk.c +6
-0@@ -799,6 +825,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,static void xsk_drop_untrans_skb (struct sk_buff *skb) { +if (!IS_ENABLED (CONFIG_64BIT ) && !xsk_skb_destructor_is_addr (skb)) { +struct xsk_addrs *xsk_addr; + + xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +kmem_cache_free (xsk_tx_generic_cache, xsk_addr); + }
Could this pass an uninitialized pointer to kmem_cache_free() and cause slab corruption?
When a new socket buffer is allocated via sock_alloc_send_skb(), the networking core's __alloc_skb() function only zeroes the skb_shared_info struct up to the dataref field. The destructor_arg field resides after dataref and contains uninitialized garbage from the slab.
If an error occurs before destructor_arg is explicitly initialized, such as when xsk_skb_init_misc() returns -ENOMEM:
xsk_build_skb_zerocopy()
...
skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err);
...
err = xsk_skb_init_misc(skb, xs, desc->addr);
if (unlikely(err)) {
xsk_drop_untrans_skb(skb);
return ERR_PTR(err);
}In xsk_drop_untrans_skb() on 32-bit systems, xsk_skb_destructor_is_addr(skb) evaluates to !skb_shinfo(skb)->destructor_arg. Because destructor_arg contains non-zero garbage, the function returns false. The check in xsk_drop_untrans_skb() (!xsk_skb_destructor_is_addr(skb)) then succeeds, passing the garbage pointer to kmem_cache_free().
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8
Re: [PATCH net v2 8/8] xsk: fix u64 descriptor address truncation on 32-bit architectures
Quoted 55 lines · On Tue, Apr 21, 2026 at 5:40 PM <[email protected]> wrote:
Sashiko AI review found 1 potential issue(s):
- [High] On 32-bit systems,xsk_drop_untrans_skb()will callkmem_cache_free()on an uninitializeddestructor_argpointer in early allocation failure paths, causing slab corruption. --commit
cf643670755512ad76d19d6d0707e86a2a05c671
Author: Jason Xing <[email protected]>xsk: fix u64 descriptor address truncation on 32-bit architectures
This commit fixes a truncation issue of the 64-bit descriptor address on 32-bit architectures by allocating an
xsk_addrsstruct to store the full address instead of packing it into thedestructor_argpointer. It also updates the related cleanup and initialization functions to handle the newly allocated struct.Quoted 4 lines
net/xdp/xsk.c +0
-0[ ... ]
net/xdp/xsk.c static int xsk_skb_metadata(struct sk_buff *skb, void *buffer, · line 833
Quoted 10 lines
net/xdp/xsk.c +6
-0@@ -799,6 +825,12 @@ static int xsk_skb_metadata(struct sk_buff *skb, void *buffer,static void xsk_drop_untrans_skb (struct sk_buff *skb) { +if (!IS_ENABLED (CONFIG_64BIT ) && !xsk_skb_destructor_is_addr (skb)) { +struct xsk_addrs *xsk_addr; + + xsk_addr = (struct xsk_addrs *)skb_shinfo (skb)->destructor_arg; +kmem_cache_free (xsk_tx_generic_cache, xsk_addr); + }Could this pass an uninitialized pointer to
kmem_cache_free()and cause slab corruption?When a new socket buffer is allocated via
sock_alloc_send_skb(), the networking core's__alloc_skb()function only zeroes theskb_shared_infostruct up to the dataref field. Thedestructor_argfield resides after dataref and contains uninitialized garbage from the slab.If an error occurs before
destructor_argis explicitly initialized, such as whenxsk_skb_init_misc()returns-ENOMEM:
xsk_build_skb_zerocopy()... skb = sock_alloc_send_skb(&xs->sk, hr, 1, &err); ... err = xsk_skb_init_misc(skb, xs, desc->addr); if (unlikely(err)) { xsk_drop_untrans_skb(skb); return ERR_PTR(err); }
Quoted 5 lines
In
xsk_drop_untrans_skb()on 32-bit systems,xsk_skb_destructor_is_addr(skb)evaluates to !skb_shinfo(skb)->destructor_arg. Becausedestructor_argcontains non-zero garbage, the function returns false. The check inxsk_drop_untrans_skb()(!xsk_skb_destructor_is_addr(skb)) then succeeds, passing the garbage pointer tokmem_cache_free().
Yes, you're right. After I removed the untrans_skb code block as suggested by Stan, the problem is gone :)
Quoted 3 lines
--
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8