Re: [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Glenn Washburn <development@efficientek.com>]

On Sun, 3 Oct 2021 22:22:40 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote:
> > The member found_uuid 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
> > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
> > will only return 1 if a search_uuid has been specified and a cryptodisk was
> > successfully setup by a crypto-backend. So the return value of
> > grub_cryptodisk_scan_device is equivalent to found_uuid.
> 
> Is that always the case though? Most notably, `scan_device_real` will
> only set `have_it = 1` in case we have recovered the key. If the
> cryptodisk existed before and was found via `get_by_source_disk`, then
> we wouldn't set `have_it` at all. This essentially means that we'd only
> set `have_it` in case we found a new cryptodisk, but not if we return a
> preexisting one.
> 
> I don't know whether this behaviour is something we rely on. My gut
> feeling says it's rather a bug in the current code, which seems to be
> confirmed by the error message in the `if (!have_it)` case, which says
> "no such cryptodisk found". We did find one, but it's not a new one.
> 
> So in total I think your patch makes sense and fixes a bug, but the
> description doesn't quite match reality.

Good catch, I actually hadn't thought about this case, and
inadvertently fixed this bug. I'll update the commit message to reflect
this. I don't believe the behavior of this "bug" is relied on anywhere
either.

Glenn

> 
> Patrick
> 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 9 ++++-----
> >  include/grub/cryptodisk.h   | 1 -
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 5e153ee0a..033894257 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >  
> >      grub_cryptodisk_insert (dev, name, source);
> >  
> > -    cargs->found_uuid = 1;
> > -
> >      goto cleanup;
> >    }
> >    goto cleanup;
> > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
> >    
> >    if (err)
> >      grub_print_error ();
> > -  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > +  return (!err && cargs->search_uuid) ? 1 : 0;
> >  }
> >  
> >  static grub_err_t
> > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >  
> >    if (state[0].set) /* uuid */
> >      {
> > +      int found_uuid = 0;
> >        grub_cryptodisk_t dev;
> >  
> >        dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >  
> >        cargs.check_boot = state[2].set;
> >        cargs.search_uuid = args[0];
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> >  
> > -      if (!cargs.found_uuid)
> > +      if (!found_uuid)
> >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >        return GRUB_ERR_NONE;
> >      }
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 230167ab0..f4afb9cbd 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -70,7 +70,6 @@ typedef gcry_err_code_t
> >  struct grub_cryptomount_args
> >  {
> >    grub_uint32_t check_boot : 1;
> > -  grub_uint32_t found_uuid : 1;
> >    char *search_uuid;
> >    grub_uint8_t *key_data;
> >    grub_size_t key_len;
> > -- 
> > 2.32.0
> >