On Tue, Jun 6, 2017 at 10:58 PM, Eric Biggers <ebiggers3@xxxxxxxxx> wrote: > No need to select CRYPTO_AEAD; it's already selected by CRYPTO_GCM. Ack. > > 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: > https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/commit/?h=keys-fixes&id=69307a05d4d58f4c29aa7e9d83dc59d63e28c382 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 wart. 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? Ack. > 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! Jason