On Tue, 2021-01-05 at 14:03 -0600, Glenn Washburn wrote: > On Mon, 04 Jan 2021 22:48:49 -0800 > James Bottomley <j...@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.
Well, I've seen the patch, but it doesn't seem to address the root of the problem of where the password data might be used: if the password data is used by more than one function then it should likely be part of the shared data; if it's only used by a single function it makes more sense as an argument. I think you need to flesh your RFC out further to make that determination. On what is passed, we do have a question about whether it's data or a functional callback. I do tend to prefer callbacks in situations like this because they solve the lifetime issues (secrets should have well defined lifetimes to make sure there's a limited window for leaking them). A simple data pointer doesn't necessarily do this. So I think the important question is functional callback or data and where it's passed is simply a more minor detail the solution to which will become apparent in the use cases and which it's not hugely important to get right in the first instance. James > > > 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. _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
