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

[netdev-bot+sashiko]

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?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260925-psp-defeat-v1-0-9f0b430107aa%40gmail.com