Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global

[Glenn Washburn]

On Wed, 8 Dec 2021 17:37:19 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sat, Dec 04, 2021 at 01:15:45AM -0600, Glenn Washburn wrote:
> > The global "have_it" was never used by the crypto-backends, but was used to
> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate() will return
> > 1 if and only if grub_cryptodisk_scan_device() returns 1. And
> > grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
> > been specified and a cryptodisk was successfully setup by a crypto-backend.
> >
> > With this change grub_device_iterate() will return 1 when a crypto device is
> > successfully decrypted or when the source device has already been
> > successfully opened. Prior to this change, trying to mount an already
> > successfully opened device would trigger an error with the message "no such
> > cryptodisk found", which is at best misleading. The mount should silently
> > succeed in this case, which is what happens with this patch.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 29 ++++++++++++++++++-----------
> >  include/grub/err.h          |  1 +
> >  2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..9224105ac 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >
> >  #endif
> >
> > -static int check_boot, have_it;
> > +static int check_boot;
> >  static char *search_uuid;
> >
> >  static void
> > @@ -1005,7 +1005,12 @@ grub_cryptodisk_scan_device_real (const char *name, 
> > grub_disk_t source)
> >    dev = grub_cryptodisk_get_by_source_disk (source);
> >
> >    if (dev)
> > -    return GRUB_ERR_NONE;
> > +    {
> > +      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> > +   return GRUB_ERR_NONE;
> > +      else
> > +   return GRUB_ERR_EXISTS;
> 
> Hmmm... This looks strange. Could you explain why you return
> GRUB_ERR_EXISTS if UUIDs do not match?

I've re-defined success (that is return == GRUB_ERR_NONE) in
grub_cryptodisk_scan_device_real to be such that, for the case where we
are looking for a particular UUID, either source we're given matches
that UUID is already opened or we successfully open it. If a UUID is
not being searched for, then the criteria is essentially the same,
except for the UUID check.

When searching, GRUB_ERR_EXISTS is returned when the source is
associated with an unlocked crypto device, but is not the UUID that is
being searched for. This in turn tells grub_cryptodisk_scan_device to
not return true (ie. do not stop searching for the requested UUID).

Looking at this again, I'm thinking it might be clearer if
grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
either opens the source or source is associated with an already opened
crypto device, and otehrwise return NULL. If the caller receives a
non-NULL, and does the UUID check itself, if it cares/needs to.

I'm also noticing that at a minimum, I think the if statement with the
UUID check needs to be updated to "search_uuid == NULL ||
grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
like being sent a NULL pointer. Though, I'm confused why this didn't
crash in my testing.

> > +    }
> >
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, 
> > grub_disk_t source)
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> > -    have_it = 1;
> > -
> >      return GRUB_ERR_NONE;
> >    }
> > -  return GRUB_ERR_NONE;
> > +  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle 
> > this device");
> 
> Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?

If this return is reached, it means that all cryptodisk backend modules
(eg. luks, luks2, geli) have tried unsuccessfully to open source. We
need to return an error here does that grub_cryptodisk_scan_device does
not confuse a success here with meaning that the source was
successfully opened.

This was not needed before because "have_it" was being used to
determine if source was or has been successfully opened. With these
changes it is not the return code of grub_cryptodisk_scan_device_real
that is being used for this.

> >  }
> >
> >  #ifdef GRUB_UTIL
> > @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
> >    err = grub_cryptodisk_scan_device_real (name, source);
> >
> >    grub_disk_close (source);
> > -
> > -  if (err)
> > +
> > +  /*
> > +   * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many 
> > unhelpful
> > +   * error messages.
> > +   */
> > +  if (err != GRUB_ERR_NONE && err != GRUB_ERR_EXISTS && err != 
> > GRUB_ERR_BAD_MODULE)
> >      grub_print_error ();
> > -  return have_it && search_uuid ? 1 : 0;
> > +  return (err == GRUB_ERR_NONE && search_uuid != NULL);
> >  }
> >
> >  static grub_err_t
> > @@ -1110,9 +1117,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> > argc, char **args)
> >    if (argc < 1 && !state[1].set && !state[2].set)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
> >
> > -  have_it = 0;
> >    if (state[0].set)
> >      {
> > +      int found_uuid;
> >        grub_cryptodisk_t dev;
> >
> >        dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1125,10 +1132,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, 
> > int argc, char **args)
> >
> >        check_boot = state[2].set;
> >        search_uuid = args[0];
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> > +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, 
> > NULL);
> >        search_uuid = NULL;
> >
> > -      if (!have_it)
> > +      if (!found_uuid)
> >     return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >        return GRUB_ERR_NONE;
> >      }
> > diff --git a/include/grub/err.h b/include/grub/err.h
> > index b08d5d0de..55219cdcc 100644
> > --- a/include/grub/err.h
> > +++ b/include/grub/err.h
> > @@ -57,6 +57,7 @@ typedef enum
> >      GRUB_ERR_MENU,
> >      GRUB_ERR_TIMEOUT,
> >      GRUB_ERR_IO,
> > +    GRUB_ERR_EXISTS,
> >      GRUB_ERR_ACCESS_DENIED,
> >      GRUB_ERR_EXTRACTOR,
> >      GRUB_ERR_NET_BAD_ADDRESS,
> 
> Daniel

Glenn