linux-kernel - Re: [PATCH v2] security/keys: rewrite all of big

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170607071053.GC29532@gondor.apana.org.au>
Date:   Wed, 7 Jun 2017 15:10:53 +0800
From:   Herbert Xu <herbert@...dor.apana.org.au>
To:     Eric Biggers <ebiggers3@...il.com>
Cc:     "Jason A. Donenfeld" <Jason@...c4.com>, keyrings@...r.kernel.org,
        kernel-hardening@...ts.openwall.com,
        LKML <linux-kernel@...r.kernel.org>,
        Theodore Ts'o <tytso@....edu>,
        David Howells <dhowells@...hat.com>,
        Kirill Marinushkin <k.marinushkin@...il.com>,
        security@...nel.org
Subject: Re: [PATCH v2] security/keys: rewrite all of big_key crypto

On Tue, Jun 06, 2017 at 09:22:19PM -0700, Eric Biggers wrote:
> Hi Jason,
> 
> On Tue, Jun 06, 2017 at 11:51:29PM +0200, Jason A. Donenfeld wrote:
> > issue now. And, some error paths forgot to zero out sensitive material, so
> > this patch changes a kfree into a kzfree.
> 
> There are other places in big_key.c that should be doing kzfree() instead of
> kfree().  Sorry, I actually recently did a patchset that fixed this for major
> parts of the keyrings API, but I skipped the big_key type because it was one of
> the more obscure key types (and frankly I have no idea what, if anything,
> actually uses it).  Probably the switch to kzfree() should be its own patch
> since it's a separate logical change.
> 
> >  {
> >  	int ret = -EINVAL;
> >  	struct scatterlist sgio;
> > -	SKCIPHER_REQUEST_ON_STACK(req, big_key_skcipher);
> > -
> > -	if (crypto_skcipher_setkey(big_key_skcipher, key, ENC_KEY_SIZE)) {
> > +	u8 req_on_stack[sizeof(struct aead_request) +
> > +			crypto_aead_reqsize(big_key_aead)];
> > +	struct aead_request *aead_req = (struct aead_request *)req_on_stack;
> > +
> 
> The crypto API with CONFIG_VMAP_STACK=y and CONFIG_DEBUG_SG=y is unhappy with
> using an aead_request on the stack, because it can't create a scatterlist from
> it.  It will need to be on the heap instead.  (Or else the crypto API fixed.)

You should not create AEAD requests on the stack.  The only reason
skcipher requests can be created on the stack is for backwards
compatibility with the sync blkcipher interface.

For new code you should always allocate the request using
aead_request_alloc.

Thanks,
-- 
Email: Herbert Xu <herbert@...dor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt