From nobody Tue Mar 21 20:04:31 2023 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 4Ph2cv3c3kz40mTM; Tue, 21 Mar 2023 20:04:31 +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 "R3" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Ph2cv32t9z41jy; Tue, 21 Mar 2023 20:04:31 +0000 (UTC) (envelope-from git@FreeBSD.org) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1679429071; 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=QHNJ+EOMHxEmzsjStRCppD0ASSx8Tvr5j435dDuYmNs=; b=HKvCNt7RAxYo3ybNJ97xLRqmQq7HbsJBd3FV7WEQ631P8lIYTrTa0p5oC2DQtPohO1rjcU hDTr3HY4bKJzHYNgwdNFjIbOnQsuAlhr9c8SebXCbICw4DYE1ix1IDfTvoR0h2xk2wB/pK LPbFhI7S9SCyIxe4BJ7OcUDWDALjJNYKVT+wYxcKAWT33IX+DZGR1pg7KxJEwDWBjs+Zfo vPBWoCiAR83UTUaQeNftMDe+E5L+H9WnOSZtFPRh7WXj1K83S2KeiB5X6oTCaWrBNZPblD /dZmmVKngD6jMt9aLTTTN9OISJfF8dYqVTWjSJHiGF59/TmVPXoZ72P8+85f/w== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=freebsd.org; s=dkim; t=1679429071; 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=QHNJ+EOMHxEmzsjStRCppD0ASSx8Tvr5j435dDuYmNs=; b=i7f6x6iGCO7OQE4qU3zxjH86J/8IInES3/8pB5fgP5p0OjytYZoKb8gUBi1LS0Kj2NtrlX 4/4F16jwsDnk0sleKzMymDbHgTH6AAzxCU7Ff7YvcUyEloSqD+CxgcyemRmgTxKxCcQUKz 7eK0NLxwlqs9GWepB/m76ZSLi+qhdeMLvY06yZ0jliV6ajMMMou9+NsCx8kWAnV3IYLYs/ WkRSTqtP5r9dnZA01/sc0MuGZcbTYer6jK99jGLVTqFovo1m5eOwxG5dcNQyi8F76ESjR8 t1iJipTUMMSdC0mZMv76tGhuojrtZDwdJaxlN8VdND0e55cFkJMbJGecXspCwA== ARC-Authentication-Results: i=1; mx1.freebsd.org; none ARC-Seal: i=1; s=dkim; d=freebsd.org; t=1679429071; a=rsa-sha256; cv=none; b=llpSbcf5I9I9VwPpdGgjNOFcP2sVlTWsYwhQBIyXZiuh88Gev3bviHe8nEuU67MwaNSFml 7FOHRURlOtvLRrgSi76CRzcIPjhyWe9s2eReNkj3HlVVyUEHWLzprgJq6pgykJjoqekxtB KekqQazVZXx3F9iltGBgz0y5iutD9aDhmzM6QLeQL7YMtMYSbRQ2k9KcsYZK+NGciCvLxk Kw/+hyT/NsMqgINFMRpLXhatMlC1okZqcJoVzdovf0QsY7ats9dF56Z5ubfjx/NAKcQ32j /HNAelkpZP5OFFSQ3y3Jmx8KyzwFemAA4kgw8CwNY+dxRVjB2BTSU7Je80JIbA== 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 4Ph2cv25ptzQM8; Tue, 21 Mar 2023 20:04:31 +0000 (UTC) (envelope-from git@FreeBSD.org) Received: from gitrepo.freebsd.org ([127.0.1.44]) by gitrepo.freebsd.org (8.16.1/8.16.1) with ESMTP id 32LK4V28007956; Tue, 21 Mar 2023 20:04:31 GMT (envelope-from git@gitrepo.freebsd.org) Received: (from git@localhost) by gitrepo.freebsd.org (8.16.1/8.16.1/Submit) id 32LK4VEx007955; Tue, 21 Mar 2023 20:04:31 GMT (envelope-from git) Date: Tue, 21 Mar 2023 20:04:31 GMT Message-Id: <202303212004.32LK4VEx007955@gitrepo.freebsd.org> To: src-committers@FreeBSD.org, dev-commits-src-all@FreeBSD.org, dev-commits-src-main@FreeBSD.org From: Mark Johnston Subject: git: b4b33821fa3d - main - ktls: Fix interlocking between ktls_enable_rx() and listen(2) 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: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: 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/main X-Git-Reftype: branch X-Git-Commit: b4b33821fa3d970121d886467d10126679876905 Auto-Submitted: auto-generated X-ThisMailContainsUnwantedMimeParts: N The branch main has been updated by markj: URL: https://cgit.FreeBSD.org/src/commit/?id=b4b33821fa3d970121d886467d10126679876905 commit b4b33821fa3d970121d886467d10126679876905 Author: Mark Johnston AuthorDate: 2023-03-21 19:51:24 +0000 Commit: Mark Johnston CommitDate: 2023-03-21 20:04:00 +0000 ktls: Fix interlocking between ktls_enable_rx() and listen(2) The TCP_TXTLS_ENABLE and TCP_RXTLS_ENABLE socket option handlers check whether the socket is listening socket and fail if so, but this check is racy. Since we have to lock the socket buffer later anyway, defer the check to that point. ktls_enable_tx() locks the send buffer's I/O lock, which will fail if the socket is a listening socket, so no explicit checks are needed. In ktls_enable_rx(), which does not acquire the I/O lock (see the review for some discussion on this), use an explicit SOLISTENING() check after locking the recv socket buffer. Otherwise, a concurrent solisten_proto() call can trigger crashes and memory leaks by wiping out socket buffers as ktls_enable_*() is modifying them. Also make sure that a KTLS-enabled socket can't be converted to a listening socket, and use SOCK_(SEND|RECV)BUF_LOCK macros instead of the old ones while here. Add some simple regression tests involving listen(2). Reported by: syzkaller MFC after: 2 weeks Reviewed by: gallatin, glebius, jhb Sponsored by: The FreeBSD Foundation Differential Revision: https://reviews.freebsd.org/D38504 --- sys/kern/uipc_ktls.c | 21 +++++++++++++-------- sys/kern/uipc_socket.c | 23 +++++++++++++++++------ tests/sys/kern/ktls_test.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 76 insertions(+), 14 deletions(-) diff --git a/sys/kern/uipc_ktls.c b/sys/kern/uipc_ktls.c index 5b41cd3f829f..c82864f379b8 100644 --- a/sys/kern/uipc_ktls.c +++ b/sys/kern/uipc_ktls.c @@ -1223,8 +1223,6 @@ ktls_enable_rx(struct socket *so, struct tls_enable *en) if (!ktls_offload_enable) return (ENOTSUP); - if (SOLISTENING(so)) - return (EINVAL); counter_u64_add(ktls_offload_enable_calls, 1); @@ -1256,7 +1254,12 @@ ktls_enable_rx(struct socket *so, struct tls_enable *en) } /* Mark the socket as using TLS offload. */ - SOCKBUF_LOCK(&so->so_rcv); + SOCK_RECVBUF_LOCK(so); + if (SOLISTENING(so)) { + SOCK_RECVBUF_UNLOCK(so); + ktls_free(tls); + return (EINVAL); + } so->so_rcv.sb_tls_seqno = be64dec(en->rec_seq); so->so_rcv.sb_tls_info = tls; so->so_rcv.sb_flags |= SB_TLS_RX; @@ -1264,7 +1267,7 @@ ktls_enable_rx(struct socket *so, struct tls_enable *en) /* Mark existing data as not ready until it can be decrypted. */ sb_mark_notready(&so->so_rcv); ktls_check_rx(&so->so_rcv); - SOCKBUF_UNLOCK(&so->so_rcv); + SOCK_RECVBUF_UNLOCK(so); /* Prefer TOE -> ifnet TLS -> software TLS. */ #ifdef TCP_OFFLOAD @@ -1290,8 +1293,6 @@ ktls_enable_tx(struct socket *so, struct tls_enable *en) if (!ktls_offload_enable) return (ENOTSUP); - if (SOLISTENING(so)) - return (EINVAL); counter_u64_add(ktls_offload_enable_calls, 1); @@ -1334,6 +1335,10 @@ ktls_enable_tx(struct socket *so, struct tls_enable *en) return (error); } + /* + * Serialize with sosend_generic() and make sure that we're not + * operating on a listening socket. + */ error = SOCK_IO_SEND_LOCK(so, SBL_WAIT); if (error) { ktls_free(tls); @@ -1347,7 +1352,7 @@ ktls_enable_tx(struct socket *so, struct tls_enable *en) */ inp = so->so_pcb; INP_WLOCK(inp); - SOCKBUF_LOCK(&so->so_snd); + SOCK_SENDBUF_LOCK(so); so->so_snd.sb_tls_seqno = be64dec(en->rec_seq); so->so_snd.sb_tls_info = tls; if (tls->mode != TCP_TLS_MODE_SW) { @@ -1357,7 +1362,7 @@ ktls_enable_tx(struct socket *so, struct tls_enable *en) if (tp->t_fb->tfb_hwtls_change != NULL) (*tp->t_fb->tfb_hwtls_change)(tp, 1); } - SOCKBUF_UNLOCK(&so->so_snd); + SOCK_SENDBUF_UNLOCK(so); INP_WUNLOCK(inp); SOCK_IO_SEND_UNLOCK(so); diff --git a/sys/kern/uipc_socket.c b/sys/kern/uipc_socket.c index 1d2b06311c8c..f529f885d17c 100644 --- a/sys/kern/uipc_socket.c +++ b/sys/kern/uipc_socket.c @@ -987,13 +987,24 @@ solisten_proto_check(struct socket *so) mtx_lock(&so->so_snd_mtx); mtx_lock(&so->so_rcv_mtx); - /* Interlock with soo_aio_queue(). */ - if (!SOLISTENING(so) && - ((so->so_snd.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0 || - (so->so_rcv.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0)) { - solisten_proto_abort(so); - return (EINVAL); + /* Interlock with soo_aio_queue() and KTLS. */ + if (!SOLISTENING(so)) { + bool ktls; + +#ifdef KERN_TLS + ktls = so->so_snd.sb_tls_info != NULL || + so->so_rcv.sb_tls_info != NULL; +#else + ktls = false; +#endif + if (ktls || + (so->so_snd.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0 || + (so->so_rcv.sb_flags & (SB_AIO | SB_AIO_RUNNING)) != 0) { + solisten_proto_abort(so); + return (EINVAL); + } } + return (0); } diff --git a/tests/sys/kern/ktls_test.c b/tests/sys/kern/ktls_test.c index 60f2218ae5f2..e925dbe32e52 100644 --- a/tests/sys/kern/ktls_test.c +++ b/tests/sys/kern/ktls_test.c @@ -2767,6 +2767,51 @@ ATF_TC_BODY(ktls_sendto_baddst, tc) ATF_REQUIRE(close(s) == 0); } +/* + * Make sure that listen(2) returns an error for KTLS-enabled sockets, and + * verify that an attempt to enable KTLS on a listening socket fails. + */ +ATF_TC_WITHOUT_HEAD(ktls_listening_socket); +ATF_TC_BODY(ktls_listening_socket, tc) +{ + struct tls_enable en; + struct sockaddr_in sin; + int s; + + ATF_REQUIRE_KTLS(); + + s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ATF_REQUIRE(s >= 0); + build_tls_enable(tc, CRYPTO_AES_NIST_GCM_16, 128 / 8, 0, + TLS_MINOR_VER_THREE, (uint64_t)random(), &en); + ATF_REQUIRE(setsockopt(s, IPPROTO_TCP, TCP_TXTLS_ENABLE, &en, + sizeof(en)) == 0); + ATF_REQUIRE_ERRNO(EINVAL, listen(s, 1) == -1); + ATF_REQUIRE(close(s) == 0); + + s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ATF_REQUIRE(s >= 0); + build_tls_enable(tc, CRYPTO_AES_NIST_GCM_16, 128 / 8, 0, + TLS_MINOR_VER_THREE, (uint64_t)random(), &en); + ATF_REQUIRE(setsockopt(s, IPPROTO_TCP, TCP_RXTLS_ENABLE, &en, + sizeof(en)) == 0); + ATF_REQUIRE_ERRNO(EINVAL, listen(s, 1) == -1); + ATF_REQUIRE(close(s) == 0); + + s = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP); + ATF_REQUIRE(s >= 0); + memset(&sin, 0, sizeof(sin)); + ATF_REQUIRE(bind(s, (struct sockaddr *)&sin, sizeof(sin)) == 0); + ATF_REQUIRE(listen(s, 1) == 0); + build_tls_enable(tc, CRYPTO_AES_NIST_GCM_16, 128 / 8, 0, + 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, + setsockopt(s, IPPROTO_TCP, TCP_RXTLS_ENABLE, &en, sizeof(en)) != 0); + ATF_REQUIRE(close(s) == 0); +} + ATF_TP_ADD_TCS(tp) { /* Transmit tests */ @@ -2792,6 +2837,7 @@ ATF_TP_ADD_TCS(tp) /* Miscellaneous */ ATF_TP_ADD_TC(tp, ktls_sendto_baddst); + ATF_TP_ADD_TC(tp, ktls_listening_socket); return (atf_no_error()); }