From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.3 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 40A24C2BB85 for ; Tue, 14 Apr 2020 19:16:23 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 188632078A for ; Tue, 14 Apr 2020 19:16:23 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="i3H3W1kH" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 188632078A Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=suse.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:In-Reply-To:MIME-Version:References: Message-ID:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=4m0V+YqPuomNac0sC7478nUizEjEXtT5fqGke1JzrP8=; b=i3H3W1kH/u3Fy7 sj7PsciE0h2nGrrFxxPv/sx3liOe3BzNR7OVDtLuc+nEoNyrz0CFeARzObrG2JgiKCPzYFYLKfk4D 8ebdVM7yH+Bkud5NPKLDN6Ri4YRJ2O2C2zCVoa3kOaGOVajlvrQHgTo3NTqNdSEvqvnLBvzuscSiO /4Z6qpnJpcQeQm/aSmqN1yhrcNmlCOgjJvNQvfvzIsDes0TpOg/20yz8pjh5Dum9RMnSwO+OnVeP5 siq1kbcLMiWKJdrQllwAfDcploT9O85jVmU5tK04FO+9ZgJPQN6CXU+FbXJM68AbqeIT9nGs2BjQq Dd3Dt9qPiyQ/4gPo1KMw==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.92.3 #3 (Red Hat Linux)) id 1jOR2e-0007xX-CF; Tue, 14 Apr 2020 19:16:16 +0000 Received: from mx2.suse.de ([195.135.220.15]) by bombadil.infradead.org with esmtps (Exim 4.92.3 #3 (Red Hat Linux)) id 1jOR2a-0007wT-PI; Tue, 14 Apr 2020 19:16:14 +0000 X-Virus-Scanned: by amavisd-new at test-mx.suse.de Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 63506AC2C; Tue, 14 Apr 2020 19:16:04 +0000 (UTC) Date: Tue, 14 Apr 2020 21:16:01 +0200 From: Michal =?iso-8859-1?Q?Such=E1nek?= To: Waiman Long Subject: Re: [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit() Message-ID: <20200414191601.GZ25468@kitsune.suse.cz> References: <20200413211550.8307-1-longman@redhat.com> <20200413222846.24240-1-longman@redhat.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.10.1 (2018-07-13) X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20200414_121613_110180_F3B2FA90 X-CRM114-Status: GOOD ( 21.23 ) X-BeenThere: linux-amlogic@lists.infradead.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-btrfs@vger.kernel.org, Jarkko Sakkinen , virtualization@lists.linux-foundation.org, David Howells , linux-mm@kvack.org, linux-sctp@vger.kernel.org, keyrings@vger.kernel.org, kasan-dev@googlegroups.com, samba-technical@lists.samba.org, linux-stm32@st-md-mailman.stormreply.com, devel@driverdev.osuosl.org, linux-s390@vger.kernel.org, linux-scsi@vger.kernel.org, x86@kernel.org, James Morris , Matthew Wilcox , cocci@systeme.lip6.fr, linux-wpan@vger.kernel.org, intel-wired-lan@lists.osuosl.org, David Rientjes , "Serge E. Hallyn" , linux-pm@vger.kernel.org, ecryptfs@vger.kernel.org, linux-nfs@vger.kernel.org, linux-fscrypt@vger.kernel.org, linux-mediatek@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-integrity@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Christophe Leroy , linux-cifs@vger.kernel.org, Linus Torvalds , linux-wireless@vger.kernel.org, linux-kernel@vger.kernel.org, linux-bluetooth@vger.kernel.org, linux-security-module@vger.kernel.org, target-devel@vger.kernel.org, tipc-discussion@lists.sourceforge.net, linux-crypto@vger.kernel.org, netdev@vger.kernel.org, Joe Perches , Andrew Morton , linuxppc-dev@lists.ozlabs.org, wireguard@lists.zx2c4.com, linux-ppp@vger.kernel.org Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Sender: "linux-amlogic" Errors-To: linux-amlogic-bounces+linux-amlogic=archiver.kernel.org@lists.infradead.org On Tue, Apr 14, 2020 at 12:24:36PM -0400, Waiman Long wrote: > On 4/14/20 2:08 AM, Christophe Leroy wrote: > > > > > > Le 14/04/2020 =E0 00:28, Waiman Long a =E9crit=A0: > >> Since kfree_sensitive() will do an implicit memzero_explicit(), there > >> is no need to call memzero_explicit() before it. Eliminate those > >> memzero_explicit() and simplify the call sites. For better correctness, > >> the setting of keylen is also moved down after the key pointer check. > >> > >> Signed-off-by: Waiman Long > >> --- > >> =A0 .../allwinner/sun8i-ce/sun8i-ce-cipher.c=A0=A0=A0=A0=A0 | 19 +++++= ------------- > >> =A0 .../allwinner/sun8i-ss/sun8i-ss-cipher.c=A0=A0=A0=A0=A0 | 20 +++++= -------------- > >> =A0 drivers/crypto/amlogic/amlogic-gxl-cipher.c=A0=A0 | 12 +++-------- > >> =A0 drivers/crypto/inside-secure/safexcel_hash.c=A0 |=A0 3 +-- > >> =A0 4 files changed, 14 insertions(+), 40 deletions(-) > >> > >> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c > >> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c > >> index aa4e8fdc2b32..8358fac98719 100644 > >> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c > >> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c > >> @@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm) > >> =A0 { > >> =A0=A0=A0=A0=A0 struct sun8i_cipher_tfm_ctx *op =3D crypto_tfm_ctx(tfm= ); > >> =A0 -=A0=A0=A0 if (op->key) { > >> -=A0=A0=A0=A0=A0=A0=A0 memzero_explicit(op->key, op->keylen); > >> -=A0=A0=A0=A0=A0=A0=A0 kfree(op->key); > >> -=A0=A0=A0 } > >> +=A0=A0=A0 kfree_sensitive(op->key); > >> =A0=A0=A0=A0=A0 crypto_free_sync_skcipher(op->fallback_tfm); > >> =A0=A0=A0=A0=A0 pm_runtime_put_sync_suspend(op->ce->dev); > >> =A0 } > >> @@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher > >> *tfm, const u8 *key, > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 dev_dbg(ce->dev, "ERROR: Invalid keylen %u= \n", keylen); > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 return -EINVAL; > >> =A0=A0=A0=A0=A0 } > >> -=A0=A0=A0 if (op->key) { > >> -=A0=A0=A0=A0=A0=A0=A0 memzero_explicit(op->key, op->keylen); > >> -=A0=A0=A0=A0=A0=A0=A0 kfree(op->key); > >> -=A0=A0=A0 } > >> -=A0=A0=A0 op->keylen =3D keylen; > >> +=A0=A0=A0 kfree_sensitive(op->key); > >> =A0=A0=A0=A0=A0 op->key =3D kmemdup(key, keylen, GFP_KERNEL | GFP_DMA); > >> =A0=A0=A0=A0=A0 if (!op->key) > >> =A0=A0=A0=A0=A0=A0=A0=A0=A0 return -ENOMEM; > >> +=A0=A0=A0 op->keylen =3D keylen; > > > > Does it matter at all to ensure op->keylen is not set when of->key is > > NULL ? I'm not sure. > > > > But if it does, then op->keylen should be set to 0 when freeing op->key= . = > = > My thinking is that if memory allocation fails, we just don't touch > anything and return an error code. I will not explicitly set keylen to 0 > in this case unless it is specified in the API documentation. You already freed the key by now so not touching anything is not possible. The key is set to NULL on allocation failure so setting keylen to 0 should be redundant. However, setting keylen to 0 is consisent with not having a key, and it avoids the possibility of leaking the length later should that ever cause any problem. Thanks Michal _______________________________________________ linux-amlogic mailing list linux-amlogic@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-amlogic