Re: [PATCH net 1/2] net: Make PSP skb extension survive packet scrubbing

[netdev-bot+sashiko@kernel.org]

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