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

[Daniel Kiper]

On Fri, Jul 22, 2022 at 02:43:14AM -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.

I expect the confusion comes from differences between older and newer
cryptsetup implementations. AIUI older versions used UUIDs without
dashes, and this is what GRUB expects today, but newer ones use UUIDs
with dashes. Right? If it is true it took me some time to get it from
the text above. So, I think it should be improved a bit to make it more
clear...

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

[...]

> diff --git a/include/grub/misc.h b/include/grub/misc.h
> index 776dbf8af..39957ecf9 100644
> --- a/include/grub/misc.h
> +++ b/include/grub/misc.h
> @@ -243,6 +243,33 @@ 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 (*s1 && *s2 && --n)
> +    {
> +      /* Skip forward to non-dash on both UUIDs. */
> +      while ('-' == *s1)
> +        ++s1;
> +
> +      while ('-' == *s2)
> +        ++s2;
> +
> +      if (grub_tolower (*s1) != grub_tolower (*s2))

I think you took this code from grub_strncasecmp(). It is missing grub_uint8_t
casts here too. However, if you take a look at grub_strcasecmp(), a few lines
above, you can see casts in similar place. It is really confusing. I thing the
casts has been added to drop sign. If I am right then grub_strncasecmp() and
grub_uuidcasecmp() both should be fixed.

> +     break;
> +
> +      s1++;
> +      s2++;
> +    }
> +
> +  return (int) grub_tolower ((grub_uint8_t) *s1)
> +    - (int) grub_tolower ((grub_uint8_t) *s2);

... and these casts suggests again something is wrong above too...

Daniel