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

[Daniel Kiper]

On Tue, Sep 06, 2022 at 05:28:40PM +0200, Patrick Steinhardt wrote:
> On Tue, Aug 30, 2022 at 03:12:36PM -0500, Glenn Washburn wrote:
> > On Mon, 29 Aug 2022 07:38:24 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> >
> > > 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.
> >
> > Why not? UUIDs are 32 hex characters with typically 4 dashes, that
> > makes 37 bytes needed including the NULL byte. In fact, I believe that
> > cryptsetup always creates the uuid field NULL terminated. Also, the
> > LUKS1 spec, and by extension LUKS2 spec, mandates NULL termination. The
> > uuid field is specified as "char[]" and "char[], a string stored as null
> > terminated sequence of 8-bit characters7".
>
> Indeed, you're right. I've read through the spec again and it matches
> your explanation. That also makes my remaining points moot.
>
> > >
> > > 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.
> >
> > This seems reasonable for what should be a very uncommon and non-spec
> > compliant case of having a uuid field with no NULL byte. I originally
> > had a patch before this that explicitly put a NULL byte at the end of
> > header.uuid so this problem didn't exist. I dropped it because it was
> > generally redundant as pointed out by Daniel, but I didn't take this
> > case into account. Perhaps its worth merging that with this one for
> > clarity.
>
> This is not as important anymore given that my initial point was moot.
> But it may still be sensible as defennse against corrupted headers.
>
> Anyway, with or without that change:
>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel