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

* [PATCH] cryptodisk: Allows UUIDs to be compared in a dash-insensitive manner
@ 2021-03-21 18:36 Glenn Washburn
  2021-03-22 18:54 ` Maksim Fomin
  0 siblings, 1 reply; 3+ messages in thread
From: Glenn Washburn @ 2021-03-21 18:36 UTC (permalink / raw)
  To: grub-devel, Daniel Kiper; +Cc: 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>
---

I have another version of grub_uuidcasecmp which is more efficient by not
copying the UUID bytes to another buffer to strip out the dashes. This also
implies that it does not require null-terminated strings as inputs because
one can use the n parameter to prevent access beyond length of array.

However, I'm not sure we're so memory constrained that the added complexity
is worth it here, so I've chosen to submit this simpler version. If that
assumption isn't valid, I can submit the more complex version. This current
version uses two extra buffer copies, one to copy the UUID out of the LUKS
header to ensure the string is null-terminated (unnecessary in geli), as
this version of grub_uuidcasecmp expects, and another in grub_uuidcasecmp
to strip out dashes so that grub_strncasecmp can be called.

Another benefit of this is that the complexity of UUID processing is moved
into one place as opposed to each cryptodisk module potentially having that
complexity (eg. current code duplication in LUKS1 and LUKS2 modules).

Glenn

---
 grub-core/disk/cryptodisk.c |  4 ++--
 grub-core/disk/geli.c       |  2 +-
 grub-core/disk/luks.c       | 16 +++-------------
 grub-core/disk/luks2.c      |  9 +++------
 include/grub/misc.h         | 27 +++++++++++++++++++++++++++
 5 files changed, 36 insertions(+), 22 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9e9c256fc..a8ae2b541 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -683,7 +683,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
@@ -917,7 +917,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 e0223e4fa..fa1ab849e 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -310,7 +310,7 @@ geli_scan (grub_disk_t disk, const char *check_uuid, int boot_only,
       return NULL;
     }
 
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  if (check_uuid && grub_uuidcasecmp (check_uuid, uuid) != 0)
     {
       grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
       return NULL;
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index b69443caa..cf7bee1a7 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -70,9 +70,7 @@ luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
 	   grub_file_t hdr)
 {
   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];
@@ -106,17 +104,9 @@ luks_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
       || 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 (*iptr != '-')
-	*optr++ = *iptr;
-    }
-  *optr = 0;
-
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  grub_memcpy(uuid, header.uuid, sizeof (header.uuid));
+  uuid[sizeof(uuid) - 1] = 0;
+  if (check_uuid && grub_uuidcasecmp (check_uuid, uuid, sizeof (uuid)) != 0)
     {
       grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
       return NULL;
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 47ff572e0..d7fceecc2 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -372,7 +372,6 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
   grub_cryptodisk_t cryptodisk;
   grub_luks2_header_t header;
   char uuid[sizeof (header.uuid) + 1];
-  grub_size_t i, j;
 
   if (check_boot)
     return NULL;
@@ -383,12 +382,10 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot,
       return NULL;
     }
 
-  for (i = 0, j = 0; i < sizeof (header.uuid); i++)
-    if (header.uuid[i] != '-')
-      uuid[j++] = header.uuid[i];
-  uuid[j] = '\0';
+  grub_memcpy(uuid, header.uuid, sizeof (header.uuid));
+  uuid[sizeof(uuid) - 1] = 0;
 
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  if (check_uuid && grub_uuidcasecmp (check_uuid, uuid, sizeof (uuid)) != 0)
     return NULL;
 
   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
diff --git a/include/grub/misc.h b/include/grub/misc.h
index 6f4624d79..a2a7622cb 100644
--- a/include/grub/misc.h
+++ b/include/grub/misc.h
@@ -27,6 +27,8 @@
 #include <grub/i18n.h>
 #include <grub/compiler.h>
 
+#define GRUB_MAX_UUID_LENGTH 71
+
 #define ALIGN_UP(addr, align) \
 	(((addr) + (typeof (addr)) (align) - 1) & ~((typeof (addr)) (align) - 1))
 #define ALIGN_UP_OVERHEAD(addr, align) ((-(addr)) & ((typeof (addr)) (align) - 1))
@@ -257,6 +259,31 @@ 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 first stripping out
+ * all dashes from both UUID arguments and then doing a regular
+ * grub_strncasecmp.
+ */
+static inline int
+grub_uuidcasecmp (const char *uuid1, const char *uuid2, grub_size_t n)
+{
+  char *ptr;
+  char ruuid1[GRUB_MAX_UUID_LENGTH + 1];
+  char ruuid2[GRUB_MAX_UUID_LENGTH + 1];
+
+  for (ptr = ruuid1; *uuid1; uuid1++)
+    if (*uuid1 != '-')
+      *(ptr++) = *uuid1;
+  *ptr = '\0';
+
+  for (ptr = ruuid2; *uuid2; uuid2++)
+    if (*uuid2 != '-')
+      *(ptr++) = *uuid2;
+  *ptr = '\0';
+
+  return grub_strncasecmp (ruuid1, ruuid2, n);
+}
+
 /*
  * 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.27.0



^ permalink raw reply related	[flat|nested] 3+ messages in thread