Re: [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer

[Paolo Abeni <pabeni@redhat.com>]

On 8/16/26 11:28 PM, Hyunwoo Kim wrote:
> tcp_inbound_ao_hash() is called before bh_lock_sock_nested() is taken,
> with only rcu_read_lock() held. On the fast path for established
> sockets, if the rnext_keyid sent by the peer differs from
> current_key->sndid, the key the peer asked for is looked up and stored
> in current_key. The lookup is inside the RCU read side, but current_key
> outlives it.
> 
> When the socket is disconnected and connect() is called again for
> another peer, tcp_ao_connect_init() unlinks every key that does not
> match the new peer and frees it with call_rcu(). If current_key points
> at such a key, it is cleared to NULL.
> 
> The fast path reads sk_state only once on entry, so a softirq that got
> into it while the socket was still established can update current_key
> after that loop has already run. The update is inside the RCU read side,
> so it comes before the call_rcu() callback, and once the callback frees
> the key, current_key is left pointing at freed memory.
> 
> The next transmission picks that pointer up in tcp_get_current_key().
> tcp_ao_transmit_skb() then reads the traffic key from the freed object,
> which is the use-after-free.
> 
> Wait for one grace period before unlinking, and only if a key is going
> to be removed. By the time tcp_connect() runs the socket is already in
> TCP_SYN_SENT, and TCP_AO_ESTABLISHED does not contain TCPF_SYN_SENT, so
> a softirq entering after the wait cannot reach the fast path, and the
> ones already in it have finished. The existing NULL handling in the loop
> is then enough.
> 
> Fixes: 0a3a809089eb ("net/tcp: Verify inbound TCP-AO signed segments")
> Cc: stable@vger.kernel.org
> Signed-off-by: Hyunwoo Kim <imv4bel@gmail.com>
> ---
>  net/ipv4/tcp_ao.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index e4ec60a3349635..5094267447aee5 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -1160,6 +1160,15 @@ void tcp_ao_connect_init(struct sock *sk)
>  	l3index = l3mdev_master_ifindex_by_index(sock_net(sk),
>  						 sk->sk_bound_dev_if);
>  
> +	hlist_for_each_entry(key, &ao_info->head, node) {
> +		if (tcp_ao_key_cmp(key, l3index, addr, key->prefixlen,
> +				   family, -1, -1)) {
> +			/* pairs with tcp_inbound_ao_hash() */
> +			synchronize_rcu();

My understanding is that the above will rate-limit _only_
disconnect()/connect() sequences. Since the former op is basically evil,
I think that the extremely severe constraint on it is acceptable.

Acked-by: Paolo Abeni <pabeni@redhat.com>

> +			break;
> +		}
> +	}
> +
>  	hlist_for_each_entry_safe(key, next, &ao_info->head, node) {
>  		if (!tcp_ao_key_cmp(key, l3index, addr, key->prefixlen, family, -1, -1))
>  			continue;