Re: [PATCH 0/3] Cryptomount detached headers

[Daniel Kiper]

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