[PATCH net v7] kcm: reject TLS on attached TCP sockets

[PATCH net v7] kcm: reject TLS on attached TCP sockets

[Rishikesh Jethwani]

KCM calls read_sock() from its receive callback in atomic context. With
kTLS RX, this reaches tls_sw_read_sock(), which can sleep. KCM and TLS
also replace socket callbacks, so detaching KCM can discard TLS handlers.

Found by code review and reproduced on an unmodified kernel with
CONFIG_DEBUG_ATOMIC_SLEEP by sending TLS records to a KCM-attached
socket with software kTLS RX.

Reject sockets with an installed ULP in kcm_attach(), and reject TLS
installation while KCM is attached. Track attachment in a tcp_sock boolean
present when CONFIG_AF_KCM is enabled, and clear it on detach. Both checks
run under the transport socket lock.

Use sk_is_tcp() before accessing TCP state. Checking only the family and
IPPROTO_TCP also accepts raw sockets, whose allocations do not contain
inet_connection_sock or tcp_sock.

Backports without commit 8cc3aef0cb198 ("tcp: Do not allow buggy
transitions between ehash and lhash2.") must also prevent attached
sockets from becoming listeners after disconnect.

Validated with KASAN and lockdep, including connected IPv4/IPv6 raw
sockets, both TLS/KCM orders, detach and concurrent installation.

Fixes: 662fbcec32f4 ("net/tls: implement ->read_sock()")
Cc: <stable@vger.kernel.org> # see backport requirement above
Link: https://lore.kernel.org/netdev/3108e934-ec00-458b-8720-450554b35b55@linux.dev/
Signed-off-by: Rishikesh Jethwani <rjethwani@purestorage.com>
---
 Documentation/networking/kcm.rst |  3 +++
 include/linux/tcp.h              |  3 +++
 include/net/tcp.h                |  9 +++++++++
 net/kcm/kcmsock.c                | 16 +++++++++++++---
 net/tls/tls_main.c               |  3 +++
 5 files changed, 31 insertions(+), 3 deletions(-)

diff --git a/Documentation/networking/kcm.rst b/Documentation/networking/kcm.rst
index 71f44d0beaa3..07778f29211c 100644
--- a/Documentation/networking/kcm.rst
+++ b/Documentation/networking/kcm.rst
@@ -167,6 +167,9 @@ The kcm_attach structure contains:
   - fd: file descriptor for TCP socket being attached
   - bpf_prog_fd: file descriptor for compiled BPF program downloaded
 
+The transport socket must be a TCP stream socket with no TCP ULP installed.
+TLS cannot be enabled on a transport socket while it is attached to KCM.
+
 Unattach transport sockets
 --------------------------
 
diff --git a/include/linux/tcp.h b/include/linux/tcp.h
index e6b79cf4798f..d4b07fbae2d5 100644
--- a/include/linux/tcp.h
+++ b/include/linux/tcp.h
@@ -516,6 +516,9 @@ struct tcp_sock {
 #if IS_ENABLED(CONFIG_MPTCP)
 	bool	is_mptcp;
 #endif
+#if IS_ENABLED(CONFIG_AF_KCM)
+	bool	kcm_attached;
+#endif
 #if IS_ENABLED(CONFIG_SMC)
 	bool	syn_smc;	/* SYN includes SMC */
 	bool	(*smc_hs_congested)(const struct sock *sk);
diff --git a/include/net/tcp.h b/include/net/tcp.h
index 5c9203aae834..5e99d3142f5e 100644
--- a/include/net/tcp.h
+++ b/include/net/tcp.h
@@ -2882,6 +2882,15 @@ void tcp_cleanup_ulp(struct sock *sk);
 void tcp_update_ulp(struct sock *sk, struct proto *p,
 		    void (*write_space)(struct sock *sk));
 
+static inline bool tcp_kcm_attached(const struct sock *sk)
+{
+#if IS_ENABLED(CONFIG_AF_KCM)
+	return tcp_sk(sk)->kcm_attached;
+#else
+	return false;
+#endif
+}
+
 #define MODULE_ALIAS_TCP_ULP(name)				\
 	MODULE_INFO(alias, name);		\
 	MODULE_INFO(alias, "tcp-ulp-" name)
diff --git a/net/kcm/kcmsock.c b/net/kcm/kcmsock.c
index a7f94d42036f..ca789812124e 100644
--- a/net/kcm/kcmsock.c
+++ b/net/kcm/kcmsock.c
@@ -30,6 +30,7 @@
 #include <net/kcm.h>
 #include <net/netns/generic.h>
 #include <net/sock.h>
+#include <net/tcp.h>
 #include <uapi/linux/kcm.h>
 #include <trace/events/sock.h>
 
@@ -1259,9 +1260,10 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
 
 	lock_sock(csk);
 
-	/* Only allow TCP sockets to be attached for now */
-	if ((csk->sk_family != AF_INET && csk->sk_family != AF_INET6) ||
-	    csk->sk_protocol != IPPROTO_TCP) {
+	/* Only allow TCP sockets to be attached for now. A raw socket
+	 * created with IPPROTO_TCP has that sk_protocol but no icsk.
+	 */
+	if (!sk_is_tcp(csk)) {
 		err = -EOPNOTSUPP;
 		goto out;
 	}
@@ -1272,6 +1274,12 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
 		goto out;
 	}
 
+	/* KCM and ULPs cannot share the receive path and socket callbacks. */
+	if (inet_csk_has_ulp(csk)) {
+		err = -EOPNOTSUPP;
+		goto out;
+	}
+
 	psock = kmem_cache_zalloc(kcm_psockp, GFP_KERNEL);
 	if (!psock) {
 		err = -ENOMEM;
@@ -1305,6 +1313,7 @@ static int kcm_attach(struct socket *sock, struct socket *csock,
 	psock->save_write_space = csk->sk_write_space;
 	psock->save_state_change = csk->sk_state_change;
 	csk->sk_user_data = psock;
+	tcp_sk(csk)->kcm_attached = true;
 	WRITE_ONCE(csk->sk_data_ready, psock_data_ready);
 	WRITE_ONCE(csk->sk_write_space, psock_write_space);
 	csk->sk_state_change = psock_state_change;
@@ -1386,6 +1395,7 @@ static void kcm_unattach(struct kcm_psock *psock)
 	WRITE_ONCE(csk->sk_write_space, psock->save_write_space);
 	csk->sk_state_change = psock->save_state_change;
 	strp_stop(&psock->strp);
+	tcp_sk(csk)->kcm_attached = false;
 
 	if (WARN_ON(psock->rx_kcm)) {
 		write_unlock_bh(&csk->sk_callback_lock);
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index b8d5a843f761..fa71c75c2861 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -1067,6 +1067,9 @@ static int tls_init(struct sock *sk)
 	if (sk->sk_state != TCP_ESTABLISHED)
 		return -ENOTCONN;
 
+	if (tcp_kcm_attached(sk))
+		return -EBUSY;
+
 	/* allocate tls context */
 	write_lock_bh(&sk->sk_callback_lock);
 	ctx = tls_ctx_create(sk);
-- 
2.50.1

Re: [PATCH net v7] kcm: reject TLS on attached TCP sockets

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found 1 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 1 · Low: 0

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

- [Medium] The Fixes: tag covers only one of the two defects the commit
  message says it fixes.

--

kcm: reject TLS on attached TCP sockets

This patch makes kcm_attach() reject TCP sockets that already have a ULP
installed. It also makes tls_init() reject sockets attached to KCM, using
a new kcm_attached flag in tcp_sock. The kcm_attach() type check now uses
sk_is_tcp(), so raw sockets created with IPPROTO_TCP are rejected.

Link: https://lore.kernel.org/netdev/3108e934-ec00-458b-8720-450554b35b55@linux.dev/

> KCM calls read_sock() from its receive callback in atomic context. With
> kTLS RX, this reaches tls_sw_read_sock(), which can sleep. KCM and TLS
> also replace socket callbacks, so detaching KCM can discard TLS handlers.

[ ... ]

> Fixes: 662fbcec32f4 ("net/tls: implement ->read_sock()")
> Cc: <stable@vger.kernel.org> # see backport requirement above

[Severity: Medium]

Does the Fixes: tag cover both of the problems described above?

662fbcec32f4 added tls_sw_read_sock() as the read_sock op, which explains
the sleep in atomic context. The callback replacement problem looks older.
In the parent of 662fbcec32f4, neither kcm_attach() nor tls_init() checked
for the other, so the two could already be stacked.

If KCM is attached first and TLS_RX is enabled afterwards,
tls_sw_strparser_arm() saves psock_data_ready and installs tls_data_ready:

net/tls/tls_sw.c:tls_sw_strparser_arm() {
    ...
	rx_ctx->saved_data_ready = sk->sk_data_ready;
	sk->sk_data_ready = tls_data_ready;
    ...
}

On SIOCKCMUNATTACH, kcm_unattach() then writes the pre-KCM callback back
over tls_data_ready:

net/kcm/kcmsock.c:kcm_unattach() {
    ...
	WRITE_ONCE(csk->sk_data_ready, psock->save_data_ready);
    ...
}

After that, TLS RX is no longer notified.

Before 662fbcec32f4 the TLS_SW proto_ops also inherited read_sock from
inet_stream_ops. So in either order, strp_read_sock() would call
tcp_read_sock(). KCM's strparser would then take raw TLS ciphertext from
the same receive queue that TLS parses.

This seems to go back to c46234ebb4d1 ("tls: RX path for ktls"). Stable
selection uses the Fixes: commit. Would LTS trees without 662fbcec32f4
(6.1, and 5.15/5.10 while they are still maintained) skip this patch? On
those trees an unprivileged user could still stack KCM and kTLS RX.

Could the Fixes: tag point at c46234ebb4d1, or could the Cc: stable note
say whether pre-6.6 trees are affected? Right now the note only mentions
the 8cc3aef0cb198 dependency.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261112232444.2865382-1-rjethwani%40purestorage.com