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

[Patrick Steinhardt]

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
>

signature.asc
Description: PGP signature