Re: [PATCH net] net/tcp-ao: transfer tcp_ao_info to the TIME_WAIT socket

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] tcp_ao_time_wait() (net/ipv4/tcp_ao.c:431) now unconditionally
  clears tp->ao_info, but two of the three callers of tcp_time_wait()…

--

net/tcp-ao: transfer tcp_ao_info to the TIME_WAIT socket

tcp_ao_time_wait() used to hand the TIME_WAIT socket a second reference to
the full socket's tcp_ao_info while leaving tp->ao_info pointing at the same
object. This patch turns that into a handover by clearing tp->ao_info so the
TIME_WAIT socket becomes the sole owner.

Closes: https://lore.kernel.org/all/YG9s0PiBKJZcXAKld3MToa1IVRJOUoKiaA57%40bugs.sh/

> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index bb7bbc20ba3fa..27525f90398b3 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -428,7 +428,7 @@ void tcp_ao_time_wait(struct tcp_timewait_sock *tcptw, struct tcp_sock *tp)
>  			omem += tcp_ao_sizeof_key(key);
>  		}
>  
> -		refcount_inc(&ao_info->refcnt);
> +		rcu_assign_pointer(tp->ao_info, NULL);

[Severity: High]

Is the clear here safe on the paths where the full socket survives?  The
commit message says this transition happens when "the full socket is going
away", but tcp_ao_time_wait() is reached from tcp_time_wait() which ends in
tcp_done(), and tcp_done() only destroys the socket when SOCK_DEAD is set:

net/ipv4/tcp.c:tcp_done() {
	...
	if (!sock_flag(sk, SOCK_DEAD))
		sk->sk_state_change(sk);
	else
		inet_csk_destroy_sock(sk);
}

The same FIN_WAIT2 path the changelog describes is one such caller:

net/ipv4/tcp_input.c:tcp_fin() {
	...
	case TCP_FIN_WAIT2:
		/* Received a FIN -- send ACK and enter TIME_WAIT. */
		tcp_send_ack(sk);
		tcp_time_wait(sk, TCP_TIME_WAIT, 0);
}

So after shutdown(fd, SHUT_WR) with the fd still open, the peer's FIN drives
tcp_fin() -> tcp_time_wait() -> tcp_ao_time_wait(), and the full socket is
left alive in TCP_CLOSE with tp->ao_info NULL.

Does that drop the whole AO association from a socket the application can
still use?  ao_info holds all the MKTs plus current_key, rnext_key and
ao_required, and nothing appears to rebuild it: tcp_disconnect() never
touches ao_info, and tcp_ao_connect_init() bails out immediately:

net/ipv4/tcp_ao.c:tcp_ao_connect_init() {
	ao_info = rcu_dereference_protected(tp->ao_info,
					    lockdep_sock_is_held(sk));
	if (!ao_info)
		return;
	...
}

Can a subsequent connect() on that fd then establish an unauthenticated
connection with no error?  The guard in tcp_connect() is keyed off
tp->ao_info, so it becomes a no-op once ao_info is NULL:

net/ipv4/tcp_output.c:tcp_connect() {
	...
	if (unlikely(rcu_dereference_protected(tp->ao_info, ...))) {
		/* Don't allow connecting if ao is configured but no
		 * matching key is found.
		 */
		if (!tp->af_specific->ao_lookup(sk, sk, -1, -1))
			return -EKEYREJECTED;
	...
}

The md5/ao arbitration and ao_required enforcement just above it in
tcp_connect() are keyed off the same pointer.

Similarly, if the application calls listen() on the surviving fd, does
tcp_ao_required() now report that no authentication is needed?

include/net/tcp.h:tcp_ao_required() {
	ao_info = rcu_dereference_check(tcp_sk(sk)->ao_info, ...);
	if (!ao_info)
		return false;
	...
}

Since the transition is driven by the peer's FIN, the remote side chooses
when the local socket's AO configuration is discarded, and no privilege is
needed on the local side.

Would copying the state into the twsk work better here, matching what the
MD5 sibling does in the same transition?

net/ipv4/tcp_minisocks.c:tcp_time_wait_init() {
	...
		tcptw->tw_md5_key = kmemdup(key, sizeof(*key), GFP_ATOMIC);
	...
}

That keeps the surviving full socket authenticated.  Alternatively, clearing
tp->ao_info only when the socket is really being destroyed, or adding
explicit AO teardown in tcp_disconnect(), would avoid the AO-less-but-usable
socket.

One more note on the changelog: it states that such a socket "no longer
exposes TCP-AO state (TCP_AO_INFO and TCP_AO_DEL_KEY return -ENOENT)".
TCP_AO_GET_KEYS and TCP_AO_REPAIR also start returning -ENOENT, and the loss
of the key material and ao_required on a still-usable socket isn't mentioned
at all.  Could the changelog spell out that consequence?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260904005851.149071-1-xmei5%40asu.edu