Re: [PATCH v3 bpf-next 3/4] bpf: Add kfunc to proxy TX HW Timestamp.

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Stale `BPF_RI_F_TX_TS_CMPL` flag is leaked across nested BPF network context executions, allowing state corruption.
- [High] Insufficient bounds checking in `bpf_skb_complete_tx_tstamp` combined with uninitialized `skb->cb` allows BPF programs to trigger an out-of-bounds read and leak kernel memory to userspace.
--

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

bpf: Add kfunc to proxy TX HW Timestamp.

This commit adds two kfuncs, bpf_skb_scrub_tx_tstamp() and
bpf_skb_complete_tx_tstamp(), to proxy TX hardware timestamps for socket
applications using SCM_TIMESTAMPING. It extracts the timestamp and enqueues
a regenerated skb to the sender's error queue.

> diff --git a/net/core/dev.c b/net/core/dev.c
> index 0c6c270d9f7d1..a45cdf38616ac 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4456,6 +4456,41 @@ tcx_run(const struct bpf_mprog_entry *entry, struct sk_buff *skb,
>  	return tcx_action_code(skb, ret);
>  }
>  
> +static int skb_do_completion(struct sk_buff *skb)
> +{
> +	enum skb_drop_reason drop_reason = SKB_DROP_REASON_TC_INGRESS;
> +	struct bpf_redirect_info *ri = bpf_net_ctx_get_ri();
> +	struct bpf_tx_tstamp_cmpl *txtscmpl;
> +
> +	if (!(ri->kern_flags & BPF_RI_F_TX_TS_CMPL))
> +		goto drop;

[Severity: High]
Does this code leak the BPF_RI_F_TX_TS_CMPL flag across nested executions?

If a nested execution occurs (for example, via bpf_clone_redirect()
triggering a synchronous sch_handle_ingress() on a tunnel device), the
inner execution reuses the outer execution's bpf_net_context. If the inner
BPF program sets the BPF_RI_F_TX_TS_CMPL flag and returns TC_ACT_ERRQUEUE,
skb_do_completion() processes the inner packet but leaves the flag set in
ri->kern_flags.

When the outer execution resumes and returns TCX_ERRQUEUE,
skb_do_completion() will be invoked again on the outer packet, incorrectly
applying the inner execution's stale txtscmpl attributes. Should
ri->kern_flags have the BPF_RI_F_TX_TS_CMPL bit cleared after processing?

> +
> +	if (skb_unclone(skb, GFP_ATOMIC))
> +		goto drop;
> +
> +	__skb_push(skb, skb->mac_len);
> +
> +	txtscmpl = &ri->txtscmpl;
> +
> +	drop_reason = pskb_may_pull_reason(skb, txtscmpl->payload_offset);
> +	if (drop_reason)
> +		goto drop;
> +
> +	skb->protocol = txtscmpl->protocol;
> +	skb_set_network_header(skb, txtscmpl->network_offset);
> +	__skb_pull(skb, txtscmpl->payload_offset);

[Severity: High]
Does this pull, combined with the uninitialized skb->cb, set up the
conditions for an out-of-bounds read?

Because skb->cb is not cleared here, it retains its contents from the TC
layer (struct qdisc_skb_cb). The BPF program can control qdisc_skb_cb->data
via the __sk_buff->cb array. If the BPF program manipulated the cb array to
set a large optlen, and payload_offset only guarantees linear buffer space
up to the end of the 20-byte IP header (as verified in
bpf_skb_complete_tx_tstamp), ip_cmsg_recv_opts() could read past the
validated linear buffer and leak adjacent kernel heap memory to userspace.

Should skb->cb be cleared or initialized before handing off the skb to the
socket error queue?

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index bd8c9a7b7505a..c8d00067536be 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c

[ ... ]

> @@ -12396,6 +12396,67 @@ __bpf_kfunc int bpf_skb_set_hwtstamp(struct __sk_buff *s,

[ ... ]

> +	delta = attrs->payload_offset - attrs->network_offset;
> +	switch (attrs->protocol) {
> +	case htons(ETH_P_IP):
> +		if (delta < (s32)sizeof(struct iphdr) || !sk_is_inet(sk))
> +			return -EINVAL;
> +		break;

[Severity: High]
Is this bounds check sufficient to prevent out-of-bounds reads during IP
options parsing?

This only validates that payload_offset - network_offset >=
sizeof(struct iphdr) (20 bytes). 

Because skb->cb is not cleared in skb_do_completion() before placing the
packet on the socket's error queue, an attacker can set cb[1] (which aligns
with optlen in struct inet_skb_parm) via the __sk_buff->cb array. When a
socket application reads the error queue with IP_CMSG_RECVOPTS enabled,
ip_cmsg_recv_opts() will interpret the uninitialized skb->cb as
struct inet_skb_parm and read optlen bytes past this 20-byte IP header.

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