Re: [PATCH net 1/2] net: psp: avoid conflicts with skb->decrypted and sk_validate_xmit_skb()

[netdev-bot+sashiko@kernel.org]

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