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

[Patrick Steinhardt <ps@pks.im>]

[-- 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 --]