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

[Daniel Kiper]

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?

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

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

Daniel