Re: [PATCH bpf v2] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] The patch introduces a multi-line comment that violates the BPF subsystem comment style guidelines.

Pre-existing issues:
- [High] Double-put and wild pointer dereference of batch sockets on the first bpf_iter_tcp_realloc_batch failure path.
--

--- Patch [1]: [PATCH bpf v2] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() ---
commit c52d637a13758169d9dce0d7c05be97965d399d3
Author: Jose Fernandez (Anthropic) <jose.fernandez@linux.dev>

bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()

This commit fixes a use-after-free issue in the TCP iterator caused by
request sockets being published to the ehash chain with a refcount of 0
before being updated. The fix uses refcount_inc_not_zero() to safely handle
these sockets during batch iteration. It also refactors the batch completion
logic to rely on whether any sockets were left behind during the walk.

Link: https://lore.kernel.org/bpf/20260620-bpf-iter-tcp-refcnt-v1-1-883bf9e69495@linux.dev

> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c
> @@ -3073,24 +3073,24 @@ static unsigned int bpf_iter_tcp_established_batch(struct seq_file *seq,

[ ... ]

>  	sk_nulls_for_each_from(sk, node) {
> -		if (seq_sk_match(seq, sk)) {
> -			if (iter->end_sk < iter->max_sk) {
> -				sock_hold(sk);
> -				iter->batch[iter->end_sk++].sk = sk;
> -			} else if (!*start_sk) {
> -				/* Remember where we left off. */
> -				*start_sk = sk;
> -			}
> -			expected++;
> +		if (!seq_sk_match(seq, sk))
> +			continue;
> +		expected++;
> +		if (iter->end_sk < iter->max_sk) {
> +			/* reqsk_queue_hash_req() inserts with sk_refcnt == 0
> +			 * and refcount_set()s it after the bucket lock drops.
> +			 */

[Severity: Low]
Does this multi-line comment follow the BPF subsystem comment style?
The BPF subsystem guide expects the opening block to be on its own line:

/*
 * reqsk_queue_hash_req() inserts with sk_refcnt == 0
 ...

> +			if (unlikely(!refcount_inc_not_zero(&sk->sk_refcnt)))
> +				continue;

[ ... ]

> @@ -3128,12 +3128,14 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
>  	struct sock *sk;
>  	int err;
>  
> +again:
>  	sk = bpf_iter_tcp_resume(seq);
>  	if (!sk)
>  		return NULL; /* Done */
>  
>  	expected = bpf_iter_fill_batch(seq, &sk);
> -	if (likely(iter->end_sk == expected))
> +	/* sk carries the first socket that did not fit in the batch. */
> +	if (likely(!sk))
>  		goto done;

[Severity: High]
This is a pre-existing issue, but I wanted to ask about the reallocation
failure path that occurs just after this block in bpf_iter_tcp_batch().

If bpf_iter_tcp_realloc_batch() fails, the function calls
bpf_iter_tcp_put_batch() and returns an error without clearing iter->end_sk.

When this happens, bpf_iter_tcp_put_batch() overwrites item->sk with a
cookie:

net/ipv4/tcp_ipv4.c:bpf_iter_tcp_put_batch() {
    ...
    while (cur_sk < iter->end_sk) {
        ...
        sock_gen_put(item->sk);
        item->cookie = cookie;
    }
}

Because the seq framework subsequently calls bpf_iter_tcp_seq_stop(), it sees
iter->cur_sk < iter->end_sk and calls bpf_iter_tcp_put_batch() a second time.

Could this lead to a wild pointer dereference when sock_gen_put() tries
to use the cookie as a socket pointer during the second pass?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260717-bpf-iter-tcp-refcnt-v2-1-8e81f0ac6f3e@linux.dev?part=1