Re: [PATCH net-next 01/14] bpf: Introduce per-packet metadata storage for BPF programs

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] bpf_dynptr_from_skb_ext() mutates skb->extensions without synchronization in TRACING and LSM programs, causing data races.
- [High] bpf_dynptr_from_skb_ext() COWs the SKB extension without invalidating existing dynptr slices in the verifier, leading to a Use-After-Free.
- [High] __bpf_skb_ext_store_bytes() and bpf_skb_ext_pointer() allow lockless mutation of shared SKB extensions on cloned SKBs.
--

--- Patch [1]: [PATCH net-next 01/14] bpf: Introduce per-packet metadata storage for BPF programs ---
commit e0c430f0cc32850ff4a67d41f0ec463446dca9ae
Author: Jakub Sitnicki <[email protected]>

bpf: Introduce per-packet metadata storage for BPF programs

This commit adds a new sk_buff extension (struct bpf_skb_ext) to provide
up to 256 bytes of per-packet storage for BPF programs. The storage is
exposed to BPF programs via the bpf_dynptr_from_skb_ext() kfunc.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 16845987b2449..2cbb40b392edd 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12259,6 +12259,48 @@ int __bpf_skb_meta_store_bytes(struct sk_buff *skb, u32 offset,

[ ... ]

> +int __bpf_skb_ext_store_bytes(struct sk_buff *skb, u32 offset,
> +			      const void *from, u32 len, u64 flags)
> +{
> +	struct bpf_skb_ext *ext;
> +
> +	if (unlikely(flags))
> +		return -EINVAL;
> +
> +	ext = skb_ext_find(skb, SKB_EXT_BPF);
> +	if (!ext)
> +		return -ENOENT;
> +
> +	memmove(ext->buf + offset, from, len);

[Severity: High]
Is it safe to directly mutate the extension buffer here without checking if
the SKB is cloned?

If a BPF program creates a writable dynptr, clones the SKB (which increments
the extension's reference count), and then writes to the dynptr, this
memmove() will modify the shared extension block while the cloned SKB might
be executing concurrently on another CPU.

Should this use a mechanism like bpf_try_make_writable() or check
ext->refcnt before mutating to ensure exclusive ownership?

> +	return 0;
> +}
> +#endif /* CONFIG_BPF_SKB_EXT */

[ ... ]

> +__bpf_kfunc int bpf_dynptr_from_skb_ext(struct __sk_buff *skb_, u32 size,
> +					u64 flags,
> +					struct bpf_dynptr *ptr__uninit)
> +{

[ ... ]

> +	ext = skb_ext_add(skb, SKB_EXT_BPF);
> +	if (!ext) {
> +		err = -ENOMEM;
> +		goto error;
> +	}

[Severity: High]
Could this COW operation lead to a use-after-free if existing dynptr slices
are not invalidated?

If a BPF program obtains a direct slice to a read-only SKB extension via
bpf_dynptr_from_skb_ext() and bpf_dynptr_slice(), and then calls
bpf_dynptr_from_skb_ext() again with BPF_SKB_EXT_F_CREATE, this forces a
Copy-On-Write in skb_ext_add(), dropping the reference on the old block.

However, because KF_bpf_dynptr_from_skb_ext is not added to
bpf_is_kfunc_pkt_changing() in the verifier, clear_all_pkt_pointers() won't
be invoked. The previously obtained slice remains valid in the verifier
state, allowing the program to read or write the freed memory.

> +	if (!exists)
> +		memset(ext, 0, sizeof(*ext));
> +out:
> +	bpf_dynptr_init(ptr, skb, BPF_DYNPTR_TYPE_SKB_EXT, 0, size);

[ ... ]

> @@ -12640,6 +12760,21 @@ static int __init bpf_kfunc_init(void)

[ ... ]

> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_NETFILTER, &bpf_kfunc_set_skb_ext);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_LSM, &bpf_kfunc_set_skb_ext);
> +	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_TRACING, &bpf_kfunc_set_skb_ext);

[Severity: High]
Can calling bpf_dynptr_from_skb_ext() from TRACING or LSM programs cause
data races on the skb structure?

TRACING and LSM programs can intercept an SKB that is currently shared
(skb->users > 1) and being processed concurrently on multiple CPUs.

Since bpf_dynptr_from_skb_ext() unconditionally calls skb_ext_add(), which
performs lockless read-modify-write operations on skb->active_extensions,
concurrent execution could corrupt the bitfield or overwrite the extension
pointer, leaking the allocated extension block.

> +#endif
>  	ret = ret ?: register_btf_kfunc_id_set(BPF_PROG_TYPE_XDP, &bpf_kfunc_set_xdp);

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