Linux-FSCrypt Archive on lore.kernel.org
 help / color / Atom feed
From: "Michal Suchánek" <msuchanek@suse.de>
To: Waiman Long <longman@redhat.com>
Cc: Christophe Leroy <christophe.leroy@c-s.fr>,
	Andrew Morton <akpm@linux-foundation.org>,
	David Howells <dhowells@redhat.com>,
	Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com>,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Joe Perches <joe@perches.com>,
	Matthew Wilcox <willy@infradead.org>,
	David Rientjes <rientjes@google.com>,
	linux-mm@kvack.org, keyrings@vger.kernel.org,
	linux-kernel@vger.kernel.org, x86@kernel.org,
	linux-crypto@vger.kernel.org, linux-s390@vger.kernel.org,
	linux-pm@vger.kernel.org,
	linux-stm32@st-md-mailman.stormreply.com,
	linux-arm-kernel@lists.infradead.org,
	linux-amlogic@lists.infradead.org,
	linux-mediatek@lists.infradead.org,
	linuxppc-dev@lists.ozlabs.org,
	virtualization@lists.linux-foundation.org,
	netdev@vger.kernel.org, intel-wired-lan@lists.osuosl.org,
	linux-ppp@vger.kernel.org, wireguard@lists.zx2c4.com,
	linux-wireless@vger.kernel.org, devel@driverdev.osuosl.org,
	linux-scsi@vger.kernel.org, target-devel@vger.kernel.org,
	linux-btrfs@vger.kernel.org, linux-cifs@vger.kernel.org,
	samba-technical@lists.samba.org, linux-fscrypt@vger.kernel.org,
	ecryptfs@vger.kernel.org, kasan-dev@googlegroups.com,
	linux-bluetooth@vger.kernel.org, linux-wpan@vger.kernel.org,
	linux-sctp@vger.kernel.org, linux-nfs@vger.kernel.org,
	tipc-discussion@lists.sourceforge.net, cocci@systeme.lip6.fr,
	linux-security-module@vger.kernel.org,
	linux-integrity@vger.kernel.org
Subject: Re: [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()
Date: Tue, 14 Apr 2020 21:16:01 +0200
Message-ID: <20200414191601.GZ25468@kitsune.suse.cz> (raw)
In-Reply-To: <e194a51f-a5e5-a557-c008-b08cac558572@redhat.com>

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 à 00:28, Waiman Long a écrit :
> >> 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 <longman@redhat.com>
> >> ---
> >>   .../allwinner/sun8i-ce/sun8i-ce-cipher.c      | 19 +++++-------------
> >>   .../allwinner/sun8i-ss/sun8i-ss-cipher.c      | 20 +++++--------------
> >>   drivers/crypto/amlogic/amlogic-gxl-cipher.c   | 12 +++--------
> >>   drivers/crypto/inside-secure/safexcel_hash.c  |  3 +--
> >>   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)
> >>   {
> >>       struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
> >>   -    if (op->key) {
> >> -        memzero_explicit(op->key, op->keylen);
> >> -        kfree(op->key);
> >> -    }
> >> +    kfree_sensitive(op->key);
> >>       crypto_free_sync_skcipher(op->fallback_tfm);
> >>       pm_runtime_put_sync_suspend(op->ce->dev);
> >>   }
> >> @@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher
> >> *tfm, const u8 *key,
> >>           dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
> >>           return -EINVAL;
> >>       }
> >> -    if (op->key) {
> >> -        memzero_explicit(op->key, op->keylen);
> >> -        kfree(op->key);
> >> -    }
> >> -    op->keylen = keylen;
> >> +    kfree_sensitive(op->key);
> >>       op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
> >>       if (!op->key)
> >>           return -ENOMEM;
> >> +    op->keylen = 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

  reply index

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-04-13 21:15 [PATCH 0/2] mm, treewide: Rename kzfree() to kfree_sensitive() Waiman Long
2020-04-13 21:15 ` [PATCH 1/2] " Waiman Long
2020-04-14  0:29   ` David Rientjes
2020-04-14  8:32   ` Jason A. Donenfeld
2020-04-14  9:01   ` Michal Hocko
2020-04-14 12:48   ` David Sterba
2020-04-14 18:26     ` Waiman Long
2020-04-15  5:01   ` Johannes Weiner
2020-06-15 18:07   ` Dan Carpenter
2020-06-15 18:39     ` Waiman Long
2020-04-13 21:15 ` [PATCH 2/2] crypto: Remove unnecessary memzero_explicit() Waiman Long
2020-04-13 21:31   ` Joe Perches
2020-04-13 21:52     ` Waiman Long
2020-04-13 22:28 ` [PATCH v2 " Waiman Long
2020-04-14  6:08   ` Christophe Leroy
2020-04-14 16:24     ` Waiman Long
2020-04-14 19:16       ` Michal Suchánek [this message]
2020-04-14 19:37         ` Waiman Long
2020-04-14 19:44           ` Joe Perches
2020-04-14 13:06 ` [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive() David Howells

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200414191601.GZ25468@kitsune.suse.cz \
    --to=msuchanek@suse.de \
    --cc=akpm@linux-foundation.org \
    --cc=christophe.leroy@c-s.fr \
    --cc=cocci@systeme.lip6.fr \
    --cc=devel@driverdev.osuosl.org \
    --cc=dhowells@redhat.com \
    --cc=ecryptfs@vger.kernel.org \
    --cc=intel-wired-lan@lists.osuosl.org \
    --cc=jarkko.sakkinen@linux.intel.com \
    --cc=jmorris@namei.org \
    --cc=joe@perches.com \
    --cc=kasan-dev@googlegroups.com \
    --cc=keyrings@vger.kernel.org \
    --cc=linux-amlogic@lists.infradead.org \
    --cc=linux-arm-kernel@lists.infradead.org \
    --cc=linux-bluetooth@vger.kernel.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-fscrypt@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mediatek@lists.infradead.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-pm@vger.kernel.org \
    --cc=linux-ppp@vger.kernel.org \
    --cc=linux-s390@vger.kernel.org \
    --cc=linux-scsi@vger.kernel.org \
    --cc=linux-sctp@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-stm32@st-md-mailman.stormreply.com \
    --cc=linux-wireless@vger.kernel.org \
    --cc=linux-wpan@vger.kernel.org \
    --cc=linuxppc-dev@lists.ozlabs.org \
    --cc=longman@redhat.com \
    --cc=netdev@vger.kernel.org \
    --cc=rientjes@google.com \
    --cc=samba-technical@lists.samba.org \
    --cc=serge@hallyn.com \
    --cc=target-devel@vger.kernel.org \
    --cc=tipc-discussion@lists.sourceforge.net \
    --cc=torvalds@linux-foundation.org \
    --cc=virtualization@lists.linux-foundation.org \
    --cc=willy@infradead.org \
    --cc=wireguard@lists.zx2c4.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-FSCrypt Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-fscrypt/0 linux-fscrypt/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-fscrypt linux-fscrypt/ https://lore.kernel.org/linux-fscrypt \
		linux-fscrypt@vger.kernel.org
	public-inbox-index linux-fscrypt

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-fscrypt


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git