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

[Daniel Kiper]

On Thu, Dec 03, 2020 at 10:24:46AM -0600, Glenn Washburn wrote:
> On Thu, 3 Dec 2020 14:31:49 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Fri, Nov 27, 2020 at 03:03:39AM -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",
> > >
> > > 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 | 80
> > > ++++++++++++++++++++++++++++++++++++++++-- include/grub/disk.h    |
> > > 16 +++++++++ 2 files changed, 93 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index b7ed0642e..01f9608e5 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -597,12 +597,26 @@ 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;
> > > +
> > > +      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)
> > >   {
> > > @@ -616,11 +630,71 @@ luks2_recover_key (grub_disk_t source,
> > >        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.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;
> > > +   crypt->total_sectors = grub_strtoull (segment.size,
> > > NULL, 10) >> crypt->log_sector_size;
> >
> > I think ">>" should not happen here...
>
> Do you think this because ">>" should not operate on a value returned
> by a call to grub_strtoull that has errored?  I don't think there's any
> problem because the return value is a number, so there's no harm in
> using ">>" on a garbage number to get another garbage number, as long
> as we don't use the value. (Its also not technically a garbage number,
> just one of two values in the error case)
>
> Also I didn't want to set total_sectors to grub_strtoull because it
> could be a little confusing since the return value of grub_strtoull is
> bytes not sectors.  And yet storing in total_sectors.  I could use a
> local variable, but was wanting to avoid that.  But I'm fine with this
> suggestion.

OK, let's leave it as it is...

> > > +   if (grub_errno == GRUB_ERR_NONE)
> > > +     ;
> >
> > It should happen here... Or to be exact...
> >   crypt->total_sectors = ALIGN_UP (crypt->total_sectors, 1 <<
> > crypt->log_sector_size); // Am I right? crypt->total_sectors >>=
> > crypt->log_sector_size;
>
> I don't think we need to decrypt partial blocks at the end.
> Cryptsetup/dm-crypt enforces disk sizes that are multiples of the
> sector size.  So if the size is not a multiple of blocksize, there's a
> bug somewhere in cryptsetup/dm-crypt, and we don't need to deal with
> the partial block.  At worse the use loses access to a partial block at
> the end that they should never have had access to in the first place.

Does GRUB fail safely in such cases?

> > > +   else if(grub_errno == GRUB_ERR_BAD_NUMBER)
> >
> > Missing space before "("...
>
> Ack.
>
> > > +     {
> > > +       /* TODO: Unparsable number-string, try to use the
> > > whole disk */
> > > +       grub_dprintf ("luks2", "Segment
> > > \"%"PRIuGRUB_UINT64_T"\" size"
> > > +                              " \"%s\" is not a parsable
> > > number\n",
> > > +                              segment.slot_key,
> > > segment.size);
> >
> >   crypt->total_sectors = max_crypt_sectors; ?
>
> Sure, I didn't think you'd want this since it potentially entails
> trying to read past the end of the luks data, and thus getting
> random data, which might crash (buggy) filesystem read code.

OK but then I would drop "TODO" comment because we do not want to
make this case work...

Daniel