[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device |
Date: |
Mon, 13 Jun 2022 21:19:42 -0500 |
On Mon, 13 Jun 2022 16:29:48 +0200
Fabian Vogt <fvogt@suse.de> wrote:
> When using grub-probe with cryptodisk, the mapped block device from the host
> is used directly instead of decrypting the source device in GRUB code.
> In that case, the sector size and count of the host device needs to be used.
> This is especially important when using luks2, which does not assign
> total_sectors and log_sector_size when scanning, but only later when the
> segments in the JSON area are evaluated. With an unset log_sector_size,
> grub_open_device complains.
>
> This fixes grub-probe failing with
> "error: sector sizes of 1 bytes aren't supported yet."
>
> Signed-off-by: Fabian Vogt <fvogt@suse.de>
> ---
> v2: Moved new code from grub_cryptodisk_cheat_mount to grub_cryptodisk_open,
> which allowed to simplify the code a bit. Also improved error handling.
Yes, I like this patch even better.
>
> grub-core/disk/cryptodisk.c | 22 ++++++++++++++++++++--
> 1 file changed, 20 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c80cf9907..14e105b84 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -686,6 +686,11 @@ static grub_err_t
> grub_cryptodisk_open (const char *name, grub_disk_t disk)
> {
> grub_cryptodisk_t dev;
> +#ifdef GRUB_UTIL
> + grub_uint64_t cheat_dev_size;
> + unsigned int cheat_log_sector_size;
> + const char *errmsg;
> +#endif
I think these variable declarations would be better put ...
>
> if (grub_memcmp (name, "crypto", sizeof ("crypto") - 1) != 0)
> return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device");
> @@ -709,8 +714,6 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> if (!dev)
> return grub_error (GRUB_ERR_UNKNOWN_DEVICE, "No such device");
>
> - disk->log_sector_size = dev->log_sector_size;
> -
> #ifdef GRUB_UTIL
> if (dev->cheat)
> {
... here.
> @@ -719,6 +722,20 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
> return grub_error (GRUB_ERR_IO, N_("cannot open `%s': %s"),
> dev->cheat, grub_util_fd_strerror ());
> +
> + /* Use the sector size and count of the cheat device */
> + cheat_dev_size = grub_util_get_fd_size (dev->cheat_fd, dev->cheat,
> &cheat_log_sector_size);
> + if (cheat_dev_size == -1)
> + {
> + errmsg = grub_util_fd_strerror ();
> + grub_util_fd_close (dev->cheat_fd);
> + dev->cheat_fd = GRUB_UTIL_FD_INVALID;
> + return grub_error (GRUB_ERR_IO, N_("failed to query size of device
> `%s': %s"),
> + dev->cheat, errmsg);
> + }
> +
> + dev->log_sector_size = cheat_log_sector_size;
> + dev->total_sectors = cheat_dev_size >> dev->log_sector_size;
Small nit, I think it reads better to have "cheat_dev_size >>
cheat_log_sector_size".
> }
> #endif
>
> @@ -732,6 +749,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> }
>
> disk->data = dev;
> + disk->log_sector_size = dev->log_sector_size;
> disk->total_sectors = dev->total_sectors;
> disk->max_agglomerate = GRUB_DISK_MAX_MAX_AGGLOMERATE;
> disk->id = dev->id;
Regardless whether the suggestions above are made:
Reviewed-by: Glenn Washburn <development@efficientek.com>
Glenn