Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn <development@efficientek.com>]

On Mon, 30 Aug 2021 20:02:26 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> >  include/grub/cryptodisk.h   |  3 +++
> >  2 files changed, 12 insertions(+), 17 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >  
> >  #endif
> >  
> > -static int check_boot, have_it;
> > -static char *search_uuid;
> > -
> >  static void
> >  cryptodisk_close (grub_cryptodisk_t dev)
> >  {
> > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > *name, 
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > -    dev = cr->scan (source, search_uuid, check_boot);
> > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > *name, 
> >      grub_cryptodisk_insert (dev, name, source);
> >  
> > -    have_it = 1;
> > +    cargs->found_uuid = 1;
> >  
> >      goto cleanup;
> >    }
> > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > *sourcedev, const char *cheat) 
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > -    dev = cr->scan (source, search_uuid, check_boot);
> > +    dev = cr->scan (source, NULL, 0);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> >    
> >    if (err)
> >      grub_print_error ();
> > -  return have_it && search_uuid ? 1 : 0;
> > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> >  }
> >  
> >  static grub_err_t
> > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) cargs.key_len =
> > grub_strlen(state[3].arg); }
> >  
> > -  have_it = 0;
> >    if (state[0].set) /* uuid */
> >      {
> >        grub_cryptodisk_t dev;
> > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> >  	}
> >  
> > -      check_boot = state[2].set;
> > -      search_uuid = args[0];
> > +      cargs.check_boot = state[2].set;
> > +      cargs.search_uuid = args[0];
> >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > -      search_uuid = NULL;
> >  
> > -      if (!have_it)
> > +      if (!cargs.found_uuid)
> >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > cryptodisk found"); return GRUB_ERR_NONE;
> >      }
> >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> >      {
> > -      search_uuid = NULL;
> > -      check_boot = state[2].set;
> > +      cargs.check_boot = state[2].set;
> >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > -      search_uuid = NULL;
> >        return GRUB_ERR_NONE;
> >      }
> >    else
> > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) char *disklast = NULL;
> >        grub_size_t len;
> >  
> > -      search_uuid = NULL;
> > -      check_boot = state[2].set;
> > +      cargs.check_boot = state[2].set;
> >        diskname = args[0];
> >        len = grub_strlen (diskname);
> >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 1070140d9..11062f43a 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -69,6 +69,9 @@ 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;
> >  };
> 
> Aren't these parameters in a different scope than the key data? These
> are only used for device discovery via `scan()`, while the other ones
> are for decrypting the key. Do we want to split those up into two
> different structs?

This struct is meant to be used for any data passed to the crypto
backend from cryptomount. All of those members are affected by
cryptomount options. So this struct isn't about anything in particular,
just a common set of data passed to the crypto backends via
cryptomount. So I don't think two structs would improve anything here.
Am I missing something?

Glenn