Re: [PATCH 06/17] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Glenn Washburn]

On Thu, 30 Jul 2020 17:21:16 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Wed, Jul 29, 2020 at 04:50:11PM -0500, development@efficientek.com
> wrote:
> > From: Glenn Washburn <development@efficientek.com>
> > 
> > The total_length field is named confusingly because length usually
> > refers to bytes, whereas in this case its really the total number
> > of sectors on the device. Also counter-intuitively,
> > grub_disk_get_size returns the total number of device native
> > sectors sectors. We need to convert the sectors from the size of
> > the underlying device to the cryptodisk sector size. And
> > segment.size is in bytes which need to be converted to cryptodisk
> > sectors.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/luks2.c | 7 ++++---
> >  1 file changed, 4 insertions(+), 3 deletions(-)
> > 
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index e3ff7c83d..632309e3c 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -416,7 +416,7 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >    grub_uint8_t salt[GRUB_CRYPTODISK_MAX_KEYLEN];
> >    grub_uint8_t *split_key = NULL;
> >    grub_size_t saltlen = sizeof (salt);
> > -  char cipher[32], *p;;
> > +  char cipher[32], *p;
> >    const gcry_md_spec_t *hash;
> >    gcry_err_code_t gcry_ret;
> >    grub_err_t ret;
> > @@ -602,9 +602,10 @@ luks2_recover_key (grub_disk_t disk,
> >        crypt->log_sector_size = sizeof (unsigned int) * 8
> >             - __builtin_clz ((unsigned int)
> > segment.sector_size) - 1; if (grub_strcmp (segment.size, "dynamic")
> > == 0)
> > -   crypt->total_length = grub_disk_get_size (disk) -
> > crypt->offset;
> > +   crypt->total_length = (grub_disk_get_size (disk) >>
> > (crypt->log_sector_size - disk->log_sector_size)) 
> > +                          - crypt->offset;
> 
> Oops, thanks for catching this. Could you maybe add a comment wrt to
> the magic going on with `(crypt->log_sector_size -
> disk->log_sector_size)`? I didn't think too hard about it and am in a
> hurry, but the conversion isn't that obvious to me.

I'm adding an extra patch to the patch set to change the name of "disk"
to "source" like it is in luks.c for clarity.  Since we're getting the
size of the source disk in source disk sector sizes, we need to convert
it to cryptodisk sector sizes, which is what offset and total_length
are in.  I'll add a comment.