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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5107 bytes --]

On Mon, Dec 07, 2020 at 10:04:01PM -0600, Glenn Washburn wrote:
> On Sun, 6 Dec 2020 20:35:13 +0100
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > 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.
> 
> Yep, thanks for catching that. I was going to complete this and forgot
> in my rush to get the series out before some travel.  I'll rework that.
> 
> > > 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`?
> 
> Technically, this isn't a cast.  Its a variable declaration.  But I'm
> using the typeof(source->total_sectors) because max_crypt_sectors can
> be no more or less than the total sectors, ie its of the same type.

Oh, of course. I somehow didn't realize this at all, probably because
this is not something one sees that often.

> > And isn't the variable always set anyway in
> > case the keyslot has a non-zero priority?
> 
> Yep. Are you suggesting that it need not be initialized?  This is true,
> but I don't think that's a problem.  I think generally initializing
> things is a good practice.  It might be problematic if this was in an
> oft used function (or it might not, would need to see if the compiler
> was smart enough to ignore the initialization).  But that
> initialization is going to happen very rarely in the lifetime of a grub
> execution instance.  I also don't really care either way.
> 
> Glenn

I just didn't realize it's a declaration, so keeping the initialization
is fine.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]