* Re: [PATCH net 1/2] net: Make PSP skb extension survive packet scrubbing
2026-10-25 4:05 ` [PATCH net 1/2] net: Make PSP skb extension survive " Wei Wang
@ 2026-10-26 4:05 ` sashiko-bot
2026-10-26 5:05 ` netdev-bot+sashiko
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: sashiko-bot @ 2026-10-26 4:05 UTC (permalink / raw)
To: Wei Wang; +Cc: bpf
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] net: skb_ext_scrub() on cloned skb causes xfrm/mctp UAF on COW or ref leak
--
commit 1f02b0c0e2664a9f83f628b54130f3d1388bbb4a
Author: Wei Wang <weiwan@google.com>
net: Make PSP skb extension survive packet scrubbing
This commit introduces skb_ext_scrub() and uses it in skb_scrub_packet() in
place of skb_ext_reset() so that every skb extension except SKB_EXT_PSP is
deleted when a packet is scrubbed across tunnels or network namespaces.
Link: https://lore.kernel.org/r/20260910-bpf-meta-inside-skb-ext-v2-3-0b21e42180b0@cloudflare.com
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..4269d91cf6078 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
[ ... ]
> @@ -7441,6 +7442,23 @@ void __skb_ext_put(struct skb_ext *ext)
> kmem_cache_free(skbuff_ext_cache, ext);
> }
> EXPORT_SYMBOL(__skb_ext_put);
> +
> +void skb_ext_scrub(struct sk_buff *skb)
> +{
> + unsigned int id;
> +
> + if (likely(!skb->active_extensions))
> + return;
> +
> + for (id = 0; id < SKB_EXT_NUM; id++) {
> +#if IS_ENABLED(CONFIG_INET_PSP)
> + if (id == SKB_EXT_PSP)
> + continue;
> +#endif
> + skb_ext_del(skb, id);
> + }
> +}
[Severity: Critical]
Can calling skb_ext_del() on SKB_EXT_SEC_PATH or SKB_EXT_MCTP when
SKB_EXT_PSP keeps skb->active_extensions non-zero cause a double-put or
reference leak on a cloned skb?
Suppose an skb has both SKB_EXT_PSP and a reference-counted extension
(SKB_EXT_SEC_PATH holding struct xfrm_state references or SKB_EXT_MCTP
holding a struct mctp_sk_key reference) and is cloned (for example via
dev_queue_xmit_nit(), tc mirred, or bpf_clone_redirect), so
skb->extensions (ext) has refcount_read(&ext->refcnt) > 1.
When skb_scrub_packet() -> skb_ext_scrub() calls skb_ext_del() ->
__skb_ext_del(), SKB_EXT_PSP keeps skb->active_extensions non-zero, and
because refcount_read(&ext->refcnt) > 1, __skb_ext_del() returns early after
only clearing the bit in skb->active_extensions:
net/core/skbuff.c:__skb_ext_del() {
struct skb_ext *ext = skb->extensions;
skb->active_extensions &= ~(1 << id);
if (skb->active_extensions == 0) {
skb->extensions = NULL;
__skb_ext_put(ext);
return;
}
if (refcount_read(&ext->refcnt) > 1)
return;
...
}
This leaves skb pointing to the shared struct skb_ext with
ext->offset[SKB_EXT_SEC_PATH] or ext->offset[SKB_EXT_MCTP] still non-zero
and its payload still populated, while the corresponding bit in
skb->active_extensions is cleared. From that state, two regressions appear
possible:
1. If another extension is later added to the scrubbed skb via skb_ext_add()
while the clone is still alive (ext->refcnt > 1), skb_ext_add() calls
skb_ext_maybe_cow(old, skb->active_extensions):
net/core/skbuff.c:skb_ext_maybe_cow() {
...
memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE);
refcount_set(&new->refcnt, 1);
#ifdef CONFIG_XFRM
if (old_active & (1 << SKB_EXT_SEC_PATH)) {
struct sec_path *sp = skb_ext_get_ptr(old, SKB_EXT_SEC_PATH);
unsigned int i;
for (i = 0; i < sp->len; i++)
xfrm_state_hold(sp->xvec[i]);
}
#endif
#ifdef CONFIG_MCTP_FLOWS
if (old_active & (1 << SKB_EXT_MCTP)) {
struct mctp_flow *flow = skb_ext_get_ptr(old, SKB_EXT_MCTP);
if (flow->key)
refcount_inc(&flow->key->refs);
}
#endif
__skb_ext_put(old);
return new;
}
Because memcpy() copies old->offset[] and the extension payload into new,
__skb_ext_exist(new, id) is true for SKB_EXT_SEC_PATH or SKB_EXT_MCTP, but
because those bits are not set in old_active (skb->active_extensions),
skb_ext_maybe_cow() does not take additional references on sp->xvec[i] or
flow->key. When both old and new are eventually freed in __skb_ext_put(),
__skb_ext_put() checks __skb_ext_exist(ext, id) rather than
active_extensions:
net/core/skbuff.c:__skb_ext_put() {
...
#ifdef CONFIG_XFRM
if (__skb_ext_exist(ext, SKB_EXT_SEC_PATH))
skb_ext_put_sp(skb_ext_get_ptr(ext, SKB_EXT_SEC_PATH));
#endif
#ifdef CONFIG_MCTP_FLOWS
if (__skb_ext_exist(ext, SKB_EXT_MCTP))
skb_ext_put_mctp(skb_ext_get_ptr(ext, SKB_EXT_MCTP));
#endif
...
}
Wouldn't that call skb_ext_put_sp() or skb_ext_put_mctp() on both old and
new, dropping the struct xfrm_state or struct mctp_sk_key references twice
and causing a use-after-free?
2. Alternatively, if the clone is freed first, __skb_ext_put() decrements
ext->refcnt from 2 to 1 and returns, leaving the scrubbed skb as the sole
owner of ext with __skb_ext_exist(ext, id) still true and holding live
references for the deactivated extension. If secpath_set() (or
mctp_skb_set_flow()) is subsequently called on the scrubbed skb:
net/xfrm/xfrm_input.c:secpath_set() {
struct sec_path *sp, *tmp = skb_ext_find(skb, SKB_EXT_SEC_PATH);
sp = skb_ext_add(skb, SKB_EXT_SEC_PATH);
if (!sp)
return NULL;
if (tmp) /* reused existing one (was COW'd if needed) */
return sp;
/* allocated new secpath */
memset(sp->ovec, 0, sizeof(sp->ovec));
sp->olen = 0;
sp->len = 0;
...
}
Here skb_ext_find() returns NULL because the active_extensions bit is clear,
while skb_ext_add() sees ext->refcnt == 1 and __skb_ext_exist(new, id) ==
true and returns the existing chunk in-place without releasing its previous
references. Doesn't secpath_set() then see tmp == NULL and overwrite
sp->len = 0 (and mctp_skb_set_flow() overwrite flow->key), leaking the
existing struct xfrm_state or struct mctp_sk_key references?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261025040523.678027-1-weiwan@google.com?part=1
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net 1/2] net: Make PSP skb extension survive packet scrubbing
2026-10-25 4:05 ` [PATCH net 1/2] net: Make PSP skb extension survive " Wei Wang
2026-10-26 4:05 ` sashiko-bot
@ 2026-10-26 5:05 ` netdev-bot+sashiko
2026-10-27 1:00 ` Jakub Kicinski
2026-10-28 6:36 ` Kiran Kella
3 siblings, 0 replies; 7+ messages in thread
From: netdev-bot+sashiko @ 2026-10-26 5:05 UTC (permalink / raw)
To: weiwan
Cc: davem, kuba, pabeni, andrew+netdev, shuah, horms, jakub,
daniel.zahka, willemb, netdev, linux-kselftest, linux-kernel, bpf
Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider.
Critical: 0 · High: 1 · Medium: 1 · Low: 0
If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).
- [High] In net/core/skbuff.c, skb_ext_scrub() (lines 7446-7461) deletes
extensions one at a time with skb_ext_del() and keeps SKB_EXT_PSP…
- [Medium] Keeping SKB_EXT_PSP across tunnel decap in skb_scrub_packet()
(net/core/skbuff.c:6402) means inner IP fragments taken from a PSP-…
--
net: Make PSP skb extension survive packet scrubbing
This patch adds skb_ext_scrub(), which deletes every skb extension except
SKB_EXT_PSP. It uses the new helper in skb_scrub_packet() in place of
skb_ext_reset(). The goal is to keep the PSP extension on packets
redirected into another netns, for example through netkit associated
with a PSP device.
Link: https://lore.kernel.org/r/20260910-bpf-meta-inside-skb-ext-v2-3-0b21e42180b0@cloudflare.com
Closes: https://lore.kernel.org/netdev/DLODSKPY6ON6.1G6SP3CFFLCPJ@gmail.com/
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 48f6764117243..4269d91cf6078 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
[ ... ]
> @@ -6398,7 +6399,7 @@ void skb_scrub_packet(struct sk_buff *skb, bool xnet)
> skb->skb_iif = 0;
> skb->ignore_df = 0;
> skb_dst_drop(skb);
> - skb_ext_reset(skb);
> + skb_ext_scrub(skb);
[Severity: Medium]
skb_scrub_packet() also runs on tunnel decap, for example in
ip_tunnel_rcv() and in vxlan_rcv()->__iptunnel_pull_header(). With this
change, inner IP fragments taken from a PSP-decrypted tunnel payload now
carry SKB_EXT_PSP.
Before this patch, a PSP extension could only be on whole transport-mode
packets. Those have no inner IP header, so they can't be fragmented.
IP reassembly takes the extensions from the head fragment only:
net/ipv4/inet_fragment.c:inet_frag_reasm_prepare() {
...
skb_morph(skb, head);
...
}
inet_frag_reasm_finish() never compares the extensions of the other
fragments.
Could a reassembled TCP segment keep a valid PSP extension when only its
head fragment came through PSP? The remaining fragments could arrive
unprotected, as plain IP or plain VXLAN with a spoofed inner source.
__psp_sk_rx_policy_check() only compares spi, generation, version and
dev_id through psp_pse_matches_pas(). It looks like such a segment would
be accepted by the PSP socket.
This seems hard to exploit:
- The PSP peer would have to send inner fragments, and inner TCP
usually sets DF.
- The attacker would have to guess the IP ID.
- Mismatched ip_summed forces CHECKSUM_NONE, so the attacker's
fragments must reproduce the checksum of encrypted, unknown data.
The commit message only justifies keeping the PSP extension for
cross-netns forwarding. Was keeping it across tunnel decapsulation also
intended?
> nf_reset_ct(skb);
> nf_reset_trace(skb);
>
> @@ -7441,6 +7442,23 @@ void __skb_ext_put(struct skb_ext *ext)
> kmem_cache_free(skbuff_ext_cache, ext);
> }
> EXPORT_SYMBOL(__skb_ext_put);
> +
> +void skb_ext_scrub(struct sk_buff *skb)
> +{
> + unsigned int id;
> +
> + if (likely(!skb->active_extensions))
> + return;
> +
> + for (id = 0; id < SKB_EXT_NUM; id++) {
> +#if IS_ENABLED(CONFIG_INET_PSP)
> + if (id == SKB_EXT_PSP)
> + continue;
> +#endif
> + skb_ext_del(skb, id);
> + }
[Severity: High]
Can this loop leave a shared extension block half-deleted, and later
cause an xfrm_state refcount underflow?
Take skb A with ext block E, where SKB_EXT_PSP and SKB_EXT_SEC_PATH are
both active and E is shared with clone B (refcnt 2). When the loop
reaches SKB_EXT_SEC_PATH, __skb_ext_del() does:
skb->active_extensions &= ~(1 << id);
if (skb->active_extensions == 0) {
...
}
if (refcount_read(&ext->refcnt) > 1)
return;
PSP is still active, so this returns early. E keeps offset[SEC_PATH] set,
and sp->len still holds the N xfrm_state references.
Now suppose skb_ext_add() runs on A while B is still alive. Callers that
can do this include nf_bridge_alloc(), tc_skb_ext_alloc() and MPTCP.
skb_ext_maybe_cow() copies the whole block, stale sec_path included, but
only takes references for active ids:
memcpy(new, old, old->chunks * SKB_EXT_ALIGN_VALUE);
refcount_set(&new->refcnt, 1);
#ifdef CONFIG_XFRM
if (old_active & (1 << SKB_EXT_SEC_PATH)) {
...
xfrm_state_hold(sp->xvec[i]);
}
__skb_ext_put() releases based on offsets, not active bits:
if (__skb_ext_exist(ext, SKB_EXT_SEC_PATH))
skb_ext_put_sp(skb_ext_get_ptr(ext, SKB_EXT_SEC_PATH));
Freeing A puts the N states through the copy. Freeing B puts the same N
states again through E.
Wouldn't that underflow the xfrm_state refcount and allow a
use-after-free of the xfrm_state?
If B is freed first instead, a later secpath_set() on A can leak the
references. This happens when the inner packet is ESP again. skb_ext_add()
reuses the stale slot through this path:
if (__skb_ext_exist(new, id))
goto set_active;
secpath_set() then sees tmp == NULL and sets sp->len = 0. That loses the
N references still stored in xvec[].
One possible path:
- A PSP packet whose nexthdr is ESP gets a sec_path from host xfrm.
- After ESP decap, an AF_PACKET tap clones it in packet_rcv(), so the
ext block is now shared.
- The inner packet is VXLAN, so
vxlan_rcv()->__iptunnel_pull_header()->skb_scrub_packet() runs.
- The VXLAN device is a bridge port with br_netfilter, so
nf_bridge_alloc() triggers the COW.
The commit message says:
Scrubbing is safe when the extension slab is shared with clones:
deleting an extension only clears the per-skb active_extensions bit,
and the shared slab payload is released lazily by __skb_ext_put()
once the last reference goes away.
Does that still hold once the scrubbed skb is COWed or re-adds the slot?
The previous skb_ext_reset() dropped the whole block and never left one
in this state.
The same COW/put asymmetry already exists for secpath_reset() callers
such as l2tp_eth_dev_recv() and xfrmi_scrub_packet(). This patch makes it
reachable from every tunnel encap/decap and netns forward.
Instead of looping over skb_ext_del(), could this call skb_ext_reset()
when PSP is not active? When PSP is active, it could attach a fresh
extension that holds only the PSP payload. Another option might be for
skb_ext_maybe_cow() to clear the offsets of inactive ids in the copy.
> +}
> +EXPORT_SYMBOL(skb_ext_scrub);
> #endif /* CONFIG_SKB_EXTENSIONS */
>
> static void kfree_skb_napi_cache(struct sk_buff *skb)
--
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261025040523.678027-1-weiwan%40google.com
^ permalink raw reply [flat|nested] 7+ messages in thread* Re: [PATCH net 1/2] net: Make PSP skb extension survive packet scrubbing
2026-10-25 4:05 ` [PATCH net 1/2] net: Make PSP skb extension survive " Wei Wang
2026-10-26 4:05 ` sashiko-bot
2026-10-26 5:05 ` netdev-bot+sashiko
@ 2026-10-27 1:00 ` Jakub Kicinski
2026-10-28 6:36 ` Kiran Kella
3 siblings, 0 replies; 7+ messages in thread
From: Jakub Kicinski @ 2026-10-27 1:00 UTC (permalink / raw)
To: Wei Wang
Cc: David S . Miller, Paolo Abeni, Andrew Lunn, Shuah Khan,
Simon Horman, Jakub Sitnicki, Daniel Zahka, Willem de Bruijn,
netdev, linux-kselftest, linux-kernel, bpf
On Sun, 25 Oct 2026 04:05:22 +0000 Wei Wang wrote:
> +EXPORT_SYMBOL(skb_ext_scrub);
Why the export? In fact as of this patch set the helper doesn't
have to be visible outside of skbuff.c at all.
--
pw-bot: cr
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net 1/2] net: Make PSP skb extension survive packet scrubbing
2026-10-25 4:05 ` [PATCH net 1/2] net: Make PSP skb extension survive " Wei Wang
` (2 preceding siblings ...)
2026-10-27 1:00 ` Jakub Kicinski
@ 2026-10-28 6:36 ` Kiran Kella
3 siblings, 0 replies; 7+ messages in thread
From: Kiran Kella @ 2026-10-28 6:36 UTC (permalink / raw)
To: Wei Wang
Cc: David S . Miller, Jakub Kicinski, Paolo Abeni, Andrew Lunn,
Shuah Khan, Simon Horman, Jakub Sitnicki, Daniel Zahka,
Willem de Bruijn, netdev, linux-kselftest, linux-kernel, bpf
[-- Attachment #1: Type: text/plain, Size: 675 bytes --]
> +void skb_ext_scrub(struct sk_buff *skb)
> +{
> + unsigned int id;
> +
> + if (likely(!skb->active_extensions))
> + return;
> +
> + for (id = 0; id < SKB_EXT_NUM; id++) {
> +#if IS_ENABLED(CONFIG_INET_PSP)
> + if (id == SKB_EXT_PSP)
> + continue;
> +#endif
> + skb_ext_del(skb, id);
> + }
> +}
Thanks for the fix.
We saw the same issue with skb extension reset when testing with
vxlans associated to a psp device.
> +EXPORT_SYMBOL(skb_ext_scrub);
> #endif /* CONFIG_SKB_EXTENSIONS */
>
> static void kfree_skb_napi_cache(struct sk_buff *skb)
> --
> 2.56.0.257.gf7497c7d27-goog
>
>
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 5465 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread