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

[Glenn Washburn]

On Fri, 20 Nov 2020 21:24:10 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

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

Why shouldn't it be possible?  LUKS doesn't care about the underlying
sector size of the disk its on.  What is the underlying sector size for
a LUKS volume in a file on a file system with 2k block size on a disk
with 4k sector sizes?  Its all just a stream of bytes to LUKS.  We use
the cryptodisks sector size to determine how to decrypt it, which is
why it matters for grub.  But the source disk sector size adds no
constraints on LUKS nor the cryptodisk.  We just have to translate
between the two occasionally.  Its certainly supported by the linux
implementation of LUKS.  This is a good candidate to add to my tests.

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

Yes, that is problematic in the context of the bit shifts that are being
done, which do not work as desired when the right operand is negative.
I'm refactoring this patch to remove that constraint.

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

I just got it, its really that there should be no periods at the end of
the first line of the commit message.  I'll take care of that.

Glenn