Re: [PATCH v2 bpf-next/net 3/5] bpf: Add bpf_skb_set_hwtstamp().

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Potential use-after-free read of `attrs->hwtstamp` if the BPF program passes a pointer to the packet payload.
--

commit 41caec1764c831fb81388c5eab46d122318a01be
Author: Kuniyuki Iwashima <kuniyu@google.com>

bpf: Add bpf_skb_set_hwtstamp().

This commit adds a new kfunc, bpf_skb_set_hwtstamp(), to allow BPF programs
at TC ingress to update the hardware timestamp of an skb. This enables user
space proxies to encapsulate and pass hardware timestamps to socket
applications bypassing the kernel networking stack.

> diff --git a/net/core/filter.c b/net/core/filter.c
> index acdc66aa4f27c..bee1e7eee5eae 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
[ ... ]
> @@ -12372,6 +12372,30 @@ __bpf_kfunc int bpf_sock_ops_enable_tx_tstamp(struct bpf_sock_ops_kern *skops,
>  	return 0;
>  }
>  
> +__bpf_kfunc int bpf_skb_set_hwtstamp(struct __sk_buff *s,
> +				     struct bpf_hwtstamp *attrs, int attrs__sz)
> +{
> +	int defined_sz = offsetofend(struct bpf_hwtstamp, hwtstamp);
> +	struct sk_buff *skb = (struct sk_buff *)s;
> +
> +	if (attrs__sz != sizeof(*attrs) ||
> +	    memchr_inv((char *)attrs + defined_sz, 0, sizeof(u64)))
> +		return -EINVAL;
> +
> +	if (!skb_at_tc_ingress(skb))
> +		return -EINVAL;
> +
> +	if (skb_unclone(skb, GFP_ATOMIC))
> +		return -ENOMEM;
> +
> +	skb_clear_tstamp(skb);
> +	skb_hwtstamps(skb)->hwtstamp = attrs->hwtstamp;

[Severity: High]
Could this result in a use-after-free if the BPF program passes a pointer
to the packet payload as the attrs argument?

The BPF verifier allows passing a packet pointer to a kfunc argument that
has an associated size parameter (attrs__sz). If the attrs pointer points
into the packet's payload (skb->data) and the skb happens to be cloned,
skb_unclone() will invoke pskb_expand_head().

This reallocation frees the original packet buffer, meaning the attrs
pointer becomes dangling. The subsequent read of attrs->hwtstamp would then
dereference freed memory.

Would it be safer to read the attrs->hwtstamp value into a local variable
before calling skb_unclone()?

> +
> +	bpf_compute_data_pointers(skb);
> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613010039.1362312-1-kuniyu@google.com?part=3