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

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

On Thu Oct 1, 2026 at 10:47 PM EDT, netdev-bot+sashiko wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential
> issue(s) to consider.
>
> Critical: 0 · High: 0 · Medium: 1 · 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).
>
> - [Medium] The new check in psp_sock_assoc_set_rx()
>   (net/psp/psp_sock.c:162) rejects every state other than…
> - [Low] The commit message says the new sk_state check in
>   psp_sock_assoc_set_tx() (net/psp/psp_sock.c:261) fixes a bug from…
> - [Low] The new psp.rst text (lines 139-141) says "connect() on a socket
>   that has PSP assoc state will return -EINVAL".
>
> --
>
> net: psp: require an established connection for association setup
>
> This commit makes the PSP rx-assoc and tx-assoc handlers reject sockets
> that are not in TCP_ESTABLISHED. It also makes tcp_v4_connect() and
> tcp_v6_connect() return -EINVAL when PSP assoc state already exists, and
> drops the PSP overhead from icsk_ext_hdr_len in the connect paths.
>
>> diff --git a/Documentation/networking/psp.rst b/Documentation/networking/psp.rst
>> index 4ac09e64e95a5..0f9b6b73f2447 100644
>> --- a/Documentation/networking/psp.rst
>> +++ b/Documentation/networking/psp.rst
>> @@ -132,6 +132,14 @@ numbers in a way that deletes a prefix of the PSP protected part of
>>  the TCP stream. If userspace cares to mitigate this type of attack, a
>>  special "start of PSP" message should be exchanged after ``tx-assoc``.
>>  
>> +Upgrade to PSP must be done on established TCP connections.
>> +``rx-assoc`` and ``tx-assoc`` will return ``-ENOTCONN`` if
>> +``sk_state`` is not ``TCP_ESTABLISHED``.
>> +
>> +The PSP assoc state of a socket is not reset when the connection is
>> +torn down. ``connect()`` on a socket that has PSP assoc state will
>> +return ``-EINVAL``.
>
> [Severity: Low]
> Is the -EINVAL statement accurate for most connect() calls on a socket
> with assoc state?
>
> rx-assoc and tx-assoc now require TCP_ESTABLISHED, so a socket with
> assoc state has sock->state == SS_CONNECTED. __inet_stream_connect()
> returns before it gets to sk_prot->connect():
>
> net/ipv4/af_inet.c:__inet_stream_connect() {
>     ...
> 	switch (sock->state) {
> 	default:
> 		err = -EINVAL;
> 		goto out;
> 	case SS_CONNECTED:
> 		err = -EISCONN;
> 		goto out;
>     ...
> }
>
> FIN or RST teardown leaves sock->state at SS_CONNECTED, and
> psp_sk_assoc_free() only runs from inet_sock_destruct(). That means the
> new psp_sk_assoc() checks in tcp_v4_connect() and tcp_v6_connect() can
> only be reached after an explicit connect(AF_UNSPEC) disconnect. Every
> other case returns -EISCONN.
>
> Should the documentation say that -EINVAL applies to reconnect attempts
> after an AF_UNSPEC disconnect?
>

I suppose I was thinking more along the lines of the AF_UNSPEC, because
otherwise, the -EISCONN is the same as any non-PSP socket. I could
change if a v3 is needed.

>> +
>>  Rotation notifications
>>  ----------------------
>>  
>
> [ ... ]
>
>> 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
>> @@ -159,6 +159,12 @@ int psp_sock_assoc_set_rx(struct sock *sk, struct psp_assoc *pas,
>>  
>>  	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: Medium]
> This check in psp_sock_assoc_set_rx() rejects every state except
> TCP_ESTABLISHED. The commit message only justifies this with closed and
> listen sockets. Does it also remove cases that could have users?
>
> A TCP Fast Open server child returned by accept() can still be in
> TCP_SYN_RECV. It already has a dst from tcp_v4_syn_recv_sock() or
> tcp_v6_syn_recv_sock(), and it is the same socket that later becomes
> ESTABLISHED. The point that the child does not inherit PSP state doesn't
> apply to it. On that socket, rx-assoc now succeeds or fails depending on
> when the final ACK arrives.
>
> A TCP_FASTOPEN_CONNECT client also returns from connect() while it is
> still in TCP_SYN_SENT.
>
> The commit message also says:
>
>   "establishing PSP state prior to connect() will result in a
>    PSP-encrypted TCP SYN sent to a listening peer"
>
> Is this true when only the Rx key is installed? The SYN is built by
> tcp_init_nondata_skb(), which calls psp_enqueue_set_decrypted():
>
> include/net/psp/functions.h:psp_enqueue_set_decrypted() {
> 	...
> 	pas = psp_sk_assoc(sk);
> 	if (pas && pas->tx.spi)
> 		skb->decrypted = 1;
> }
>
> With only an Rx key, the SYN goes out in plaintext.
> __psp_sk_rx_policy_check() accepts plaintext as long as no Tx key is
> set:
>
> 	if (!pse) {
> 		if (!pas->tx.spi ||
> 		    (!pas->peer_tx && psp_is_allowed_nondata(skb, pas)))
> 			return 0;
> 	}
>
> So the handshake completes normally, and tx-assoc can follow once the
> connection is ESTABLISHED. With an explicit dev-id,
> psp_assoc_device_get_locked() allows rx-assoc on a socket that has not
> called connect() yet.
>
> Before this patch, setting up only the Rx key before connect() appeared
> to work. Should the commit message and psp.rst say that this sequence
> and the TFO cases are being removed as well?
>

Not supporting TFO in this way is by design. I considered that rx assoc
could be allowed as long as sk_state is not TCP_CLOSE or TCP_LISTEN, but
that seemed more complicated than needed. For the upgrade model,
requiring both sides to just be ESTABLISHED seems simplest.

>>  	if (psp_sk_assoc(sk)) {
>>  		NL_SET_ERR_MSG(extack, "Socket already has PSP state");
>>  		err = -EBUSY;
>> @@ -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: Low]
> The commit message says this check fixes a bug from commit 6b46ca260e22
> ("net: psp: add socket security association code"), but the patch has
> no Fixes: tag. Could the tx-assoc check be split out and sent to net
> with a Fixes: tag?
>
> After tcp_time_wait(), psp_twsk_init() makes the timewait socket share
> the same assoc:
>
> net/psp/psp_sock.c:psp_twsk_init() {
> 	...
> 	if (pas)
> 		refcount_inc(&pas->refcnt);
> 	rcu_assign_pointer(tw->psp_assoc, pas);
> 	...
> }
>
> The original sk is then in TCP_CLOSE with sk->psp_assoc still set. If
> only the Rx key was installed, the old psp_sock_assoc_set_tx() accepted
> the request. psp_sock_set_tx_key() then wrote pas->tx, pas->upgrade_seq
> and pas->drv_data. Meanwhile the timewait socket reads pas->tx.spi
> without locks from softirq, in psp_twsk_rx_policy_check() and
> psp_reply_set_decrypted().
>
> The commit message says the patch targets net-next because it depends on
> commit 8cc3aef0cb19 ("tcp: Do not allow buggy transitions between ehash
> and lhash2."). That dependency only matters for the rx-assoc and listen
> socket part. The tx-assoc state check works without it.
>
> As posted, stable kernels with PSP would keep this race, and backporters
> would have nothing pointing them to the fix.
>

The tx-assoc and connect() bits could be split out into a dedicated
patch for net. Tests in psp.py will break without the first commit in
this series.

Alternatively, the whole commit can be taken without
splitting, but the changelog may need to be changed given that it over
promises on defeaturing listen without the dependencies on
unhashed_state

>>  	pas = psp_sk_assoc(sk);
>>  	if (!pas) {
>>  		NL_SET_ERR_MSG(extack, "Socket has no Rx key");