Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider:
Critical: 0 · High: 0 · Medium: 1 · Low: 0
- [Medium] The new mutual-exclusion gate keys off
`sk->sk_validate_xmit_skb`, a function pointer that is inherited…
--
net: psp: avoid conflicts with skb->decrypted and sk_validate_xmit_skb()
This adds a mutual exclusion gate so that a socket cannot carry both PSP
state and a TCP ULP, since the two conflict over skb->decrypted and
sk->sk_validate_xmit_skb(). The gate is applied in __tcp_set_ulp() and in
psp_sock_assoc_set_rx().
> diff --git a/include/net/sock.h b/include/net/sock.h
> index 51185222aac29..ba5e5092f192c 100644
> --- a/include/net/sock.h
> +++ b/include/net/sock.h
> @@ -2312,6 +2312,17 @@ static inline void sk_gso_disable(struct sock *sk)
> sk->sk_route_caps &= ~NETIF_F_GSO_MASK;
> }
>
> +static inline bool sk_has_validate_xmit_skb(const struct sock *sk)
> +{
> +#ifdef CONFIG_SOCK_VALIDATE_XMIT
> + return !!sk->sk_validate_xmit_skb;
> +#else
> + return false;
> +#endif
> +}
[Severity: Medium]
Is sk_validate_xmit_skb a reliable indicator of live per-socket state here?
The field sits after sk_dontcopy_end in struct sock, so sock_copy() copies
it verbatim into a cloned socket, and neither sk_clone() nor
inet_csk_clone_lock() resets it. By contrast, sk_clone() does explicitly
clear the PSP association:
net/core/sock.c:sk_clone() {
sock_copy(newsk, sk);
...
#if IS_ENABLED(CONFIG_INET_PSP)
RCU_INIT_POINTER(newsk->psp_assoc, NULL);
#endif
So a socket accepted from a listener that had PSP Tx state installed ends up
with sk_validate_xmit_skb == psp_validate_xmit while psp_assoc == NULL, and
sk_has_validate_xmit_skb() then reports true for a child that has neither a
PSP association nor a ULP.
Can PSP state be installed on a listening socket in the first place? The only
socket check before installation appears to be in psp_nl.c:
net/psp/psp_nl.c:psp_assoc_device_get_locked() {
...
if (!sk_is_tcp(socket->sk)) {
and neither psp_sock_assoc_set_rx() nor psp_sock_assoc_set_tx() inspects
sk_state, while psp_sock_assoc_set_tx() does:
net/psp/psp_sock.c:psp_sock_assoc_set_tx() {
...
WRITE_ONCE(sk->sk_validate_xmit_skb, psp_validate_xmit);
> +
> +bool sk_has_decrypt_user(const struct sock *sk);
> +
> static inline int skb_do_copy_data_nocache(struct sock *sk, struct sk_buff
> *skb,
> struct iov_iter *from, char *to,
> int copy, int offset)
> diff --git a/net/core/sock.c b/net/core/sock.c
> index fa60b7494c586..9489d9c479493 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
[ ... ]
> @@ -2670,6 +2671,12 @@ void sk_setup_caps(struct sock *sk, struct dst_entry
> *dst)
> }
> EXPORT_SYMBOL_GPL(sk_setup_caps);
>
> +bool sk_has_decrypt_user(const struct sock *sk)
> +{
> + return psp_sk_assoc(sk) ||
> + (sk_is_inet(sk) && inet_csk_has_ulp(sk)); /* for tls */
> +}
> +
> /*
> * Simple resource managers for sockets.
> */
> diff --git a/net/ipv4/tcp_ulp.c b/net/ipv4/tcp_ulp.c
> index 2aa442128630e..f3e0c9d379bb6 100644
> --- a/net/ipv4/tcp_ulp.c
> +++ b/net/ipv4/tcp_ulp.c
> @@ -136,6 +136,10 @@ static int __tcp_set_ulp(struct sock *sk, const struct
> tcp_ulp_ops *ulp_ops)
> if (icsk->icsk_ulp_ops)
> goto out_err;
>
> + err = -EINVAL;
> + if (sk_has_decrypt_user(sk) || sk_has_validate_xmit_skb(sk))
> + goto out_err;
> +
[Severity: Medium]
For such an accepted child, sk_has_decrypt_user() returns false (psp_assoc is
NULL and there is no ULP yet), but sk_has_validate_xmit_skb() returns true
from the inherited pointer, so setsockopt(TCP_ULP) now fails with -EINVAL for
every socket accepted from that listener.
Before this patch the stale pointer was harmless, since psp_validate_xmit()
reads the association from the skb extension and treats a NULL association as
good. Should the check key off live per-socket state via psp_sk_assoc()
instead, or should sk_validate_xmit_skb be cleared in sk_clone() next to the
existing psp_assoc reset?
> if (sk->sk_socket)
> clear_bit(SOCK_SUPPORT_ZC, &sk->sk_socket->flags);
>
> diff --git a/net/psp/psp_sock.c b/net/psp/psp_sock.c
> index 1a2a6b7516b0f..509035617894a 100644
> --- a/net/psp/psp_sock.c
> +++ b/net/psp/psp_sock.c
> @@ -143,6 +143,10 @@ int psp_sock_assoc_set_rx(struct sock *sk, struct
> psp_assoc *pas,
> NL_SET_ERR_MSG(extack, "Socket already has PSP state");
> err = -EBUSY;
> goto exit_unlock;
> + } else if (sk_has_decrypt_user(sk) || sk_has_validate_xmit_skb(sk)) {
> + NL_SET_ERR_MSG(extack, "Socket has incompatible state");
> + err = -EINVAL;
> + goto exit_unlock;
> }
[Severity: Medium]
The same inherited pointer hits this branch too: the psp_sk_assoc(sk) test
above passes because the child's association was cleared, then this else-if
fires on sk_validate_xmit_skb and returns -EINVAL with "Socket has
incompatible state". Does that mean PSP itself can no longer be set up on any
socket accepted from a listener that had PSP Tx state?
Also worth noting on reachability: PSP_CMD_RX_ASSOC and PSP_CMD_TX_ASSOC carry
only GENL_CMD_CAP_DO in net/psp/psp-nl-gen.c, without GENL_ADMIN_PERM, so a
process with access to the PSP device can install the association on a
TCP_LISTEN fd. On a listener the preconditions in psp_sock_assoc_set_tx()
hold: psp_sock_recv_queue_check() walks empty queues and returns 0,
tcp_write_collapse_fence() is a no-op on the empty write queue, and
icsk_sync_mss is tcp_sync_mss from tcp_init_sock().
The follow-up commit in the series, "selftests: drv-net: psp: test PSP and TCP
ULP mutual exclusion", only touches tools/testing/selftests, so this case does
not appear to be addressed later in the series.
>
> refcount_inc(&pas->refcnt);
--
Sashiko AI review ·
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260910-psp-ktls-fix-v1-0-e3f30aaeca4e%40gmail.com
