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

[Patrick Steinhardt]

On Tue, Sep 07, 2021 at 04:43:18AM +0000, Glenn Washburn wrote:
> 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:
[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

Yeah, that does sound reasonable to me.

Patrick

signature.asc
Description: PGP signature