All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer
@ 2026-08-16 21:28 Hyunwoo Kim
  2026-08-19 16:56 ` Simon Horman
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Hyunwoo Kim @ 2026-08-16 21:28 UTC (permalink / raw)
  To: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms, dsahern,
	0x7f454c46, fruggeri, noureddine
  Cc: netdev, imv4bel

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();
+			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;
-- 
2.43.0


^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer
  2026-08-16 21:28 [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer Hyunwoo Kim
@ 2026-08-19 16:56 ` Simon Horman
  2026-08-20 10:49 ` Paolo Abeni
  2026-08-20 19:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Simon Horman @ 2026-08-19 16:56 UTC (permalink / raw)
  To: Hyunwoo Kim
  Cc: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, dsahern,
	0x7f454c46, fruggeri, noureddine, netdev

On Mon, Aug 17, 2026 at 06:28:42AM +0900, 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>

I do have some concern about potential performance impact of using
synchronize_rcu(). But on the balance I agree that this is a good
bug-fix patch.

Reviewed-by: Simon Horman <horms@kernel.org>

^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer
  2026-08-16 21:28 [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer Hyunwoo Kim
  2026-08-19 16:56 ` Simon Horman
@ 2026-08-20 10:49 ` Paolo Abeni
  2026-08-20 19:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: Paolo Abeni @ 2026-08-20 10:49 UTC (permalink / raw)
  To: Hyunwoo Kim, edumazet, ncardwell, kuniyu, davem, kuba, horms,
	dsahern, 0x7f454c46, fruggeri, noureddine
  Cc: netdev

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;


^ permalink raw reply	[flat|nested] 4+ messages in thread

* Re: [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer
  2026-08-16 21:28 [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer Hyunwoo Kim
  2026-08-19 16:56 ` Simon Horman
  2026-08-20 10:49 ` Paolo Abeni
@ 2026-08-20 19:30 ` patchwork-bot+netdevbpf
  2 siblings, 0 replies; 4+ messages in thread
From: patchwork-bot+netdevbpf @ 2026-08-20 19:30 UTC (permalink / raw)
  To: Hyunwoo Kim
  Cc: edumazet, ncardwell, kuniyu, davem, kuba, pabeni, horms, dsahern,
	0x7f454c46, fruggeri, noureddine, netdev

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Mon, 17 Aug 2026 06:28:42 +0900 you 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.
> 
> [...]

Here is the summary with links:
  - [net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer
    https://git.kernel.org/netdev/net/c/da4471557f27

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html



^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2026-08-20 19:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-08-16 21:28 [PATCH net] net/tcp-ao: fix use-after-free of current_key on reconnect to another peer Hyunwoo Kim
2026-08-19 16:56 ` Simon Horman
2026-08-20 10:49 ` Paolo Abeni
2026-08-20 19:30 ` patchwork-bot+netdevbpf

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.