grub-devel
[Top][All Lists]
Advanced

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

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


From: Glenn Washburn
Subject: Re: [PATCH v4 14/15] luks2: Better error handling when setting up the cryptodisk.
Date: Fri, 20 Nov 2020 02:44:53 -0600

On Tue, 17 Nov 2020 15:26:08 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

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

I'll put it in a new 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 ?

Why should this be a constraint? I could add it, but I don't see the
problem when 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...

Good catch.

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

Will do.

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

Ok.

> > +#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().

I'll add to a new patch.

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

If I understand correctly, you dislike periods ('.') at the end of the
email subject line.  I can not add them.  What do you dislike about
them there? (extra superfluous character?)

Glenn



reply via email to

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