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

[Glenn Washburn]

On Fri, 4 Dec 2020 14:19:29 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

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

I think the question boils down to the filesystem driver code. On
second thought, I'll take the suggestion to round up to nearest sector.

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

Ideally I think we do want to make this case work, which would entail
being confident enough that the file system code won't horribly break.
We may actually be at that point, I just don't know the various
filesystem drivers well enough to know either way.  If we had a setup to
run a filesystem fuzzer over it that might help confidence levels.

Since I think its pretty improbable that there's an unparsable
segment.size but we can get all other needed params to unlock the
drive, I'll drop the todo.

Glenn