On Wed, Dec 08, 2021 at 12:18:13PM -0600, Glenn Washburn wrote:
> On Wed, 8 Dec 2021 17:37:19 +0100
> Daniel Kiper <dki...@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 <developm...@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
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
