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

[Patrick Steinhardt]

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.

> 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`? And isn't the variable always set anyway in
case the keyslot has a non-zero priority?

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
>

signature.asc
Description: PGP signature