grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 1/7] Cryptomount support LUKS detached header


From: TJ
Subject: Re: [PATCH 1/7] Cryptomount support LUKS detached header
Date: Thu, 22 Mar 2018 14:22:13 +0000
User-agent: Mozilla/5.0 (X11; Linux x86_64; rv:52.0) Gecko/20100101 Thunderbird/52.6.0

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 <address@hidden>
>>>
>>> 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 <address@hidden>
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 = {



reply via email to

[Prev in Thread] Current Thread [Next in Thread]