Re: [PATCH net-next 2/4] net: psp: require an established connection for association setup

["Daniel Zahka" <daniel.zahka@gmail.com>]

On Sat Sep 26, 2026 at 9:31 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 1 · Medium: 0 · Low: 2
>
> 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 new `sk->sk_state != TCP_ESTABLISHED` check in
>   psp_sock_assoc_set_tx() (net/psp/psp_sock.c:261-265) does not close…
> - [Low] The commit message says the tx-assoc check 'fixes a bug in commit
>   6b46ca260e22 ("net: psp: add socket security association code")', but…
> - [Low] Baseline decision: false positive, kept only because
>   requires_validation=true.
>
> --
>
> net: psp: require an established connection for association setup
>
> With this commit, rx-assoc and tx-assoc return -ENOTCONN unless the socket
> is in TCP_ESTABLISHED when checked under the socket lock. It also documents
> that the upgrade to PSP must happen on an established connection, and that
> disconnecting a socket with PSP assoc state is unsupported. The commit
> message describes the tx-assoc check as a fix for an unsynchronized write
> to an assoc shared with a timewait socket.
>
>> The check in the tx-assoc path fixes a bug in commit 6b46ca260e22 ("net:
>> psp: add socket security association code") where an unsynchronized
>> write can be performed an assoc shared with a timewait socket when the
>> socket is in TCP_CLOSE after shutdown. This commit is not included in
>> net, because its premise of preventing listen sockets from holding assoc
>> state depends on the net-next commit 8cc3aef0cb19 ("tcp: Do not allow
>> buggy transitions between ehash and lhash2.")
>
> [Severity: Low]
> This is described as a fix for 6b46ca260e22, so should it carry a Fixes:
> tag? Documentation/process/maintainer-netdev.rst says the Fixes: tag is
> required for fixes regardless of the tree.
>
> The dependency on 8cc3aef0cb19 explains why this goes to net-next. That
> dependency is about listen sockets, though. The tx-assoc case of TCP_CLOSE
> after shutdown does not seem to involve ehash/lhash2 transitions.
>
>> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
>> index a9cfeebe4ba1b..a6b1c42dd626f 100644
>> --- a/net/psp/psp_sock.c
>> +++ b/net/psp/psp_sock.c
>
> [ ... ]
>
>> @@ -252,6 +258,12 @@ int psp_sock_assoc_set_tx(struct sock *sk, struct psp_dev *psd,
>>  
>>  	lock_sock(sk);
>>  
>> +	if (sk->sk_state != TCP_ESTABLISHED) {
>> +		NL_SET_ERR_MSG(extack, "Socket must be in established state");
>> +		err = -ENOTCONN;
>> +		goto exit_unlock;
>> +	}
>> +
>
> [Severity: High]
> Does this check fully close the timewait sharing problem that the commit
> message describes? A socket can return to TCP_ESTABLISHED after it has
> already shared its psp_assoc with a timewait socket.
>
> psp_twsk_init() shares the assoc unconditionally, even when only the Rx
> key is installed:
>
> net/psp/psp_sock.c:psp_twsk_init() {
> 	struct psp_assoc *pas = psp_sk_assoc(sk);
>
> 	if (pas)
> 		refcount_inc(&pas->refcnt);
> 	rcu_assign_pointer(tw->psp_assoc, pas);
> 	...
> }
>
> tcp_disconnect() does not touch sk->psp_assoc. The new psp.rst text says
> the same thing: "Disconnect on a socket after rx-assoc will leave a socket
> that can be reconnected, but with potentially stale PSP assoc state
> present".
>
> rx-assoc and tx-assoc do not need admin permission, so this sequence looks
> reachable:
>
>   connect(fd, peer1)
>   rx-assoc                       pas->tx.spi == 0
>   shutdown(fd, SHUT_WR)
>     peer FIN in FIN_WAIT2
>     tcp_time_wait()
>       inet_twsk_alloc()
>         psp_twsk_init()          tw->psp_assoc = pas
>     tcp_done()                   sk_state = TCP_CLOSE
>   connect(fd, AF_UNSPEC)
>     tcp_disconnect()             sk->psp_assoc left in place
>   connect(fd, peer2)             cleartext SYN/SYN-ACK allowed, !tx.spi
>                                  sk_state = TCP_ESTABLISHED
>   tx-assoc                       passes the new check
>     psp_sock_set_tx_key()
>       psp_assoc_set_tx()
>         memcpy(&pas->tx, key, sizeof(*key));
>       pas->upgrade_seq = tcp_sk(sk)->rcv_nxt;
>
> Only the new connection's socket lock is held for these writes. The old
> timewait socket is still hashed, and packets from peer1 go through:
>
> tcp_timewait_state_process()
>   psp_twsk_rx_policy_check()
>     __psp_sk_rx_policy_check()
>
> That path reads pas->tx.spi and pas->upgrade_seq and writes pas->peer_tx,
> with only RCU protection:
>
> 	if (!pse) {
> 		if (!pas->tx.spi ||
> 		    (!pas->peer_tx && psp_is_allowed_nondata(skb, pas)))
> 			return 0;
> 	}
>
> Apart from the data race and possible torn reads, does the behavior of the
> timewait socket also change? Once tx.spi is set for peer2, the timewait
> socket would start dropping peer1's legitimate cleartext FIN retransmits
> with SKB_DROP_REASON_PSP_INPUT.
>
> On the timewait reply path, psp_reply_set_decrypted() would also see
> pas->tx.spi != 0 and mark the ACK/RST replies to peer1 as decrypted.
> psp_validate_xmit() then resolves tw->psp_assoc to the same pas, so those
> old-flow replies would be PSP encapsulated with peer2's Tx key and SPI.
>
> The new documentation calls disconnect unsupported, but nothing prevents
> it. Would the fix need to check whether the assoc is actually shared, or
> reset or refuse assoc state across disconnect, instead of relying only on
> a socket state check?

Ok, I will need to rethink the approach a bit.

pw-bot: cr