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

[netdev-bot+sashiko@kernel.org]

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?

> +
>  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?

>  	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.

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

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260930-psp-defeat-v2-0-f266e7447129%40gmail.com