In `tcp_ao_prepare_reset()`, when an active reset is sent on a TCP-AO
socket (when `skb` is NULL, such as `tcp_send_active_reset()` during
`tcp_disconnect()`), `rnext_key` is read via
`READ_ONCE(ao_info->rnext_key)` and immediately dereferenced at
`rnext_key->rcvid` without checking whether `rnext_key` is NULL.

When a TCP-AO socket uses `TCP_FASTOPEN_CONNECT` (or disconnects before
a peer `rnext_key` is selected), `ao_info->rnext_key` can be NULL while
`ao_info->current_key` is set, causing a kernel NULL pointer dereference
in `tcp_ao_prepare_reset()`.

Return `-ENOENT` if `rnext_key` is NULL in `tcp_ao_prepare_reset()`.

Fixes: 06b22ef29591 ("net/tcp: Wire TCP-AO to request sockets")
Assisted-by: LLM
Signed-off-by: Hui Peng <benquike@gmail.com>

---
 net/ipv4/tcp_ao.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
index bb7bbc20ba3f..18aca0dd5252 100644
--- a/net/ipv4/tcp_ao.c
+++ b/net/ipv4/tcp_ao.c
@@ -857,6 +857,8 @@ int tcp_ao_prepare_reset(const struct sock *sk, struct sk_buff *skb,
 			return -ENOENT;
 		*traffic_key = snd_other_key(*key);
 		rnext_key = READ_ONCE(ao_info->rnext_key);
+		if (!rnext_key)
+			return -ENOENT;
 		*keyid = rnext_key->rcvid;
 		*sne = tcp_ao_compute_sne(READ_ONCE(ao_info->snd_sne),
 					  snd_basis, seq);
-- 
2.55.0.1082.g2b9226bbc0-goog

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

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

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] The patch closes only the RST-generation site while the same
  broken state it is built around — `ao_info` alive with `rnext_key ==…
- [High] net/tcp: silently dropping RST when rnext_key is NULL
- [Medium] The changelog describes a trigger that cannot exist.
- [Low] Concern that the fix is incomplete because peer readers of
  `ao_info->rnext_key` stay unguarded: `tcp_inbound_ao_hash()`…

--

net/tcp: check rnext_key for NULL in tcp_ao_prepare_reset()

The patch adds a NULL check on ao_info->rnext_key in the
established/time-wait branch of tcp_ao_prepare_reset() and returns -ENOENT
instead of dereferencing rnext_key->rcvid.  No Link: tags are present in
the commit header.

A few questions about the changelog and about the scope of the fix.

> In `tcp_ao_prepare_reset()`, when an active reset is sent on a TCP-AO
> socket (when `skb` is NULL, such as `tcp_send_active_reset()` during
> `tcp_disconnect()`), `rnext_key` is read via
> `READ_ONCE(ao_info->rnext_key)` and immediately dereferenced at
> `rnext_key->rcvid` without checking whether `rnext_key` is NULL.

[Severity: Medium]
Can tcp_ao_prepare_reset() actually be called with skb == NULL?

There are only two callers, tcp_v4_ao_sign_reset() and
tcp_v6_send_reset(), and both are driven by a received segment: they
dereference tcp_hdr(skb), ip_hdr(skb)/ipv6_hdr(skb) and parse aoh out of
that segment before calling in.

Also, the very first statement of the function dereferences skb:

net/ipv4/tcp_ao.c:tcp_ao_prepare_reset() {
	const struct tcphdr *th = tcp_hdr(skb);
	...
	*key = tcp_ao_established_key(sk, ao_info, aoh->rnext_keyid, -1);

so a NULL skb would oops at tcp_hdr(skb), and the else branch also reads
aoh->rnext_keyid before reaching the newly added check.  Would the new
check help at all in a NULL-skb scenario?

For the active reset path, tcp_send_active_reset() does not call this
helper; it emits AO options via tcp_options_write() ->
process_tcp_ao_options(), which already handles a NULL rnext_key:

net/ipv4/tcp_output.c:process_tcp_ao_options() {
	rnext_key = READ_ONCE(ao_info->rnext_key);
	if (WARN_ON_ONCE(!rnext_key))
		return ptr;

And tcp_disconnect() on a deferred-connect socket does not send an active
reset either: tcp_need_reset() covers only ESTABLISHED, CLOSE_WAIT,
FIN_WAIT1, FIN_WAIT2 and SYN_RECV, while TCP_SYN_SENT takes the
WRITE_ONCE(sk->sk_err, ECONNRESET) branch.

The state that does look reachable is different: TCP_AO_ADD_KEY issued
while the socket is TCP_CLOSE leaves both pointers unset, because
tcp_ao_add_cmd() only assigns them for other states:

net/ipv4/tcp_ao.c:tcp_ao_add_cmd() {
	if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) {
		tcp_ao_cache_traffic_keys(sk, ao_info, key);
		if (first) {
			ao_info->current_key = key;
			ao_info->rnext_key = key;

combined with TCP_FASTOPEN_CONNECT, where tcp_v4_connect() returns at
tcp_fastopen_defer_connect() and never runs tcp_ao_connect_init().  Could
the changelog describe that sequence instead, so the Fixes: tag and any
stable backport can be validated against something reproducible?

> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index bb7bbc20ba3f..18aca0dd5252 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -857,6 +857,8 @@ int tcp_ao_prepare_reset(const struct sock *sk, struct sk_buff *skb,
>  			return -ENOENT;
>  		*traffic_key = snd_other_key(*key);
>  		rnext_key = READ_ONCE(ao_info->rnext_key);
> +		if (!rnext_key)
> +			return -ENOENT;

[Severity: High]
Returning -ENOENT here does not only skip the *keyid assignment, it
suppresses the RST entirely.  Both callers treat any non-zero return
from tcp_ao_prepare_reset() as "give up on this segment":

net/ipv4/tcp_ipv4.c:tcp_v4_send_reset() {
		if (tcp_ao_prepare_reset(sk, skb, aoh, l3index, seq,
					 &key.ao_key, &key.traffic_key,
					 &allocated_traffic_key,
					 &key.rcv_next, &key.sne))
			goto out;

and tcp_v6_send_reset() does the same, so the outgoing RST is dropped
silently: no MIB counter, no drop reason, nothing in the tcp_ao_counters
that an operator could look at.  The remote peer is left holding a
half-open connection until its own keepalive/retransmit timers expire.
For a fix whose stated purpose is to make a reachable configuration not
crash, trading an oops for an unobservable protocol stall on every reset
attempt in that state seems worth spelling out in the changelog at
least.

Is the error return actually the behaviour you want here?  Note that the
listen/TCP_NEW_SYN_RECV branch a few lines above solves the same problem
differently: it never consults ao_info->rnext_key and instead uses

	*keyid = (*key)->rcvid;

where *key is the key just selected from the incoming segment's
aoh->rnext_keyid.  In the established/time-wait branch *key has already
been resolved by tcp_ao_established_key(sk, ao_info, aoh->rnext_keyid,
-1), so (*key)->rcvid is a valid receive ID for this connection and would
let the RST go out properly authenticated, as RFC 5925 requires, rather
than not at all.  Would falling back to (*key)->rcvid when
ao_info->rnext_key is NULL be preferable to bailing out, and is there a
reason the two branches should disagree about where the RNext KeyID of
the outgoing segment comes from?

If you do conclude that the segment genuinely cannot be signed in this
state and must be dropped, could the drop at least be made visible --
something along the lines of the existing TCP-AO counters or a
SKB_DROP_REASON -- so that a silently missing RST is diagnosable in the
field?

[Severity: High]
If the socket state being guarded against here is "ao_info alive with
rnext_key == NULL", does the transmit path still write a MAC through an
uninitialized stack pointer for that same state?

The state is user-constructible.  TCP_AO_ADD_KEY on a TCP_CLOSE socket
skips the current/rnext assignment in tcp_ao_add_cmd():

net/ipv4/tcp_ao.c:tcp_ao_add_cmd() {
	if (!((1 << sk->sk_state) & (TCPF_LISTEN | TCPF_CLOSE))) {
		...
		if (first) {
			ao_info->current_key = key;
			ao_info->rnext_key = key;

and set_current without set_rnext is accepted, so current_key != NULL with
rnext_key == NULL is possible.  With TCP_FASTOPEN_CONNECT and a cached
cookie, tcp_v4_connect() returns before tcp_connect():

net/ipv4/tcp_ipv4.c:tcp_v4_connect() {
	if (tcp_fastopen_defer_connect(sk, &err))
		return err;

so tcp_connect_init() -> tcp_ao_connect_init() never runs and the NULL
rnext_key survives on a hashed, transmit-capable TCP_SYN_SENT socket.

An AO-authenticated bare SYN for that 4-tuple then reaches the
simultaneous-open branch of tcp_rcv_synsent_state_process(), which calls
tcp_send_synack(sk).  On the way out, hash_location is deliberately not
cleared:

net/ipv4/tcp_output.c:tcp_out_options {
	/* Caution: following fields are not cleared in __tcp_transmit_skb() */
	...
	__u8 *hash_location;	/* temporary pointer, overloaded */

and __tcp_transmit_skb() declares struct tcp_out_options opts; with no
initializer, memset-ing only opts.cleared.  tcp_get_current_key() sets
type = TCP_KEY_AO whenever ao_info exists, tcp_syn_options() reserves the
AO option space, then process_tcp_ao_options() returns early:

net/ipv4/tcp_output.c:process_tcp_ao_options() {
	rnext_key = READ_ONCE(ao_info->rnext_key);
	if (WARN_ON_ONCE(!rnext_key))
		return ptr;
	...
	opts->hash_location = (__u8 *)ptr;

so opts->hash_location is never assigned, yet it is still handed to the
MAC writer:

net/ipv4/tcp_output.c:__tcp_transmit_skb() {
	} else if (tcp_key_is_ao(&key)) {
		tcp_ao_transmit_skb(sk, skb, key.ao_key, th,
				    opts.hash_location);

net/ipv4/tcp_ao.c:tcp_ao_transmit_skb() {
	tp->af_specific->calc_ao_hash(hash_location, key, sk, skb, traffic_key,
				      hash_location - (u8 *)th, sne);

Can this write the MAC through stale stack contents at a garbage offset,
and can the reserved-but-unwritten AO option bytes leak uninitialized skb
head data onto the wire?

There is also a sibling state: a key added on a closed socket with neither
flag leaves current_key == NULL while tcp_get_current_key() still reports
TCP_KEY_AO:

include/net/tcp.h:tcp_get_current_key() {
		out->ao_key = READ_ONCE(ao->current_key);
		out->type = TCP_KEY_AO;

which makes tcp_syn_options()/tcp_established_options() dereference
key->ao_key inside tcp_ao_len_aligned().  Would it be better to repair the
invariant at the source, for example by running the AO connect fixup for
the deferred-connect case, refusing that configuration, or failing the
transmit, rather than adding a check at one consumer?

[Severity: Low]
On the receive and time-wait sides, the other readers of
ao_info->rnext_key remain unguarded, for example:

net/ipv4/tcp_ao.c:tcp_inbound_ao_hash() {
		key = READ_ONCE(info->rnext_key);
		if (key->rcvid != aoh->keyid) {

net/ipv4/tcp_ipv4.c:tcp_v4_timewait_ack() {
		rnext_key = READ_ONCE(ao_info->rnext_key);
		key.rcv_next = rnext_key->rcvid;

These look unreachable with a NULL rnext_key today: the
tcp_inbound_ao_hash() read sits behind the TCP_AO_ESTABLISHED state mask,
which excludes TCPF_SYN_SENT, TCPF_SYN_RECV, TCPF_CLOSE and
TCPF_TIME_WAIT, and the only writer that can publish NULL outside a
socket-locked window in tcp_ao_connect_init() is the del_async path, which
tcp_ao_del_cmd() restricts to TCP_LISTEN:

net/ipv4/tcp_ao.c:tcp_ao_del_cmd() {
	if (cmd.del_async && sk->sk_state != TCP_LISTEN)
		return -EINVAL;

Is that the intended invariant, i.e. that rnext_key is only ever NULL
before a connection becomes established, and if so should the comment in
struct tcp_ao_info say so, given the new check implies otherwise?

>  		*keyid = rnext_key->rcvid;
>  		*sne = tcp_ao_compute_sne(READ_ONCE(ao_info->snd_sne),
>  					  snd_basis, seq);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260919213630.3313360-1-benquike%40gmail.com