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

[Matthieu Baerts]

Hi Cen,

On 26/08/2026 19:13, Cen Zhang (Microsoft) wrote:
> From: "Cen Zhang (Microsoft Security FORGE Labs)" <blbllhy@xxxxxxxxx>
> 
> do_tcp_getsockopt() reads icsk->icsk_ca_ops->name without holding
> rcu_read_lock(). Since commit 0baf26b0fcd7 ("bpf: tcp: Support
> tcp_congestion_ops in bpf"), icsk_ca_ops can point to dynamically
> allocated BPF struct_ops memory that may be freed concurrently via
> setsockopt(TCP_CONGESTION), leading to a use-after-free.
> 
>   BUG: KASAN: slab-use-after-free in _copy_to_user+0x37/0x60
>   Read of size 16 at addr ffff888013505260 by task exploit/149
>    _copy_to_user+0x37/0x60
>    do_tcp_getsockopt+0x158a/0x2460 (net/ipv4/tcp.c:4585)
>    tcp_getsockopt+0x91/0xf0
>    __sys_getsockopt+0xf7/0x170
> 
> Fix this by holding rcu_read_lock() around the ca_ops->name access,
> using READ_ONCE() to load icsk_ca_ops, and copying the name to a
> stack buffer before releasing the lock. Also annotate every explicit
> icsk_ca_ops store with WRITE_ONCE() to fix the accompanying KCSAN
> data-race issue.

(...)

> diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
> index b4237d0e994d..4918f48ba76d 100644
> --- a/net/ipv4/tcp.c
> +++ b/net/ipv4/tcp.c
> @@ -4577,16 +4577,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();
> +		ca_ops = READ_ONCE(icsk->icsk_ca_ops);

It looks like you (or your assistant) forgot to declare ca_ops:

  const struct tcp_congestion_ops *ca_ops;

Also, I *guess* a 'rcu_read_lock' is needed in mptcp_ca_reset() from
net/mptcp/protocol.c, above the modification you did, to get the name, no?

Apart from that, the modifications look good to me.

Cheers,
Matt
-- 
Sponsored by the NGI0 Core fund.