On Thu, Sep 28, 2017 at 09:14:58AM +1000, James Morris wrote: > On Wed, 27 Sep 2017, David Howells wrote: > > > (2) Fixing big_key to use safe crypto from Jason A. Donenfeld. > > > > I'm concerned about the lack of crypto review mentioned by Jason -- I > wonder if we can get this rewrite any more review from crypto folk. > > Also, are there any tests for this code? If not, it would be good to make > some. > There is a test for the big_key key type in the keyutils test suite. I also manually tested Jason's change. And as far as I can tell there isn't actually a whole lot to test besides adding a big_key larger than BIG_KEY_FILE_THRESHOLD bytes, reading it back, and verifying that the data is unchanged --- since that covers the code that was changed. An earlier version of the patch produced a warning with CONFIG_DEBUG_SG=y since it put the aead_request on the stack, but that's been fixed. It would be great if someone else would comment on the crypto too, but for what it's worth I'm satisfied with the crypto changes. GCM is a much better choice than ECB as long as we don't repeat (key, IV) pairs --- which we don't. And in any case ECB mode makes no sense in this context; you'd need a *very* good reason to actually choose to encrypt something with ECB mode. Unfortunately it tends to be a favorite of people who don't understand encryption modes... Plus, getting all the randomness at boot time didn't make sense because that's when entropy is the most scarce. Eric
