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 8:19 PM EDT, Willem de Bruijn wrote:
> Daniel Zahka wrote:
>> Check sk_state under the socket lock in both the rx-assoc and tx-assoc
>> handlers, and only allow association setup on sockets in
>> TCP_ESTABLISHED. Also, fail connect() when PSP assoc state is already
>> present, and remove the dead PSP MSS adjustment from
>> tcp_v[46]_connect().
>> 
>> The net effect of this commit is:
>> 1. PSP assoc state can never exist on a listen socket.
>> 2. The upgrade to PSP must be done while the socket is in
>>    TCP_ESTABLISHED.
>> 
>> This change defeatures behavior that was previously allowed under the
>> PSP uapi. My justification is:
>> 
>> 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, which in turn has the aforementioned
>> limitations. That implies that there cannot be any users of this
>> feature, so it should be safe to remove it from the PSP uapi.
>> 
>> In theory, the check in the tx-assoc path is more restrictive than
>> necessary. FIN_WAIT1/2, CLOSING, LAST_ACK and 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 it is simpler
>> to disallow those states because they don't fit the upgrade model.
>
> And preferable to do so. While it could be allowed, there is no real
> use case for it. Simpler state model allows simpler code.
>
>> 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 on an assoc shared with a timewait socket when
>> the socket is in TCP_CLOSE after shutdown. This commit is not targeted
>> at net because its premise of preventing listen sockets from holding
>> assoc state depends on net-next commit 8cc3aef0cb19 ("tcp: Do not allow
>> buggy transitions between ehash and lhash2.")
>> 
>> Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
>
> Reviewed-by: Willem de Bruijn <willemb@google.com>
>
> Even if targeting to net-next, you could consider keeping the Fixes tag.
> Importantly the prerequisite patch is mentioned. But is only one of two
> mentioned here?

Yes, sorry. I replied with the full Fixes info to the bot here:
https://lore.kernel.org/netdev/DLTLF0VWT5UY.2WUPQRDZJQO3W@gmail.com/