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

[Glenn Washburn]

On Tue, 2 Aug 2022 18:17:44 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> 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...

This may be true, I honestly don't know nor recall that being old
behavior of cryptsetup. Do you want me to assume that it is true, and
to modify the commit message accordingly? I could hedge and add a
sentence here like: "It is possible that very old versions of
cryptsetup displayed the UUID without dashes, accounting for the LUKS
handling of UUIDs without dashes".

> 
> > 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.

Yes, this part was just copied. So I'll add the casts here and to
grub_strcasecmp(). Should I add the changes to grub_strcasecmp() in
this patch and add a note in the commit message of the change?

> 
> > +   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

Glenn