Re: [PATCH v4 14/15] luks2: Better error handling when setting up the cryptodisk.

[Daniel Kiper]

On Fri, Nov 06, 2020 at 10:44:34PM -0600, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/luks2.c | 70 +++++++++++++++++++++++++++++++++++++++---
>  include/grub/misc.h    |  2 ++
>  2 files changed, 67 insertions(+), 5 deletions(-)
>
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 4a4a0dec4..751b48d6a 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -600,9 +600,16 @@ luks2_recover_key (grub_disk_t source,
>        goto err;
>      }
>
> +  if (source->total_sectors == GRUB_DISK_SIZE_UNKNOWN)
> +    {
> +      ret = grub_error (GRUB_ERR_BUG, "not a luks2 device");
> +      goto err;
> +    }
> +
>    /* Try all keyslot */
>    for (i = 0; i < size; i++)
>      {
> +      grub_errno = GRUB_ERR_NONE;
>        ret = luks2_get_keyslot (&keyslot, &digest, &segment, json, i);
>        if (ret)
>       goto err;
> @@ -617,13 +624,66 @@ luks2_recover_key (grub_disk_t source,
>
>        /* 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;
> +      crypt->log_sector_size = grub_log2ull (segment.sector_size);

This change does not belong to this patch.

>        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;
> +     {
> +       /* Convert source sized number of sectors to cryptodisk sized sectors 
> */
> +       crypt->total_sectors = source->total_sectors >> 
> (crypt->log_sector_size - source->log_sector_size);

If you add the other checks could you also add the following check:
  crypt->log_sector_size >= source->log_sector_size ?

> +       if (crypt->total_sectors < crypt->offset_sectors)
> +         {
> +           grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" offset"
> +                                  " is greater than disk size.",

Please replace "." at the end of the message with "\n" here and below...

> +                                   segment.slot_key);
> +           continue;
> +         }
> +
> +       crypt->total_sectors -= crypt->offset_sectors;
> +     }
>        else
> -     crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> 
> crypt->log_sector_size;
> +     {
> +       crypt->total_sectors = grub_strtoull (segment.size, NULL, 10) >> 
> crypt->log_sector_size;
> +       if (grub_errno == GRUB_ERR_NONE)
> +         ;
> +       else if(grub_errno == GRUB_ERR_BAD_NUMBER)
> +         {
> +           /* TODO: Unparsable number-string, try to use the whole disk */
> +           grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" size"
> +                                  " is not a parsable number.",
> +                                  segment.slot_key);
> +           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. */

Please format this comment properly.

> +           if ((source->total_sectors
> +                 >> (crypt->log_sector_size - source->log_sector_size))
> +               > crypt->total_sectors)
> +             {
> +               grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\""
> +                                      " size is very large. The end may be"
> +                                      " inaccessible.",
> +                                      segment.slot_key);
> +             }
> +           else
> +             {
> +               /* FIXME: Set total_sectors as in "dynamic" case. */
> +               grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\""
> +                                      " size greater than the source"
> +                                      " device.",
> +                                      segment.slot_key);
> +               continue;
> +             }
> +         }
> +     }
> +
> +      if (crypt->total_sectors == 0)
> +     {
> +       grub_dprintf ("luks2", "Segment \"%"PRIuGRUB_UINT64_T"\" has"
> +                              " zero sectors, skipping.",
> +                              segment.slot_key);
> +       continue;
> +     }
>
>        ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
>                              (const grub_uint8_t *) passphrase, grub_strlen 
> (passphrase));
> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index b7ca6dd58..ec25131ba 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -481,5 +481,7 @@ void EXPORT_FUNC(grub_real_boot_time) (const char *file,
>
>  #define grub_max(a, b) (((a) > (b)) ? (a) : (b))
>  #define grub_min(a, b) (((a) < (b)) ? (a) : (b))

Please add empty line here.

> +#define grub_log2ull(n) (GRUB_TYPE_BITS (grub_uint64_t) \
> +                         - __builtin_clzll (n) - 1)

This change does not belong to this patch too. Additionally, please
double check that misc.h includes all headers which define
GRUB_TYPE_BITS(), grub_uint64_t and __builtin_clzll().

By the way, may I ask you to stop adding full stop to the end of
email subject?

Daniel