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. 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
