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

[Daniel Kiper]

On Tue, Aug 02, 2022 at 01:17:25PM -0500, Glenn Washburn wrote:
> 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".

I thought you know this. If not you can ignore my comment.

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

Please fix grub_strcasecmp() in separate patch. However, it can be a
part of this patch series.

Daniel