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

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

[Glenn Washburn]

Updates since v4:
 * Add patch to add missing casts to grub_strncasecmp()
 * Remove patch forcing null termination of uuid from header
 * Use correct patch for uuid comparison. The previous had bad
   variable name usage, but the logic is the same.

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         | 31 ++++++++++++++++++++++++++++++-
 5 files changed, 41 insertions(+), 32 deletions(-)

-- 
2.34.1

[PATCH v5 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>
---
 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 v5 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         | 28 ++++++++++++++++++++++++++++
 5 files changed, 39 insertions(+), 31 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f1fe0d390..00d66ab02 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -701,7 +701,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
@@ -928,7 +928,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 bf741d70f..ac3ab095c 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..2ed378188 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -244,6 +244,34 @@ 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 */
+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 v5 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Patrick Steinhardt]

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

On Thu, Aug 11, 2022 at 12:48:43PM -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         | 28 ++++++++++++++++++++++++++++
>  5 files changed, 39 insertions(+), 31 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index f1fe0d390..00d66ab02 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -701,7 +701,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
> @@ -928,7 +928,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 bf741d70f..ac3ab095c 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..2ed378188 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -244,6 +244,34 @@ 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 */
> +static inline int
> +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)

Nit: I find it a bit unclear with how `n` is supposed to be used. You
might expect it to denote the maximum length of either of the UUIDs, but
it's not really given that you skip over dashes without decrementing it.
So this rather looks like the "length of non-dash characters in either
of the UUIDs", which is likely not all that useful to the caller.

We should either clearly document the intent of this parameter, or make
it more useful by e.g. allowing the caller to pass both a `uuid1len` and
`uuid2len`, which get properly decremented when skipping over dashes.

Patrick

> +{
> +  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 v5 2/2] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner

[Glenn Washburn]

On Mon, 15 Aug 2022 17:28:24 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Thu, Aug 11, 2022 at 12:48:43PM -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         | 28 ++++++++++++++++++++++++++++
> >  5 files changed, 39 insertions(+), 31 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index f1fe0d390..00d66ab02 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -701,7 +701,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
> > @@ -928,7 +928,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 bf741d70f..ac3ab095c 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..2ed378188 100644
> > --- a/include/grub/misc.h
> > +++ b/include/grub/misc.h
> > @@ -244,6 +244,34 @@ 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 */
> > +static inline int
> > +grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
> 
> Nit: I find it a bit unclear with how `n` is supposed to be used. You
> might expect it to denote the maximum length of either of the UUIDs, but
> it's not really given that you skip over dashes without decrementing it.
> So this rather looks like the "length of non-dash characters in either
> of the UUIDs", which is likely not all that useful to the caller.

The intent was to make grub_uuidcasecmp bounded regardless of the
contents pointed to by uuid1 or uuid2 _and_ to allow specifying the
number of significant characters to compare. This way the function
could be used to compare other uuid-like strings, like fat volume ids,
which have only 8 significant characters. I'll admit that this series
uses grub_uuidcasecmp with the size of the LUKS header uuid, which
includes the dashes. But that's only a problem if one of the strings is
not null terminated and still a problem in the alternate implementation
below. And this could be easily change to use n = 32, which can't be
done with the suggestion below. So the current implementation handles
well uuid strings that are not null terminated (eg. in the middle of a
larger string). Admittedly, that feature is not being used at the
moment.

> 
> We should either clearly document the intent of this parameter, or make
> it more useful by e.g. allowing the caller to pass both a `uuid1len` and
> `uuid2len`, which get properly decremented when skipping over dashes.

I'll add documentation.

Glenn

> 
> Patrick
> 
> > +{
> > +  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 v5 1/2] misc: Add cast in grub_strncasecmp() to drop sign when calling grub_tolower()

[Daniel Kiper]

On Thu, Aug 11, 2022 at 12:48:42PM -0500, Glenn Washburn wrote:
> 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>

Daniel