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

[sashiko-bot@kernel.org]

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