On 22/03/18 14:22, TJ wrote: > On 22/03/18 12:38, Daniel Kiper wrote: >> Hi John, >> >> On Wed, Mar 14, 2018 at 07:00:11PM +0000, John Lane wrote: >>> On 14/03/18 13:05, Daniel Kiper wrote: >>>> On Wed, Mar 14, 2018 at 09:44:58AM +0000, John Lane wrote: >>>>> From: John Lane <j...@lane.uk.net> >>>> >>>> I have just skimmed through the series. First of all, most if not >>>> all patches beg for full blown commit messages. Just vague statements >>>> in the subject are insufficient for me. And please add patch 0 which >>>> introduces the series. git send-email --compose is your friend. >>>> >>>> Daniel >>>> >>> >>> Sorry Daniel, this isn't something I do that often - submitting patches >> >> Not a problem. >> >>> to ML. Do you want me to resubmit them again, or is the below ok for you ? >> >> Please resubmit whole patch series and do not forget to take into >> account comments posted by others. >> >> Daniel > > I've spent a couple of days doing a thorough review of this patch series. > > Firstly I want to say a big thanks to John for bringing this along - > it's long been a large missing piece of the LUKS support.
Thanks for that. I will take a look at the below and try and work a patch series as you suggest. But it might take a few days to find some time to dedicate to it, due to other commitments I have right now. I'll report back when I have something to show. > > My observations: > > Break the series up. There are five distinct sets of functionality > change here: > a) LUKS detached headers > b) LUKS key files > c) allow multiple unlock attempts > d) plain dm-crypt > e) hyphens in UUIDs > > (a) and (b) are in reasonable shape but there's some work to do; mostly > in preparing the way for the new functionality by cleaning up error exit > paths in luks.c::luks_recover_key() first - which I've done and will attach. > > With that clean-up John's changes are easier to insert and verify. > > (c) creates a lot of churn just due to indenting code that is being > wrapped in a while() loop. I've refactored that so the loop is in a > separate function which reduces the patch from 323 to 41 lines. It's in > my branch detailed below. > > I'd suggest submitting (a) (b) and (c) as a series on their own. Get > them accepted and then introduce (e) and (d). I'd say (e) first since > it's relatively small. > > (d) is a major new tranch of functionality dealing with core > cryptographic constructs so will need careful review by cryptographers > as well as GRUB developers and therefore could take some time. It'd be a > shame to have this hold up the excellent improvements to LUKS which > don't touch the cryptographic operations. > > All in all an excellent contribution which I look forward to being > available. > > My "cryptomount_luks_v5" branch for the LUKS changes can be got using: > > git remote add iam.tj git://iam.tj/grub.git > git fetch iam.tj > > and seen at: > > http://iam.tj/gitweb/?p=grub.git;a=shortlog;h=refs/heads/cryptomount_luks_v5 > > > --- > commit 19896820737344fb820ab6487d16719e31cae763 > Author: TJ <grub-de...@iam.tj> > Date: Wed Mar 21 14:07:22 2018 +0000 > > LUKS: refactor multiple return paths > > Convert multiple return statements to goto with a single return so there > is only one place where memory needs to be free-d in error conditions. > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c > index 86c50c6..a7c5b39 100644 > --- a/grub-core/disk/luks.c > +++ b/grub-core/disk/luks.c > @@ -318,18 +318,23 @@ luks_recover_key (grub_disk_t source, > grub_uint8_t candidate_digest[sizeof (header.mkDigest)]; > unsigned i; > grub_size_t length; > - grub_err_t err; > + grub_err_t err = GRUB_ERR_NONE; > + char *err_msg = NULL; > grub_size_t max_stripes = 1; > char *tmp; > > err = grub_disk_read (source, 0, 0, sizeof (header), &header); > if (err) > - return err; > + goto fail; > > grub_puts_ (N_("Attempting to decrypt master key...")); > keysize = grub_be_to_cpu32 (header.keyBytes); > if (keysize > GRUB_CRYPTODISK_MAX_KEYLEN) > - return grub_error (GRUB_ERR_BAD_FS, "key is too long"); > + { > + err = GRUB_ERR_BAD_FS; > + err_msg = "key is too long"; > + goto fail; > + } > > for (i = 0; i < ARRAY_SIZE (header.keyblock); i++) > if (grub_be_to_cpu32 (header.keyblock[i].active) == LUKS_KEY_ENABLED > @@ -338,7 +343,10 @@ luks_recover_key (grub_disk_t source, > > split_key = grub_malloc (keysize * max_stripes); > if (!split_key) > - return grub_errno; > + { > + err = grub_errno; > + goto fail; > + } > > /* Get the passphrase from the user. */ > tmp = NULL; > @@ -350,8 +358,9 @@ luks_recover_key (grub_disk_t source, > grub_free (tmp); > if (!grub_password_get (passphrase, MAX_PASSPHRASE)) > { > - grub_free (split_key); > - return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied"); > + err = GRUB_ERR_BAD_ARGUMENT; > + err_msg = "Passphrase not supplied"; > + goto fail; > } > > /* Try to recover master key from each active keyslot. */ > @@ -378,8 +387,8 @@ luks_recover_key (grub_disk_t source, > > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > grub_dprintf ("luks", "PBKDF2 done\n"); > @@ -387,8 +396,8 @@ luks_recover_key (grub_disk_t source, > gcry_err = grub_cryptodisk_setkey (dev, digest, keysize); > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes)); > @@ -399,16 +408,13 @@ luks_recover_key (grub_disk_t source, > [i].keyMaterialOffset), 0, > length, split_key); > if (err) > - { > - grub_free (split_key); > - return err; > - } > + goto fail; > > gcry_err = grub_cryptodisk_decrypt (dev, split_key, length, 0); > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > /* Merge the decrypted key material to get the candidate master > key. */ > @@ -416,8 +422,8 @@ luks_recover_key (grub_disk_t source, > grub_be_to_cpu32 (header.keyblock[i].stripes)); > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > grub_dprintf ("luks", "candidate key recovered\n"); > @@ -433,8 +439,8 @@ luks_recover_key (grub_disk_t source, > sizeof (candidate_digest)); > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > /* Compare the calculated PBKDF2 to the digest stored > @@ -454,17 +460,19 @@ luks_recover_key (grub_disk_t source, > gcry_err = grub_cryptodisk_setkey (dev, candidate_key, keysize); > if (gcry_err) > { > - grub_free (split_key); > - return grub_crypto_gcry_error (gcry_err); > + err = grub_crypto_gcry_error (gcry_err); > + goto fail; > } > > - grub_free (split_key); > - > - return GRUB_ERR_NONE; > + err = GRUB_ERR_NONE; > } > > +fail: > grub_free (split_key); > - return GRUB_ACCESS_DENIED; > + if(err && err_msg) > + grub_error (err, errmsg); > + > + return err; > } > > struct grub_cryptodisk_dev luks_crypto = { > _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
