Re: [PATCH net v3 1/2] tcp: fix use-after-free in do_tcp_getsockopt(TCP_CONGESTION)

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] BPF getsockopt helper can race with user-space getsockopt(TCP_SAVED_SYN) causing a use-after-free on tp->saved_syn.
--

commit aaf314b305f9bf7c88d6539be45ea64300819450
Author: Cen Zhang (Microsoft Security FORGE Labs) <blbllhy@gmail.com>

tcp: fix use-after-free in do_tcp_getsockopt(TCP_CONGESTION)

This commit fixes a use-after-free in do_tcp_getsockopt() for TCP_CONGESTION
by holding rcu_read_lock() while reading the congestion control name. It also
annotates icsk_ca_ops updates with WRITE_ONCE() to address KCSAN data-races.

Link: https://lore.kernel.org/all/20260821182449.79785-2-blbllhy@gmail.com/

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index 455441f1b6949..55ca74f6029fa 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4576,16 +4576,24 @@ int do_tcp_getsockopt(struct sock *sk, int level,
>  		val = !inet_csk_in_pingpong_mode(sk);
>  		break;
>  
> -	case TCP_CONGESTION:
> +	case TCP_CONGESTION: {
> +		char ca_name[TCP_CA_NAME_MAX] = {};
> +
>  		if (copy_from_sockptr(&len, optlen, sizeof(int)))
>  			return -EFAULT;
>  		len = min_t(unsigned int, len, TCP_CA_NAME_MAX);
>  		if (copy_to_sockptr(optlen, &len, sizeof(int)))
>  			return -EFAULT;
> -		if (copy_to_sockptr(optval, icsk->icsk_ca_ops->name, len))
> +
> +		rcu_read_lock();
> +		memcpy(ca_name, READ_ONCE(icsk->icsk_ca_ops)->name,
> +		       sizeof(ca_name));
> +		rcu_read_unlock();
> +
> +		if (copy_to_sockptr(optval, ca_name, len))
>  			return -EFAULT;
>  		return 0;
> -
> +	}
>  	case TCP_ULP:
>  		if (copy_from_sockptr(&len, optlen, sizeof(int)))
>  			return -EFAULT;

[Severity: High]
This is a pre-existing issue, but while reviewing this TCP getsockopt
fix, I noticed another potential use-after-free regression in the BPF
getsockopt helper.

In net/core/filter.c:sol_tcp_sockopt():

	if (getopt) {
		if (optname == TCP_SAVED_SYN) {
			struct tcp_sock *tp = tcp_sk(sk);

			if (!tp->saved_syn ||
			    *optlen > tcp_saved_syn_len(tp->saved_syn))
				return -EINVAL;
			memcpy(optval, tp->saved_syn->data, *optlen);

Since BPF iterators run without holding the socket lock, can this
locklessly read tp->saved_syn and call memcpy() on its data?

If a concurrent user-space thread calls getsockopt(TCP_SAVED_SYN) on the
same socket, it acquires sockopt_lock_sock(sk) and frees the memory via
kfree(tp->saved_syn) inside tcp_saved_syn_free().

Will the BPF program's memcpy() then dereference the freed memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787870710.git.blbllhy@gmail.com?part=1