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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 7963 bytes --]

On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote:
> On Sun, 12 Sep 2021 13:17:29 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > > 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
> > 
> > I'm mostly wondering about lifetimes of these parameters. They are used
> > in different phases of the cryptomount: some are used only at the time
> > of discovery, while others are used at decryption time, where it's not
> > immediately clear which parameters are used when without having a look
> > at the cryptodisk implementations. That's why I was thinking it might
> > make more sense to split them up by those phases such that this becomes
> > explicit.
> > 
> > Patrick
> 
> Okay, I think I see what you're saying. Essentially, as someone wanting
> to write a new crypto-backend, when I'm writing by scan function, how
> do I know what fields in the cargs struct are valid (have been
> assigned)? The answer would be to check if they are not NULL, and
> otherwise they're available. I don't really see an issue.
> 
> Would your objection be alleviated by having cargs be redefined like so:
> 
> struct grub_cryptomount_args
> {
>   struct {
>     grub_uint32_t check_boot : 1;
>     char *search_uuid;
>   } scan;
>   struct {
>     grub_uint8_t *key_data;
>     grub_size_t key_len;
>   } recover_key;
>   struct {
>     ...
>   } common;
> };
> 
> Then in scan you know you can only use scan and common structs. I have
> a common because the detached header changes will be such that scan and
> recover_key need to be provided with detached header.  I think this way
> is more than is needed at the present moment. If I'm still not getting
> it, can you be a little more concrete in what is problematic and how
> you think it could be changed to be better?
> 
> Glenn

Sorry, took me quite some time to get to this. I like your proposed
approach, and if we sprinkle in some comments about when those structs
should be used then I think it would be easy enough to understand. It
does raise the question whether we should instead define
`grub_cryptomount_args_common` and then embed it in
`grub_cryptomount_args_{scan,recover_key}` structs such that it is
impossible to use args in the wrong phase (e.g. `recover_key` args
during scan). But I feels a bit like bikeshedding, so please feel free
to go with your preferred style.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]