Re: [PATCH 3/4] luks2: set up dummy sector size during scan

[Michael Chang <mchang@suse.com>]

Hi,

Enclosed herewith please find the revised patch from openSUSE that could
also fix this very same problem.

According to Fabian, the author of this patch, the reason for having
this patch is that he found some problem in the posted one. I have added
him to the CC list so that he could provide more in detail later.

Thanks,
Michael

From: Fabian Vogt <fvogt@suse.de>
Date: Wed, 4 Aug 2021 14:56:16 +0200
Subject: [PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info
 of the cheat device

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>
---
 grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..c2bb2b6eb 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1040,6 +1040,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
   grub_cryptodisk_t dev;
   grub_cryptodisk_dev_t cr;
   grub_disk_t source;
+  unsigned int cheat_sector_size;
 
   /* Try to open disk.  */
   source = grub_disk_open (sourcedev);
@@ -1062,6 +1063,25 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
     if (!dev)
       continue;
 
+    /* Use the sector size and count of the cheat device */
+    dev->cheat_fd = grub_util_fd_open (cheat, GRUB_UTIL_FD_O_RDONLY);
+    if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
+      {
+        grub_free (dev);
+        return grub_errno;
+      }
+    dev->total_sectors = grub_util_get_fd_size (dev->cheat_fd, cheat, &cheat_sector_size);
+    if (dev->total_sectors == -1)
+      {
+        grub_util_fd_close (dev->cheat_fd);
+        grub_free (dev);
+        return grub_errno;
+      }
+    dev->log_sector_size = cheat_sector_size;
+    dev->total_sectors >>= dev->log_sector_size;
+    grub_util_fd_close (dev->cheat_fd);
+    dev->cheat_fd = GRUB_UTIL_FD_INVALID;
+
     grub_util_info ("cheatmounted %s (%s) at %s", sourcedev, dev->modname,
 		    cheat);
     err = grub_cryptodisk_cheat_insert (dev, sourcedev, source, cheat);
-- 
2.32.0



On Sat, May 30, 2020 at 02:25:17PM +0200, Patrick Steinhardt wrote:
> GRUB currently only supports disk sector sizes of at least 9 bits. While
> not a problem when using decrypted LUKS2 disks, where we configure the
> sector size after we have decrypted the disk, it will cause failure as
> soon as we implement support for probing of LUKS2 encrypted disks: we
> only cheat-mount devices there and don't perform a real decryption, and
> thus the sector size will remain "0", causing errors at a later point.
> 
> The problem here is that we can only determine the sector size as soon
> as we have decrypted a key slot, as key slots may refer to different
> segments, where each segment in turn may have a different sector size.
> As we don't really need the sector size during cheat-mounting anyway,
> let's just specify the minimum value as dummy to fix such errors.
> 
> This patch is in preparation for probing support.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  grub-core/disk/luks2.c | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index c847b4ac4..5c00d9775 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -374,6 +374,14 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
>    grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
>  
>    cryptodisk->modname = "luks2";
> +  /*
> +   * This dummy value is required when cheat-mounting and is overridden by
> +   * `luks2_verify_key ()`. We can't determine it here yet, as its value
> +   * depends on which disk sector we're going to open, which in turn depends on
> +   * the keyslot.
> +   */
> +  cryptodisk->log_sector_size = GRUB_DISK_SECTOR_BITS;
> +
>    return cryptodisk;
>  }
>  
> -- 
> 2.26.2
> 



> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel