Re: [PATCH net-next 03/14] bpf: Make BPF skb extension survive packet scrubbing

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Scrubbing and later re-adding SKB extensions breaks the `skb_ext` bump allocator, leading to a slab out-of-bounds write.
- [High] A Time-Of-Check to Time-Of-Use (TOCTOU) race condition in `__skb_ext_scrub` causes a memory leak of inner SKB extension references (such as `sec_path` or `mctp_flow`).
--

--- Patch [3]: [PATCH net-next 03/14] bpf: Make BPF skb extension survive packet scrubbing ---
commit 5b9486076275b0379f7c87d595b7fafe3fc1ef2a
Author: Jakub Sitnicki <[email protected]>

bpf: Make BPF skb extension survive packet scrubbing

skb_scrub_packet() drops all skb extensions unconditionally via
skb_ext_reset(), making it impossible for a BPF program to pass metadata
via bpf_skb_ext through a tunnel or across a netns boundary. Introduce
skb_ext_scrub() which consults each active extension before discarding it,
allowing extensions that request preservation to be kept. Update
udp_try_make_stateless() to use this new scrub method.

> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index 9947d2ed6635b..700bee045b5c5 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
[ ... ]
> @@ -7269,18 +7284,61 @@ void __skb_ext_put(struct skb_ext *ext)
[ ... ]
> +static int __skb_ext_scrub(struct sk_buff *skb, unsigned int keep)
> +{
> +	struct skb_ext *old = skb->extensions;
> +	struct skb_ext *ext;
> +	int i;
> +
> +	if (refcount_read(&old->refcnt) == 1) {
> +		skb_ext_put_each(old, keep);
> +		ext = old;
> +	} else {
> +		ext = skb_ext_maybe_cow(old, keep);

[Severity: High]
Does this introduce a time-of-check to time-of-use (TOCTOU) race condition?

If an SKB with extensions is cloned, and one clone passes through
skb_ext_scrub() while another is concurrently freed, the refcount could drop
to 1 just before skb_ext_maybe_cow() is called.

Looking at skb_ext_maybe_cow():

	if (refcount_read(&old->refcnt) == 1)
		return old;

If skb_ext_maybe_cow() returns the old extension directly, skb_ext_put_each()
is bypassed, meaning unkept inner references are not released. Furthermore,
since the offsets are subsequently zeroed out below, the final __skb_ext_put()
will also fail to discover and free those leaked resources.

This seems to permanently leak state references like xfrm_state.

> +		if (!ext)
> +			return -ENOMEM;
> +		skb->extensions = ext;
> +	}
> +
> +	for (i = 0; i < SKB_EXT_NUM; i++) {
> +		if (!(keep & (1 << i)))
> +			ext->offset[i] = 0;

[Severity: Critical]
Can this cause a slab out-of-bounds write due to how the skb_ext bump
allocator works?

By zeroing the offset here, we leave ext->chunks at its previous size, which
permanently consumes bump allocator space. If a packet retains one extension
and later re-acquires a scrubbed extension, skb_ext_add() will append it at
the end of the chunk space because it appears non-existent:

	if (__skb_ext_exist(new, id))
		goto set_active;

	newoff = new->chunks;
	...
	newlen = newoff + skb_ext_type_len[id];
	new->chunks = newlen;

Repeated scrub and add cycles (for instance, crossing netns boundaries into
IPsec) could cause new->chunks to exceed the skbuff_ext_cache slab object
bounds.

> +	}
> +	skb->active_extensions = keep;
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260814-bpf-meta-inside-skb-ext-v1-0-767edd862656@cloudflare.com?part=3