[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 0/3] Cryptomount detached headers
Re: [PATCH 0/3] Cryptomount detached headers
Fri, 13 May 2022 12:31:48 -0500
On Fri, 13 May 2022 13:24:12 +0200
Daniel Kiper <firstname.lastname@example.org> 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.
Agreed. The problem that I was trying to solve was not about future
hook users, but current ones. I was not 100% sure that there wouldn't
be a state where the source disk might currently have a hook on it. No
current hooks can modify the read buffer because this patch series is
what allows for hooks that can modify the read buffer. And I believe
that none have any side effects that are consequential for the
functioning of GRUB (eg. only used for informational purposes). And
since none even look at the read buffer, because they couldn't before
this series) it doesn't matter whether they go before or after. But it
also probably doesn't matter if they get called at all (a tiny less
accurate progress indicator isn't a big deal). I wanted to call
previous hooks just for completeness.
> > 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.
A couple ideas are coming to me. With GELI the last 512 bytes of the
device are the header. So I could have the backends specify header
offset in the grub_cryptodisk_dev and by default have the value 0. This
would allow for GELI using the hook mechanism. Instead of or in
addition to this, there could be a flag field in the
grub_cryptodisk_dev which has a flag indicating not to use the read
hook. The backend gets the header file in the cargs struct, so it can
then decide how it wants to implement detached headers itself.
What do you think about having either of these ideas in the next
version of the series? Or just wait until need arises?
[PATCH 3/3] docs: Add documentation on detached header option to cryptomount, Glenn Washburn, 2022/05/11
Re: [PATCH 0/3] Cryptomount detached headers, Daniel Kiper, 2022/05/13