Re: [CRYPTO-LUKS v1 07/19] luks2: grub_cryptodisk_t->total_length is the max number of device native sectors.

[Patrick Steinhardt]

On Fri, Jul 31, 2020 at 07:01:48AM -0500, Glenn Washburn wrote:
> 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.

Is this actually true in all cases?

```
grub_uint64_t
grub_disk_get_size (grub_disk_t disk)
{
  if (disk->partition)
    return grub_partition_get_len (disk->partition);
  else if (disk->total_sectors != GRUB_DISK_SIZE_UNKNOWN)
    return disk->total_sectors << (disk->log_sector_size - 
GRUB_DISK_SECTOR_BITS);
  else
    return GRUB_DISK_SIZE_UNKNOWN;
}

static inline grub_uint64_t
grub_partition_get_len (const grub_partition_t p)
{
  return p->len;
}

/* Partition description.  */
struct grub_partition
{
...
  /* The length in sector units.  */
  grub_uint64_t len;
};
```

So for partitions, this does seem correct as it'll return the number of
sectors. But if it's not a partition, we return `disk->total_sectors`
shifted by the sector size. Which is the length in bytes, right? So to
me, it does rather feel like `grub_disk_get_size` is actually wrong.
Naturally, there's no documentation stating what the function is
supposed to do, but judging by its name it should've returned the number
of bytes and not the number of sectors.

I think the fix should rather be:

diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index ffb09c8ee..f57cf99a1 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -536,7 +536,7 @@ grub_uint64_t
 grub_disk_get_size (grub_disk_t disk)
 {
   if (disk->partition)
-    return grub_partition_get_len (disk->partition);
+    return grub_partition_get_len (disk->partition) << (disk->log_sector_size 
- GRUB_DISK_SECTOR_BITS);
   else if (disk->total_sectors != GRUB_DISK_SIZE_UNKNOWN)
     return disk->total_sectors << (disk->log_sector_size - 
GRUB_DISK_SECTOR_BITS);
   else

I'll send a patch to address this finding.

Patrick


> 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 | 11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 9f58780a2..41329f745 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,14 @@ luks2_recover_key (grub_disk_t source,
>        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 (source) - crypt->offset;
> +     /*
> +      * Convert to cryptodisk sized sectors from source disk sized sectors
> +      * before subtracting the offset, which is in cryptodisk sized sectors.
> +      */
> +     crypt->total_length = (grub_disk_get_size (source) >> 
> (crypt->log_sector_size - source->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;
>  
>        ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
>                              (const grub_uint8_t *) passphrase, grub_strlen 
> (passphrase));
> -- 
> 2.25.1
>

signature.asc
Description: PGP signature