[PATCH v6 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[PATCH v6 0/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

Updates since v5:
 * Add better documentation of n parameter to grub_strncasecmp() at Patrick's
   suggestion.

Glenn

Glenn Washburn (2):
  misc: Add cast in grub_strncasecmp() to drop sign when calling
    grub_tolower()
  cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/geli.c       |  2 +-
 grub-core/disk/luks.c       | 21 ++++-----------------
 grub-core/disk/luks2.c      | 15 ++++-----------
 include/grub/misc.h         | 35 ++++++++++++++++++++++++++++++++++-
 5 files changed, 45 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v6 1/2] misc: Add cast in grub_strncasecmp() to drop sign when calling grub_tolower()

[Glenn Washburn]

Note this cast was fixed in grub_strcasecmp() in commit ce41ab7aab
(* grub-core/kern/misc.c (grub_strcmp): Use unsigned comparison as per
common usage and preffered in several parts of code.), but this commit
omitted fixing it in grub_strncasecmp().

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 include/grub/misc.h | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/include/grub/misc.h b/include/grub/misc.h
index 776dbf8af..1c25c1767 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -232,7 +232,8 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
 
   while (*s1 && *s2 && --n)
     {
-      if (grub_tolower (*s1) != grub_tolower (*s2))
+      if (grub_tolower ((grub_uint8_t) *s1)
+	  != grub_tolower ((grub_uint8_t) *s2))
 	break;
 
       s1++;
-- 
2.34.1

[PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

A user can now specify UUID strings with dashes, instead of having to remove
dashes. This is backwards-compatability preserving and also fixes a source
of user confusion over the inconsistency with how UUIDs are specified
between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
reference implementation for LUKS, displays and generates UUIDs with dashes
there has been additional confusion when using the UUID strings from
cryptsetup as exact input into GRUB does not find the expected cryptodisk.

A new function grub_uuidcasecmp is added that is general enough to be used
other places where UUIDs are being compared.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/geli.c       |  2 +-
 grub-core/disk/luks.c       | 21 ++++-----------------
 grub-core/disk/luks2.c      | 15 ++++-----------
 include/grub/misc.h         | 32 ++++++++++++++++++++++++++++++++
 5 files changed, 43 insertions(+), 31 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index e89430812..a4cd8445f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
   if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
     {
       for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
+	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, sizeof (dev->uuid)) == 0)
 	  break;
     }
   else
@@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
 {
   grub_cryptodisk_t dev;
   for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
-    if (grub_strcasecmp (dev->uuid, uuid) == 0)
+    if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0)
       return dev;
   return NULL;
 }
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index e190066f9..722910d64 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
     {
       grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
       return NULL;
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 7f837d52c..9e1e6a5d9 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -66,10 +66,7 @@ static grub_cryptodisk_t
 luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
-  const char *iptr;
   struct grub_luks_phdr header;
-  char *optr;
-  char uuid[sizeof (header.uuid) + 1];
   char ciphername[sizeof (header.cipherName) + 1];
   char ciphermode[sizeof (header.cipherMode) + 1];
   char hashspec[sizeof (header.hashSpec) + 1];
@@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
       || grub_be_to_cpu16 (header.version) != 1)
     return NULL;
 
-  grub_memset (uuid, 0, sizeof (uuid));
-  optr = uuid;
-  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
-       iptr++)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
     {
-      if (*iptr != '-')
-	*optr++ = *iptr;
-    }
-  *optr = 0;
-
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
-    {
-      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
+      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
       return NULL;
     }
 
@@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   newdev->source_disk = NULL;
   newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
   newdev->total_sectors = grub_disk_native_sectors (disk) - newdev->offset_sectors;
-  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
+  grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid));
   newdev->modname = "luks";
 
   /* Configure the hash used for the AF splitter and HMAC.  */
@@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
-  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
+  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
   return newdev;
 }
 
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 5b3b36c8a..1174c4c2b 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t cryptodisk;
   grub_luks2_header_t header;
-  char uuid[sizeof (header.uuid) + 1];
-  grub_size_t i, j;
 
   if (cargs->check_boot)
     return NULL;
@@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
       return NULL;
     }
 
-  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
-    if (header.uuid[i] != '-')
-      uuid[j++] = header.uuid[i];
-  uuid[j] = '\0';
-
-  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
+  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
     {
-      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
+      grub_dprintf ("luks2", "%s != %s\n", header.uuid, cargs->search_uuid);
       return NULL;
     }
 
@@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
   if (!cryptodisk)
     return NULL;
 
-  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
-  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
+  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
+  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));
 
   cryptodisk->modname = "luks2";
   return cryptodisk;
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 1c25c1767..76c5c7992 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
     - (int) grub_tolower ((grub_uint8_t) *s2);
 }
 
+/*
+ * Do a case insensitive compare of two UUID strings by ignoring all dashes.
+ * Note that the parameter n, is the number of significant characters to
+ * compare, where significant characters are any except the dash.
+ */
+static inline int
+grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
+{
+  if (n == 0)
+    return 0;
+
+  while (*uuid1 && *uuid2 && --n)
+    {
+      /* Skip forward to non-dash on both UUIDs. */
+      while ('-' == *uuid1)
+        ++uuid1;
+
+      while ('-' == *uuid2)
+        ++uuid2;
+
+      if (grub_tolower ((grub_uint8_t) *uuid1)
+	  != grub_tolower ((grub_uint8_t) *uuid2))
+	break;
+
+      uuid1++;
+      uuid2++;
+    }
+
+  return (int) grub_tolower ((grub_uint8_t) *uuid1)
+    - (int) grub_tolower ((grub_uint8_t) *uuid2);
+}
+
 /*
  * Note that these differ from the C standard's definitions of strtol,
  * strtoul(), and strtoull() by the addition of two const qualifiers on the end
-- 
2.34.1

Re: [PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Patrick Steinhardt]

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

On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote:
> A user can now specify UUID strings with dashes, instead of having to remove
> dashes. This is backwards-compatability preserving and also fixes a source
> of user confusion over the inconsistency with how UUIDs are specified
> between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
> reference implementation for LUKS, displays and generates UUIDs with dashes
> there has been additional confusion when using the UUID strings from
> cryptsetup as exact input into GRUB does not find the expected cryptodisk.
> 
> A new function grub_uuidcasecmp is added that is general enough to be used
> other places where UUIDs are being compared.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c |  4 ++--
>  grub-core/disk/geli.c       |  2 +-
>  grub-core/disk/luks.c       | 21 ++++-----------------
>  grub-core/disk/luks2.c      | 15 ++++-----------
>  include/grub/misc.h         | 32 ++++++++++++++++++++++++++++++++
>  5 files changed, 43 insertions(+), 31 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index e89430812..a4cd8445f 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
>    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
>      {
>        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> +	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, sizeof (dev->uuid)) == 0)
>  	  break;
>      }
>    else
> @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
>  {
>    grub_cryptodisk_t dev;
>    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> +    if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0)
>        return dev;
>    return NULL;
>  }
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index e190066f9..722910d64 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>        return NULL;
>      }
>  
> -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
>      {
>        grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
>        return NULL;
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 7f837d52c..9e1e6a5d9 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -66,10 +66,7 @@ static grub_cryptodisk_t
>  luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>  {
>    grub_cryptodisk_t newdev;
> -  const char *iptr;
>    struct grub_luks_phdr header;
> -  char *optr;
> -  char uuid[sizeof (header.uuid) + 1];
>    char ciphername[sizeof (header.cipherName) + 1];
>    char ciphermode[sizeof (header.cipherMode) + 1];
>    char hashspec[sizeof (header.hashSpec) + 1];
> @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>        || grub_be_to_cpu16 (header.version) != 1)
>      return NULL;
>  
> -  grub_memset (uuid, 0, sizeof (uuid));
> -  optr = uuid;
> -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> -       iptr++)
> +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
>      {
> -      if (*iptr != '-')
> -	*optr++ = *iptr;
> -    }
> -  *optr = 0;
> -
> -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> -    {
> -      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
> +      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
>        return NULL;
>      }

I haven't noticed this in my previous review round, but I think this is
wrong because `header.uuid` is never NUL-terminated. It is explicitly 40
characters long and read directly from disk, so there wouldn't be any
room for it to be NUL-terminated.

So we'd rather have to:

    grub_dprintf ("luks2", "%.*s != %s\n", header.uuid, sizeof (header.uuid),
                  cargs->search_uuid);

Sorry for not catching this in my first round.

>  
> @@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>    newdev->source_disk = NULL;
>    newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
>    newdev->total_sectors = grub_disk_native_sectors (disk) - newdev->offset_sectors;
> -  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> +  grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid));
>    newdev->modname = "luks";

This should be fine though because `newdev->uuid` is initialized to
all-zeroes.

>    /* Configure the hash used for the AF splitter and HMAC.  */
> @@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>        return NULL;
>      }
>  
> -  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
>    return newdev;
>  }

This has an off-by-one bug now though: `sizeof(uuid)` is not the same as
`sizeof(header.uuid)`, but instead it was `sizeof(header.uuid)+1` to
account for the trailing NUL-byte. So we have to make this:

    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) > sizeof (header.uuid));

> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index 5b3b36c8a..1174c4c2b 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>  {
>    grub_cryptodisk_t cryptodisk;
>    grub_luks2_header_t header;
> -  char uuid[sizeof (header.uuid) + 1];
> -  grub_size_t i, j;
>  
>    if (cargs->check_boot)
>      return NULL;
> @@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>        return NULL;
>      }
>  
> -  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
> -    if (header.uuid[i] != '-')
> -      uuid[j++] = header.uuid[i];
> -  uuid[j] = '\0';
> -
> -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
>      {
> -      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
> +      grub_dprintf ("luks2", "%s != %s\n", header.uuid, cargs->search_uuid);
>        return NULL;
>      }

Same here.

> @@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>    if (!cryptodisk)
>      return NULL;
>  
> -  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
> -  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
> +  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
> +  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));

And here.

Patrick

>    cryptodisk->modname = "luks2";
>    return cryptodisk;
> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index 1c25c1767..76c5c7992 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
>      - (int) grub_tolower ((grub_uint8_t) *s2);
>  }
>  
> +/*
> + * Do a case insensitive compare of two UUID strings by ignoring all dashes.
> + * Note that the parameter n, is the number of significant characters to
> + * compare, where significant characters are any except the dash.
> + */
> +static inline int
> +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> +{
> +  if (n == 0)
> +    return 0;
> +
> +  while (*uuid1 && *uuid2 && --n)
> +    {
> +      /* Skip forward to non-dash on both UUIDs. */
> +      while ('-' == *uuid1)
> +        ++uuid1;
> +
> +      while ('-' == *uuid2)
> +        ++uuid2;
> +
> +      if (grub_tolower ((grub_uint8_t) *uuid1)
> +	  != grub_tolower ((grub_uint8_t) *uuid2))
> +	break;
> +
> +      uuid1++;
> +      uuid2++;
> +    }
> +
> +  return (int) grub_tolower ((grub_uint8_t) *uuid1)
> +    - (int) grub_tolower ((grub_uint8_t) *uuid2);
> +}
> +
>  /*
>   * Note that these differ from the C standard's definitions of strtol,
>   * strtoul(), and strtoull() by the addition of two const qualifiers on the end
> -- 
> 2.34.1
> 

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

Re: [PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

On Mon, 29 Aug 2022 07:38:24 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote:
> > A user can now specify UUID strings with dashes, instead of having to remove
> > dashes. This is backwards-compatability preserving and also fixes a source
> > of user confusion over the inconsistency with how UUIDs are specified
> > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
> > reference implementation for LUKS, displays and generates UUIDs with dashes
> > there has been additional confusion when using the UUID strings from
> > cryptsetup as exact input into GRUB does not find the expected cryptodisk.
> > 
> > A new function grub_uuidcasecmp is added that is general enough to be used
> > other places where UUIDs are being compared.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c |  4 ++--
> >  grub-core/disk/geli.c       |  2 +-
> >  grub-core/disk/luks.c       | 21 ++++-----------------
> >  grub-core/disk/luks2.c      | 15 ++++-----------
> >  include/grub/misc.h         | 32 ++++++++++++++++++++++++++++++++
> >  5 files changed, 43 insertions(+), 31 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index e89430812..a4cd8445f 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> >    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
> >      {
> >        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > -	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> > +	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, sizeof (dev->uuid)) == 0)
> >  	  break;
> >      }
> >    else
> > @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
> >  {
> >    grub_cryptodisk_t dev;
> >    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> > +    if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0)
> >        return dev;
> >    return NULL;
> >  }
> > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > index e190066f9..722910d64 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >        return NULL;
> >      }
> >  
> > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
> >      {
> >        grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
> >        return NULL;
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 7f837d52c..9e1e6a5d9 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -66,10 +66,7 @@ static grub_cryptodisk_t
> >  luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >  {
> >    grub_cryptodisk_t newdev;
> > -  const char *iptr;
> >    struct grub_luks_phdr header;
> > -  char *optr;
> > -  char uuid[sizeof (header.uuid) + 1];
> >    char ciphername[sizeof (header.cipherName) + 1];
> >    char ciphermode[sizeof (header.cipherMode) + 1];
> >    char hashspec[sizeof (header.hashSpec) + 1];
> > @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >        || grub_be_to_cpu16 (header.version) != 1)
> >      return NULL;
> >  
> > -  grub_memset (uuid, 0, sizeof (uuid));
> > -  optr = uuid;
> > -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> > -       iptr++)
> > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
> >      {
> > -      if (*iptr != '-')
> > -	*optr++ = *iptr;
> > -    }
> > -  *optr = 0;
> > -
> > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > -    {
> > -      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
> > +      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
> >        return NULL;
> >      }
> 
> I haven't noticed this in my previous review round, but I think this is
> wrong because `header.uuid` is never NUL-terminated. It is explicitly 40
> characters long and read directly from disk, so there wouldn't be any
> room for it to be NUL-terminated.

Why not? UUIDs are 32 hex characters with typically 4 dashes, that
makes 37 bytes needed including the NULL byte. In fact, I believe that
cryptsetup always creates the uuid field NULL terminated. Also, the
LUKS1 spec, and by extension LUKS2 spec, mandates NULL termination. The
uuid field is specified as "char[]" and "char[], a string stored as null
terminated sequence of 8-bit characters7".

> 
> So we'd rather have to:
> 
>     grub_dprintf ("luks2", "%.*s != %s\n", header.uuid, sizeof (header.uuid),
>                   cargs->search_uuid);
> 
> Sorry for not catching this in my first round.

This seems reasonable for what should be a very uncommon and non-spec
compliant case of having a uuid field with no NULL byte. I originally
had a patch before this that explicitly put a NULL byte at the end of
header.uuid so this problem didn't exist. I dropped it because it was
generally redundant as pointed out by Daniel, but I didn't take this
case into account. Perhaps its worth merging that with this one for
clarity.

> 
> >  
> > @@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >    newdev->source_disk = NULL;
> >    newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
> >    newdev->total_sectors = grub_disk_native_sectors (disk) - newdev->offset_sectors;
> > -  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> > +  grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid));
> >    newdev->modname = "luks";
> 
> This should be fine though because `newdev->uuid` is initialized to
> all-zeroes.
> 
> >    /* Configure the hash used for the AF splitter and HMAC.  */
> > @@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >        return NULL;
> >      }
> >  
> > -  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> > +  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
> >    return newdev;
> >  }
> 
> This has an off-by-one bug now though: `sizeof(uuid)` is not the same as
> `sizeof(header.uuid)`, but instead it was `sizeof(header.uuid)+1` to
> account for the trailing NUL-byte. So we have to make this:
> 
>     COMPILE_TIME_ASSERT (sizeof (newdev->uuid) > sizeof (header.uuid));

I don't think this is needed per above.

Please let me know if I've missed something and thanks for being
cautious. 

Glenn

> 
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index 5b3b36c8a..1174c4c2b 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >  {
> >    grub_cryptodisk_t cryptodisk;
> >    grub_luks2_header_t header;
> > -  char uuid[sizeof (header.uuid) + 1];
> > -  grub_size_t i, j;
> >  
> >    if (cargs->check_boot)
> >      return NULL;
> > @@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >        return NULL;
> >      }
> >  
> > -  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
> > -    if (header.uuid[i] != '-')
> > -      uuid[j++] = header.uuid[i];
> > -  uuid[j] = '\0';
> > -
> > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
> >      {
> > -      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
> > +      grub_dprintf ("luks2", "%s != %s\n", header.uuid, cargs->search_uuid);
> >        return NULL;
> >      }
> 
> Same here.
> 
> > @@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >    if (!cryptodisk)
> >      return NULL;
> >  
> > -  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
> > -  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
> > +  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
> > +  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));
> 
> And here.
> 
> Patrick
> 
> >    cryptodisk->modname = "luks2";
> >    return cryptodisk;
> > diff --git a/include/grub/misc.h b/include/grub/misc.h
> > index 1c25c1767..76c5c7992 100644
> > --- a/include/grub/misc.h
> > +++ b/include/grub/misc.h
> > @@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
> >      - (int) grub_tolower ((grub_uint8_t) *s2);
> >  }
> >  
> > +/*
> > + * Do a case insensitive compare of two UUID strings by ignoring all dashes.
> > + * Note that the parameter n, is the number of significant characters to
> > + * compare, where significant characters are any except the dash.
> > + */
> > +static inline int
> > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> > +{
> > +  if (n == 0)
> > +    return 0;
> > +
> > +  while (*uuid1 && *uuid2 && --n)
> > +    {
> > +      /* Skip forward to non-dash on both UUIDs. */
> > +      while ('-' == *uuid1)
> > +        ++uuid1;
> > +
> > +      while ('-' == *uuid2)
> > +        ++uuid2;
> > +
> > +      if (grub_tolower ((grub_uint8_t) *uuid1)
> > +	  != grub_tolower ((grub_uint8_t) *uuid2))
> > +	break;
> > +
> > +      uuid1++;
> > +      uuid2++;
> > +    }
> > +
> > +  return (int) grub_tolower ((grub_uint8_t) *uuid1)
> > +    - (int) grub_tolower ((grub_uint8_t) *uuid2);
> > +}
> > +
> >  /*
> >   * Note that these differ from the C standard's definitions of strtol,
> >   * strtoul(), and strtoull() by the addition of two const qualifiers on the end
> > -- 
> > 2.34.1
> > 

Re: [PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Patrick Steinhardt]

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

On Tue, Aug 30, 2022 at 03:12:36PM -0500, Glenn Washburn wrote:
> On Mon, 29 Aug 2022 07:38:24 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote:
> > > A user can now specify UUID strings with dashes, instead of having to remove
> > > dashes. This is backwards-compatability preserving and also fixes a source
> > > of user confusion over the inconsistency with how UUIDs are specified
> > > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
> > > reference implementation for LUKS, displays and generates UUIDs with dashes
> > > there has been additional confusion when using the UUID strings from
> > > cryptsetup as exact input into GRUB does not find the expected cryptodisk.
> > > 
> > > A new function grub_uuidcasecmp is added that is general enough to be used
> > > other places where UUIDs are being compared.
> > > 
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c |  4 ++--
> > >  grub-core/disk/geli.c       |  2 +-
> > >  grub-core/disk/luks.c       | 21 ++++-----------------
> > >  grub-core/disk/luks2.c      | 15 ++++-----------
> > >  include/grub/misc.h         | 32 ++++++++++++++++++++++++++++++++
> > >  5 files changed, 43 insertions(+), 31 deletions(-)
> > > 
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index e89430812..a4cd8445f 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> > >    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
> > >      {
> > >        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > > -	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> > > +	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, sizeof (dev->uuid)) == 0)
> > >  	  break;
> > >      }
> > >    else
> > > @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
> > >  {
> > >    grub_cryptodisk_t dev;
> > >    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > > -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> > > +    if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0)
> > >        return dev;
> > >    return NULL;
> > >  }
> > > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > > index e190066f9..722910d64 100644
> > > --- a/grub-core/disk/geli.c
> > > +++ b/grub-core/disk/geli.c
> > > @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >        return NULL;
> > >      }
> > >  
> > > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
> > >      {
> > >        grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
> > >        return NULL;
> > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > > index 7f837d52c..9e1e6a5d9 100644
> > > --- a/grub-core/disk/luks.c
> > > +++ b/grub-core/disk/luks.c
> > > @@ -66,10 +66,7 @@ static grub_cryptodisk_t
> > >  luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >  {
> > >    grub_cryptodisk_t newdev;
> > > -  const char *iptr;
> > >    struct grub_luks_phdr header;
> > > -  char *optr;
> > > -  char uuid[sizeof (header.uuid) + 1];
> > >    char ciphername[sizeof (header.cipherName) + 1];
> > >    char ciphermode[sizeof (header.cipherMode) + 1];
> > >    char hashspec[sizeof (header.hashSpec) + 1];
> > > @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >        || grub_be_to_cpu16 (header.version) != 1)
> > >      return NULL;
> > >  
> > > -  grub_memset (uuid, 0, sizeof (uuid));
> > > -  optr = uuid;
> > > -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> > > -       iptr++)
> > > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
> > >      {
> > > -      if (*iptr != '-')
> > > -	*optr++ = *iptr;
> > > -    }
> > > -  *optr = 0;
> > > -
> > > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > > -    {
> > > -      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
> > > +      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
> > >        return NULL;
> > >      }
> > 
> > I haven't noticed this in my previous review round, but I think this is
> > wrong because `header.uuid` is never NUL-terminated. It is explicitly 40
> > characters long and read directly from disk, so there wouldn't be any
> > room for it to be NUL-terminated.
> 
> Why not? UUIDs are 32 hex characters with typically 4 dashes, that
> makes 37 bytes needed including the NULL byte. In fact, I believe that
> cryptsetup always creates the uuid field NULL terminated. Also, the
> LUKS1 spec, and by extension LUKS2 spec, mandates NULL termination. The
> uuid field is specified as "char[]" and "char[], a string stored as null
> terminated sequence of 8-bit characters7".

Indeed, you're right. I've read through the spec again and it matches
your explanation. That also makes my remaining points moot.

> > 
> > So we'd rather have to:
> > 
> >     grub_dprintf ("luks2", "%.*s != %s\n", header.uuid, sizeof (header.uuid),
> >                   cargs->search_uuid);
> > 
> > Sorry for not catching this in my first round.
> 
> This seems reasonable for what should be a very uncommon and non-spec
> compliant case of having a uuid field with no NULL byte. I originally
> had a patch before this that explicitly put a NULL byte at the end of
> header.uuid so this problem didn't exist. I dropped it because it was
> generally redundant as pointed out by Daniel, but I didn't take this
> case into account. Perhaps its worth merging that with this one for
> clarity.

This is not as important anymore given that my initial point was moot.
But it may still be sensible as defennse against corrupted headers.

Anyway, with or without that change:

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

Thanks!

Patrick

> > 
> > >  
> > > @@ -123,7 +110,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >    newdev->source_disk = NULL;
> > >    newdev->log_sector_size = GRUB_LUKS1_LOG_SECTOR_SIZE;
> > >    newdev->total_sectors = grub_disk_native_sectors (disk) - newdev->offset_sectors;
> > > -  grub_memcpy (newdev->uuid, uuid, sizeof (uuid));
> > > +  grub_memcpy (newdev->uuid, header.uuid, sizeof (header.uuid));
> > >    newdev->modname = "luks";
> > 
> > This should be fine though because `newdev->uuid` is initialized to
> > all-zeroes.
> > 
> > >    /* Configure the hash used for the AF splitter and HMAC.  */
> > > @@ -143,7 +130,7 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >        return NULL;
> > >      }
> > >  
> > > -  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> > > +  COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (header.uuid));
> > >    return newdev;
> > >  }
> > 
> > This has an off-by-one bug now though: `sizeof(uuid)` is not the same as
> > `sizeof(header.uuid)`, but instead it was `sizeof(header.uuid)+1` to
> > account for the trailing NUL-byte. So we have to make this:
> > 
> >     COMPILE_TIME_ASSERT (sizeof (newdev->uuid) > sizeof (header.uuid));
> 
> I don't think this is needed per above.
> 
> Please let me know if I've missed something and thanks for being
> cautious. 
> 
> Glenn
> 
> > 
> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index 5b3b36c8a..1174c4c2b 100644
> > > --- a/grub-core/disk/luks2.c
> > > +++ b/grub-core/disk/luks2.c
> > > @@ -350,8 +350,6 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >  {
> > >    grub_cryptodisk_t cryptodisk;
> > >    grub_luks2_header_t header;
> > > -  char uuid[sizeof (header.uuid) + 1];
> > > -  grub_size_t i, j;
> > >  
> > >    if (cargs->check_boot)
> > >      return NULL;
> > > @@ -362,14 +360,9 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >        return NULL;
> > >      }
> > >  
> > > -  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
> > > -    if (header.uuid[i] != '-')
> > > -      uuid[j++] = header.uuid[i];
> > > -  uuid[j] = '\0';
> > > -
> > > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
> > >      {
> > > -      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
> > > +      grub_dprintf ("luks2", "%s != %s\n", header.uuid, cargs->search_uuid);
> > >        return NULL;
> > >      }
> > 
> > Same here.
> > 
> > > @@ -377,8 +370,8 @@ luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > >    if (!cryptodisk)
> > >      return NULL;
> > >  
> > > -  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (uuid));
> > > -  grub_memcpy (cryptodisk->uuid, uuid, sizeof (uuid));
> > > +  COMPILE_TIME_ASSERT (sizeof (cryptodisk->uuid) >= sizeof (header.uuid));
> > > +  grub_memcpy (cryptodisk->uuid, header.uuid, sizeof (header.uuid));
> > 
> > And here.
> > 
> > Patrick
> > 
> > >    cryptodisk->modname = "luks2";
> > >    return cryptodisk;
> > > diff --git a/include/grub/misc.h b/include/grub/misc.h
> > > index 1c25c1767..76c5c7992 100644
> > > --- a/include/grub/misc.h
> > > +++ b/include/grub/misc.h
> > > @@ -244,6 +244,38 @@ grub_strncasecmp (const char *s1, const char *s2, grub_size_t n)
> > >      - (int) grub_tolower ((grub_uint8_t) *s2);
> > >  }
> > >  
> > > +/*
> > > + * Do a case insensitive compare of two UUID strings by ignoring all dashes.
> > > + * Note that the parameter n, is the number of significant characters to
> > > + * compare, where significant characters are any except the dash.
> > > + */
> > > +static inline int
> > > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> > > +{
> > > +  if (n == 0)
> > > +    return 0;
> > > +
> > > +  while (*uuid1 && *uuid2 && --n)
> > > +    {
> > > +      /* Skip forward to non-dash on both UUIDs. */
> > > +      while ('-' == *uuid1)
> > > +        ++uuid1;
> > > +
> > > +      while ('-' == *uuid2)
> > > +        ++uuid2;
> > > +
> > > +      if (grub_tolower ((grub_uint8_t) *uuid1)
> > > +	  != grub_tolower ((grub_uint8_t) *uuid2))
> > > +	break;
> > > +
> > > +      uuid1++;
> > > +      uuid2++;
> > > +    }
> > > +
> > > +  return (int) grub_tolower ((grub_uint8_t) *uuid1)
> > > +    - (int) grub_tolower ((grub_uint8_t) *uuid2);
> > > +}
> > > +
> > >  /*
> > >   * Note that these differ from the C standard's definitions of strtol,
> > >   * strtoul(), and strtoull() by the addition of two const qualifiers on the end
> > > -- 
> > > 2.34.1
> > > 

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

Re: [PATCH v6 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Daniel Kiper]

On Tue, Sep 06, 2022 at 05:28:40PM +0200, Patrick Steinhardt wrote:
> On Tue, Aug 30, 2022 at 03:12:36PM -0500, Glenn Washburn wrote:
> > On Mon, 29 Aug 2022 07:38:24 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> >
> > > On Fri, Aug 19, 2022 at 06:06:15PM -0500, Glenn Washburn wrote:
> > > > A user can now specify UUID strings with dashes, instead of having to remove
> > > > dashes. This is backwards-compatability preserving and also fixes a source
> > > > of user confusion over the inconsistency with how UUIDs are specified
> > > > between file system UUIDs and cryptomount UUIDs. Since cryptsetup, the
> > > > reference implementation for LUKS, displays and generates UUIDs with dashes
> > > > there has been additional confusion when using the UUID strings from
> > > > cryptsetup as exact input into GRUB does not find the expected cryptodisk.
> > > >
> > > > A new function grub_uuidcasecmp is added that is general enough to be used
> > > > other places where UUIDs are being compared.
> > > >
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  grub-core/disk/cryptodisk.c |  4 ++--
> > > >  grub-core/disk/geli.c       |  2 +-
> > > >  grub-core/disk/luks.c       | 21 ++++-----------------
> > > >  grub-core/disk/luks2.c      | 15 ++++-----------
> > > >  include/grub/misc.h         | 32 ++++++++++++++++++++++++++++++++
> > > >  5 files changed, 43 insertions(+), 31 deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > > index e89430812..a4cd8445f 100644
> > > > --- a/grub-core/disk/cryptodisk.c
> > > > +++ b/grub-core/disk/cryptodisk.c
> > > > @@ -702,7 +702,7 @@ grub_cryptodisk_open (const char *name, grub_disk_t disk)
> > > >    if (grub_memcmp (name, "cryptouuid/", sizeof ("cryptouuid/") - 1) == 0)
> > > >      {
> > > >        for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > > > -	if (grub_strcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid) == 0)
> > > > +	if (grub_uuidcasecmp (name + sizeof ("cryptouuid/") - 1, dev->uuid, sizeof (dev->uuid)) == 0)
> > > >  	  break;
> > > >      }
> > > >    else
> > > > @@ -929,7 +929,7 @@ grub_cryptodisk_get_by_uuid (const char *uuid)
> > > >  {
> > > >    grub_cryptodisk_t dev;
> > > >    for (dev = cryptodisk_list; dev != NULL; dev = dev->next)
> > > > -    if (grub_strcasecmp (dev->uuid, uuid) == 0)
> > > > +    if (grub_uuidcasecmp (dev->uuid, uuid, sizeof (dev->uuid)) == 0)
> > > >        return dev;
> > > >    return NULL;
> > > >  }
> > > > diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > > > index e190066f9..722910d64 100644
> > > > --- a/grub-core/disk/geli.c
> > > > +++ b/grub-core/disk/geli.c
> > > > @@ -305,7 +305,7 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > > >        return NULL;
> > > >      }
> > > >
> > > > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > > > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, uuid, sizeof (uuid)) != 0)
> > > >      {
> > > >        grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
> > > >        return NULL;
> > > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > > > index 7f837d52c..9e1e6a5d9 100644
> > > > --- a/grub-core/disk/luks.c
> > > > +++ b/grub-core/disk/luks.c
> > > > @@ -66,10 +66,7 @@ static grub_cryptodisk_t
> > > >  luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > > >  {
> > > >    grub_cryptodisk_t newdev;
> > > > -  const char *iptr;
> > > >    struct grub_luks_phdr header;
> > > > -  char *optr;
> > > > -  char uuid[sizeof (header.uuid) + 1];
> > > >    char ciphername[sizeof (header.cipherName) + 1];
> > > >    char ciphermode[sizeof (header.cipherMode) + 1];
> > > >    char hashspec[sizeof (header.hashSpec) + 1];
> > > > @@ -92,19 +89,9 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> > > >        || grub_be_to_cpu16 (header.version) != 1)
> > > >      return NULL;
> > > >
> > > > -  grub_memset (uuid, 0, sizeof (uuid));
> > > > -  optr = uuid;
> > > > -  for (iptr = header.uuid; iptr < &header.uuid[ARRAY_SIZE (header.uuid)];
> > > > -       iptr++)
> > > > +  if (cargs->search_uuid != NULL && grub_uuidcasecmp (cargs->search_uuid, header.uuid, sizeof (header.uuid)) != 0)
> > > >      {
> > > > -      if (*iptr != '-')
> > > > -	*optr++ = *iptr;
> > > > -    }
> > > > -  *optr = 0;
> > > > -
> > > > -  if (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> > > > -    {
> > > > -      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
> > > > +      grub_dprintf ("luks", "%s != %s\n", header.uuid, cargs->search_uuid);
> > > >        return NULL;
> > > >      }
> > >
> > > I haven't noticed this in my previous review round, but I think this is
> > > wrong because `header.uuid` is never NUL-terminated. It is explicitly 40
> > > characters long and read directly from disk, so there wouldn't be any
> > > room for it to be NUL-terminated.
> >
> > Why not? UUIDs are 32 hex characters with typically 4 dashes, that
> > makes 37 bytes needed including the NULL byte. In fact, I believe that
> > cryptsetup always creates the uuid field NULL terminated. Also, the
> > LUKS1 spec, and by extension LUKS2 spec, mandates NULL termination. The
> > uuid field is specified as "char[]" and "char[], a string stored as null
> > terminated sequence of 8-bit characters7".
>
> Indeed, you're right. I've read through the spec again and it matches
> your explanation. That also makes my remaining points moot.
>
> > >
> > > So we'd rather have to:
> > >
> > >     grub_dprintf ("luks2", "%.*s != %s\n", header.uuid, sizeof (header.uuid),
> > >                   cargs->search_uuid);
> > >
> > > Sorry for not catching this in my first round.
> >
> > This seems reasonable for what should be a very uncommon and non-spec
> > compliant case of having a uuid field with no NULL byte. I originally
> > had a patch before this that explicitly put a NULL byte at the end of
> > header.uuid so this problem didn't exist. I dropped it because it was
> > generally redundant as pointed out by Daniel, but I didn't take this
> > case into account. Perhaps its worth merging that with this one for
> > clarity.
>
> This is not as important anymore given that my initial point was moot.
> But it may still be sensible as defennse against corrupted headers.
>
> Anyway, with or without that change:
>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>

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

Daniel