[PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello,

These two draft patches make devmapper set up LUKS2 cryptomount
properties when pulling, as well as report LUKS2 cryptomounts as
having GRUB_DEV_ABSTRACTION_LUKS.  This makes grub-probe and
grub-install behave properly wrt. LUKS2 drives: `grub-probe -t
abstraction /` reports all the needed modules for the GRUB image, and
grub-install leads to a working GRUB without manually adding modules.

One small part that I am unsure about, although I have tested it and
it does seem to work properly: if I understand correctly, all dm
devices have a 512 sector size, however LUKS2 lets one choose up to
4096 for the encryption sector size.  Which of these two should be
used as cryptodisk->sector_size?  I put 512 here since we're reading
through a cheated mount, but I'm not so sure.

Best,
Josselin Poiret

Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 59 ++++++++++++++++++++++++++---
 1 file changed, 54 insertions(+), 5 deletions(-)

-- 
2.34.0

[PATCH 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.
---
 grub-core/osdep/devmapper/getroot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..ad1daf9c8 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +179,7 @@ grub_util_pull_devmapper (const char *os_dev)
 	  grub_util_pull_device (subdev);
 	}
     }
-  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+  if (uuid && strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +253,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.34.0

[PATCH 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
filled out, otherwise they wouldn't be initialized if cheat mounted.
---
 grub-core/osdep/devmapper/getroot.c | 51 ++++++++++++++++++++++++++++-
 1 file changed, 50 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index ad1daf9c8..574d07a20 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -51,6 +51,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -183,7 +185,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -191,7 +192,55 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /* set LUKS2 cipher and size from dm parameter, since it is not
+                 possible to determine the correct ones without unlocking, as
+                 there might be multiple segments. */
+              grub_disk_t source = grub_disk_open (grdev);
+              grub_cryptodisk_t cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              grub_disk_close (source);
+              grub_addr_t start, length;
+              char *target_type = NULL;
+              char *params;
+              const char *name = dm_tree_node_get_name (node);
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+              grub_util_info ("creating dm task for `%s'", name);
+              if (!(dmt = dm_task_create (DM_DEVICE_TABLE)))
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (!dm_task_set_name (dmt, name))
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (!dm_task_run (dmt))
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target is not `crypt'"));
+
+              cryptodisk->total_sectors = length;
+              cryptodisk->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
+
+              char *seek_head, *c;
+              c = params;
+              if (!(seek_head = grub_memchr (c, '-', grub_strlen (c))))
+                grub_util_error (_("can't get cipher from dm task"));
+              cipher = grub_strndup (c, seek_head - c);
+
+              c = seek_head + 1;
+              if (!(seek_head = grub_memchr (c, ' ', grub_strlen (c))))
+                grub_util_error (_("can't get cipher mode from dm task"));
+              cipher_mode = grub_strndup (c, seek_head - c);
+
+              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+
+              /* This is the only hash usable by PBKDF2, so set it as such */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.34.0

Re: [PATCH 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Glenn Washburn]

On Thu,  9 Dec 2021 18:38:51 +0100
Josselin Poiret via Grub-devel <grub-devel@gnu.org> wrote:

> Hello,
> 
> These two draft patches make devmapper set up LUKS2 cryptomount
> properties when pulling, as well as report LUKS2 cryptomounts as
> having GRUB_DEV_ABSTRACTION_LUKS.  This makes grub-probe and
> grub-install behave properly wrt. LUKS2 drives: `grub-probe -t
> abstraction /` reports all the needed modules for the GRUB image, and
> grub-install leads to a working GRUB without manually adding modules.
> 
> One small part that I am unsure about, although I have tested it and
> it does seem to work properly: if I understand correctly, all dm
> devices have a 512 sector size, however LUKS2 lets one choose up to
> 4096 for the encryption sector size.  Which of these two should be
> used as cryptodisk->sector_size?  I put 512 here since we're reading
> through a cheated mount, but I'm not so sure.

Its not clear to me, did you test a LUKS2 device with sector size 4096
with this change? I believe DM does use 512-byte sectors internally,
but it can create block devices that report and use other sector sizes.
You can verfiy this by creating a 4096 sector size LUKS2 devices, open
it with cryptsetup, and then run "blockdev --getbsz /dev/mapper/<dm name>".

When having a 4096 byte sector size LUKS2 device opened via cyptsetup,
here's what dmsetup table --show returns "sector_size:4096" as part of
the output for the device. I'm not familiar with this code, but I'm
thinking tht might show up in the "params" variable for you to use when
setting log_sector_size.

I have a feeling that this is not going to work as is with non-512-byte
sector size LUKS2 devices. 

Glenn

[PATCH v2 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Glenn Washburn <development@efficientek.com> writes:
> Its not clear to me, did you test a LUKS2 device with sector size 4096
> with this change? I believe DM does use 512-byte sectors internally,
> but it can create block devices that report and use other sector sizes.
> You can verfiy this by creating a 4096 sector size LUKS2 devices, open
> it with cryptsetup, and then run "blockdev --getbsz /dev/mapper/<dm name>".

You're right, blockdev does indeed report 4096.  Here is an updated
patch that parses the optional sector_size argument from the DM
parameters, I have checked that it does indeed set the right
log_sector_size.  I think it worked without it because you can
technically just read with a lower sector size, but better be safe
than sorry!

Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 107 ++++++++++++++++++++++++++--
 1 file changed, 102 insertions(+), 5 deletions(-)

-- 
2.34.0

[PATCH v2 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.
---
 grub-core/osdep/devmapper/getroot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..ad1daf9c8 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +179,7 @@ grub_util_pull_devmapper (const char *os_dev)
 	  grub_util_pull_device (subdev);
 	}
     }
-  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+  if (uuid && strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +253,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.34.0

Re: [PATCH v2 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Glenn Washburn]

On Sat, 11 Dec 2021 13:29:44 +0100
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
> as being LUKS cryptodisks.
> ---
>  grub-core/osdep/devmapper/getroot.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 9ba5c9865..ad1daf9c8 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -138,7 +138,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
>        grub_free (uuid);
>        return GRUB_DEV_ABSTRACTION_LVM;
>      }
> -  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
> +  if (strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0)
>      {
>        grub_free (uuid);
>        return GRUB_DEV_ABSTRACTION_LUKS;
> @@ -179,7 +179,7 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  grub_util_pull_device (subdev);
>  	}
>      }
> -  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
> +  if (uuid && strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> @@ -253,11 +253,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
>        {
>  	char *dash;
>  
> -	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
> +	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
>  	if (dash)
>  	  *dash = 0;
>  	grub_dev = grub_xasprintf ("cryptouuid/%s",
> -				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
> +				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
>  	grub_free (uuid);
>  	return grub_dev;
>        }

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

Glenn

[PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
filled out, otherwise they wouldn't be initialized if cheat mounted.
---
 grub-core/osdep/devmapper/getroot.c | 99 ++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index ad1daf9c8..4b3458639 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -51,6 +51,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -183,7 +185,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -191,7 +192,103 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /* set LUKS2 cipher and size from dm parameter, since it is not
+               * possible to determine the correct ones without unlocking, as
+               * there might be multiple segments.
+               */
+              grub_disk_t source = grub_disk_open (grdev);
+              grub_cryptodisk_t cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              grub_disk_close (source);
+              grub_addr_t start, length;
+              char *target_type = NULL;
+              char *params;
+              const char *name = dm_tree_node_get_name (node);
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
+                              uuid, name);
+              if (!(dmt = dm_task_create (DM_DEVICE_TABLE)))
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (!dm_task_set_name (dmt, name))
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (!dm_task_run (dmt))
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target is not `crypt'"));
+
+              cryptodisk->total_sectors = length;
+              cryptodisk->log_sector_size = 9; /* default dm sector size */
+
+              /* dm target parameters for dm-crypt is
+               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_params>]
+               */
+              char *seek_head, *c;
+              c = params;
+
+              /* first, get the cipher name from the cipher */
+              if (!(seek_head = grub_memchr (c, '-', grub_strlen (c))))
+                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
+                                 params);
+              cipher = grub_strndup (c, seek_head - c);
+              c = seek_head + 1;
+
+              /* now, the cipher mode */
+              if (!(seek_head = grub_memchr (c, ' ', grub_strlen (c))))
+                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
+                                 params);
+              cipher_mode = grub_strndup (c, seek_head - c);
+
+              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+
+              /* This is the only hash usable by PBKDF2, so set it as such */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+
+              /* drop key, iv_offset, device path and offset */
+              for (int dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++)
+                {
+                  seek_head++;
+                  seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head));
+                }
+
+              /* if we have optional parameters, skip #opt_params */
+              if (seek_head && (seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head))))
+                {
+                  seek_head++;
+                  for (;seek_head;seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head)))
+                    {
+                      seek_head++;
+                      if (strncmp (seek_head, "sector_size:", sizeof ("sector_size:") - 1) == 0)
+                        {
+                          char *sector_size_str;
+                          unsigned long long sector_size;
+                          c = seek_head + sizeof ("sector_size:") - 1;
+                          seek_head = grub_memchr (c, ' ', grub_strlen (c));
+                          if (seek_head)
+                            sector_size_str = grub_strndup (c, seek_head - c);
+                          else
+                            sector_size_str = c;
+                          sector_size = grub_strtoull (sector_size_str, NULL, 10);
+
+                          if (sector_size != 512 && sector_size != 1024 &&
+                              sector_size != 2048 && sector_size != 4096)
+                            grub_util_error (_("dm-crypt parameter sector_size `%s' is not a valid LUKS2 sector size"),
+                                             sector_size_str);
+                          cryptodisk->log_sector_size = grub_log2ull (sector_size);
+                          grub_util_info ("set sector_size for dm `%s' to `%d'",
+                                          name, 1 << cryptodisk->log_sector_size);
+                          break;
+                        }
+                    }
+                }
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.34.0

Re: [PATCH v2 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Glenn Washburn]

On Sat, 11 Dec 2021 13:29:45 +0100
Josselin Poiret <dev@jpoiret.xyz> wrote:

> This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
> filled out, otherwise they wouldn't be initialized if cheat mounted.
> ---
>  grub-core/osdep/devmapper/getroot.c | 99 ++++++++++++++++++++++++++++-
>  1 file changed, 98 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index ad1daf9c8..4b3458639 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -183,7 +185,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -191,7 +192,103 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /* set LUKS2 cipher and size from dm parameter, since it is not
> +               * possible to determine the correct ones without unlocking, as
> +               * there might be multiple segments.
> +               */
> +              grub_disk_t source = grub_disk_open (grdev);
> +              grub_cryptodisk_t cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              grub_disk_close (source);
> +              grub_addr_t start, length;
> +              char *target_type = NULL;
> +              char *params;
> +              const char *name = dm_tree_node_get_name (node);
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;

I think these declarations should be before non-declaration statements,
eg. grub_disk_close(). On the other hand, Daniel has been known to want
declarations at the start of the function block.

> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +              if (!(dmt = dm_task_create (DM_DEVICE_TABLE)))

This assignment should be a separate statement to more conform to the
coding style of GRUB.

> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
> +              if (!dm_task_set_name (dmt, name))

This condition should be dm_task_set_name (dmt, name) == 0. And same
for similar usages below.

> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (!dm_task_run (dmt))
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);

It looks like this might return NULL on error? Or does it never error
here?

> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target is not `crypt'"));

It would be nice for debugging to know what type the dm target was on
error. So I'd rather this line:

  grub_util_error (_("dm target of type `%s' is not type `crypt'"), target_type);

> +
> +              cryptodisk->total_sectors = length;

I believe this is in device mapper sector sized sectors, which is fine,
until we find out below that the volume is using a non-default sector
size.

> +              cryptodisk->log_sector_size = 9; /* default dm sector size */

The comment is nice, but probably better is to create a macro called
DM_LOG_SECTOR_SIZE.

> +
> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_params>]
> +               */
> +              char *seek_head, *c;
> +              c = params;
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', grub_strlen (c))))

I would have have grub_strlen() called once on this string and that
value decremented appropriately for the rest below.

> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', grub_strlen (c))))
> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +
> +              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, so set it as such */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");

So does this mean that when/if argon2 support is added, that this will
need to be detected as well? If so, is there a way to get the hash via
devmapper? It doesn't show up with the table command, so I'm guessing
not. On the otherhand, I don't think cryptodisk->hash is ever used in
the user-space utilities because the cryptodisk is never attempted to
be unlocked, so there's no password to hash. Perhaps this should be
NULL with a comment that its not used (if I'm right about that).

> +
> +              /* drop key, iv_offset, device path and offset */
> +              for (int dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++)
> +                {
> +                  seek_head++;
> +                  seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head));

Instead of these two lines, I believe it could be:

  seek_head = grub_memchr (seek_head + 1, ' ', grub_strlen (seek_head));

> +                }
> +
> +              /* if we have optional parameters, skip #opt_params */
> +              if (seek_head && (seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head))))

If seek_head != NULL, then it points to space because of the
grub_memchr() in the for loop above. So calling grub_memchr() again is
pointless, right? ie. seek_head will never change in the assignment
above. I think this if above can be removed and just use the for loop
below, and in the initializer section have seek_head++.

> +                {
> +                  seek_head++;
> +                  for (;seek_head;seek_head = grub_memchr (seek_head, ' ', grub_strlen (seek_head)))
> +                    {
> +                      seek_head++;

On the first iteration in the loop seek_head starts pointing at an
optional parameter, then the line above will increment the pointer to
point to the second byte of the optional parameter. If the optional
parameter is the one we're looking for, eg. sector_size, we will not
find a match. This code only works because the sector_size optinoal
parameter is rarely first, but this should not be assumed. The
seek_head++ should happen at the end or in the loop update part of the
for loop above.

> +                      if (strncmp (seek_head, "sector_size:", sizeof ("sector_size:") - 1) == 0)
> +                        {
> +                          char *sector_size_str;
> +                          unsigned long long sector_size;
> +                          c = seek_head + sizeof ("sector_size:") - 1;
> +                          seek_head = grub_memchr (c, ' ', grub_strlen (c));
> +                          if (seek_head)
> +                            sector_size_str = grub_strndup (c, seek_head - c);

This has no corresponding grub_free().

> +                          else
> +                            sector_size_str = c;
> +                          sector_size = grub_strtoull (sector_size_str, NULL, 10);

This should check for in grub_strtoull errors as is done in commit
ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).

> +
> +                          if (sector_size != 512 && sector_size != 1024 &&
> +                              sector_size != 2048 && sector_size != 4096)
> +                            grub_util_error (_("dm-crypt parameter sector_size `%s' is not a valid LUKS2 sector size"),
> +                                             sector_size_str);
> +                          cryptodisk->log_sector_size = grub_log2ull (sector_size);

Now that the log_sector_size is changed from the default above, length
should also be updated to reflect the sector count in the newly
detected sector size.

> +                          grub_util_info ("set sector_size for dm `%s' to `%d'",

s/dm/cryptodisk/

> +                                          name, 1 << cryptodisk->log_sector_size);
> +                          break;
> +                        }
> +                    }
> +                }
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else

Glenn

[PATCH v3 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello Glenn,

Thanks for your review!  I've adressed all of your comments with these
revised patches (only the second one has changed), with the caveats below:

Glenn Washburn <development@efficientek.com> writes:
>
>> +                grub_util_error (_("can't set dm task name to `%s'"), name);
>> +              if (!dm_task_run (dmt))
>> +                grub_util_error (_("can't run dm task for `%s'"), name);
>> +              dm_get_next_target (dmt, NULL, &start, &length,
>> +                                  &target_type, &params);
>
> It looks like this might return NULL on error? Or does it never error
> here?

dm_get_next_target only unpacks the result of a dm_task, it does no
error handling at all.  We only need the first (and only) target here,
hence the NULL parameter.

> [...]
>
>> +              /* This is the only hash usable by PBKDF2, so set it as such */
>> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
>
> So does this mean that when/if argon2 support is added, that this will
> need to be detected as well? If so, is there a way to get the hash via
> devmapper? It doesn't show up with the table command, so I'm guessing
> not. On the otherhand, I don't think cryptodisk->hash is ever used in
> the user-space utilities because the cryptodisk is never attempted to
> be unlocked, so there's no password to hash. Perhaps this should be
> NULL with a comment that its not used (if I'm right about that).

Looking at the LUKS2 spec (see Key Derivation Object, section 3.2.5 of
[1]), it doesn't seem that argon2 has a way to specify different hash
functions, so I don't think we would need to detect that.  We need to
set cryptodisk->hash though so that the right abstractions are picked
up and included by grub-install in the binary (this was my motivation
for this whole thing in the first place)!

> [...]
>
>> +                          else
>> +                            sector_size_str = c;
>> +                          sector_size = grub_strtoull (sector_size_str, NULL, 10);
>
> This should check for in grub_strtoull errors as is done in commit
> ac8a37dda (net/http: Allow use of non-standard TCP/IP ports).

I didn't spot anything out of the ordinary in that commit, but note
that just after grub_strtoull, we check that sector_size is either
512, 1024, 2048 or 4096, and error if it isn't (unlikely).

In any case, I've tested this in a VM with a LUKS2 device that has a
4096 sector_size, and `-v` does indeed report the right sector_size as
well as the number of (encryption) sectors!  Installation of GNU Guix
went as expected on it, I even stacked LVM with BTRFS on top and
everything worked properly.

[1] https://gitlab.com/cryptsetup/LUKS2-docs/blob/master/luks2_doc_wip.pdf

Best,
Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 138 +++++++++++++++++++++++++++-
 1 file changed, 133 insertions(+), 5 deletions(-)

-- 
2.36.0

[PATCH v3 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.
---
 grub-core/osdep/devmapper/getroot.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..ad1daf9c8 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,7 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +179,7 @@ grub_util_pull_devmapper (const char *os_dev)
 	  grub_util_pull_device (subdev);
 	}
     }
-  if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+  if (uuid && strncmp (uuid, "CRYPT-LUKS", sizeof ("CRYPT-LUKS") - 1) == 0
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +253,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.36.0

[PATCH v3 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
filled out, otherwise they wouldn't be initialized if cheat mounted.
---
 grub-core/osdep/devmapper/getroot.c | 130 +++++++++++++++++++++++++++-
 1 file changed, 129 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index ad1daf9c8..b8d66029a 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -43,6 +43,8 @@
 #include <grub/osdep/major.h>
 #include <libdevmapper.h>
 
+#define DM_LOG_SECTOR_SIZE 9
+
 #include <grub/types.h>
 #include <grub/util/misc.h>
 
@@ -51,6 +53,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -183,7 +187,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -191,7 +194,132 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /* set LUKS2 cipher and size from dm parameters, since it is not
+               * possible to determine the correct ones without unlocking, as
+               * there might be multiple segments.
+               */
+              grub_disk_t source;
+              grub_cryptodisk_t cryptodisk;
+              grub_addr_t start, length;
+              char *target_type;
+              char *params;
+              const char *name;
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+
+              source = grub_disk_open (grdev);
+              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              grub_disk_close (source);
+
+              name = dm_tree_node_get_name (node);
+
+              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
+                              uuid, name);
+
+              dmt = dm_task_create (DM_DEVICE_TABLE);
+              if (dmt == 0)
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (dm_task_set_name (dmt, name) == 0)
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (dm_task_run (dmt) == 0)
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              /* dm_get_next_target doesn't have any error modes, everything has
+               * been handled by dm_task_run.
+               */
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target of type `%s' is not `crypt'"),
+                                 target_type);
+
+              cryptodisk->total_sectors = length;
+              cryptodisk->log_sector_size = DM_LOG_SECTOR_SIZE;
+
+              /* dm target parameters for dm-crypt is
+               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
+               */
+              char *seek_head, *c;
+              unsigned int remaining;
+              c = params;
+              remaining = grub_strlen (c);
+
+              /* first, get the cipher name from the cipher */
+              if (!(seek_head = grub_memchr (c, '-', remaining)))
+                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
+                                 params);
+              cipher = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c;
+              c = seek_head + 1;
+
+              /* now, the cipher mode */
+              if (!(seek_head = grub_memchr (c, ' ', remaining)))
+                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
+                                 params);
+              cipher_mode = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c;
+              c = seek_head + 1;
+
+              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+
+              grub_free (cipher);
+              grub_free (cipher_mode);
+
+              /* This is the only hash usable by PBKDF2, so set it as such */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+
+              /* drop key, iv_offset, device path and offset */
+              for (char dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++)
+                {
+                  seek_head = grub_memchr (c, ' ', remaining);
+                  remaining -= seek_head - c;
+                  c = seek_head + 1;
+                }
+
+              /* if we have optional parameters, skip #opt_params */
+              if (seek_head && (seek_head = grub_memchr (c, ' ', remaining)))
+                {
+                  remaining -= seek_head - c;
+                  c = seek_head + 1;
+                  while (remaining > 0)
+                    {
+                      if (strncmp (c, "sector_size:", sizeof ("sector_size:") - 1) == 0)
+                        {
+                          char *sector_size_str;
+                          unsigned long long sector_size;
+                          c += sizeof ("sector_size:") - 1;
+                          remaining -= sizeof ("sector_size:") - 1;
+                          seek_head = grub_memchr (c, ' ', remaining);
+                          if (seek_head)
+                            sector_size_str = grub_strndup (c, seek_head - c);
+                          else
+                            sector_size_str = grub_strndup (c, remaining);
+                          sector_size = grub_strtoull (sector_size_str, NULL, 10);
+                          grub_free (sector_size_str);
+
+                          if (sector_size != 512 && sector_size != 1024 &&
+                              sector_size != 2048 && sector_size != 4096)
+                            grub_util_error (_("dm-crypt parameter sector_size `%s' is not a valid LUKS2 sector size"),
+                                             sector_size_str);
+                          cryptodisk->log_sector_size = grub_log2ull (sector_size);
+                          cryptodisk->total_sectors = cryptodisk->total_sectors /
+                            (sector_size / (1 << DM_LOG_SECTOR_SIZE));
+                          grub_util_info ("set sector_size for cryptodisk `%s' to `%d', total_sectors to `%lu'",
+                                          name, 1 << cryptodisk->log_sector_size,
+                                          cryptodisk->total_sectors);
+                          break;
+                        }
+                      seek_head = grub_memchr (c, ' ', remaining);
+                      remaining -= seek_head - c;
+                      c = seek_head + 1;
+                    }
+                }
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.36.0

Re: [PATCH v3 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Glenn Washburn]

Thanks for making these updates.

On Fri, 20 May 2022 20:20:39 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> This lets a LUKS2 cryptodisk have all the cipher, hash, and sizes
> filled out, otherwise they wouldn't be initialized if cheat mounted.
> ---
>  grub-core/osdep/devmapper/getroot.c | 130 +++++++++++++++++++++++++++-
>  1 file changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index ad1daf9c8..b8d66029a 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -43,6 +43,8 @@
>  #include <grub/osdep/major.h>
>  #include <libdevmapper.h>
>  
> +#define DM_LOG_SECTOR_SIZE 9
> +
>  #include <grub/types.h>
>  #include <grub/util/misc.h>
>  
> @@ -51,6 +53,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -183,7 +187,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -191,7 +194,132 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /* set LUKS2 cipher and size from dm parameters, since it is not
> +               * possible to determine the correct ones without unlocking, as
> +               * there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_addr_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +
> +              source = grub_disk_open (grdev);
> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              grub_disk_close (source);
> +
> +              name = dm_tree_node_get_name (node);
> +
> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == 0)
> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
> +              if (dm_task_set_name (dmt, name) == 0)
> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (dm_task_run (dmt) == 0)
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              /* dm_get_next_target doesn't have any error modes, everything has
> +               * been handled by dm_task_run.
> +               */
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);
> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target of type `%s' is not `crypt'"),
> +                                 target_type);
> +
> +              cryptodisk->total_sectors = length;
> +              cryptodisk->log_sector_size = DM_LOG_SECTOR_SIZE;
> +
> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
> +               */
> +              char *seek_head, *c;
> +              unsigned int remaining;

Daniel might want these declared up with the rest at the start of the
block.

> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', remaining)))
> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c;

I think remaining should technically be one less here because of the
line below so that it can be used in the following grub_memchr() as is.

> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', remaining)))
> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c;

Ditto. Now we're off by 2 after the next line.

> +              c = seek_head + 1;
> +
> +              grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);

Probably should error check the return here.

> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, so set it as such */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");

This should do an error check for if (cryptodisk->hash == NULL).

My prior review had a comment here on if the hash might change if/when
argon2 is supported. I'm thinking that we can visit that when argon2
support gets added. So I'm fine with having hash be a constant.

> +
> +              /* drop key, iv_offset, device path and offset */
> +              for (char dropped_tokens = 0; dropped_tokens < 4; dropped_tokens++)

Why did dropped_tokens get changed to a "char"? Does it really buy us
anything?

> +                {

Probably should check for (seek_head == NULL), just in case. Perhaps if
true do: 
  grub_util_error (_("dm-crypt parameters missing required fields
`%s'"), params);

I'm suggesting this above the grub_memchr() so that we don't check the
last iteration of the loop, in which seek_head == NULL is not an error.
If you want to restructure this some other way, go ahead.

> +                  seek_head = grub_memchr (c, ' ', remaining);
> +                  remaining -= seek_head - c;

Ditto, on an off-by-one error here. These will accumulate such that
grub_memchr() may return a position outside the bounds of the params
string.

> +                  c = seek_head + 1;
> +                }
> +
> +              /* if we have optional parameters, skip #opt_params */

This comment doesn't quite make sense to me. What is #opt_params? I
presume "number of optional parameters", but the code doesn't skip
optional parameters. It searches through the optional parameters for
sector_size, and once it finds it, then breaks out. It seems like it
should be something like:

/* if we have optional parameters, search for sector_size */

This comment would still be good even if getting rid of the if block
enclosing the while loop as suggested below.

> +              if (seek_head && (seek_head = grub_memchr (c, ' ', remaining)))
> +                {
> +                  remaining -= seek_head - c;
> +                  c = seek_head + 1;

I still don't see the utility of these lines above, along with the "if"
statement. You should be able to remove them, thus having the while
outside of this block, and still the code would be correct.

The check for seek_head != NULL can be done in the while condition. If
there are optional parameters, then seek_head should be non-NULL and
"c" should point to the first character of the first listed optional
parameter, assuming all parameters are separated by a single space.
Even if that is not true and there are multiple spaces between some
parameters, the while loop will handle it fine, just perhaps not as
efficiently as it could.

> +                  while (remaining > 0)
> +                    {
> +                      if (strncmp (c, "sector_size:", sizeof ("sector_size:") - 1) == 0)

If we're concerned about multiple spaces between parameters, we could
make this more efficient by using this conditional instead:

if (*c != ' ' && strncmp (c, "sector_size:", sizeof ("sector_size:") -
1) == 0)

This way the strncmp only runs if the character c points to is not a
space. I'm not really concerned about extra spaces though and am fine
with the inefficiency if there are.

> +                        {
> +                          char *sector_size_str;
> +                          unsigned long long sector_size;
> +                          c += sizeof ("sector_size:") - 1;
> +                          remaining -= sizeof ("sector_size:") - 1;
> +                          seek_head = grub_memchr (c, ' ', remaining);
> +                          if (seek_head)
> +                            sector_size_str = grub_strndup (c, seek_head - c);
> +                          else
> +                            sector_size_str = grub_strndup (c, remaining);
> +                          sector_size = grub_strtoull (sector_size_str, NULL, 10);

This 

> +                          grub_free (sector_size_str);
> +
> +                          if (sector_size != 512 && sector_size != 1024 &&
> +                              sector_size != 2048 && sector_size != 4096)
> +                            grub_util_error (_("dm-crypt parameter sector_size `%s' is not a valid LUKS2 sector size"),
> +                                             sector_size_str);
> +                          cryptodisk->log_sector_size = grub_log2ull (sector_size);
> +                          cryptodisk->total_sectors = cryptodisk->total_sectors /
> +                            (sector_size / (1 << DM_LOG_SECTOR_SIZE));

Should use grub_convert_sector() in include/grub/disk.h, eg:

 cryptodisk->total_sectors = grub_convert_sector
 (cryptodisk->total_sectors,
      cryptodisk->log_sector_size, DM_LOG_SECTOR_SIZE);

> +                          grub_util_info ("set sector_size for cryptodisk `%s' to `%d', total_sectors to `%lu'",
> +                                          name, 1 << cryptodisk->log_sector_size,

could use sector_size instead of 1 << cryptodisk->log_sector_size

> +                                          cryptodisk->total_sectors);
> +                          break;
> +                        }
> +                      seek_head = grub_memchr (c, ' ', remaining);
> +                      remaining -= seek_head - c;

If none of the optional parameters start with "sector_size:" and
remaining is correct, then we'll come to the last optional parameter
and grub_memchr() will return NULL. Then remaining will be (remaining -
( 0 - c )) or (remaining + c), where c is likely a large number. Then
the while condition, remaining > 0, will continue to be true and go
through another iteration. But onthis next iteration c will be pointing
to address 1 because of the line below. Accessing *c at that point
might cause a crash or later on as the train has been derailed.

> +                      c = seek_head + 1;
> +                    }
> +                }
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else

After reviewing this, I stumbled upon Fabian's patch. I wrote a
response on the thread of that patch with the suggestion that his patch
be used to get the total number of sectors and sector size and this
patch be used to get the crypto information. This would obviate the
problematic parsing part of this patch. What do you think about
removing all the parsing code after the setting of cryptodisk->hash?

Glenn

Re: [PATCH v3 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

Hello Glenn, 

Glenn Washburn <development@efficientek.com> writes:

> After reviewing this, I stumbled upon Fabian's patch. I wrote a
> response on the thread of that patch with the suggestion that his patch
> be used to get the total number of sectors and sector size and this
> patch be used to get the crypto information. This would obviate the
> problematic parsing part of this patch. What do you think about
> removing all the parsing code after the setting of cryptodisk->hash?

I just read that mail, and it does seem like the proper way (I don't
need much convincing to remove most of that ugly parsing code!).  I'll
send a cleaned up patch later with all the logic errors adressed, and
without the further sector size code.

Best,
-- 
Josselin Poiret

[PATCH v4 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello Glenn,

I took the time to rebase all the patches on the latest master, then
remove the problematic parsing, keeping only the cipher part, as well
as add some additional error checking for grub_cryptodisk_setcipher
and grub_crypto_lookup_md_by_name.  I also took note of another mail
saying that we shouldn't pretend to support LUKS3 if it ever comes
out, so I've modified the first patch to only recognize LUKS1 and
LUKS2.  With Fabian's patch [1] applied, this made grub-install
install fine on a LUKS2 drive with a 4096 sector size.

I hope all the logic errors were corrected (removing the complicated
part should have helped).

[1] 8075647.T7Z3S40VBb@linux-e202.suse.de
    (https://lists.gnu.org/archive/html/grub-devel/2022-06/msg00097.html)

Best,
Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 102 ++++++++++++++++++++++++++--
 1 file changed, 97 insertions(+), 5 deletions(-)

-- 
2.36.1

[PATCH v4 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.
---
 grub-core/osdep/devmapper/getroot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..2bf4264cf 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,8 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+      || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +180,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
+  if (uuid
+      && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+          || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +256,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.36.1

[PATCH v4 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have its cipher and hash filled out,
otherwise they wouldn't be initialized if cheat mounted.
---
 grub-core/osdep/devmapper/getroot.c | 91 ++++++++++++++++++++++++++++-
 1 file changed, 90 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 2bf4264cf..ac90761ea 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -51,6 +51,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -194,7 +195,95 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /* set LUKS2 cipher from dm parameters, since it is not
+               * possible to determine the correct one without
+               * unlocking, as there might be multiple segments.
+               */
+              grub_disk_t source;
+              grub_cryptodisk_t cryptodisk;
+              grub_addr_t start, length;
+              char *target_type;
+              char *params;
+              const char *name;
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+              char *seek_head, *c;
+              unsigned int remaining;
+
+              source = grub_disk_open (grdev);
+              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              grub_disk_close (source);
+
+              name = dm_tree_node_get_name (node);
+
+              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
+                              uuid, name);
+
+              dmt = dm_task_create (DM_DEVICE_TABLE);
+              if (dmt == 0)
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (dm_task_set_name (dmt, name) == 0)
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (dm_task_run (dmt) == 0)
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              /* dm_get_next_target doesn't have any error modes, everything has
+               * been handled by dm_task_run.
+               */
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target of type `%s' is not `crypt'"),
+                                 target_type);
+
+              /* dm target parameters for dm-crypt is
+               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
+               */
+              c = params;
+              remaining = grub_strlen (c);
+
+              /* first, get the cipher name from the cipher */
+              if (!(seek_head = grub_memchr (c, '-', remaining)))
+                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
+                                 params);
+              cipher = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              /* now, the cipher mode */
+              if (!(seek_head = grub_memchr (c, ' ', remaining)))
+                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
+                                 params);
+              cipher_mode = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+              if (err)
+                {
+                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
+                                   uuid, cipher, cipher_mode);
+                }
+
+              grub_free (cipher);
+              grub_free (cipher_mode);
+
+              /* This is the only hash usable by PBKDF2, and we don't
+               * have Argon2 support yet, so set it by default,
+               * otherwise grub-probe would miss the required
+               * abstraction
+               */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+              if (cryptodisk->hash == 0)
+                {
+                  grub_util_error (_("can't lookup hash sha256 by name"));
+                }
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.36.1

Re: [PATCH v4 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Glenn Washburn]

On Tue, 14 Jun 2022 15:47:30 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.
> ---
>  grub-core/osdep/devmapper/getroot.c | 91 ++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..ac90761ea 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -194,7 +195,95 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /* set LUKS2 cipher from dm parameters, since it is not
> +               * possible to determine the correct one without
> +               * unlocking, as there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_addr_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +              char *seek_head, *c;
> +              unsigned int remaining;
> +
> +              source = grub_disk_open (grdev);
> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              grub_disk_close (source);
> +
> +              name = dm_tree_node_get_name (node);
> +
> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == 0)
> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
> +              if (dm_task_set_name (dmt, name) == 0)
> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (dm_task_run (dmt) == 0)
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              /* dm_get_next_target doesn't have any error modes, everything has
> +               * been handled by dm_task_run.
> +               */
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);
> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target of type `%s' is not `crypt'"),
> +                                 target_type);
> +
> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
> +               */
> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', remaining)))
> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', remaining)))
> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +              if (err)
> +                {
> +                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
> +                                   uuid, cipher, cipher_mode);
> +                }

One small nit, Daniel will likely request that the curly braces be
removed above.

> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, and we don't
> +               * have Argon2 support yet, so set it by default,
> +               * otherwise grub-probe would miss the required
> +               * abstraction
> +               */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
> +              if (cryptodisk->hash == 0)
> +                {
> +                  grub_util_error (_("can't lookup hash sha256 by name"));
> +                }

Same here for curly braces.

> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else

Other than that, looks good.

Glenn

Re: [PATCH v4 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Michael Chang]

On Tue, Jun 14, 2022 at 03:47:30PM +0200, The development of GNU GRUB wrote:
> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.
> ---
>  grub-core/osdep/devmapper/getroot.c | 91 ++++++++++++++++++++++++++++-
>  1 file changed, 90 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..ac90761ea 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -194,7 +195,95 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /* set LUKS2 cipher from dm parameters, since it is not
> +               * possible to determine the correct one without
> +               * unlocking, as there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_addr_t start, length;

Just a heads up. This failed to build on 32-bit architectures (i586,
armv7l) with following error:

[  141s] ../grub-core/osdep/devmapper/getroot.c: In function 'grub_util_pull_devmapper':
[  141s] ../grub-core/osdep/devmapper/getroot.c:234:46: error: passing argument 3 of 'dm_get_next_target' from incompatible pointer type [-Werror=incompatible-pointer-types]
[  141s]   234 |               dm_get_next_target (dmt, NULL, &start, &length,
[  141s]       |                                              ^~~~~~
[  141s]       |                                              |
[  141s]       |                                              grub_addr_t * {aka unsigned int *}
[  141s] In file included from ../grub-core/osdep/devmapper/getroot.c:44:
[  141s] /usr/include/libdevmapper.h:288:48: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'grub_addr_t *' {aka 'unsigned int *'}
[  141s]   288 |                          void *next, uint64_t *start, uint64_t *length,
[  141s]       |                                      ~~~~~~~~~~^~~~~
[  141s] ../grub-core/osdep/devmapper/getroot.c:234:54: error: passing argument 4 of 'dm_get_next_target' from incompatible pointer type [-Werror=incompatible-pointer-types]
[  141s]   234 |               dm_get_next_target (dmt, NULL, &start, &length,
[  141s]       |                                                      ^~~~~~~
[  141s]       |                                                      |
[  141s]       |                                                      grub_addr_t * {aka unsigned int *}
[  141s] /usr/include/libdevmapper.h:288:65: note: expected 'uint64_t *' {aka 'long long unsigned int *'} but argument is of type 'grub_addr_t *' {aka 'unsigned int *'}
[  141s]   288 |                          void *next, uint64_t *start, uint64_t *length,
[  141s]       |                                                       ~~~~~~~~~~^~~~~~

Apparently changing to use 'grub_uint64_t' for both start and length
fixed the problem for me.

Thanks,
Michael

> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +              char *seek_head, *c;
> +              unsigned int remaining;
> +
> +              source = grub_disk_open (grdev);
> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              grub_disk_close (source);
> +
> +              name = dm_tree_node_get_name (node);
> +
> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == 0)
> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
> +              if (dm_task_set_name (dmt, name) == 0)
> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (dm_task_run (dmt) == 0)
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              /* dm_get_next_target doesn't have any error modes, everything has
> +               * been handled by dm_task_run.
> +               */
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);
> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target of type `%s' is not `crypt'"),
> +                                 target_type);
> +
> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
> +               */
> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', remaining)))
> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', remaining)))
> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +              if (err)
> +                {
> +                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
> +                                   uuid, cipher, cipher_mode);
> +                }
> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, and we don't
> +               * have Argon2 support yet, so set it by default,
> +               * otherwise grub-probe would miss the required
> +               * abstraction
> +               */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
> +              if (cryptodisk->hash == 0)
> +                {
> +                  grub_util_error (_("can't lookup hash sha256 by name"));
> +                }
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else
> -- 
> 2.36.1
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v4 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Glenn Washburn]

On Tue, 14 Jun 2022 15:47:28 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Hello Glenn,
> 
> I took the time to rebase all the patches on the latest master, then
> remove the problematic parsing, keeping only the cipher part, as well
> as add some additional error checking for grub_cryptodisk_setcipher
> and grub_crypto_lookup_md_by_name.  I also took note of another mail
> saying that we shouldn't pretend to support LUKS3 if it ever comes
> out, so I've modified the first patch to only recognize LUKS1 and
> LUKS2.  With Fabian's patch [1] applied, this made grub-install
> install fine on a LUKS2 drive with a 4096 sector size.
> 
> I hope all the logic errors were corrected (removing the complicated
> part should have helped).

Great, thanks for the update! Except for a couple minor nits, it all
looks good to me. Adding Patrick and Daniel.

For the whole series:
  Reviewed-by: Glenn Washburn <development@efficientek.com>

Glenn

> 
> [1] 8075647.T7Z3S40VBb@linux-e202.suse.de
>     (https://lists.gnu.org/archive/html/grub-devel/2022-06/msg00097.html)
> 
> Best,
> Josselin Poiret (2):
>   devmapper/getroot: Have devmapper recognize LUKS2
>   devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
>     parameters
> 
>  grub-core/osdep/devmapper/getroot.c | 102 ++++++++++++++++++++++++++--
>  1 file changed, 97 insertions(+), 5 deletions(-)
> 

[PATCH v5 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello again,

Following Michael's mail, here's hopefully the latest patch series!
This fixes building on 32-bit by using grub_uint64_t indiscriminately,
and removes the curly braces for the the two ifs at the end.

Sorry for all the noise.

Best,
-- 
Josselin Poiret

[PATCH v5 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.
---
 grub-core/osdep/devmapper/getroot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..2bf4264cf 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,8 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+      || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +180,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
+  if (uuid
+      && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+          || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +256,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.36.1

[PATCH v5 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have its cipher and hash filled out,
otherwise they wouldn't be initialized if cheat mounted.
---
 grub-core/osdep/devmapper/getroot.c | 87 ++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 2bf4264cf..80c7e54da 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -51,6 +51,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -194,7 +195,91 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /* set LUKS2 cipher from dm parameters, since it is not
+               * possible to determine the correct one without
+               * unlocking, as there might be multiple segments.
+               */
+              grub_disk_t source;
+              grub_cryptodisk_t cryptodisk;
+              grub_uint64_t start, length;
+              char *target_type;
+              char *params;
+              const char *name;
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+              char *seek_head, *c;
+              unsigned int remaining;
+
+              source = grub_disk_open (grdev);
+              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              grub_disk_close (source);
+
+              name = dm_tree_node_get_name (node);
+
+              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
+                              uuid, name);
+
+              dmt = dm_task_create (DM_DEVICE_TABLE);
+              if (dmt == 0)
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (dm_task_set_name (dmt, name) == 0)
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (dm_task_run (dmt) == 0)
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              /* dm_get_next_target doesn't have any error modes, everything has
+               * been handled by dm_task_run.
+               */
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target of type `%s' is not `crypt'"),
+                                 target_type);
+
+              /* dm target parameters for dm-crypt is
+               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
+               */
+              c = params;
+              remaining = grub_strlen (c);
+
+              /* first, get the cipher name from the cipher */
+              if (!(seek_head = grub_memchr (c, '-', remaining)))
+                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
+                                 params);
+              cipher = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              /* now, the cipher mode */
+              if (!(seek_head = grub_memchr (c, ' ', remaining)))
+                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
+                                 params);
+              cipher_mode = grub_strndup (c, seek_head - c);
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+              if (err)
+                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
+                                   uuid, cipher, cipher_mode);
+
+              grub_free (cipher);
+              grub_free (cipher_mode);
+
+              /* This is the only hash usable by PBKDF2, and we don't
+               * have Argon2 support yet, so set it by default,
+               * otherwise grub-probe would miss the required
+               * abstraction
+               */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+              if (cryptodisk->hash == 0)
+                  grub_util_error (_("can't lookup hash sha256 by name"));
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.36.1

Re: [PATCH v5 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Daniel Kiper]

On Wed, Jun 15, 2022 at 12:02:29PM +0200, Josselin Poiret via Grub-devel wrote:
> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.

Please add your Signed-off-by here. Same applies to first patch too.

> ---
>  grub-core/osdep/devmapper/getroot.c | 87 ++++++++++++++++++++++++++++-
>  1 file changed, 86 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..80c7e54da 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);
>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -194,7 +195,91 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /* set LUKS2 cipher from dm parameters, since it is not
> +               * possible to determine the correct one without
> +               * unlocking, as there might be multiple segments.
> +               */

Please format comments, here and below, properly [1].

> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_uint64_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +              char *seek_head, *c;
> +              unsigned int remaining;
> +
> +              source = grub_disk_open (grdev);

This function may fail. Please check the returned value and fail if needed.

> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);

Ditto.

> +              grub_disk_close (source);
> +
> +              name = dm_tree_node_get_name (node);

I think same applies here...

> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == 0)

s/0/NULL/ Please use NULL instead of 0 for pointers. Same below...

> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));

Yes, fail properly like here...

> +              if (dm_task_set_name (dmt, name) == 0)
> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (dm_task_run (dmt) == 0)
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              /* dm_get_next_target doesn't have any error modes, everything has
> +               * been handled by dm_task_run.
> +               */
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);
> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target of type `%s' is not `crypt'"),
> +                                 target_type);

I am OK with lines a bit longer than 80. So, you can put this in one line.

> +              /* dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
> +               */
> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              if (!(seek_head = grub_memchr (c, '-', remaining)))
> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);

grub_strndup() may fail. Please check if cipher != NULL.

> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              if (!(seek_head = grub_memchr (c, ' ', remaining)))

seek_head = grub_memchr (c, ' ', remaining);
if (seek_head == NULL)

> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);

Again, grub_strndup() may fail. In general please do not ignore errors
from functions which may fail.

> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +              if (err)
> +                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
> +                                   uuid, cipher, cipher_mode);
> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /* This is the only hash usable by PBKDF2, and we don't
> +               * have Argon2 support yet, so set it by default,
> +               * otherwise grub-probe would miss the required
> +               * abstraction
> +               */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
> +              if (cryptodisk->hash == 0)
> +                  grub_util_error (_("can't lookup hash sha256 by name"));
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else

Daniel

[1] https://www.gnu.org/software/grub/manual/grub-dev/grub-dev.html#Comments

[PATCH v6 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Josselin Poiret]

Hello Daniel,

Thanks for the review.  The following updated patches should contain all the changes you asked for.

>Please add your Signed-off-by here. Same applies to first patch too.
Done.

> Please format comments, here and below, properly [1].
Sorry about that, I missed the empty first line. Fixed.

> This function may fail. Please check the returned value and fail if needed.
> [...]
> Ditto.
Fixed (I've used grub_util_error for all the unrecoverable errors, I
hope that's ok).

>> +              name = dm_tree_node_get_name (node);
> I think same applies here...
Right, it can be "", so added a check and comment about the error mode.

> s/0/NULL/ Please use NULL instead of 0 for pointers. Same below...
Done.

> grub_strndup() may fail. Please check if cipher != NULL.
Right, sorry.  Fixed.

> seek_head = grub_memchr (c, ' ', remaining);
> if (seek_head == NULL)
Done in both places.

> Again, grub_strndup() may fail. In general please do not ignore errors
> from functions which may fail.
Done as above.

Josselin Poiret (2):
  devmapper/getroot: Have devmapper recognize LUKS2
  devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
    parameters

 grub-core/osdep/devmapper/getroot.c | 118 ++++++++++++++++++++++++++--
 1 file changed, 113 insertions(+), 5 deletions(-)

Range-diff against v5:
1:  7af629fca = 1:  f4cbda414 devmapper/getroot: Have devmapper recognize LUKS2
2:  5f9f26118 ! 2:  25ce8bbcc devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters
    @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
      			     lastsubdev, grub_errmsg);
     +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
     +            {
    -+              /* set LUKS2 cipher from dm parameters, since it is not
    ++              /*
    ++               * set LUKS2 cipher from dm parameters, since it is not
     +               * possible to determine the correct one without
     +               * unlocking, as there might be multiple segments.
     +               */
    @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
     +              unsigned int remaining;
     +
     +              source = grub_disk_open (grdev);
    ++              if (! source)
    ++                grub_util_error (_("cannot open grub disk `%s'"), grdev);
     +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
    ++              if (! cryptodisk)
    ++                grub_util_error (_("cannot get cryptodisk from source disk `%s'"), grdev);
     +              grub_disk_close (source);
     +
    ++              /*
    ++               * the following function always returns a non-NULL pointer,
    ++               * but the string may be empty if the relevant info is not present
    ++               */
     +              name = dm_tree_node_get_name (node);
    ++              if (grub_strlen (name) == 0)
    ++                grub_util_error (_("cannot get dm node name for grub dev `%s'"), grdev);
     +
     +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
     +                              uuid, name);
     +
     +              dmt = dm_task_create (DM_DEVICE_TABLE);
    -+              if (dmt == 0)
    ++              if (dmt == NULL)
     +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
     +              if (dm_task_set_name (dmt, name) == 0)
     +                grub_util_error (_("can't set dm task name to `%s'"), name);
     +              if (dm_task_run (dmt) == 0)
     +                grub_util_error (_("can't run dm task for `%s'"), name);
    -+              /* dm_get_next_target doesn't have any error modes, everything has
    ++              /*
    ++               * dm_get_next_target doesn't have any error modes, everything has
     +               * been handled by dm_task_run.
     +               */
     +              dm_get_next_target (dmt, NULL, &start, &length,
     +                                  &target_type, &params);
     +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
    -+                grub_util_error (_("dm target of type `%s' is not `crypt'"),
    -+                                 target_type);
    ++                grub_util_error (_("dm target of type `%s' is not `crypt'"), target_type);
     +
    -+              /* dm target parameters for dm-crypt is
    ++              /*
    ++               * dm target parameters for dm-crypt is
     +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
     +               */
     +              c = params;
     +              remaining = grub_strlen (c);
     +
     +              /* first, get the cipher name from the cipher */
    -+              if (!(seek_head = grub_memchr (c, '-', remaining)))
    ++              seek_head = grub_memchr (c, '-', remaining);
    ++              if (seek_head == NULL)
     +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
     +                                 params);
     +              cipher = grub_strndup (c, seek_head - c);
    ++              if (cipher == NULL)
    ++                grub_util_error (_("could not strndup cipher of length `%lu'"), seek_head - c);
     +              remaining -= seek_head - c + 1;
     +              c = seek_head + 1;
     +
     +              /* now, the cipher mode */
    -+              if (!(seek_head = grub_memchr (c, ' ', remaining)))
    ++              seek_head = grub_memchr (c, ' ', remaining);
    ++              if (seek_head == NULL)
     +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
     +                                 params);
     +              cipher_mode = grub_strndup (c, seek_head - c);
    ++              if (cipher_mode == NULL)
    ++                grub_util_error (_("could not strndup cipher_mode of length `%lu'"), seek_head - c);
    ++
     +              remaining -= seek_head - c + 1;
     +              c = seek_head + 1;
     +
    @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
     +              grub_free (cipher);
     +              grub_free (cipher_mode);
     +
    -+              /* This is the only hash usable by PBKDF2, and we don't
    ++              /*
    ++               * This is the only hash usable by PBKDF2, and we don't
     +               * have Argon2 support yet, so set it by default,
     +               * otherwise grub-probe would miss the required
     +               * abstraction
     +               */
     +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
    -+              if (cryptodisk->hash == 0)
    ++              if (cryptodisk->hash == NULL)
     +                  grub_util_error (_("can't lookup hash sha256 by name"));
     +
     +              dm_task_destroy (dmt);

base-commit: 351c9c2fd0bcd94c7fda5b697d3283f7f0ff7930
-- 
2.36.1

[PATCH v6 1/2] devmapper/getroot: Have devmapper recognize LUKS2

[Josselin Poiret]

Changes UUID comparisons so that LUKS1 and LUKS2 are both recognized
as being LUKS cryptodisks.

Signed-off-by: Josselin Poiret <dev@jpoiret.xyz>
---
 grub-core/osdep/devmapper/getroot.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 9ba5c9865..2bf4264cf 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -138,7 +138,8 @@ grub_util_get_dm_abstraction (const char *os_dev)
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LVM;
     }
-  if (strncmp (uuid, "CRYPT-LUKS1-", 12) == 0)
+  if (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+      || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
     {
       grub_free (uuid);
       return GRUB_DEV_ABSTRACTION_LUKS;
@@ -179,7 +180,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
+  if (uuid
+      && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - 1) == 0
+          || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
@@ -253,11 +256,11 @@ grub_util_get_devmapper_grub_dev (const char *os_dev)
       {
 	char *dash;
 
-	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS1-") - 1, '-');
+	dash = grub_strchr (uuid + sizeof ("CRYPT-LUKS*-") - 1, '-');
 	if (dash)
 	  *dash = 0;
 	grub_dev = grub_xasprintf ("cryptouuid/%s",
-				   uuid + sizeof ("CRYPT-LUKS1-") - 1);
+				   uuid + sizeof ("CRYPT-LUKS*-") - 1);
 	grub_free (uuid);
 	return grub_dev;
       }
-- 
2.36.1

[PATCH v6 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Josselin Poiret]

This lets a LUKS2 cryptodisk have its cipher and hash filled out,
otherwise they wouldn't be initialized if cheat mounted.

Signed-off-by: Josselin Poiret <dev@jpoiret.xyz>
---
 grub-core/osdep/devmapper/getroot.c | 107 +++++++++++++++++++++++++++-
 1 file changed, 106 insertions(+), 1 deletion(-)

diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
index 2bf4264cf..090ac7167 100644
--- a/grub-core/osdep/devmapper/getroot.c
+++ b/grub-core/osdep/devmapper/getroot.c
@@ -51,6 +51,8 @@
 #include <grub/emu/misc.h>
 #include <grub/emu/hostdisk.h>
 
+#include <grub/cryptodisk.h>
+
 static int
 grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
 		   struct dm_tree_node **node)
@@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
       && lastsubdev)
     {
       char *grdev = grub_util_get_grub_dev (lastsubdev);
-      dm_tree_free (tree);
       if (grdev)
 	{
 	  grub_err_t err;
@@ -194,7 +195,111 @@ grub_util_pull_devmapper (const char *os_dev)
 	  if (err)
 	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
 			     lastsubdev, grub_errmsg);
+          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
+            {
+              /*
+               * set LUKS2 cipher from dm parameters, since it is not
+               * possible to determine the correct one without
+               * unlocking, as there might be multiple segments.
+               */
+              grub_disk_t source;
+              grub_cryptodisk_t cryptodisk;
+              grub_uint64_t start, length;
+              char *target_type;
+              char *params;
+              const char *name;
+              char *cipher, *cipher_mode;
+              struct dm_task *dmt;
+              char *seek_head, *c;
+              unsigned int remaining;
+
+              source = grub_disk_open (grdev);
+              if (! source)
+                grub_util_error (_("cannot open grub disk `%s'"), grdev);
+              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
+              if (! cryptodisk)
+                grub_util_error (_("cannot get cryptodisk from source disk `%s'"), grdev);
+              grub_disk_close (source);
+
+              /*
+               * the following function always returns a non-NULL pointer,
+               * but the string may be empty if the relevant info is not present
+               */
+              name = dm_tree_node_get_name (node);
+              if (grub_strlen (name) == 0)
+                grub_util_error (_("cannot get dm node name for grub dev `%s'"), grdev);
+
+              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
+                              uuid, name);
+
+              dmt = dm_task_create (DM_DEVICE_TABLE);
+              if (dmt == NULL)
+                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
+              if (dm_task_set_name (dmt, name) == 0)
+                grub_util_error (_("can't set dm task name to `%s'"), name);
+              if (dm_task_run (dmt) == 0)
+                grub_util_error (_("can't run dm task for `%s'"), name);
+              /*
+               * dm_get_next_target doesn't have any error modes, everything has
+               * been handled by dm_task_run.
+               */
+              dm_get_next_target (dmt, NULL, &start, &length,
+                                  &target_type, &params);
+              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
+                grub_util_error (_("dm target of type `%s' is not `crypt'"), target_type);
+
+              /*
+               * dm target parameters for dm-crypt is
+               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
+               */
+              c = params;
+              remaining = grub_strlen (c);
+
+              /* first, get the cipher name from the cipher */
+              seek_head = grub_memchr (c, '-', remaining);
+              if (seek_head == NULL)
+                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
+                                 params);
+              cipher = grub_strndup (c, seek_head - c);
+              if (cipher == NULL)
+                grub_util_error (_("could not strndup cipher of length `%lu'"), seek_head - c);
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              /* now, the cipher mode */
+              seek_head = grub_memchr (c, ' ', remaining);
+              if (seek_head == NULL)
+                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
+                                 params);
+              cipher_mode = grub_strndup (c, seek_head - c);
+              if (cipher_mode == NULL)
+                grub_util_error (_("could not strndup cipher_mode of length `%lu'"), seek_head - c);
+
+              remaining -= seek_head - c + 1;
+              c = seek_head + 1;
+
+              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
+              if (err)
+                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
+                                   uuid, cipher, cipher_mode);
+
+              grub_free (cipher);
+              grub_free (cipher_mode);
+
+              /*
+               * This is the only hash usable by PBKDF2, and we don't
+               * have Argon2 support yet, so set it by default,
+               * otherwise grub-probe would miss the required
+               * abstraction
+               */
+              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
+              if (cryptodisk->hash == NULL)
+                  grub_util_error (_("can't lookup hash sha256 by name"));
+
+              dm_task_destroy (dmt);
+            }
 	}
+      dm_tree_free (tree);
       grub_free (grdev);
     }
   else
-- 
2.36.1

Re: [PATCH v6 2/2] devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters

[Fabian Vogt]

Hi,

I've played around with LUKS2 using cipher_null and discovered a small issue
with error handling.

Am Freitag, 8. Juli 2022, 12:06:08 CET schrieb Josselin Poiret via Grub-devel:
> This lets a LUKS2 cryptodisk have its cipher and hash filled out,
> otherwise they wouldn't be initialized if cheat mounted.
> 
> Signed-off-by: Josselin Poiret <dev@jpoiret.xyz>
> ---
>  grub-core/osdep/devmapper/getroot.c | 107 +++++++++++++++++++++++++++-
>  1 file changed, 106 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/osdep/devmapper/getroot.c b/grub-core/osdep/devmapper/getroot.c
> index 2bf4264cf..090ac7167 100644
> --- a/grub-core/osdep/devmapper/getroot.c
> +++ b/grub-core/osdep/devmapper/getroot.c
> @@ -51,6 +51,8 @@
>  #include <grub/emu/misc.h>
>  #include <grub/emu/hostdisk.h>
>  
> +#include <grub/cryptodisk.h>
> +
>  static int
>  grub_util_open_dm (const char *os_dev, struct dm_tree **tree,
>  		   struct dm_tree_node **node)
> @@ -186,7 +188,6 @@ grub_util_pull_devmapper (const char *os_dev)
>        && lastsubdev)
>      {
>        char *grdev = grub_util_get_grub_dev (lastsubdev);
> -      dm_tree_free (tree);

This change means that in the error paths (grub_util_error), tree is not freed
when exit is called. This should not matter much as the program is about to
exit anyway, but with libdm it does. It registers an exit handler which checks
for leaked objects:

Installing for x86_64-efi platform.
/usr/sbin/grub2-install: error: can't set cipher of cryptodisk `CRYPT-LUKS2-8c0468e27cd4453083107943f126d303-luks' to `cipher_null' with mode `ecb'.
You have a memory leak (not released memory pool):
 [0x562f91463370] dtree
Internal error: Unreleased memory pool(s) found.

This only happens in the failure case so it's just another error message and
not a critical issue.

Cheers,
Fabian

>        if (grdev)
>  	{
>  	  grub_err_t err;
> @@ -194,7 +195,111 @@ grub_util_pull_devmapper (const char *os_dev)
>  	  if (err)
>  	    grub_util_error (_("can't mount encrypted volume `%s': %s"),
>  			     lastsubdev, grub_errmsg);
> +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
> +            {
> +              /*
> +               * set LUKS2 cipher from dm parameters, since it is not
> +               * possible to determine the correct one without
> +               * unlocking, as there might be multiple segments.
> +               */
> +              grub_disk_t source;
> +              grub_cryptodisk_t cryptodisk;
> +              grub_uint64_t start, length;
> +              char *target_type;
> +              char *params;
> +              const char *name;
> +              char *cipher, *cipher_mode;
> +              struct dm_task *dmt;
> +              char *seek_head, *c;
> +              unsigned int remaining;
> +
> +              source = grub_disk_open (grdev);
> +              if (! source)
> +                grub_util_error (_("cannot open grub disk `%s'"), grdev);
> +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
> +              if (! cryptodisk)
> +                grub_util_error (_("cannot get cryptodisk from source disk `%s'"), grdev);
> +              grub_disk_close (source);
> +
> +              /*
> +               * the following function always returns a non-NULL pointer,
> +               * but the string may be empty if the relevant info is not present
> +               */
> +              name = dm_tree_node_get_name (node);
> +              if (grub_strlen (name) == 0)
> +                grub_util_error (_("cannot get dm node name for grub dev `%s'"), grdev);
> +
> +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
> +                              uuid, name);
> +
> +              dmt = dm_task_create (DM_DEVICE_TABLE);
> +              if (dmt == NULL)
> +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
> +              if (dm_task_set_name (dmt, name) == 0)
> +                grub_util_error (_("can't set dm task name to `%s'"), name);
> +              if (dm_task_run (dmt) == 0)
> +                grub_util_error (_("can't run dm task for `%s'"), name);
> +              /*
> +               * dm_get_next_target doesn't have any error modes, everything has
> +               * been handled by dm_task_run.
> +               */
> +              dm_get_next_target (dmt, NULL, &start, &length,
> +                                  &target_type, &params);
> +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
> +                grub_util_error (_("dm target of type `%s' is not `crypt'"), target_type);
> +
> +              /*
> +               * dm target parameters for dm-crypt is
> +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
> +               */
> +              c = params;
> +              remaining = grub_strlen (c);
> +
> +              /* first, get the cipher name from the cipher */
> +              seek_head = grub_memchr (c, '-', remaining);
> +              if (seek_head == NULL)
> +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher = grub_strndup (c, seek_head - c);
> +              if (cipher == NULL)
> +                grub_util_error (_("could not strndup cipher of length `%lu'"), seek_head - c);
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              /* now, the cipher mode */
> +              seek_head = grub_memchr (c, ' ', remaining);
> +              if (seek_head == NULL)
> +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
> +                                 params);
> +              cipher_mode = grub_strndup (c, seek_head - c);
> +              if (cipher_mode == NULL)
> +                grub_util_error (_("could not strndup cipher_mode of length `%lu'"), seek_head - c);
> +
> +              remaining -= seek_head - c + 1;
> +              c = seek_head + 1;
> +
> +              err = grub_cryptodisk_setcipher (cryptodisk, cipher, cipher_mode);
> +              if (err)
> +                  grub_util_error (_("can't set cipher of cryptodisk `%s' to `%s' with mode `%s'"),
> +                                   uuid, cipher, cipher_mode);
> +
> +              grub_free (cipher);
> +              grub_free (cipher_mode);
> +
> +              /*
> +               * This is the only hash usable by PBKDF2, and we don't
> +               * have Argon2 support yet, so set it by default,
> +               * otherwise grub-probe would miss the required
> +               * abstraction
> +               */
> +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
> +              if (cryptodisk->hash == NULL)
> +                  grub_util_error (_("can't lookup hash sha256 by name"));
> +
> +              dm_task_destroy (dmt);
> +            }
>  	}
> +      dm_tree_free (tree);
>        grub_free (grdev);
>      }
>    else
> 

Re: [PATCH v6 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Glenn Washburn]

On Fri,  8 Jul 2022 12:06:06 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Hello Daniel,
> 
> Thanks for the review.  The following updated patches should contain all the changes you asked for.

In case this got forgotten, I've reviewed this patch series. The only
nit that Daniel can probably fix before committing is to upper case the
first word in the added comments.

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

Glenn

> 
> >Please add your Signed-off-by here. Same applies to first patch too.
> Done.
> 
> > Please format comments, here and below, properly [1].
> Sorry about that, I missed the empty first line. Fixed.
> 
> > This function may fail. Please check the returned value and fail if needed.
> > [...]
> > Ditto.
> Fixed (I've used grub_util_error for all the unrecoverable errors, I
> hope that's ok).
> 
> >> +              name = dm_tree_node_get_name (node);
> > I think same applies here...
> Right, it can be "", so added a check and comment about the error mode.
> 
> > s/0/NULL/ Please use NULL instead of 0 for pointers. Same below...
> Done.
> 
> > grub_strndup() may fail. Please check if cipher != NULL.
> Right, sorry.  Fixed.
> 
> > seek_head = grub_memchr (c, ' ', remaining);
> > if (seek_head == NULL)
> Done in both places.
> 
> > Again, grub_strndup() may fail. In general please do not ignore errors
> > from functions which may fail.
> Done as above.
> 
> Josselin Poiret (2):
>   devmapper/getroot: Have devmapper recognize LUKS2
>   devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
>     parameters
> 
>  grub-core/osdep/devmapper/getroot.c | 118 ++++++++++++++++++++++++++--
>  1 file changed, 113 insertions(+), 5 deletions(-)
> 
> Range-diff against v5:
> 1:  7af629fca = 1:  f4cbda414 devmapper/getroot: Have devmapper recognize LUKS2
> 2:  5f9f26118 ! 2:  25ce8bbcc devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM parameters
>     @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
>       			     lastsubdev, grub_errmsg);
>      +          if (strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == 0)
>      +            {
>     -+              /* set LUKS2 cipher from dm parameters, since it is not
>     ++              /*
>     ++               * set LUKS2 cipher from dm parameters, since it is not
>      +               * possible to determine the correct one without
>      +               * unlocking, as there might be multiple segments.
>      +               */
>     @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
>      +              unsigned int remaining;
>      +
>      +              source = grub_disk_open (grdev);
>     ++              if (! source)
>     ++                grub_util_error (_("cannot open grub disk `%s'"), grdev);
>      +              cryptodisk = grub_cryptodisk_get_by_source_disk (source);
>     ++              if (! cryptodisk)
>     ++                grub_util_error (_("cannot get cryptodisk from source disk `%s'"), grdev);
>      +              grub_disk_close (source);
>      +
>     ++              /*
>     ++               * the following function always returns a non-NULL pointer,
>     ++               * but the string may be empty if the relevant info is not present
>     ++               */
>      +              name = dm_tree_node_get_name (node);
>     ++              if (grub_strlen (name) == 0)
>     ++                grub_util_error (_("cannot get dm node name for grub dev `%s'"), grdev);
>      +
>      +              grub_util_info ("populating parameters of cryptomount `%s' from DM device `%s'",
>      +                              uuid, name);
>      +
>      +              dmt = dm_task_create (DM_DEVICE_TABLE);
>     -+              if (dmt == 0)
>     ++              if (dmt == NULL)
>      +                grub_util_error (_("can't create dm task DM_DEVICE_TABLE"));
>      +              if (dm_task_set_name (dmt, name) == 0)
>      +                grub_util_error (_("can't set dm task name to `%s'"), name);
>      +              if (dm_task_run (dmt) == 0)
>      +                grub_util_error (_("can't run dm task for `%s'"), name);
>     -+              /* dm_get_next_target doesn't have any error modes, everything has
>     ++              /*
>     ++               * dm_get_next_target doesn't have any error modes, everything has
>      +               * been handled by dm_task_run.
>      +               */
>      +              dm_get_next_target (dmt, NULL, &start, &length,
>      +                                  &target_type, &params);
>      +              if (strncmp (target_type, "crypt", sizeof ("crypt")) != 0)
>     -+                grub_util_error (_("dm target of type `%s' is not `crypt'"),
>     -+                                 target_type);
>     ++                grub_util_error (_("dm target of type `%s' is not `crypt'"), target_type);
>      +
>     -+              /* dm target parameters for dm-crypt is
>     ++              /*
>     ++               * dm target parameters for dm-crypt is
>      +               * <cipher> <key> <iv_offset> <device path> <offset> [<#opt_params> <opt_param1> ...]
>      +               */
>      +              c = params;
>      +              remaining = grub_strlen (c);
>      +
>      +              /* first, get the cipher name from the cipher */
>     -+              if (!(seek_head = grub_memchr (c, '-', remaining)))
>     ++              seek_head = grub_memchr (c, '-', remaining);
>     ++              if (seek_head == NULL)
>      +                grub_util_error (_("can't get cipher from dm-crypt parameters `%s'"),
>      +                                 params);
>      +              cipher = grub_strndup (c, seek_head - c);
>     ++              if (cipher == NULL)
>     ++                grub_util_error (_("could not strndup cipher of length `%lu'"), seek_head - c);
>      +              remaining -= seek_head - c + 1;
>      +              c = seek_head + 1;
>      +
>      +              /* now, the cipher mode */
>     -+              if (!(seek_head = grub_memchr (c, ' ', remaining)))
>     ++              seek_head = grub_memchr (c, ' ', remaining);
>     ++              if (seek_head == NULL)
>      +                grub_util_error (_("can't get cipher mode from dm-crypt parameters `%s'"),
>      +                                 params);
>      +              cipher_mode = grub_strndup (c, seek_head - c);
>     ++              if (cipher_mode == NULL)
>     ++                grub_util_error (_("could not strndup cipher_mode of length `%lu'"), seek_head - c);
>     ++
>      +              remaining -= seek_head - c + 1;
>      +              c = seek_head + 1;
>      +
>     @@ grub-core/osdep/devmapper/getroot.c: grub_util_pull_devmapper (const char *os_de
>      +              grub_free (cipher);
>      +              grub_free (cipher_mode);
>      +
>     -+              /* This is the only hash usable by PBKDF2, and we don't
>     ++              /*
>     ++               * This is the only hash usable by PBKDF2, and we don't
>      +               * have Argon2 support yet, so set it by default,
>      +               * otherwise grub-probe would miss the required
>      +               * abstraction
>      +               */
>      +              cryptodisk->hash = grub_crypto_lookup_md_by_name ("sha256");
>     -+              if (cryptodisk->hash == 0)
>     ++              if (cryptodisk->hash == NULL)
>      +                  grub_util_error (_("can't lookup hash sha256 by name"));
>      +
>      +              dm_task_destroy (dmt);
> 
> base-commit: 351c9c2fd0bcd94c7fda5b697d3283f7f0ff7930

Re: [PATCH v2 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Glenn Washburn]

Hi Josselin,

Have this on my list of things to circle back to but it got pushed to
the bottom. So sorry about taking so long. Thanks for the submitting
this. This approach seems the most complete of the other patch series
attempting to fix this issue and I'd like to get it merged in in some
form.

On Sat, 11 Dec 2021 13:29:43 +0100
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Glenn Washburn <development@efficientek.com> writes:
> > Its not clear to me, did you test a LUKS2 device with sector size 4096
> > with this change? I believe DM does use 512-byte sectors internally,
> > but it can create block devices that report and use other sector sizes.
> > You can verfiy this by creating a 4096 sector size LUKS2 devices, open
> > it with cryptsetup, and then run "blockdev --getbsz /dev/mapper/<dm name>".
> 
> You're right, blockdev does indeed report 4096.  Here is an updated
> patch that parses the optional sector_size argument from the DM
> parameters, I have checked that it does indeed set the right
> log_sector_size.  I think it worked without it because you can
> technically just read with a lower sector size, but better be safe
> than sorry!

Actually you can't read an encrypted LUKS devices with a lower sector
size. Well, you can partially, but its like every nth sector. However,
I don't think this code every needs to do the decryption itself anyway,
which is the real reason a different sector size still works.

Glenn

> Josselin Poiret (2):
>   devmapper/getroot: Have devmapper recognize LUKS2
>   devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
>     parameters
> 
>  grub-core/osdep/devmapper/getroot.c | 107 ++++++++++++++++++++++++++--
>  1 file changed, 102 insertions(+), 5 deletions(-)
> 

Re: [PATCH v2 0/2] Have LUKS2 cryptomounts be useable with grub-probe

[Fabian Vogt]

Hi,

Am Freitag, 13. Mai 2022, 00:20:38 CEST schrieb Glenn Washburn:
> Hi Josselin,
> 
> Have this on my list of things to circle back to but it got pushed to
> the bottom. So sorry about taking so long. Thanks for the submitting
> this. This approach seems the most complete of the other patch series
> attempting to fix this issue and I'd like to get it merged in in some
> form.

As author of one of the other patch series implementing this, I also +1 any
approach which gets a real sector size through DM or the blockdev directly.
Hacking around an unknown sector size by treating it as 512 should be avoided,
especially as we have better approaches pending.

Thanks,
Fabian

> On Sat, 11 Dec 2021 13:29:43 +0100
> Josselin Poiret <dev@jpoiret.xyz> wrote:
> 
> > Glenn Washburn <development@efficientek.com> writes:
> > > Its not clear to me, did you test a LUKS2 device with sector size 4096
> > > with this change? I believe DM does use 512-byte sectors internally,
> > > but it can create block devices that report and use other sector sizes.
> > > You can verfiy this by creating a 4096 sector size LUKS2 devices, open
> > > it with cryptsetup, and then run "blockdev --getbsz /dev/mapper/<dm name>".
> > 
> > You're right, blockdev does indeed report 4096.  Here is an updated
> > patch that parses the optional sector_size argument from the DM
> > parameters, I have checked that it does indeed set the right
> > log_sector_size.  I think it worked without it because you can
> > technically just read with a lower sector size, but better be safe
> > than sorry!
> 
> Actually you can't read an encrypted LUKS devices with a lower sector
> size. Well, you can partially, but its like every nth sector. However,
> I don't think this code every needs to do the decryption itself anyway,
> which is the real reason a different sector size still works.
>
> Glenn
> 
> > Josselin Poiret (2):
> >   devmapper/getroot: Have devmapper recognize LUKS2
> >   devmapper/getroot: Set up cheated LUKS2 cryptodisk mount from DM
> >     parameters
> > 
> >  grub-core/osdep/devmapper/getroot.c | 107 ++++++++++++++++++++++++++--
> >  1 file changed, 102 insertions(+), 5 deletions(-)
> > 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel
>