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

[John Lane]

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

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 <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 = {
>