Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Glenn Washburn]

On Mon, 30 Aug 2021 19:55:59 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
> > As an example, passing a password as a cryptomount argument is
> > implemented. However, the backends are not implemented, so testing
> > this will return a not implemented error.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++---------
> >  grub-core/disk/geli.c       |  4 ++++
> >  grub-core/disk/luks.c       |  4 ++++
> >  grub-core/disk/luks2.c      |  4 ++++
> >  include/grub/cryptodisk.h   |  8 ++++++++
> >  5 files changed, 42 insertions(+), 9 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 90f82b2d3..b966b19ab 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> >      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
> >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag
> > set."), 0, 0},
> > +    {"password", 'p', 0, N_("Password to open volumes."), 0,
> > ARG_TYPE_STRING}, {0, 0, 0, 0, 0, 0}
> >    };
> >  
> > @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> >  }
> >  
> >  static grub_err_t
> > -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t
> > source) +grub_cryptodisk_scan_device_real (const char *name,
> > +                             grub_disk_t source,
> > +                             grub_cryptomount_args_t cargs)
> >  {
> >    grub_err_t err;
> >    grub_cryptodisk_t dev;
> > @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char
> > *name, grub_disk_t source) if (!dev)
> >        continue;
> >      
> > +    *dev->cargs = *cargs;
> >      err = cr->recover_key (source, dev);
> > +    *dev->cargs = NULL;
> >      if (err)
> >      {
> >        cryptodisk_close (dev);
> 
> Is there any specific reason why you choose to set `cargs` as a struct
> field? Seeing uses like this makes me wonder whether it wouldn't be
> better to pass in `cargs` as explicit arguments whenever required.
> This would also automatically answer any lifetime questions which
> arise.

I'm not opposed to this. One of my original goals was to try and not
change the function interfaces between cryptomount and the backends. I
also was originally going to have the dev->cargs stick around for the
lifetime of the dev, but I'm not remembering a good use case for that.
I'll send another series with cargs being passed as an argument.

> [snip]
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 13103ea6a..e2a4a3bf5 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
> >    grub_size_t max_stripes = 1;
> >    char *tmp;
> >  
> > +  /* Keyfiles are not implemented yet */
> > +  if (dev->cargs->key_data || dev->cargs->key_len)
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> >      return err;
> 
> Hum. This makes me wonder whether we'll have to adjust all backends
> whenever we add a new parameter to `cargs` to return
> `NOT_IMPLEMENTED`. I fear that it won't scale nicely, and that it is
> a recipe for passing unsupported arguments to backends even though
> they don't know to handle them.
> 
> Not sure about a nice alternative though. The only idea I have right
> now is something like a capabilities bitfield exposed by backends:
> only if a specific bit is set will we pass the corresponding field to
> such a backend. This would allow us to easily centralize the logic
> into a single function which only receives both the capabilities
> bitset and the `cargs` struct.
> 
> Other than that I really like where this is going.

I see your concern, and it would be nice to have an elegant solution to
it. The capability bitfield idea seems workable. I don't think this
needs to be solved right now though. This is a problem with all
proposed approaches. I think that this *could* lead to scalability
issues, but that's likely way down the road (considering the rate at
which we're adding args to cryptomount). Also, I don't think this patch
series hampers any such solution. So I think we can punt on this for
now. Does that sound reasonable?

Glenn