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

From: Patrick Steinhardt <ps@pks.im>
To: development@efficientek.com
Cc: grub-devel@gnu.org, Daniel Kiper <daniel.kiper@oracle.com>
Subject: Re: [PATCH 06/17] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.
Date: Thu, 30 Jul 2020 17:21:16 +0200	[thread overview]
Message-ID: <20200730152116.GB39309@tanuki.pks.im> (raw)
In-Reply-To: <1435ab49fdbac61ad4c189982724f8ef25c533a2.1596056714.git.development@efficientek.com>

[-- Attachment #1: Type: text/plain, Size: 2326 bytes --]

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.

>        else
> -	crypt->total_length = grub_strtoull (segment.size, NULL, 10);
> +	crypt->total_length = grub_strtoull (segment.size, NULL, 10) >> crypt->log_sector_size;
>  
>        ret = luks2_decrypt_key (candidate_key, disk, crypt, &keyslot,
>  			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
> -- 
> 2.25.1
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]