On Fri, May 13, 2022 at 01:24:12PM +0200, Daniel Kiper wrote: > On Tue, May 10, 2022 at 11:53:06PM -0500, Glenn Washburn wrote: > > This patch series is, I believe, a better approach to supporting detached > > headers for cryptomount and backends. This series will probably not apply > > cleanly without the changes from the recent series entitled "[PATCH 0/4] > > Cryptomount keyfile support". But its only because they touch code in the > > same vicinity, not because there's any real dependency. > > > > Conceptually the approach is different than the previous detach header > > series because this one uses the disk objects read hook to hook reads to > > the disk in scan() and recover_key() such that if there is an associated > > header file, the hook will cause the read to return data from the header > > file instead of from the source disk. > > > > For this the read hook implementation needed to be upgaded because prior > > it didn't get the read buffer sent from the read caller and so could not > > modify its contents. Patch #1 updates the hook accordingly and all instances > > of its use, but doesn't functionally change how GRUB operates. > > > > The second patch adds the header option to cryptomount and the read hook > > to the source disk during the scan() and recover_key() stages. It takes > > care of the case where there is already a previous read hook on the source > > device and will call that read hook after modifying the read buffer. I don't > > believe this is strictly necessary currently because I don't think there > > ever is a read hook already set since the disk was just created with a > > grub_disk_open(). I'm not opposed to getting rid of this code. The one > > benefit I see if a bit of future proofing. > > I would get rid of this code. The first question which comes to my mind > is: which hook has to process the data first? If we do not know potential > users of that "multi-hook" feature I would not introduce it to not > create a feeling the hook interface is well defined. So, at this point > I would suggest to stick with one hook only. > > > The benefit of this approach is its simpler/less code and does not require > > the modification of backends, except to potentially cause a failure in > > scan() if the backend doesn't support the current implementation of detached > > headers, as with the GELI backend. This implementation requires that the > > crypto volume header reside at the beginning of the volume. GELI has its > > header at the end, which is why it can not currently be supported. In > > theory, GELI could be supported if extra assumptions about its source > > access pattern during scan() and recovery_key() were made. I don't use GELI, > > no one seems to be asking for GELI detached headers, and I don't think that > > GELI even support detached headers with the official tools. So for me, not > > supporting crypto volumes with headers at the end of the disk is not a big > > deal. > > I am OK with the idea though I would like to hear Patrick's opinion here. > > Daniel
It's rather intimate with how the backends are working right now, but has the big advantage that it's backend-agnostic except for GELI. I feel like the code warrants some more comments to explain what the underlying idea is, but overall I like it. Patrick
signature.asc
Description: PGP signature
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
