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

[Daniel Kiper]

On Fri, Nov 20, 2020 at 02:44:53AM -0600, Glenn Washburn wrote:
> 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.

Thanks!

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

Is it possible and supported? If yes OK but then I think the result of
(crypt->log_sector_size - source->log_sector_size) has limited set of
values which do not overflow/underflow crypt->total_sectors.

[...]

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

Yep!

> email subject line.  I can not add them.  What do you dislike about
> them there? (extra superfluous character?)

Yeah, extra superfluous character which should not be there.

Daniel