From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.71) id 1f0Qs8-0007E2-60 for mharc-grub-devel@gnu.org; Mon, 26 Mar 2018 08:05:08 -0400 Received: from eggs.gnu.org ([2001:4830:134:3::10]:59139) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1f0Qs0-0007Cz-BK for grub-devel@gnu.org; Mon, 26 Mar 2018 08:05:06 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1f0Qrv-0007pN-Qx for grub-devel@gnu.org; Mon, 26 Mar 2018 08:05:00 -0400 Received: from johnlane.plus.com ([212.159.104.145]:57098 helo=sodium.amajohn.co.uk) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1f0Qrv-0007ng-FP for grub-devel@gnu.org; Mon, 26 Mar 2018 08:04:55 -0400 Received: by sodium.amajohn.co.uk (Postfix, from userid 1000) id F1FFCAD4; Mon, 26 Mar 2018 13:04:44 +0100 (BST) Received: from [10.0.200.8] (oxygen.amajohn.co.uk [10.0.200.8]) by sodium.amajohn.co.uk (Postfix) with ESMTPSA id 286C7AD0; Mon, 26 Mar 2018 13:04:43 +0100 (BST) Subject: Re: [PATCH 1/7] Cryptomount support LUKS detached header To: TJ , grub-devel@gnu.org References: <20180314094504.6177-1-grub@jelmail.com> <20180314130551.GC21143@router-fw-old.local.net-space.pl> <593f8408-d54b-ab12-2660-f15d55418acb@jelmail.com> <20180322123824.GA7226@router-fw-old.local.net-space.pl> <42f5392e-575d-2d06-6476-006107773c71@iam.tj> From: John Lane Message-ID: <422cd640-4ddc-9c51-7ced-46a9d16846a3@jelmail.com> Date: Mon, 26 Mar 2018 14:10:48 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:52.0) Gecko/20100101 Thunderbird/52.6.0 MIME-Version: 1.0 In-Reply-To: <42f5392e-575d-2d06-6476-006107773c71@iam.tj> Content-Type: text/plain; charset=utf-8 Content-Language: en-GB Content-Transfer-Encoding: 7bit X-Outbound-Checked: Yes X-detected-operating-system: by eggs.gnu.org: GNU/Linux 3.x [fuzzy] X-Received-From: 212.159.104.145 X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 26 Mar 2018 12:05:06 -0000 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 >>>> >>>> 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 > 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 = { >