Re: [PATCH net-next 02/15] bpf: Add bpf_skb_fclone_orig() kfunc

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] bpf: UAF/Leak via trusted logically freed skb in bpf_skb_fclone_orig()
- [Low] bpf: kernel-doc parameter mismatch for bpf_skb_fclone_orig
--

commit cbb8066a0a59ac3028e52e52059cc238816dbe14
Author: Jakub Sitnicki <jakub@cloudflare.com>

bpf: Add bpf_skb_fclone_orig() kfunc

This patch introduces a new kfunc bpf_skb_fclone_orig() to allow BPF
programs to retrieve the fast-clone original of a TCP write-queue skb.
This enables associating metadata with the original skb that will show
up during retransmissions.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 33c1ee3b966b0..fdfc1016030a2 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -12469,6 +12469,30 @@ __bpf_kfunc bool bpf_skb_is_traced(struct sk_buff *skb)
>  	return skb->traced;
>  }
>  
> +/**
> + * bpf_skb_fclone_orig() - Return the fast-clone original of an skb
> + * @skb_: socket buffer allocated from the fclone cache

[Severity: Low]
This isn't a bug, but there is a kernel-doc parameter mismatch here.
The documentation refers to @skb_, but the function signature below uses
skb. Could this trigger a warning during W=1 builds?

> + *
> + * TCP write-queue skbs are allocated from the fclone cache, the packet
> + * transmitted on the wire is a fast clone. A prog can only associate
> + * metadata with an skb by keying on its address. On retransmission TCP
> + * reuses the write-queue original, a different address with no packet
> + * hook, so the retransmit carries no metadata. A prog can use this kfunc
> + * to retrieve the address of the write-queue original and populate the
> + * metadata stash for it, so the metadata shows up on skb retransmits.
> + *
> + * Return:
> + * * orig skb address  - skb is a fast clone
> + * * %NULL             - skb is not a fast clone
> + */
> +__bpf_kfunc struct sk_buff *bpf_skb_fclone_orig(struct sk_buff *skb)
> +{
> +	if (skb->fclone == SKB_FCLONE_CLONE)
> +		return &container_of(skb, struct sk_buff_fclones, skb2)->skb1;
> +
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_SKB_GATED_TRACEPOINTS */

[ ... ]

> @@ -12856,6 +12880,7 @@ BTF_KFUNCS_END(bpf_kfunc_check_set_skb_meta)
>  BTF_KFUNCS_START(bpf_kfunc_check_set_skb_tracepoints)
>  BTF_ID_FLAGS(func, bpf_trace_skb)
>  BTF_ID_FLAGS(func, bpf_skb_is_traced)
> +BTF_ID_FLAGS(func, bpf_skb_fclone_orig, KF_RET_NULL)

[Severity: High]
Can this allow a BPF program to modify a logically dead skb, leading to a
memory leak or use-after-free?

If TCP receives an ACK and logically frees the original skb (skb1), its
users count drops to 0. If a qdisc or driver later drops the fast clone
(skb2) and triggers a tracepoint, calling bpf_skb_fclone_orig() returns a
pointer to the dead skb1.

Since the kfunc is registered here without KF_ACQUIRE or KF_RCU, does the
verifier treat the returned pointer as implicitly PTR_TRUSTED?

If a BPF program passes this returned skb to bpf_dynptr_from_skb() and
calls bpf_dynptr_write(), pskb_expand_head() could allocate a new head
buffer for the dead skb1, permanently leaking it.

>  BTF_KFUNCS_END(bpf_kfunc_check_set_skb_tracepoints)
>  #endif

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260928-bpf-meta-gated-tracepoints-v1-0-844dbf3e1edf@cloudflare.com?part=2