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

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

[Daniel Zahka]

This series removes support for using PSP's assoc uapi on TCP sockets
not in established state.

The PSP uapi and connection upgrade described in psp.rst is only fleshed
out for established TCP connections. Installing PSP assoc state on a
listen socket, or closed socket ahead of connect() is possible, but not
something that results in useful outcomes.

With commit 8cc3aef0cb19 ("tcp: Do not allow buggy transitions between
ehash and lhash2.") in place, this series makes it impossible to have
PSP assoc state on a listen socket. It is still possible to have a
closed socket with assoc state that connect() can be used on due to
tcp_disconnect() not clearing PSP state. Patch two updates psp.rst to
discuss what this means for users.

The first patch converts some tests that used TCP_CLOSE sockets for
basic uapi tests with connected sockets, so that the subsequent commit
doesn't break them.

The second patch introduces the actual checks on sk->sk_state during the
rx and tx assoc handlers. The commit message contains my argument for
why removing these "features" is appropriate and doesn't constitute a
fix.

The third patch unwinds commit 1d2929d0850f ("net: psp: do not inherit
the Rx association on clone"), which was introduced to workaround assoc
state not being handled correctly from listen sockets.

The fourth patch has some tests for the new checks introduced.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
Daniel Zahka (4):
      selftests: drv-net: psp: swap closed for connected sockets in assoc tests
      net: psp: require an established connection for association setup
      net: psp: drop psp assoc clear in sk_clone()
      selftests: drv-net: psp: test that assocs require an established socket

 Documentation/networking/psp.rst           | 13 ++++++
 net/core/sock.c                            |  2 +-
 net/psp/psp_sock.c                         | 12 +++++
 tools/testing/selftests/drivers/net/psp.py | 73 +++++++++++++++++++++++++-----
 4 files changed, 88 insertions(+), 12 deletions(-)
---
base-commit: 4a0f98a164f7d049f337a1a17579f402707a460e
change-id: 20260916-psp-defeat-a271649be9dc

Best regards,
-- 
Daniel Zahka <daniel.zahka@gmail.com>

[PATCH net-next 1/4] selftests: drv-net: psp: swap closed for connected sockets in assoc tests

[Daniel Zahka]

Future work will only allow rx-assoc and tx-assoc to be performed when
the sock is in TCP_ESTABLISHED state.

Several assoc_ tests, as well as dev_rotate_spi, test using the rx-assoc
and tx-assoc uapi calls against sockets in TCP_CLOSE state.
These tests don't involve sending or receiving data, nor involve looking
up psp device by dst entry, so using a disposable disconnected socket
was just a convenience. These can be replaced by a disposable loopback
socket.

Some users of rx-assoc and tx-assoc on closed sockets are left, if they
validate errors that are returned before the kernel will check the
socket for TCP_ESTABLISHED.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 tools/testing/selftests/drivers/net/psp.py | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index 5a81f40cac7d..0a2329f41431 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -11,6 +11,8 @@ import struct
 import termios
 import time
 
+from contextlib import contextmanager
+
 from lib.py import defer
 from lib.py import ksft_run, ksft_exit, ksft_pr
 from lib.py import ksft_true, ksft_eq, ksft_ne, ksft_gt, ksft_raises
@@ -58,6 +60,17 @@ def _make_psp_conn(cfg, version=0, ipver=None):
     return s
 
 
+@contextmanager
+def _make_lo_conn():
+    # After tx-assoc, the client's egress is dropped, since lo has no
+    # psp_dev, so its FIN never reaches the server. Closing the server
+    # resets the unaccepted child, and the client accepts the cleartext
+    # RST because it hasn't received any PSP traffic yet.
+    with socket.create_server(("localhost", 0)) as srv, \
+         socket.create_connection(srv.getsockname()[:2]) as s:
+        yield s
+
+
 def _close_conn(cfg, s):
     _send_with_ack(cfg, b'data close\0')
     s.close()
@@ -200,20 +213,18 @@ def dev_rotate_spi(cfg):
     _init_psp_dev(cfg)
 
     top_a = top_b = 0
-    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+    with _make_lo_conn() as s:
         assoc_a = cfg.pspnl.rx_assoc({"version": 0,
                                      "dev-id": cfg.psp_dev_id,
                                      "sock-fd": s.fileno()})
         top_a = assoc_a['rx-key']['spi'] >> 31
-        s.close()
     rot = cfg.pspnl.key_rotate({"id": cfg.psp_dev_id})
-    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+    with _make_lo_conn() as s:
         ksft_eq(rot['id'], cfg.psp_dev_id)
         assoc_b = cfg.pspnl.rx_assoc({"version": 0,
                                     "dev-id": cfg.psp_dev_id,
                                     "sock-fd": s.fileno()})
         top_b = assoc_b['rx-key']['spi'] >> 31
-        s.close()
     ksft_ne(top_a, top_b)
 
 
@@ -221,7 +232,7 @@ def assoc_basic(cfg):
     """ Test creating associations """
     _init_psp_dev(cfg)
 
-    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+    with _make_lo_conn() as s:
         assoc = cfg.pspnl.rx_assoc({"version": 0,
                                   "dev-id": cfg.psp_dev_id,
                                   "sock-fd": s.fileno()})
@@ -234,7 +245,6 @@ def assoc_basic(cfg):
                                   "tx-key": assoc['rx-key'],
                                   "sock-fd": s.fileno()})
         ksft_eq(len(assoc), 0)
-        s.close()
 
 
 def assoc_bad_dev(cfg):
@@ -320,7 +330,7 @@ def assoc_version_mismatch(cfg):
     # Translate versions to integers
     versions = [cfg.pspnl.consts["version"].entries[v].value for v in versions]
 
-    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+    with _make_lo_conn() as s:
         rx = cfg.pspnl.rx_assoc({"version": versions[0],
                                  "dev-id": cfg.psp_dev_id,
                                  "sock-fd": s.fileno()})
@@ -393,7 +403,7 @@ def assoc_twice(cfg):
 
         return assoc
 
-    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+    with _make_lo_conn() as s:
         assoc = rx_assoc_check(s)
         tx = cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
                                "version": 0,
@@ -402,7 +412,7 @@ def assoc_twice(cfg):
         ksft_eq(len(tx), 0)
 
         # Use the same Tx assoc second time
-        with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s2:
+        with _make_lo_conn() as s2:
             rx_assoc_check(s2)
             tx = cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
                                    "version": 0,
@@ -410,8 +420,6 @@ def assoc_twice(cfg):
                                    "sock-fd": s2.fileno()})
             ksft_eq(len(tx), 0)
 
-        s.close()
-
 
 def _data_basic_send(cfg, version, ipver):
     """ Test basic data send """

-- 
2.52.0

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

[Daniel Zahka]

Return -ENOTCONN if sk_state is not TCP_ESTABLISHED when read under the
socket lock in the rx and tx assoc paths.

Nothing useful can be done after association setup on closed or listen
sockets today. Listen sockets could accept a PSP encrypted TCP SYN, but
the child socket will not inherit any PSP state. On the other side,
establishing PSP state prior to connect() will result in a PSP encrypted
TCP SYN sent to a listening peer that will in turn have the
aforementioned limitations. That implies that there cannot be any users
of this feature, and thus it should be safe to remove doing so as a
feature of the PSP uapi.

These can be reintroduced when there is a use case and a design that
takes into account all of the socket states that are reachable by
relaxing this constraint.

In theory, the check in the tx-assoc path is more restrictive than
necessary. FIN_WAIT1/2, CLOSING, LAST_ACK, CLOSE_WAIT could be allowed
and the peer would accept PSP encrypted ACKs in the post FIN sent
states, or data in the half close case, but for now we can just document
that connection upgrade protocol should avoid these.

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

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 Documentation/networking/psp.rst | 13 +++++++++++++
 net/psp/psp_sock.c               | 12 ++++++++++++
 2 files changed, 25 insertions(+)

diff --git a/Documentation/networking/psp.rst b/Documentation/networking/psp.rst
index 4ac09e64e95a..2aa971e16d95 100644
--- a/Documentation/networking/psp.rst
+++ b/Documentation/networking/psp.rst
@@ -132,6 +132,19 @@ 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``.
+
+Disconnecting a socket with PSP assoc state (``connect()`` with a
+family of ``AF_UNSPEC``) will succeed, but should be considered
+unsupported. Disconnect does not reset the PSP assoc state of a
+socket to avoid potential for clear text leak. Disconnect on a socket
+after ``rx-assoc`` will leave a socket that can be reconnected, but
+with potentially stale PSP assoc state present and a reduced MSS.
+Disconnect after ``tx-assoc`` will likely result in a dead socket, as
+the subsequent ``connect()`` will send a PSP encapsulated SYN.
+
 Rotation notifications
 ----------------------
 
diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
index a9cfeebe4ba1..a6b1c42dd626 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;
+	}
+
 	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;
+	}
+
 	pas = psp_sk_assoc(sk);
 	if (!pas) {
 		NL_SET_ERR_MSG(extack, "Socket has no Rx key");

-- 
2.52.0

[PATCH net-next 3/4] net: psp: drop psp assoc clear in sk_clone()

[Daniel Zahka]

Unwind the change from commit 1d2929d0850f ("net: psp: do not inherit
the Rx association on clone"), now that assoc state cannot exist on
listen sockets.

With commit 8cc3aef0cb19 ("tcp: Do not allow buggy transitions between
ehash and lhash2."), and the change in this series restricting PSP assoc
operations to established sockets, this state should be unreachable.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 net/core/sock.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/core/sock.c b/net/core/sock.c
index 2948dffcc3e1..d7ddd365abeb 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -2496,7 +2496,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
 	RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
 #endif
 #if IS_ENABLED(CONFIG_INET_PSP)
-	RCU_INIT_POINTER(newsk->psp_assoc, NULL);
+	DEBUG_NET_WARN_ON_ONCE(rcu_access_pointer(sk->psp_assoc));
 #endif
 
 	/* SANITY */

-- 
2.52.0

[PATCH net-next 4/4] selftests: drv-net: psp: test that assocs require an established socket

[Daniel Zahka]

Cover the three ways a PSP association could previously be installed on a
socket with no connection behind it: an unconnected socket, a listener,
and a socket whose connection has already started to go away.

Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---
 tools/testing/selftests/drivers/net/psp.py | 43 ++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/tools/testing/selftests/drivers/net/psp.py b/tools/testing/selftests/drivers/net/psp.py
index 0a2329f41431..6b761c981c6b 100755
--- a/tools/testing/selftests/drivers/net/psp.py
+++ b/tools/testing/selftests/drivers/net/psp.py
@@ -319,6 +319,49 @@ def assoc_sk_only_unconn(cfg):
         ksft_eq(the_exception.nl_msg.error, -errno.EINVAL)
 
 
+def assoc_rx_unconnected(cfg):
+    """ Test that an Rx assoc is rejected on an unconnected socket """
+    _init_psp_dev(cfg)
+
+    with socket.socket(socket.AF_INET6, socket.SOCK_STREAM) as s:
+        with ksft_raises(NlError) as cm:
+            cfg.pspnl.rx_assoc({"version": 0,
+                                "dev-id": cfg.psp_dev_id,
+                                "sock-fd": s.fileno()})
+        ksft_eq(cm.exception.nl_msg.error, -errno.ENOTCONN)
+        ksft_eq(cm.exception.nl_msg.extack['bad-attr'], ".sock-fd")
+
+
+def assoc_rx_listener(cfg):
+    """ Test that an Rx assoc is rejected on a listening socket """
+    _init_psp_dev(cfg)
+
+    with socket.create_server(("localhost", 0)) as s:
+        with ksft_raises(NlError) as cm:
+            cfg.pspnl.rx_assoc({"version": 0,
+                                "dev-id": cfg.psp_dev_id,
+                                "sock-fd": s.fileno()})
+        ksft_eq(cm.exception.nl_msg.error, -errno.ENOTCONN)
+        ksft_eq(cm.exception.nl_msg.extack['bad-attr'], ".sock-fd")
+
+
+def assoc_tx_non_established(cfg):
+    """ Test that a Tx assoc is rejected in a post established state after shutdown """
+    _init_psp_dev(cfg)
+
+    with _make_lo_conn() as s:
+        assoc = cfg.pspnl.rx_assoc({"version": 0,
+                                    "dev-id": cfg.psp_dev_id,
+                                    "sock-fd": s.fileno()})
+        s.shutdown(socket.SHUT_WR)
+        with ksft_raises(NlError) as cm:
+            cfg.pspnl.tx_assoc({"dev-id": cfg.psp_dev_id,
+                                "version": 0,
+                                "tx-key": assoc['rx-key'],
+                                "sock-fd": s.fileno()})
+        ksft_eq(cm.exception.nl_msg.error, -errno.ENOTCONN)
+
+
 def assoc_version_mismatch(cfg):
     """ Test creating associations where Rx and Tx PSP versions do not match """
     _init_psp_dev(cfg)

-- 
2.52.0

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

Re: [PATCH net-next 3/4] net: psp: drop psp assoc clear in sk_clone()

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] sk_clone() in net/core/sock.c (line 2499) no longer clears the
  psp_assoc pointer that sock_copy() copies raw into the child.

--

net: psp: drop psp assoc clear in sk_clone()

This patch reverts the unconditional NULL init of newsk->psp_assoc in
sk_clone() that 1d2929d0850f added. It replaces the init with a debug-only
warning. The commit message says listeners can no longer hold PSP
association state, because of 8cc3aef0cb19 and the earlier patch in this
series that limits association setup to established sockets.

> diff --git a/net/core/sock.c b/net/core/sock.c
> index 2948dffcc3e1b..d7ddd365abebb 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -2496,7 +2496,7 @@ struct sock *sk_clone(const struct sock *sk, const gfp_t priority,
>  	RCU_INIT_POINTER(newsk->sk_bpf_storage, NULL);
>  #endif
>  #if IS_ENABLED(CONFIG_INET_PSP)
> -	RCU_INIT_POINTER(newsk->psp_assoc, NULL);
> +	DEBUG_NET_WARN_ON_ONCE(rcu_access_pointer(sk->psp_assoc));

[Severity: Low]
Would it be worth keeping the RCU_INIT_POINTER() next to the new warning?

I could not find a path in the tree at the end of this series that reaches
this state. So this is about defense in depth, not a live bug.

psp_assoc sits past sk_dontcopy_end, so sock_copy() copies it raw into
newsk. With CONFIG_DEBUG_NET=n, DEBUG_NET_WARN_ON_ONCE() becomes
BUILD_BUG_ON_INVALID() and generates no code. With CONFIG_DEBUG_NET=y it
warns but still leaves the copied pointer in newsk.

If a parent ever did hold an association, each child would share the
pointer without taking a reference. Each child's destructor would then drop
a reference it never took:

inet_sock_destruct()
  psp_sk_assoc_free()
    rcu_assign_pointer(sk->psp_assoc, NULL);
    psp_assoc_put(pas);

This is the refcount underflow and use-after-free of struct psp_assoc that
1d2929d0850f fixed.

That protection now depends on two invariants in other files:

  - psp_sock_assoc_set_rx() and psp_sock_assoc_set_tx() rejecting sockets
    that are not TCP_ESTABLISHED
  - inet_hash() rejecting a non-LISTEN unhashed_state, which blocks
    established -> tcp_disconnect() -> listen()

tcp_disconnect() still leaves sk->psp_assoc set. If either check is relaxed
later, or this patch is backported without them, could the use-after-free
come back with nothing catching it on production builds? The earlier patch
in the series says the established-only restriction "can be reintroduced
when there is a use case".

sk_clone() is a slow path, so keeping the unconditional clear next to the
warning should cost very little. For example:

	DEBUG_NET_WARN_ON_ONCE(rcu_access_pointer(sk->psp_assoc));
	RCU_INIT_POINTER(newsk->psp_assoc, NULL);

Wrapping the clear in if (DEBUG_NET_WARN_ON_ONCE(...)) would not work,
because BUILD_BUG_ON_INVALID() is a void expression when
CONFIG_DEBUG_NET=n.

>  #endif
>  
>  	/* SANITY */

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

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

[Daniel Zahka]

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