grub-devel
[Top][All Lists]
Advanced

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

Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cr


From: Glenn Washburn
Subject: Re: [PATCH v7 12/17] luks2: Better error handling when setting up the cryptodisk
Date: Mon, 7 Dec 2020 22:04:01 -0600

On Sun, 6 Dec 2020 20:35:13 +0100
Patrick Steinhardt <ps@pks.im> wrote:

> On Fri, Dec 04, 2020 at 10:43:41AM -0600, Glenn Washburn wrote:
> > First, check to make sure that source disk has a known size. If
> > not, print debug message and return error. There are 4 cases where
> > GRUB_DISK_SIZE_UNKNOWN is set (biosdisk, obdisk, ofdisk, and
> > uboot), and in all those cases processing continues. So this is
> > probably a bit conservative. However, 3 of the cases seem
> > pathological, and the other, biosdisk, happens when booting from a
> > cd. Since I doubt booting from a LUKS2 volume on a cd is a big use
> > case, we'll error until someone complains.
> > 
> > Do some sanity checking on data coming from the luks header. If
> > segment.size is "dynamic",
> 
> Nit: there's something missing here.

Yep, thanks for catching that. I was going to complete this and forgot
in my rush to get the series out before some travel.  I'll rework that.

> > Check for errors from grub_strtoull when converting segment size
> > from string. If a GRUB_ERR_BAD_NUMBER error was returned, then the
> > string was not a valid parsable number, so skip the key. If
> > GRUB_ERR_OUT_OF_RANGE was returned, then there was an overflow in
> > converting to a 64-bit unsigned integer. So this could be a very
> > large disk (perhaps large raid array). In this case, we want to
> > continue to try to use this key so long as the source disk's size
> > is greater than this segment size. Otherwise, this is an invalid
> > segment, and continue on to the next key.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/luks2.c | 98
> > +++++++++++++++++++++++++++++++++++++----- include/grub/disk.h    |
> > 17 ++++++++ 2 files changed, 105 insertions(+), 10 deletions(-)
> > 
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index de2e70796..1bb3a333d 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -290,7 +290,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > grub_luks2_digest_t *d, grub_luks2_s break;
> >      }
> >    if (i == size)
> > -      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->slot_key);
> > +      return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No digest for
> > keyslot \"%"PRIuGRUB_UINT64_T"\"", k->json_slot_key); 
> >    /* Get segment that matches the digest. */
> >    if (grub_json_getvalue (&segments, root, "segments") ||
> > @@ -308,7 +308,7 @@ luks2_get_keyslot (grub_luks2_keyslot_t *k,
> > grub_luks2_digest_t *d, grub_luks2_s break;
> >      }
> >    if (i == size)
> > -    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > digest \"%"PRIuGRUB_UINT64_T"\"", d->slot_key);
> > +    return grub_error (GRUB_ERR_FILE_NOT_FOUND, "No segment for
> > digest \"%"PRIuGRUB_UINT64_T"\"", d->json_slot_key); 
> >    return GRUB_ERR_NONE;
> >  }
> > @@ -600,37 +600,115 @@ luks2_recover_key (grub_disk_t source,
> >        goto err;
> >      }
> >  
> > +  if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> > +    {
> > +      /* FIXME: Allow use of source disk, and maybe cause errors
> > in read. */
> > +      grub_dprintf ("luks2", "Source disk %s has an unknown size, "
> > +                        "conservatively returning error\n",
> > source->name);
> > +      ret = grub_error (GRUB_ERR_BUG, "Unknown size of luks2
> > source device");
> > +      goto err;
> > +    }
> > +
> >    /* Try all keyslot */
> >    for (i = 0; i < size; i++)
> >      {
> > +      typeof(source->total_sectors) max_crypt_sectors = 0;
> 
> Please bear with me if this has been discussed in a previous round,
> but why exactly do we cast `max_crypt_sectors` to the type of
> `source->total_sectors`?

Technically, this isn't a cast.  Its a variable declaration.  But I'm
using the typeof(source->total_sectors) because max_crypt_sectors can
be no more or less than the total sectors, ie its of the same type.

> And isn't the variable always set anyway in
> case the keyslot has a non-zero priority?

Yep. Are you suggesting that it need not be initialized?  This is true,
but I don't think that's a problem.  I think generally initializing
things is a good practice.  It might be problematic if this was in an
oft used function (or it might not, would need to see if the compiler
was smart enough to ignore the initialization).  But that
initialization is going to happen very rarely in the lifetime of a grub
execution instance.  I also don't really care either way.

Glenn

> Patrick
>
> > +
> > +      grub_errno = GRUB_ERR_NONE;
> >        ret = luks2_get_keyslot (&keyslot, &digest, &segment, json,
> > i); if (ret)
> >     goto err;
> > +      if (grub_errno != GRUB_ERR_NONE)
> > +     grub_dprintf ("luks2", "Ignoring unhandled error %d from
> > luks2_get_keyslot\n", grub_errno); 
> >        if (keyslot.priority == 0)
> >     {
> > -     grub_dprintf ("luks2", "Ignoring keyslot
> > \"%"PRIuGRUB_UINT64_T"\" due to priority\n", keyslot.slot_key);
> > +     grub_dprintf ("luks2", "Ignoring keyslot
> > \"%"PRIuGRUB_UINT64_T"\" due to priority\n",
> > keyslot.json_slot_key); continue; }
> >  
> > -      grub_dprintf ("luks2", "Trying keyslot
> > \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.slot_key);
> > +      grub_dprintf ("luks2", "Trying keyslot
> > \"%"PRIuGRUB_UINT64_T"\"\n", keyslot.json_slot_key); 
> >        /* Set up disk according to keyslot's segment. */
> >        crypt->offset_sectors = grub_divmod64 (segment.offset,
> > segment.sector_size, NULL); crypt->log_sector_size = sizeof
> > (unsigned int) * 8
> >             - __builtin_clz ((unsigned int)
> > segment.sector_size) - 1;
> > +      /* Set to the source disk size, which is the maximum we
> > allow. */
> > +      max_crypt_sectors = grub_disk_convert_sector(source,
> > +
> > source->total_sectors,
> > +
> > crypt->log_sector_size); +
> > +      if (max_crypt_sectors < crypt->offset_sectors)
> > +   {
> > +     grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has offset"
> > +                            " %"PRIuGRUB_UINT64_T" which is
> > greater than"
> > +                            " source disk size
> > %"PRIuGRUB_UINT64_T","
> > +                            " skipping\n",
> > +                            segment.json_slot_key,
> > crypt->offset_sectors,
> > +                            max_crypt_sectors);
> > +     continue;
> > +   }
> > +
> >        if (grub_strcmp (segment.size, "dynamic") == 0)
> > -   crypt->total_sectors = (grub_disk_get_size (source) >>
> > (crypt->log_sector_size - source->log_sector_size))
> > -                          - crypt->offset_sectors;
> > +   crypt->total_sectors = max_crypt_sectors -
> > crypt->offset_sectors; else
> > -   crypt->total_sectors = grub_strtoull (segment.size, NULL,
> > 10) >> crypt->log_sector_size;
> > +   {
> > +     grub_errno = GRUB_ERR_NONE;
> > +     /* Convert segment.size to sectors, rounding up to
> > nearest sector */
> > +     crypt->total_sectors = grub_strtoull (segment.size,
> > NULL, 10);
> > +     crypt->total_sectors = ALIGN_UP (crypt->total_sectors,
> > +                                      1 <<
> > crypt->log_sector_size);
> > +     crypt->total_sectors >>= crypt->log_sector_size;
> > +
> > +     if (grub_errno == GRUB_ERR_NONE)
> > +       ;
> > +     else if (grub_errno == GRUB_ERR_BAD_NUMBER)
> > +       {
> > +         grub_dprintf ("luks2", "Segment
> > \"%"PRIuGRUB_UINT64_T"\" size"
> > +                                " \"%s\" is not a parsable
> > number\n",
> > +                                segment.json_slot_key,
> > segment.size);
> > +         continue;
> > +       }
> > +     else if (grub_errno == GRUB_ERR_OUT_OF_RANGE)
> > +       {
> > +         /*
> > +          * There was an overflow in parsing segment.size, so
> > disk must
> > +          * be very large or the string is incorrect.
> > +          */
> > +         grub_dprintf ("luks2", "Segment
> > \"%"PRIuGRUB_UINT64_T"\" size"
> > +                                " %s overflowed 64-bit
> > unsigned integer,"
> > +                                " the end of the crypto
> > device will be"
> > +                                " inaccessible\n",
> > +                                segment.json_slot_key,
> > segment.size);
> > +         if (crypt->total_sectors > max_crypt_sectors)
> > +           crypt->total_sectors = max_crypt_sectors;
> > +       }
> > +   }
> > +
> > +      if (crypt->total_sectors == 0)
> > +   {
> > +     grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has zero"
> > +                            " sectors, skipping\n",
> > +                            segment.json_slot_key);
> > +     continue;
> > +   }
> > +      else if (max_crypt_sectors < (crypt->offset_sectors +
> > crypt->total_sectors))
> > +   {
> > +     grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\"
> > has last"
> > +                            " data position greater than
> > source disk size,"
> > +                            " the end of the crypto device
> > will be"
> > +                            " inaccessible\n",
> > +                            segment.json_slot_key);
> > +     /* Allow decryption up to the end of the source disk. */
> > +     crypt->total_sectors = max_crypt_sectors -
> > crypt->offset_sectors;
> > +   }
> >  
> >        ret = luks2_decrypt_key (candidate_key, source, crypt,
> > &keyslot, (const grub_uint8_t *) passphrase, grub_strlen
> > (passphrase)); if (ret)
> >     {
> >       grub_dprintf ("luks2", "Decryption with keyslot
> > \"%"PRIuGRUB_UINT64_T"\" failed: %s\n",
> > -                   keyslot.slot_key, grub_errmsg);
> > +                   keyslot.json_slot_key, grub_errmsg);
> >       continue;
> >     }
> >  
> > @@ -638,7 +716,7 @@ luks2_recover_key (grub_disk_t source,
> >        if (ret)
> >     {
> >       grub_dprintf ("luks2", "Could not open keyslot
> > \"%"PRIuGRUB_UINT64_T"\": %s\n",
> > -                   keyslot.slot_key, grub_errmsg);
> > +                   keyslot.json_slot_key, grub_errmsg);
> >       continue;
> >     }
> >  
> > @@ -646,7 +724,7 @@ luks2_recover_key (grub_disk_t source,
> >         * TRANSLATORS: It's a cryptographic key slot: one element
> > of an array
> >         * where each element is either empty or holds a key.
> >         */
> > -      grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"),
> > keyslot.slot_key);
> > +      grub_printf_ (N_("Slot \"%"PRIuGRUB_UINT64_T"\" opened\n"),
> > keyslot.json_slot_key); 
> >        candidate_key_len = keyslot.key_size;
> >        break;
> > diff --git a/include/grub/disk.h b/include/grub/disk.h
> > index 0fb727d3d..f9227f285 100644
> > --- a/include/grub/disk.h
> > +++ b/include/grub/disk.h
> > @@ -27,6 +27,8 @@
> >  #include <grub/device.h>
> >  /* For NULL.  */
> >  #include <grub/mm.h>
> > +/* For ALIGN_UP.  */
> > +#include <grub/misc.h>
> >  
> >  /* These are used to set a device id. When you add a new disk
> > device, you must define a new id for it here.  */
> > @@ -174,6 +176,21 @@ typedef struct grub_disk_memberlist
> > *grub_disk_memberlist_t; /* Return value of
> > grub_disk_native_sectors() in case disk size is unknown. */ #define
> > GRUB_DISK_SIZE_UNKNOWN       0xffffffffffffffffULL 
> > +/* Convert sector number from disk sized sectors to a log-size
> > sized sector. */ +static inline grub_disk_addr_t
> > +grub_disk_convert_sector (grub_disk_t disk,
> > +                     grub_disk_addr_t sector,
> > +                     grub_size_t log_sector_size)
> > +{
> > +  if (disk->log_sector_size < log_sector_size)
> > +    {
> > +      sector = ALIGN_UP (sector, 1 << (log_sector_size /
> > disk->log_sector_size));
> > +      return sector >> (log_sector_size - disk->log_sector_size);
> > +    }
> > +  else
> > +    return sector << (disk->log_sector_size - log_sector_size);
> > +}
> > +
> >  /* Convert to GRUB native disk sized sector from disk sized
> > sector. */ static inline grub_disk_addr_t
> >  grub_disk_from_native_sector (grub_disk_t disk, grub_disk_addr_t
> > sector) -- 
> > 2.27.0
> > 



reply via email to

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