git: 163cdf6a32b9 - main - ktls: Fix races that can lead to double initialization

The branch main has been updated by markj:

URL: https://cgit.FreeBSD.org/src/commit/?id=163cdf6a32b9a0f84226a70101d143c10707336f

commit 163cdf6a32b9a0f84226a70101d143c10707336f
Author:     Mark Johnston <markj@FreeBSD.org>
AuthorDate: 2024-07-08 15:49:29 +0000
Commit:     Mark Johnston <markj@FreeBSD.org>
CommitDate: 2024-07-08 16:10:48 +0000

    ktls: Fix races that can lead to double initialization
    
    ktls_enable_rx() and ktls_enable_tx() have checks to return EALREADY if
    the socket already has KTLS enabled.  However, these are done without
    any locks held and nothing blocks concurrent attempts to set the socket
    option.  I believe the worst outcome of the race is leaked memory.
    
    Fix the problem by rechecking under the sockbuf lock.  While here, unify
    the locking protocol for sb_tls_info: require both the sockbuf and
    socket I/O locks in order to enable KTLS.  This means that either lock
    is sufficient for checking whether KTLS is enabled in a given sockbuf,
    which simplifies some refactoring further down the road.
    
    Note that the SOLISTENING() check can go away because
    SOCK_IO_RECV_LOCK() atomically locks the socket buffer and checks
    whether the socket is a listening socket.  This changes the returned
    errno value, so update a test which checks it.
    
    Reviewed by:    gallatin
    MFC after:      2 weeks
    Sponsored by:   Klara, Inc.
    Sponsored by:   Stormshield
    Differential Revision:  https://reviews.freebsd.org/D45674
---
 sys/kern/uipc_ktls.c       | 23 +++++++++++++++++++++--
 sys/sys/sockbuf.h          |  3 ++-
 tests/sys/kern/ktls_test.c |  2 +-
 3 files changed, 24 insertions(+), 4 deletions(-)

diff --git a/sys/kern/uipc_ktls.c b/sys/kern/uipc_ktls.c
index fd1bc7bf8bfe..5ea99966bf13 100644
--- a/sys/kern/uipc_ktls.c
+++ b/sys/kern/uipc_ktls.c
@@ -1320,12 +1320,23 @@ ktls_enable_rx(struct socket *so, struct tls_enable *en)
 		return (error);
 	}
 
+	/*
+	 * Serialize with soreceive_generic() and make sure that we're not
+	 * operating on a listening socket.
+	 */
+	error = SOCK_IO_RECV_LOCK(so, SBL_WAIT);
+	if (error) {
+		ktls_free(tls);
+		return (error);
+	}
+
 	/* Mark the socket as using TLS offload. */
 	SOCK_RECVBUF_LOCK(so);
-	if (SOLISTENING(so)) {
+	if (__predict_false(so->so_rcv.sb_tls_info != NULL)) {
 		SOCK_RECVBUF_UNLOCK(so);
+		SOCK_IO_RECV_UNLOCK(so);
 		ktls_free(tls);
-		return (EINVAL);
+		return (EALREADY);
 	}
 	so->so_rcv.sb_tls_seqno = be64dec(en->rec_seq);
 	so->so_rcv.sb_tls_info = tls;
@@ -1335,6 +1346,7 @@ ktls_enable_rx(struct socket *so, struct tls_enable *en)
 	sb_mark_notready(&so->so_rcv);
 	ktls_check_rx(&so->so_rcv);
 	SOCK_RECVBUF_UNLOCK(so);
+	SOCK_IO_RECV_UNLOCK(so);
 
 	/* Prefer TOE -> ifnet TLS -> software TLS. */
 #ifdef TCP_OFFLOAD
@@ -1420,6 +1432,13 @@ ktls_enable_tx(struct socket *so, struct tls_enable *en)
 	inp = so->so_pcb;
 	INP_WLOCK(inp);
 	SOCK_SENDBUF_LOCK(so);
+	if (__predict_false(so->so_snd.sb_tls_info != NULL)) {
+		SOCK_SENDBUF_UNLOCK(so);
+		INP_WUNLOCK(inp);
+		SOCK_IO_SEND_UNLOCK(so);
+		ktls_free(tls);
+		return (EALREADY);
+	}
 	so->so_snd.sb_tls_seqno = be64dec(en->rec_seq);
 	so->so_snd.sb_tls_info = tls;
 	if (tls->mode != TCP_TLS_MODE_SW) {
diff --git a/sys/sys/sockbuf.h b/sys/sys/sockbuf.h
index c6093883be4a..a2e327bbb5df 100644
--- a/sys/sys/sockbuf.h
+++ b/sys/sys/sockbuf.h
@@ -128,7 +128,8 @@ struct sockbuf {
 			struct	mbuf *sb_mtls;	/*  TLS mbuf chain */
 			struct	mbuf *sb_mtlstail; /* last mbuf in TLS chain */
 			uint64_t sb_tls_seqno;	/* TLS seqno */
-			struct	ktls_session *sb_tls_info; /* TLS state */
+			/* TLS state, locked by sockbuf and sock I/O mutexes. */
+			struct	ktls_session *sb_tls_info;
 		};
 		/*
 		 * PF_UNIX/SOCK_DGRAM
diff --git a/tests/sys/kern/ktls_test.c b/tests/sys/kern/ktls_test.c
index f57ae74112a2..72497196b945 100644
--- a/tests/sys/kern/ktls_test.c
+++ b/tests/sys/kern/ktls_test.c
@@ -2812,7 +2812,7 @@ ATF_TC_BODY(ktls_listening_socket, tc)
 	    TLS_MINOR_VER_THREE, (uint64_t)random(), &en);
 	ATF_REQUIRE_ERRNO(ENOTCONN,
 	    setsockopt(s, IPPROTO_TCP, TCP_TXTLS_ENABLE, &en, sizeof(en)) != 0);
-	ATF_REQUIRE_ERRNO(EINVAL,
+	ATF_REQUIRE_ERRNO(ENOTCONN,
 	    setsockopt(s, IPPROTO_TCP, TCP_RXTLS_ENABLE, &en, sizeof(en)) != 0);
 	ATF_REQUIRE(close(s) == 0);
 }