Re: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key

[Glenn Washburn]

On Mon, 04 Jan 2021 22:48:49 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> > James,
> > 
> > I like the improvements here. However, I've been thinking more about
> > this and other improvements that deal with passing parameters to
> > modules used by cryptomount. I have some ideas that I've not had the
> > time to fully investigate or code up proof of concepts. One idea is
> > that we shouldn't be changing the function declaration of recover
> > key, that is to say adding new parameters. Instead we should be
> > adding the parameters to grub_cryptodisk_t and setting them in
> > grub_cryptodisk_scan_device_real. This would satisfy needs of this
> > patch series as well as the key file, detached header, sending
> > password as cryptomount arg, and master key features without
> > cluttering the function signature.
> 
> Keeping large amounts of shared state between caller and callee can be
> a debugger's nightmare.  In this case the only consumer of the
> password callback is the recover function, so it seems appropriate it
> should be an argument to that function.

Please see my recently submitted RFC patch for a more concrete idea of
exactly what I'm suggesting. I don't see the concern about shared state
as being applicable in this case, even though your point is valid in
other situations.

> > So, I don't think this is the right approach.
> 
> The thing this patch demonstrates is that altering the function
> signatures is fairly easy, so it would be a simple patch to alter them
> again if the password callback were decided to be an essential
> component of the cryptodisk device ... but it should really driven the
> need which isn't apparent yet.

By easy, I take you to mean there aren't a lot of places needing to be
modified because of the change in signature, and in that sense its
easy, which is good. But its also unnecessary. I'm not sure if you've
looked at the struct grub_cryptodisk yet, but its already pretty
cluttered. So I can see why you might not want to add more. However, I
think my solution (again see RFC patch) is the cleaner, more scalable
solution.

Glenn