From nobody Tue Jul 23 13:19:33 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4WSyRV2Phtz5Rx09; Tue, 23 Jul 2024 13:19:34 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from mxrelay.nyi.freebsd.org (mxrelay.nyi.freebsd.org [IPv6:2610:1c1:1:606c::19:3]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (4096 bits) client-digest SHA256) (Client CN "mxrelay.nyi.freebsd.org", Issuer "R11" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4WSyRV0cd6z4Hxq; Tue, 23 Jul 2024 13:19:34 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1721740774; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=1UNWTWH/tinUasOW/tx3sav2rne2qteBkScdp1J1Q0I=; b=AO69R2b4Ps/wlO7yxZL9LGOPkIUKoKLieyi87MSrY3dzgANeW5s/nbnBSWU8E3vx0odSRj sJbfcZPC/NDUZe8Euju2n3TNqbSsspPf/pH0zveHqVJvSPUBRiRDwZ2Ie6NX6bBU7DypQ5 zx4X2pWRjeMMeHewkPCKm71qUrVlJEhYDJxnTORMQpdk61NqVgZA/XrPM6MK4ZiFBAYsJ/ PrZ3KTsHr9IcmTL9W6bgVZAwoDl39AoIbMVYRvFfQk0Ryi5xLTieefEnLU9LIYM1CrFOrT wh2lX9p8lXwuF6nC3uatxj404jc1hOBNnjwlLlyN3HpW+HuEMwzkqI7IvP/rdQ== ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1721740774; a=rsa-sha256; cv=none; b=VTYHGXUc6tqHYi3tB1t/QsBBD55qczUug/Fha13imNYOtyuuhiJCKZ81fwaZxfgC/PSrrq A92hb1scSYI5f4PzYLgllBzDY90k+x/w8tumtU6YX3PkhGNRU7rRkytkUjk1g4jhQptd6M PasC+YKsMyQYSGmBYYl40PY7ZKvz5+VC2oXl6yFyaSdPol5Rm+Dyl0MaZAIZGefYk5sE0/ VOPbaruAtWzdigVm4chKlKiieviF6N9FMclAhs7nPRvI/rMN5OP21GCWmXwUghswYqV2K9 TQs5LF82Q04pnzTOPP774/CMliFE6AwlTtUAJT9RabOLLtRghX2o0KDOAeQJVg== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1721740774; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding; bh=1UNWTWH/tinUasOW/tx3sav2rne2qteBkScdp1J1Q0I=; b=u8F56yqHqglsHyX62yiECMFOoJ7h2dQ0k78+Is9s7HIN/4imqc193YvtfGkMvOM/JuGN6Z f/uE64f4jpOxAF3VfM7LmyiArUYq8O3xCrcucy6BJ8SMYvNfe9KO+KDMG37lRS8l9CXoIa 6vcRdavahkyhXONBm3Gdg6acRZZKUvnrn5bVhw3U0+PS3o5WHXFGj+ip2RfY8w3ecEyurS fBo3xf7b0QANF1mytAcS1cxwPg6WA09W1sDsOpeV6yNSau7fDYA+Vev3k2VNbTnICeypMb QFsCjtK+5BUkoUOy26kLYpkKOy++SD7yrxttwsW6nqnF9tEQypsctqeq9Rrikg== Received: from gitrepo.freebsd.org (gitrepo.freebsd.org [IPv6:2610:1c1:1:6068::e6a:5]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (Client did not present a certificate) by mxrelay.nyi.freebsd.org (Postfix) with ESMTPS id 4WSyRV0CddzTcN; Tue, 23 Jul 2024 13:19:34 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.18.1/8.18.1) with ESMTP id 46NDJX4K004382; Tue, 23 Jul 2024 13:19:33 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.18.1/8.18.1/Submit) id 46NDJXfu004379; Tue, 23 Jul 2024 13:19:33 GMT (envelope-from git) Date: Tue, 23 Jul 2024 13:19:33 GMT Message-Id: <202407231319.46NDJXfu004379@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-branches@FreeBSD.org From: Mark Johnston Subject: git: b14a49128381 - stable/14 - ktls: Fix races that can lead to double initialization List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: X-BeenThere: dev-commits-src-all@freebsd.org Sender: owner-dev-commits-src-all@FreeBSD.org MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-Git-Committer: markj X-Git-Repository: src X-Git-Refname: refs/heads/stable/14 X-Git-Reftype: branch X-Git-Commit: b14a491283816864e4b8ec88744efbfd7d4e2bc7 Auto-Submitted: auto-generated The branch stable/14 has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=b14a491283816864e4b8ec88744efbfd7d4e2bc7 commit b14a491283816864e4b8ec88744efbfd7d4e2bc7 Author: Mark Johnston AuthorDate: 2024-07-08 15:49:29 +0000 Commit: Mark Johnston CommitDate: 2024-07-23 13:01:30 +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 (cherry picked from commit 163cdf6a32b9a0f84226a70101d143c10707336f) --- 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 b4b169c4daf2..294a196db60d 100644 --- a/sys/kern/uipc_ktls.c +++ b/sys/kern/uipc_ktls.c @@ -1243,12 +1243,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; @@ -1258,6 +1269,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 @@ -1343,6 +1355,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 14107f5b2a10..a6ec72975252 100644 --- a/sys/sys/sockbuf.h +++ b/sys/sys/sockbuf.h @@ -130,7 +130,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); }