Follow @Openwall on Twitter for new release announcements and other news
[<prev] [next>] [day] [month] [year] [list]
Date: Tue, 6 Jun 2017 23:31:31 +0200
From: "Jason A. Donenfeld" <>
To: Eric Biggers <>
Cc:, David Howells <>, 
	Herbert Xu <>, Kirill Marinushkin <>,,, "Theodore Ts'o" <>, 
	LKML <>,
Subject: Re: [PATCH] security/keys: rewrite all of big_key crypto

On Tue, Jun 6, 2017 at 10:58 PM, Eric Biggers <> wrote:
> No need to select CRYPTO_AEAD; it's already selected by CRYPTO_GCM.


> Actually I just noticed another bug, which I suppose you might as well fix too.
> Because different big_keys may be added or read concurrently, and each is
> encrypted using a different encryption key, it's not safe to use a global
> 'crypto_aead'.  Instead, a separate crypto_aead must be created for each key, or
> for each encryption/decryption, or else a mutex needs to be held during each
> rekeying and encryption/decryption.  For what it's worth, I recently fixed a
> similar bug for the "encrypted" key type:

I noticed that too, but I cynically supposed the flaw in the old code
could carry over to the new code without too much difficulty... But
sure, I'll fix this.

This is a real flaw with the crypto API, IMHO. It seems like the key
should be part of the request rather than the tfm. I understand some
primitives can do per-key precomputations, but still, this is a real

Will be fixed in the next revision.

> memset(req_on_stack, 0, sizeof(req_on_stack));
> memzero_explicit(req_on_stack, sizeof(req_on_stack));

I didn't really want to do sizeof with a VLA, but actually, it looks
like gcc does the right thing, so I'll make that change.

> It isn't "aligned" anymore --- instead, it's leaving room for the authentiation
> tag.  I don't think it even needs to be zeroed, does it?  Can you update the
> code and comments below this?


> I think these fixes are going to have to be done separately from the switch to
> get_random_bytes_wait().  Just make it use get_random_bytes() for now, and
> switch it to get_random_bytes_wait() later in a separate patch.

Blah, I really dislike doing that. Can I just roll it into the RNG
patchset, and then it'll go into Ted's tree with DavidH's sign-off?

> Please also add a brief comment that anyone who might try to add an update()
> method will find, e.g.:
> struct key_type key_type_big_key = {
> [...]
>         .describe               = big_key_describe,
>         .read                   = big_key_read,
>         /* no ->update() yet; don't add it without changing big_key_crypt() */
> };
> Otherwise someone will add one, and the crypto will be broken again.

Great idea, again.

Thanks a bunch for the review!


Powered by blists - more mailing lists

Confused about mailing lists and their use? Read about mailing lists on Wikipedia and check out these guidelines on proper formatting of your messages.