grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Daniel Kiper
Subject: Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global
Date: Thu, 9 Dec 2021 15:24:02 +0100
User-agent: NeoMutt/20170113 (1.7.2)

On Wed, Dec 08, 2021 at 12:18:13PM -0600, Glenn Washburn wrote:
> 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.

OK, please fix the patch and add a comment if you do some not obvious
things like here.

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

I think this begs for a comment too...

Daniel



reply via email to

[Prev in Thread] Current Thread [Next in Thread]