Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors

[Daniel Kiper]

On Mon, Sep 21, 2020 at 06:28:28AM +0000, Glenn Washburn wrote:
> Sep 8, 2020 7:21:31 AM Daniel Kiper <daniel.kiper@oracle.com>:
> > On Mon, Sep 07, 2020 at 05:27:46PM +0200, Patrick Steinhardt 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
> >
> > Could we change total_length name? Or should it stay as is because this
> > name is used in other implementations too?
>
> I sent a patch which renamed total_length to total_sectors. I believe
> Patrick chose not to include it because I did not fix a bug in the
> code and this patch series was only patches he thought essential to be
> included in the next release. I'll include that patch again in a
> follow up patch series.

Please do. I want to have this fixed before 2.06 release...

> >> 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>
> >> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> >> ---
> >> 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 c4c6ac90c..5f15a4d2c 100644
> >> --- a/grub-core/disk/luks2.c
> >> +++ b/grub-core/disk/luks2.c
> >> @@ -417,7 +417,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;
> >
> > I am OK with changes like that but they should be mentioned shortly in
> > the commit message.
>
> Noted, I'll put update the commit message.
>
> >> const gcry_md_spec_t *hash;
> >> gcry_err_code_t gcry_ret;
> >> grub_err_t ret;
> >> @@ -603,9 +603,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;
> >> else
> >> - crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> >> + crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> 
> >> crypt->log_sector_size;
> >
> > I do not like that you ignore grub_strtoull() errors. Additionally, what
> > will happen if segment.size is smaller than LUKS2 sector size? Should
> > not you round segment.size up to the nearest multiple of LUKS2 sector
> > size first? I think the same applies to the earlier change too.
>
> Again, I was making a minimal set of changes for this fix. Your
> comments about grub_strtoull, while valid, don't apply to this patch
> and should be addressed in a new patch.

OK, please fix it then in separate patch.

> Your concern about rounding segment.size up, is also valid and
> pertinent to this patch, I'll update that in a following patch series.
> This may get more complicated if the last partial sector is at the end
> of the disk.

Yeah, but please try to fix it somehow...

Daniel