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

[Patrick Steinhardt]

On Mon, Oct 04, 2021 at 11:51:33PM -0500, Glenn Washburn wrote:
> On Mon, 4 Oct 2021 10:55:21 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > 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.
> 
> I'm gathering that the issue with partitioning the cargs struct is
> based on a concern about the proper use of the data. Perhaps the reason
> I don't see the value in partitioning the struct is because I also
> don't see how the data can be used in "the wrong phase". I'm guessing
> this is mainly a concern with key_data and key_len and that they might
> not be set during the scan phase. In that case key_len should be 0 and
> key_data a NULL pointer. If the scan phase wants to use them, I don't
> see why not, just make sure to check the values for validity first. Is
> the concern that a crypto-backend author will try to "unlock" the
> encrypted volume in scan? Perhaps I should add a couple of comments in
> the grub_cryptodisk_dev struct briefly describing scan and recover_key
> instead.

It's rather about passing data that we know to be  completely irrelevant
to the function at this point in time, even if it's unset. But as I said
above, this really isn't much of a strong concern, but rather feels like
a small inconsistency in the API design to me.

> I don't really like the nested struct solution I proposed
> because then the backend has to use both the phase-specific child
> struct and the common one, needing to remember which member is in which
> struct. I don't like embedding the common members in two different
> structs for, one for each phase, because then we have duplication of
> data. Or is there a way I'm not thinking of which can define the
> structs such that they share the common members?
> 
> What about a compromise, which would be to document in the member
> comment which phase its intented to be used in?

That's fine with me. I don't want to spend too much time discussing this
point: in the end it won't really matter anyway given that we only got
so many backends, and regardless of which way we go with, the end result
is cleaner than what we've got right now.

So again, please don't take my criticism on this point as a blocker.

Patrick

signature.asc
Description: PGP signature