Re: [PATCH net-next v13 08/19] net: psp: add socket security association code

On Tue, Sep 16, 2025 at 5:10 PM Daniel Zahka [off-list ref] wrote:

From: Jakub Kicinski <kuba@kernel.org>

Add the ability to install PSP Rx and Tx crypto keys on TCP
connections. Netlink ops are provided for both operations.
Rx side combines allocating a new Rx key and installing it
on the socket. Theoretically these are separate actions,
but in practice they will always be used one after the
other. We can add distinct "alloc" and "install" ops later.

Reviewed-by: Willem de Bruijn <willemb@google.com>
Signed-off-by: Jakub Kicinski <kuba@kernel.org>
Co-developed-by: Daniel Zahka <daniel.zahka@gmail.com>
Signed-off-by: Daniel Zahka <daniel.zahka@gmail.com>
---

+
+static inline struct psp_assoc *psp_sk_get_assoc_rcu(struct sock *sk)

nit : const struct sock *sk;

+{
+       struct inet_timewait_sock *tw;
+       struct psp_assoc *pas;
+       int state;
+
+       state = 1 << READ_ONCE(sk->sk_state);

nit: Not sure why you need flags, you could later compare with TCP_NEW_SYN_RECV
and

+       if (!sk_is_inet(sk) || state & TCPF_NEW_SYN_RECV)
+               return NULL;
+
+       tw = inet_twsk(sk);

It seems strange to use inet_twsk() helper without knowing it is a tw socket.
(fine today, but strange)

pas = state == TCP_TIME_WAIT ? rcu_dereference(inet_twsk(sk)->psp_assoc) :

+       pas = state & TCPF_TIME_WAIT ? rcu_dereference(tw->psp_assoc) :
+                                      rcu_dereference(sk->psp_assoc);
+       return pas;
 }

...

+
+struct psp_dev *psp_dev_get_for_sock(struct sock *sk)
+{
+       struct dst_entry *dst;
+       struct psp_dev *psd;
+
+       dst = sk_dst_get(sk);
+       if (!dst)
+               return NULL;
+
+       rcu_read_lock();
+       psd = rcu_dereference(dst->dev->psp_dev);
+       if (psd && !psp_dev_tryget(psd))
+               psd = NULL;
+       rcu_read_unlock();
+
+       dst_release(dst);
+
+       return psd;
+}

I would rather not use sk_dst_get() and risk UAF later on dst->dev->psp_dev;

I would instead use dst_dev_rcu() and __sk_dst_get().

{
   struct psp_dev *psd = NULL;
   struct dst_entry *dst;

   rcu_read_lock();
   dst  = __sk_dst_get(sk);
   if (!dst)
       goto unlock;
   psd = rcu_dereference(dst_dev_rcu(dst)->psp_dev);
   if (psd && !psp_dev_tryget(psd))
           psd = NULL;
unlock:
   rcu_read_unlock();
   return psd;
}

This can be done later, I can provide a patch myself after this series
is merged.

Reviewed-by: Eric Dumazet <edumazet@google.com>