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

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

Pre-existing issues:
- [High] Double release of socket references when batch reallocation fails, leading to a crash.
--

--- Patch [1]: [PATCH bpf v3] bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch() ---
commit a3c30aa4537ca1f20e7ead4267815490d8e97126
Author: Jose Fernandez (Anthropic) <[email protected]>
Subject: bpf: tcp: Fix use-after-free in bpf_iter_tcp_established_batch()

This commit fixes a use-after-free condition in the TCP iterator caused by
reqsk_queue_hash_req() modifying a socket's refcount after insertion into
the hash chain. It switches to using refcount_inc_not_zero() while holding
the bucket lock and skipping unincremented sockets. Batch completeness is
now evaluated by checking if the walk left any socket behind.

Link to v2: https://lore.kernel.org/bpf/[email protected]
Link to v1: https://lore.kernel.org/bpf/[email protected]

> diff --git a/net/ipv4/tcp_ipv4.c b/net/ipv4/tcp_ipv4.c
> index 209ef7522508f..1034757a53280 100644
> --- a/net/ipv4/tcp_ipv4.c
> +++ b/net/ipv4/tcp_ipv4.c

[ ... ]

> @@ -3162,10 +3163,14 @@ static struct sock *bpf_iter_tcp_batch(struct seq_file *seq)
>  		return ERR_PTR(err);
>  	}

[Severity: High]
This is a pre-existing issue, but could this lead to a double release and
crash if the batch reallocation fails?

When bpf_iter_tcp_realloc_batch() fails (e.g. with -ENOMEM), it returns the
error here, but iter->end_sk is not reset. 

The seq_read() loop will then abort and invoke the stop callback,
bpf_iter_tcp_seq_stop(), which blindly calls bpf_iter_tcp_put_batch()
again:

	if (iter->cur_sk < iter->end_sk)
		bpf_iter_tcp_put_batch(iter);

Inside bpf_iter_tcp_put_batch(), the sk pointer and cookie share a union. The
first release clobbers the sk pointer:

	cookie = sock_gen_cookie(item->sk);
	sock_gen_put(item->sk);
	item->cookie = cookie;

On the second call, would this dereference the previously stored cookie as a
socket pointer, causing a panic?

It is noted in the commit message that this double-put bug was fixed upstream
by commit 980a81345275 ("bpf: tcp: fix double sock release on batch
realloc").

> -	expected = bpf_iter_fill_batch(seq, &sk);
> -	WARN_ON_ONCE(iter->end_sk != expected);
> +	bpf_iter_fill_batch(seq, &sk);
> +	WARN_ON_ONCE(sk);
>  done:
>  	bpf_iter_tcp_unlock_bucket(seq);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1