From nobody Fri Mar 15 11:02:15 2024 X-Original-To: dev-commits-src-main@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 4Tx1YF1VPxz5D9XC; Fri, 15 Mar 2024 11:02:25 +0000 (UTC) (envelope-from markjdb@gmail.com) Received: from mail-pf1-x42b.google.com (mail-pf1-x42b.google.com [IPv6:2607:f8b0:4864:20::42b]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4Tx1YD4BGjz47YK; Fri, 15 Mar 2024 11:02:24 +0000 (UTC) (envelope-from markjdb@gmail.com) Authentication-Results: mx1.freebsd.org; dkim=pass header.d=gmail.com header.s=20230601 header.b=G1X1PvkQ; dmarc=fail reason="SPF not aligned (relaxed), DKIM not aligned (relaxed)" header.from=freebsd.org (policy=none); spf=pass (mx1.freebsd.org: domain of markjdb@gmail.com designates 2607:f8b0:4864:20::42b as permitted sender) smtp.mailfrom=markjdb@gmail.com Received: by mail-pf1-x42b.google.com with SMTP id d2e1a72fcca58-6e6b729669bso2070439b3a.3; Fri, 15 Mar 2024 04:02:24 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1710500540; x=1711105340; darn=freebsd.org; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:sender:from:to:cc:subject:date:message-id :reply-to; bh=s2Jt3SrTKpv3mydfkTLpkMiSqT8l1dx1rYm+ZGfIrOw=; b=G1X1PvkQfjNywHbtTNM0ArR0ks+I5Ch+Uq9gxT9oj2/kTraQLyCHWbuyij48pCw/HS voEQ/pQyN9yX6qGOfKP5WcTQDaAfYEJZs6kqyNbAcjqN1CFIOebe17jHz19x+faGeQIn BAY5M04SQ/CiUMozXwQfjCtaR1yaDxw6lAHNbIMXf+WhuANN5qezOP8w0wVJgAynKlwK gz4j7twGw3/yHP6RJhhZRzmLrq+sVXclos+6TH/kan8zVn3y+qQVHu5BIkuDxXJdU+Se kMFm+Y8quqfadYNH0r5GcrBso8eWjDjCp2dGhSLkLBN/2qx2qQUoB/BB1W1GLtqjZIVD MkdQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1710500540; x=1711105340; h=in-reply-to:content-disposition:mime-version:references:message-id :subject:cc:to:from:date:sender:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=s2Jt3SrTKpv3mydfkTLpkMiSqT8l1dx1rYm+ZGfIrOw=; b=ENFu/tQDBIE08R7zWVBErtReysbyWc4fc5YTumeSuTDeUKdp78hm5Q6VkOD7ry/mI3 Nl6OculUk/DOOP+hMoPOQTe39MlMXkrGr+LljrvMNoW2vEC3QTxNpWrJ1adT5tM8Vl90 qycOcuyXXuNB06x/wUGcoSv714ykbtizuW+ZiHHhfc76SgQnikjDCibJyamZzvrjMfo/ kkF9qyC9LgGAbx7DDh5DZUoV3Wgs5/Ek6hwHzWm/wTdTBiOxU7/IeOSdDxAdHYdDqnuK BsiNOSB9zH8ENiaDb7DWCN4b/OllyoRa4dHYsmNn0DK17QWg1+hcZBUluK8eQ3QrvzIB AGdg== X-Forwarded-Encrypted: i=1; AJvYcCXTjPS2r2wmmks1g56FH5zqeJqmPoj07rNDjed3uCUyP7vvrk2T145ut6l7zsVAOE4g6b1UB4bbUceNTcH49iIN85SBruwIoulQb/Ud+47ZvZNbA7H3REc2txxIWO0udSjaFURnspqwv6ngxmD7gz8ztw== X-Gm-Message-State: AOJu0Yzr3VCU5sVwKOJJqMaikMRv0sh20vbvwDYeycIIeAyrz8IjbSux FB7c7wiy0hWokmFRgUMPymeUZyiKrNUujmf+24UhSJ4UOvEC04eCk5JI+7zh X-Google-Smtp-Source: AGHT+IGFidREzzzggtaEgJrrgkqbFq2ni2lsQwJumCyYEzZUBOtSIcyDIEro2iARZ1cdrsSSN0qQnw== X-Received: by 2002:a05:6a00:3991:b0:6e6:977e:6427 with SMTP id fi17-20020a056a00399100b006e6977e6427mr5123922pfb.8.1710500540016; Fri, 15 Mar 2024 04:02:20 -0700 (PDT) Received: from framework.home (36-224-168-33.dynamic-ip.hinet.net. [36.224.168.33]) by smtp.gmail.com with ESMTPSA id fi37-20020a056a0039a500b006e69a142458sm3105468pfb.213.2024.03.15.04.02.18 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 15 Mar 2024 04:02:19 -0700 (PDT) Date: Fri, 15 Mar 2024 07:02:15 -0400 From: Mark Johnston To: Richard Scheffenegger Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Subject: Re: git: 85df11a1dec6 - main - ktls: deep copy tls_enable struct for in-kernel tcp consumers Message-ID: References: <202403132108.42DL8TbO057293@gitrepo.freebsd.org> List-Id: Commit messages for the main branch of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-main List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-main@freebsd.org X-BeenThere: dev-commits-src-main@freebsd.org MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <202403132108.42DL8TbO057293@gitrepo.freebsd.org> X-Spamd-Bar: --- X-Spamd-Result: default: False [-3.10 / 15.00]; NEURAL_HAM_MEDIUM(-1.00)[-1.000]; NEURAL_HAM_LONG(-1.00)[-1.000]; NEURAL_HAM_SHORT(-1.00)[-0.996]; FORGED_SENDER(0.30)[markj@freebsd.org,markjdb@gmail.com]; R_DKIM_ALLOW(-0.20)[gmail.com:s=20230601]; R_SPF_ALLOW(-0.20)[+ip6:2607:f8b0:4000::/36:c]; MIME_GOOD(-0.10)[text/plain]; DMARC_POLICY_SOFTFAIL(0.10)[freebsd.org : SPF not aligned (relaxed), DKIM not aligned (relaxed),none]; RCVD_COUNT_TWO(0.00)[2]; MIME_TRACE(0.00)[0:+]; TO_DN_SOME(0.00)[]; FROM_HAS_DN(0.00)[]; FREEMAIL_ENVFROM(0.00)[gmail.com]; MISSING_XM_UA(0.00)[]; DWL_DNSWL_NONE(0.00)[gmail.com:dkim]; ASN(0.00)[asn:15169, ipnet:2607:f8b0::/32, country:US]; RCVD_VIA_SMTP_AUTH(0.00)[]; RCVD_IN_DNSWL_NONE(0.00)[2607:f8b0:4864:20::42b:from]; RCVD_TLS_LAST(0.00)[]; TO_MATCH_ENVRCPT_ALL(0.00)[]; ARC_NA(0.00)[]; MLMMJ_DEST(0.00)[dev-commits-src-all@freebsd.org,dev-commits-src-main@freebsd.org]; RCPT_COUNT_THREE(0.00)[4]; FROM_NEQ_ENVFROM(0.00)[markj@freebsd.org,markjdb@gmail.com]; DKIM_TRACE(0.00)[gmail.com:+] X-Rspamd-Queue-Id: 4Tx1YD4BGjz47YK On Wed, Mar 13, 2024 at 09:08:29PM +0000, Richard Scheffenegger wrote: > The branch main has been updated by rscheff: > > URL: https://cgit.FreeBSD.org/src/commit/?id=85df11a1dec6eab9efbce9fd20712402a8e7ac7c > > commit 85df11a1dec6eab9efbce9fd20712402a8e7ac7c > Author: Richard Scheffenegger > AuthorDate: 2024-03-13 11:35:51 +0000 > Commit: Richard Scheffenegger > CommitDate: 2024-03-13 12:23:13 +0000 > > ktls: deep copy tls_enable struct for in-kernel tcp consumers > > Doing a deep copy of the keys early allows users of the > tls_enable structure to assume kernel memory. > This enables the socket options to be set by kernel threads. > > Reviewed By: #transport, tuexen, jhb, rrs > Sponsored by: NetApp, Inc. > X-NetApp-PR: #79 > Differential Revision: https://reviews.freebsd.org/D44250 > --- > sys/kern/uipc_ktls.c | 96 ++++++++++++++++++++++++++++++++++++++++-------- > sys/netinet/tcp_usrreq.c | 44 ++++------------------ > sys/sys/ktls.h | 17 +++++---- > 3 files changed, 97 insertions(+), 60 deletions(-) > > diff --git a/sys/kern/uipc_ktls.c b/sys/kern/uipc_ktls.c > index deba6940bbee..df296090ec97 100644 > --- a/sys/kern/uipc_ktls.c > +++ b/sys/kern/uipc_ktls.c > @@ -297,10 +297,86 @@ SYSCTL_COUNTER_U64(_kern_ipc_tls_toe, OID_AUTO, chacha20, CTLFLAG_RD, > > static MALLOC_DEFINE(M_KTLS, "ktls", "Kernel TLS"); > > +static void ktls_reclaim_thread(void *ctx); > static void ktls_reset_receive_tag(void *context, int pending); > static void ktls_reset_send_tag(void *context, int pending); > static void ktls_work_thread(void *ctx); > -static void ktls_reclaim_thread(void *ctx); > + > +int > +ktls_copyin_tls_enable(struct sockopt *sopt, struct tls_enable *tls) > +{ > + struct tls_enable_v0 tls_v0; > + int error; > + uint8_t *cipher_key = NULL, *iv = NULL, *auth_key = NULL; > + > + if (sopt->sopt_valsize == sizeof(tls_v0)) { > + error = sooptcopyin(sopt, &tls_v0, sizeof(tls_v0), sizeof(tls_v0)); > + if (error != 0) > + goto done; > + memset(tls, 0, sizeof(*tls)); > + tls->cipher_key = tls_v0.cipher_key; > + tls->iv = tls_v0.iv; > + tls->auth_key = tls_v0.auth_key; > + tls->cipher_algorithm = tls_v0.cipher_algorithm; > + tls->cipher_key_len = tls_v0.cipher_key_len; > + tls->iv_len = tls_v0.iv_len; > + tls->auth_algorithm = tls_v0.auth_algorithm; > + tls->auth_key_len = tls_v0.auth_key_len; > + tls->flags = tls_v0.flags; > + tls->tls_vmajor = tls_v0.tls_vmajor; > + tls->tls_vminor = tls_v0.tls_vminor; > + } else > + error = sooptcopyin(sopt, tls, sizeof(*tls), sizeof(*tls)); > + > + if (error != 0) > + goto done; > + > + /* > + * Now do a deep copy of the variable-length arrays in the struct, so that > + * subsequent consumers of it can reliably assume kernel memory. This > + * requires doing our own allocations, which we will free in the > + * error paths so that our caller need only worry about outstanding > + * allocations existing on successful return. > + */ > + cipher_key = malloc(tls->cipher_key_len, M_KTLS, M_WAITOK); > + iv = malloc(tls->iv_len, M_KTLS, M_WAITOK); > + auth_key = malloc(tls->auth_key_len, M_KTLS, M_WAITOK); Hi Richard, These lengths need to be validated against some maximum and minimum values, as they are provided by userspace and thus aren't to be trusted. See https://syzkaller.appspot.com/bug?extid=72022fa9163fa958b66c > + if (sopt->sopt_td != NULL) { > + error = copyin(tls->cipher_key, cipher_key, tls->cipher_key_len); > + if (error != 0) > + goto done; > + error = copyin(tls->iv, iv, tls->iv_len); > + if (error != 0) > + goto done; > + error = copyin(tls->auth_key, auth_key, tls->auth_key_len); > + if (error != 0) > + goto done; > + } else { > + bcopy(tls->cipher_key, cipher_key, tls->cipher_key_len); > + bcopy(tls->iv, iv, tls->iv_len); > + bcopy(tls->auth_key, auth_key, tls->auth_key_len); > + } > + tls->cipher_key = cipher_key; > + tls->iv = iv; > + tls->auth_key = auth_key; > + > +done: > + if (error != 0) { > + zfree(cipher_key, M_KTLS); > + zfree(iv, M_KTLS); > + zfree(auth_key, M_KTLS); > + } > + > + return (error); > +} > + > +void > +ktls_cleanup_tls_enable(struct tls_enable *tls) > +{ > + zfree(__DECONST(void *, tls->cipher_key), M_KTLS); > + zfree(__DECONST(void *, tls->iv), M_KTLS); > + zfree(__DECONST(void *, tls->auth_key), M_KTLS); > +} > > static u_int > ktls_get_cpu(struct socket *so) > @@ -702,18 +778,12 @@ ktls_create_session(struct socket *so, struct tls_enable *en, > tls->params.auth_key_len = en->auth_key_len; > tls->params.auth_key = malloc(en->auth_key_len, M_KTLS, > M_WAITOK); > - error = copyin(en->auth_key, tls->params.auth_key, > - en->auth_key_len); > - if (error) > - goto out; > + bcopy(en->auth_key, tls->params.auth_key, en->auth_key_len); > } > > tls->params.cipher_key_len = en->cipher_key_len; > tls->params.cipher_key = malloc(en->cipher_key_len, M_KTLS, M_WAITOK); > - error = copyin(en->cipher_key, tls->params.cipher_key, > - en->cipher_key_len); > - if (error) > - goto out; > + bcopy(en->cipher_key, tls->params.cipher_key, en->cipher_key_len); > > /* > * This holds the implicit portion of the nonce for AEAD > @@ -722,9 +792,7 @@ ktls_create_session(struct socket *so, struct tls_enable *en, > */ > if (en->iv_len != 0) { > tls->params.iv_len = en->iv_len; > - error = copyin(en->iv, tls->params.iv, en->iv_len); > - if (error) > - goto out; > + bcopy(en->iv, tls->params.iv, en->iv_len); > > /* > * For TLS 1.2 with GCM, generate an 8-byte nonce as a > @@ -740,10 +808,6 @@ ktls_create_session(struct socket *so, struct tls_enable *en, > > *tlsp = tls; > return (0); > - > -out: > - ktls_free(tls); > - return (error); > } > > static struct ktls_session * > diff --git a/sys/netinet/tcp_usrreq.c b/sys/netinet/tcp_usrreq.c > index a73d2a15c1d5..916fe33e8704 100644 > --- a/sys/netinet/tcp_usrreq.c > +++ b/sys/netinet/tcp_usrreq.c > @@ -1914,37 +1914,6 @@ CTASSERT(TCP_CA_NAME_MAX <= TCP_LOG_ID_LEN); > CTASSERT(TCP_LOG_REASON_LEN <= TCP_LOG_ID_LEN); > #endif > > -#ifdef KERN_TLS > -static int > -copyin_tls_enable(struct sockopt *sopt, struct tls_enable *tls) > -{ > - struct tls_enable_v0 tls_v0; > - int error; > - > - if (sopt->sopt_valsize == sizeof(tls_v0)) { > - error = sooptcopyin(sopt, &tls_v0, sizeof(tls_v0), > - sizeof(tls_v0)); > - if (error) > - return (error); > - memset(tls, 0, sizeof(*tls)); > - tls->cipher_key = tls_v0.cipher_key; > - tls->iv = tls_v0.iv; > - tls->auth_key = tls_v0.auth_key; > - tls->cipher_algorithm = tls_v0.cipher_algorithm; > - tls->cipher_key_len = tls_v0.cipher_key_len; > - tls->iv_len = tls_v0.iv_len; > - tls->auth_algorithm = tls_v0.auth_algorithm; > - tls->auth_key_len = tls_v0.auth_key_len; > - tls->flags = tls_v0.flags; > - tls->tls_vmajor = tls_v0.tls_vmajor; > - tls->tls_vminor = tls_v0.tls_vminor; > - return (0); > - } > - > - return (sooptcopyin(sopt, tls, sizeof(*tls), sizeof(*tls))); > -} > -#endif > - > extern struct cc_algo newreno_cc_algo; > > static int > @@ -2292,15 +2261,16 @@ unlock_and_done: > #ifdef KERN_TLS > case TCP_TXTLS_ENABLE: > INP_WUNLOCK(inp); > - error = copyin_tls_enable(sopt, &tls); > - if (error) > + error = ktls_copyin_tls_enable(sopt, &tls); > + if (error != 0) > break; > error = ktls_enable_tx(so, &tls); > + ktls_cleanup_tls_enable(&tls); > break; > case TCP_TXTLS_MODE: > INP_WUNLOCK(inp); > error = sooptcopyin(sopt, &ui, sizeof(ui), sizeof(ui)); > - if (error) > + if (error != 0) > return (error); > > INP_WLOCK_RECHECK(inp); > @@ -2309,11 +2279,11 @@ unlock_and_done: > break; > case TCP_RXTLS_ENABLE: > INP_WUNLOCK(inp); > - error = sooptcopyin(sopt, &tls, sizeof(tls), > - sizeof(tls)); > - if (error) > + error = ktls_copyin_tls_enable(sopt, &tls); > + if (error != 0) > break; > error = ktls_enable_rx(so, &tls); > + ktls_cleanup_tls_enable(&tls); > break; > #endif > case TCP_MAXUNACKTIME: > diff --git a/sys/sys/ktls.h b/sys/sys/ktls.h > index 693864394ffe..9b3433f4b1fd 100644 > --- a/sys/sys/ktls.h > +++ b/sys/sys/ktls.h > @@ -174,6 +174,7 @@ struct m_snd_tag; > struct mbuf; > struct sockbuf; > struct socket; > +struct sockopt; > > struct ktls_session { > struct ktls_ocf_session *ocf_session; > @@ -213,27 +214,29 @@ typedef enum { > } ktls_mbuf_crypto_st_t; > > void ktls_check_rx(struct sockbuf *sb); > -ktls_mbuf_crypto_st_t ktls_mbuf_crypto_state(struct mbuf *mb, int offset, int len); > +void ktls_cleanup_tls_enable(struct tls_enable *tls); > +int ktls_copyin_tls_enable(struct sockopt *sopt, struct tls_enable *tls); > void ktls_disable_ifnet(void *arg); > int ktls_enable_rx(struct socket *so, struct tls_enable *en); > int ktls_enable_tx(struct socket *so, struct tls_enable *en); > +void ktls_enqueue(struct mbuf *m, struct socket *so, int page_count); > +void ktls_enqueue_to_free(struct mbuf *m); > void ktls_destroy(struct ktls_session *tls); > void ktls_frame(struct mbuf *m, struct ktls_session *tls, int *enqueue_cnt, > uint8_t record_type); > -bool ktls_permit_empty_frames(struct ktls_session *tls); > -void ktls_seq(struct sockbuf *sb, struct mbuf *m); > -void ktls_enqueue(struct mbuf *m, struct socket *so, int page_count); > -void ktls_enqueue_to_free(struct mbuf *m); > int ktls_get_rx_mode(struct socket *so, int *modep); > -int ktls_set_tx_mode(struct socket *so, int mode); > int ktls_get_tx_mode(struct socket *so, int *modep); > int ktls_get_rx_sequence(struct inpcb *inp, uint32_t *tcpseq, uint64_t *tlsseq); > void ktls_input_ifp_mismatch(struct sockbuf *sb, struct ifnet *ifp); > -int ktls_output_eagain(struct inpcb *inp, struct ktls_session *tls); > +ktls_mbuf_crypto_st_t ktls_mbuf_crypto_state(struct mbuf *mb, int offset, int len); > #ifdef RATELIMIT > int ktls_modify_txrtlmt(struct ktls_session *tls, uint64_t max_pacing_rate); > #endif > +int ktls_output_eagain(struct inpcb *inp, struct ktls_session *tls); > bool ktls_pending_rx_info(struct sockbuf *sb, uint64_t *seqnop, size_t *residp); > +bool ktls_permit_empty_frames(struct ktls_session *tls); > +void ktls_seq(struct sockbuf *sb, struct mbuf *m); > +int ktls_set_tx_mode(struct socket *so, int mode); > > static inline struct ktls_session * > ktls_hold(struct ktls_session *tls) >