[PATCH 0/4] Probing support for LUKS2

[PATCH 0/4] Probing support for LUKS2

[Patrick Steinhardt]

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

Hi,

while basic LUKS2 support is there already, there is currently no
support yet for auto-detection of LUKS2 for of grub-probe, grub-install
and companions. As a result, users have to manually configure GRUB to
include required modules. This series is a first step towards
auto-detection and implements probing support for LUKS2:

    $ grub-probe -d /dev/mapper/luks2 -t cryptodisk_uuid
    b2e7039b5dd0bdd4d476f4467c1f7168

Noticably missing is auto-detection of required cryptographic modules,
but this will require some refactoring of the cryptodisk code as the
current assumption is that there will be always exactly one cipher, KDF
and hash, which doesn't hold true for LUKS2. I'll thus do this as a
follow up at a later point.

The first two patches make sense on their own and are worthwhile to be
included in GRUB 2.06. The first one is an out-of-bounds read in LUKS
code, while the second one adjusts the internal UUID format of the
cryptodisk to match the dash-less format that we currently use for LUKS1
disks. As such, it breaks current configs using the dashed format, so
including it pre-2.06 would make sense from my point of view.

The latter two patches are required to implement probing. I'm fine with
deferring them until after 2.06.

@Daniel: please let me know if you want me to split up this series into
two. I didn't think it necessary as you can just apply the first two
patches separately.

Patrick

Patrick Steinhardt (4):
  luks: fix out-of-bounds copy of UUID
  luks2: strip dashes off of the UUID
  luks2: set up dummy sector size during scan
  osdep: detect LUKS2-encrypted devices

 grub-core/disk/luks.c               |  2 +-
 grub-core/disk/luks2.c              | 21 ++++++++++++++++++---
 grub-core/osdep/devmapper/getroot.c | 23 +++++++++++++++++++++--
 include/grub/emu/getroot.h          |  1 +
 util/getroot.c                      |  1 +
 5 files changed, 42 insertions(+), 6 deletions(-)

-- 
2.26.2


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

[PATCH 1/4] luks: fix out-of-bounds copy of UUID

[Patrick Steinhardt]

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

When configuring a LUKS disk, we copy over the UUID from the LUKS header
into the new `grub_cryptodisk_t` structure via `grub_memcpy ()`. As size
we mistakenly use the size of the `grub_cryptodisk_t` UUID field, which
is guaranteed to be strictly bigger than the LUKS UUID field we're
copying. As a result, the copy always goes out-of-bounds and copies some
garbage from other surrounding fields. During runtime, this isn't
noticed due to the fact that we always NUL-terminate the UUID and thus
never hit the trailing garbage.

Fix the issue by using the size of the local stripped UUID field.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 410cd6f84..2c730d072 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -125,7 +125,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   newdev->source_disk = NULL;
   newdev->log_sector_size = 9;
   newdev->total_length = grub_disk_get_size (disk) - newdev->offset;
-  grub_memcpy (newdev->uuid, uuid, sizeof (newdev->uuid));
+  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
-- 
2.26.2


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

[PATCH 2/4] luks2: strip dashes off of the UUID

[Patrick Steinhardt]

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

The UUID header for LUKS2 uses a format with dashes, same as for
LUKS(1). But while we strip these dashes for the latter, we don't for
the former. This isn't wrong per se, but it's definitely inconsistent
for users as they need to use the dashed format for LUKS2 and the
non-dashed format for LUKS when e.g. calling `cryptomount -u $UUID`.

Fix this inconsistency by stripping dashes off of the LUKS2 UUID.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/disk/luks2.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e3ff7c83d..c847b4ac4 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -346,6 +346,8 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
 {
   grub_cryptodisk_t cryptodisk;
   grub_luks2_header_t header;
+  char uuid[sizeof (header.uuid) + 1];
+  grub_size_t i, j;
 
   if (check_boot)
     return NULL;
@@ -356,16 +358,21 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
       return NULL;
     }
 
-  if (check_uuid && grub_strcasecmp (check_uuid, header.uuid) != 0)
+  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
+    if (header.uuid[i] != '-')
+      uuid[j++] = header.uuid[i];
+  uuid[j] = '\0';
+
+  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
     return NULL;
 
   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
   if (!cryptodisk)
     return NULL;
 
-  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
+  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
+  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
 
-  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));
   cryptodisk->modname = "luks2";
   return cryptodisk;
 }
-- 
2.26.2


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

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

[Patrick Steinhardt]

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

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


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

[PATCH 4/4] osdep: detect LUKS2-encrypted devices

[Patrick Steinhardt]

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

While support for LUKS2 has landed already, grub-install(1) doesn't yet
detect it as an installation target. Users of grub-install(1) may thus
end up with a bootloader that cannot read the encrypted disk, rendering
it unusable.

As a first step towards auto-detection, this patch implements detection
for device-mappers LUKS2 signature. As it's mostly similar to LUKS'
original signature except for the incremented version number, detection
mostly echoes what we have for LUKS already.

Note that this doesn't yet implement auto-detection of required
cryptographic modules. This is due to some limitations in the current
implementation, where the assumption is that there's exactly one
encrypted segment and KDF for a given crypto disk. Existing
implementations for LUKS and Geli thus set up ciphers, hashes and KDFs
during the scanning phase, which isn't possible for LUKS2 as there may
be multiple ones. As a result, auto-detecting required modules will
require additional refactoring and is thus postponed to a later point.

Signed-off-by: Patrick Steinhardt <ps@pks.im>
---
 grub-core/osdep/devmapper/getroot.c | 23 +++++++++++++++++++++--
 include/grub/emu/getroot.h          |  1 +
 util/getroot.c                      |  1 +
 3 files changed, 23 insertions(+), 2 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index a13a39c96..74401fd5a 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -148,6 +148,11 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
     }
+  if (strncmp (uuid, "CRYPT-LUKS2-", 12) == 0)
+    {
+      grub_free (uuid);
+      return GRUB_DEV_ABSTRACTION_LUKS2;
+    }
 
   grub_free (uuid);
   return GRUB_DEV_ABSTRACTION_NONE;
@@ -184,8 +189,9 @@ grub_util_pull_devmapper (const char *os_dev)
 	  grub_util_pull_device (subdev);
 	}
     }
-  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
-      && lastsubdev)
+  if (uuid && lastsubdev
+      && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+          || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0))
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
       dm_tree_free (tree);
@@ -267,6 +273,19 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
 	return grub_dev;
       }
 
+    case GRUB_DEV_ABSTRACTION_LUKS2:
+      {
+	char *dash;
+
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS2-") - 1, '-');
+	if (dash)
+	  *dash = 0;
+	grub_dev = grub_xasprintf ("cryptouuid/%s",
+				   uuid + sizeof ("CRYPT-LUKS2-") - 1);
+	grub_free (uuid);
+	return grub_dev;
+      }
+
     default:
       grub_free (uuid);
       return NULL;
diff --git a/include/grub/emu/getroot.h b/include/grub/emu/getroot.h
index 73fa2d34a..be3faf500 100644
--- a/include/grub/emu/getroot.h
+++ b/include/grub/emu/getroot.h
@@ -30,6 +30,7 @@ enum grub_dev_abstraction_types {
   GRUB_DEV_ABSTRACTION_LVM,
   GRUB_DEV_ABSTRACTION_RAID,
   GRUB_DEV_ABSTRACTION_LUKS,
+  GRUB_DEV_ABSTRACTION_LUKS2,
   GRUB_DEV_ABSTRACTION_GELI,
 };
 
diff --git a/util/getroot.c b/util/getroot.c
index 847406fba..07ad92317 100644
--- a/util/getroot.c
+++ b/util/getroot.c
@@ -101,6 +101,7 @@ grub_util_pull_device (const char *os_dev)
       grub_util_pull_lvm_by_command (os_dev);
       /* Fallthrough - in case that lvm-tools are unavailable.  */
     case GRUB_DEV_ABSTRACTION_LUKS:
+    case GRUB_DEV_ABSTRACTION_LUKS2:
       grub_util_pull_devmapper (os_dev);
       return;
 
-- 
2.26.2


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

Re: [PATCH 1/4] luks: fix out-of-bounds copy of UUID

[Petr Vorel]

Hi,

Reviewed-by: Petr Vorel <petr.vorel@gmail.com>

Kind regards,
Petr

Re: [PATCH 2/4] luks2: strip dashes off of the UUID

[Daniel Kiper]

On Sat, May 30, 2020 at 02:25:11PM +0200, Patrick Steinhardt wrote:
> The UUID header for LUKS2 uses a format with dashes, same as for
> LUKS(1). But while we strip these dashes for the latter, we don't for
> the former. This isn't wrong per se, but it's definitely inconsistent
> for users as they need to use the dashed format for LUKS2 and the
> non-dashed format for LUKS when e.g. calling `cryptomount -u $UUID`.
>
> Fix this inconsistency by stripping dashes off of the LUKS2 UUID.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[Michael Chang]

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

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

[Patrick Steinhardt]

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

On Fri, Aug 06, 2021 at 12:51:10PM +0800, Michael Chang via Grub-devel wrote:
[snip]
> 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;
> +      }

We may want to print error messages for both error cases. Otherwise it's
hard to tell why cheat mounting might've failed.

> +    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;

Wouldn't it make more sense to just use a separate variable for the FD?
Feels kind of weird to put it into `dev->cheat_fd`, only to clone and
unset the member variable immediately after we're done again.

In any case, thanks for the patch. It does look a lot more sensible
compared to what I'd been doing. Do you want to keep owning the patch
and try to upstream it, or shall I pick it up as part of this patch
series? I'd of course retain author information.

Patrick

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

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

[Fabian Vogt]

Hi,

(I *finally* got to this topic again...)

Am Sonntag, 8. August 2021, 16:20:12 CET schrieb Patrick Steinhardt:
> On Fri, Aug 06, 2021 at 12:51:10PM +0800, Michael Chang via Grub-devel wrote:
> [snip]
> > 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;
> > +      }
> 
> We may want to print error messages for both error cases. Otherwise it's
> hard to tell why cheat mounting might've failed.

Agreed.

> > +    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;
> 
> Wouldn't it make more sense to just use a separate variable for the FD?
> Feels kind of weird to put it into `dev->cheat_fd`, only to clone and
> unset the member variable immediately after we're done again.

A bit, yes, but the alternative is having a variable "int cheat_fd" which makes
it more confusing IMO. I'm fine with either option.

> In any case, thanks for the patch. It does look a lot more sensible
> compared to what I'd been doing. Do you want to keep owning the patch
> and try to upstream it, or shall I pick it up as part of this patch
> series? I'd of course retain author information.

Feel free to incorporate it, or I can refine it and resend as v2.
(I'll most likely get to the latter only in the next year though)

It looks like we have a third patch (series) for this feature meanwhile:
[PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe

I CC'd the author, let's try to coordinate.

Thanks,
Fabian

> Patrick

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

[Josselin Poiret]

Hello everyone,

Fabian Vogt <fvogt@suse.de> writes:
> It looks like we have a third patch (series) for this feature meanwhile:
> [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe
>
> I CC'd the author, let's try to coordinate.
>
> Thanks,
> Fabian

Let me just say that I had not found this patch series while searching
beforehand.  Let me just recap what my patches do differently (in
relation to patches 3 and 4 of this series):

Because cheat-mounting cryptodisks only happens (from my understanding)
when pulling devmapper devices, we can simply ask the kernel for the dm
and dm-crypt parameters that it's opened with, and populate our
cheat-mounted device from that.  This completely circumvents the multiple
segments issue, as this will always yield the parameters corresponding
to the user-specified mountpoint of `grub-probe` or `grub-install`.

I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as
to reuse all existing code that supports LUKS1, although that can be
confusing.  We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to
GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how
they're unlocked, not in underlying algorithms.

What do you think?

-- 
Josselin Poiret

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

[Fabian Vogt]

Hi,

Am Mittwoch, 22. Dezember 2021, 19:17:37 CET schrieb Josselin Poiret via Grub-devel:
> Hello everyone,
> 
> Fabian Vogt <fvogt@suse.de> writes:
> > It looks like we have a third patch (series) for this feature meanwhile:
> > [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe
> >
> > I CC'd the author, let's try to coordinate.

And there's a forth one now (author CC'd)!
("[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices")

So we have:

"[PATCH 3/4] luks2: set up dummy sector size during scan", which hardcodes 512,

"[PATCH 1/2] disk/cryptodisk: When cheatmounting, use the sector info of the
 cheat device", which queries the sector size of the underlying host device,

"[PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from
 DM parameters", which parses the DM table to get the sector_size, and now

"[PATCH 3/3] grub-core/kern/disk.c: handle LUKS2 devices", which changes the
grub core code to accept a sector size of 0 for LUKS2 devices.

Should be enough options to pick a good one ;-)

> > Thanks,
> > Fabian
> 
> Let me just say that I had not found this patch series while searching
> beforehand.  Let me just recap what my patches do differently (in
> relation to patches 3 and 4 of this series):
> 
> Because cheat-mounting cryptodisks only happens (from my understanding)
> when pulling devmapper devices, we can simply ask the kernel for the dm
> and dm-crypt parameters that it's opened with, and populate our
> cheat-mounted device from that.  This completely circumvents the multiple
> segments issue, as this will always yield the parameters corresponding
> to the user-specified mountpoint of `grub-probe` or `grub-install`.

Yup.

Did you have a look at my approach? That effectively does the same, but using a
single ioctl instead of anything complex with DM directly.

> I also opted not to add a GRUB_DEV_ABSTRACTION_LUKS2 abstraction, so as
> to reuse all existing code that supports LUKS1, although that can be
> confusing.  We could simply rename GRUB_DEV_ABSTRACTION_LUKS1 to
> GRUB_DEV_ABSTRACTION_CRYTODISK, as LUKS1 and LUKS2 only differ in how
> they're unlocked, not in underlying algorithms.
> 
> What do you think?

Sounds good to me, though I'd count that as a separate refactoring step for
the future.

Cheers,
Fabian

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

[Josselin Poiret]

Hi Fabian,

Fabian Vogt <fvogt@suse.de> writes:

> Did you have a look at my approach? That effectively does the same, but using a
> single ioctl instead of anything complex with DM directly.

I agree that it's sufficient for sector_size, but we still need the
cryptodisk algorithm so that grub-install will know which crypto modules
to include.  libdm is actually a helper library around dm-specific
ioctls, since they are apparently annoying to use, so in the end we're
still using the same interface :)

As for the 4 patches, since the actual sector_size is available to us, I
think using 512 in all cases or adding a specific 0 sector_size is
something we should avoid.

Best,
-- 
Josselin Poiret

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

[Glenn Washburn]

On Mon, 07 Feb 2022 14:15:07 +0100
Josselin Poiret via Grub-devel <grub-devel@gnu.org> wrote:

> Hi Fabian,
> 
> Fabian Vogt <fvogt@suse.de> writes:
> 
> > Did you have a look at my approach? That effectively does the same, but using a
> > single ioctl instead of anything complex with DM directly.

I skipped this thread because I didn't really care about probing for
LUKS2 (I don't use it). However, things change and now I'm evaluating
the different patch series that implement this. I had missed Fabian's
patch and I'm just now seeing it and this thread.

Now that I've taken a look at Fabian's patch, I think its the better
way to go for getting the sector size. The reason is that it uses
grub_util_get_fd_size() to get the size and sector size of the cheat
mount device. That function's implementation is platform dependant. So,
for instance, if FreeBSD has a LUKS* implementation that is not
libdevicemapper based, we'll correctly get the number of sectors and
sector size. But yes, its very unlikely that FreeBSD will support LUKS.
However, this same code should allow us to get the correct sector size
for GELI devices also. I don't believe GRUB has GELI probe support, so
ths would make things easier for whoever wants to develop it. The same
goes for future cryptodisk backends.

> I agree that it's sufficient for sector_size, but we still need the
> cryptodisk algorithm so that grub-install will know which crypto modules
> to include.  libdm is actually a helper library around dm-specific

I'm now thinking that GRUB should use Fabian's patch to get the sector
size and number of sectors and Josselin's method of querying device
mapper to get the other properies. We could have Josselin's patch first
check if log_sector_size is set and if not then try to get it from DM.
However, I think that's overkill and not really useful because
grub_util_get_fd_size() should always give us the correct information,
otherwise we've found a kernel bug. Also, by not having to parse out
the sector size from the dm-crypt params string, we avoid much the
complexity of that code, which has been the source of several bugs in
reviewing a couple iterations of that patch.

> ioctls, since they are apparently annoying to use, so in the end we're
> still using the same interface :)

Technically different interface, the code Fabian is using is not using
device mapper related ioctls, they are generic block device ioctls (on
Linux systems).

> As for the 4 patches, since the actual sector_size is available to us, I
> think using 512 in all cases or adding a specific 0 sector_size is
> something we should avoid.

I agree.

Fabian, would you be able to send an updated patch with Patrick's
suggestions in the near future?

Glenn

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

[Fabian Vogt]

Hi,

Am Samstag, 21. Mai 2022, 02:13:53 CEST schrieb Glenn Washburn:
> On Mon, 07 Feb 2022 14:15:07 +0100
> Josselin Poiret via Grub-devel <grub-devel@gnu.org> wrote:
> 
> > Hi Fabian,
> > 
> > Fabian Vogt <fvogt@suse.de> writes:
> > 
> > > Did you have a look at my approach? That effectively does the same, but using a
> > > single ioctl instead of anything complex with DM directly.
> 
> I skipped this thread because I didn't really care about probing for
> LUKS2 (I don't use it). However, things change and now I'm evaluating
> the different patch series that implement this. I had missed Fabian's
> patch and I'm just now seeing it and this thread.
> 
> Now that I've taken a look at Fabian's patch, I think its the better
> way to go for getting the sector size. The reason is that it uses
> grub_util_get_fd_size() to get the size and sector size of the cheat
> mount device. That function's implementation is platform dependant. So,
> for instance, if FreeBSD has a LUKS* implementation that is not
> libdevicemapper based, we'll correctly get the number of sectors and
> sector size. But yes, its very unlikely that FreeBSD will support LUKS.
> However, this same code should allow us to get the correct sector size
> for GELI devices also. I don't believe GRUB has GELI probe support, so
> ths would make things easier for whoever wants to develop it. The same
> goes for future cryptodisk backends.
> 
> > I agree that it's sufficient for sector_size, but we still need the
> > cryptodisk algorithm so that grub-install will know which crypto modules
> > to include.  libdm is actually a helper library around dm-specific
> 
> I'm now thinking that GRUB should use Fabian's patch to get the sector
> size and number of sectors and Josselin's method of querying device
> mapper to get the other properies. We could have Josselin's patch first
> check if log_sector_size is set and if not then try to get it from DM.
> However, I think that's overkill and not really useful because
> grub_util_get_fd_size() should always give us the correct information,
> otherwise we've found a kernel bug. Also, by not having to parse out
> the sector size from the dm-crypt params string, we avoid much the
> complexity of that code, which has been the source of several bugs in
> reviewing a couple iterations of that patch.
> 
> > ioctls, since they are apparently annoying to use, so in the end we're
> > still using the same interface :)
> 
> Technically different interface, the code Fabian is using is not using
> device mapper related ioctls, they are generic block device ioctls (on
> Linux systems).
> 
> > As for the 4 patches, since the actual sector_size is available to us, I
> > think using 512 in all cases or adding a specific 0 sector_size is
> > something we should avoid.
> 
> I agree.
> 
> Fabian, would you be able to send an updated patch with Patrick's
> suggestions in the near future?

Sure! I'll be back from vacation after end of May, I hope that still counts.

Cheers,
Fabian

> Glenn

[PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Fabian Vogt]

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.

 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
 
   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)
     {
@@ -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;
     }
 #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;
-- 
2.36.1

Re: [PATCH v2] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Glenn Washburn]

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

[PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Fabian Vogt]

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.
v3: More liberal placement of variable declarations.

 grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++--
 1 file changed, 18 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index c80cf9907..b3c76ef57 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -709,16 +709,31 @@ 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)
     {
+      grub_uint64_t cheat_dev_size;
+      unsigned int cheat_log_sector_size;
+
       if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
 	dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY);
       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)
+        {
+          const char *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 >> cheat_log_sector_size;
     }
 #endif
 
@@ -732,6 +747,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;
-- 
2.36.1

Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Glenn Washburn]

On Tue, 14 Jun 2022 15:55:21 +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>

Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

> ---
> 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.
> v3: More liberal placement of variable declarations.
> 
>  grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c80cf9907..b3c76ef57 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -709,16 +709,31 @@ 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)
>      {
> +      grub_uint64_t cheat_dev_size;
> +      unsigned int cheat_log_sector_size;
> +
>        if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
>  	dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY);
>        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)
> +        {
> +          const char *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 >> cheat_log_sector_size;
>      }
>  #endif
>  
> @@ -732,6 +747,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;

Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Patrick Steinhardt]

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

On Tue, Jun 14, 2022 at 03:55:21PM +0200, Fabian Vogt 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>

Reviewed-by: Patrick Steinhardt <ps@pks.im>

> ---
> 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.
> v3: More liberal placement of variable declarations.
> 
>  grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c80cf9907..b3c76ef57 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -709,16 +709,31 @@ 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)
>      {
> +      grub_uint64_t cheat_dev_size;
> +      unsigned int cheat_log_sector_size;
> +
>        if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
>  	dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY);
>        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)
> +        {
> +          const char *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 >> cheat_log_sector_size;
>      }
>  #endif
>  
> @@ -732,6 +747,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;
> -- 
> 2.36.1
> 
> 
> 
> 

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

Re: [PATCH v3] disk/cryptodisk: When cheatmounting, use the sector info of the cheat device

[Glenn Washburn]

Adding Daniel to this email.

Glenn

On Tue, 14 Jun 2022 15:55:21 +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.
> v3: More liberal placement of variable declarations.
> 
>  grub-core/disk/cryptodisk.c | 20 ++++++++++++++++++--
>  1 file changed, 18 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index c80cf9907..b3c76ef57 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -709,16 +709,31 @@ 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)
>      {
> +      grub_uint64_t cheat_dev_size;
> +      unsigned int cheat_log_sector_size;
> +
>        if (!GRUB_UTIL_FD_IS_VALID (dev->cheat_fd))
>  	dev->cheat_fd = grub_util_fd_open (dev->cheat, GRUB_UTIL_FD_O_RDONLY);
>        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)
> +        {
> +          const char *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 >> cheat_log_sector_size;
>      }
>  #endif
>  
> @@ -732,6 +747,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;