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

From: Glenn Washburn <development@efficientek.com>
To: Daniel Kiper <daniel.kiper@oracle.com>
Cc: Patrick Steinhardt <ps@pks.im>,
	grub-devel@gnu.org,
	Denis GNUtoo Carikli <GNUtoo@cyberdimension.org>
Subject: Re: [PATCH v3 4/9] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors
Date: Mon, 21 Sep 2020 06:28:28 +0000 (UTC)	[thread overview]
Message-ID: <94e72828-212b-49ed-8a59-e33a85f7afce@efficientek.com> (raw)
In-Reply-To: <20200908132119.6o4xaaubq6oe2vxh@tomti.i.net-space.pl>

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.

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

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.

> Daniel
>