Re: [PATCH 2/3] cryptodisk: Add --header option to cryptomount to support detached headers

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 4883 bytes --]

On Mon, May 16, 2022 at 09:26:39AM -0500, Glenn Washburn wrote:
> On Sun, 15 May 2022 18:47:47 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Tue, May 10, 2022 at 11:53:08PM -0500, Glenn Washburn wrote:
[snip]
> > > +      source->read_hook = cryptodisk_read_hook;
> > > +      source->read_hook_data = (void *) &read_hook_data;
> > > +    }
> > > +
> > >    FOR_CRYPTODISK_DEVS (cr)
> > >    {
> > >      dev = cr->scan (source, cargs);
> > > @@ -1058,6 +1107,11 @@ grub_cryptodisk_scan_device_real (const char *name,
> > >    dev = NULL;
> > >  
> > >   cleanup:
> > > +  if (read_hook_data.prev_read_hook != NULL)
> > > +    {
> > > +      source->read_hook = read_hook_data.prev_read_hook;
> > > +      source->read_hook_data = read_hook_data.prev_read_hook_data;
> > > +    }
> > 
> > So let me check whether I get this right. We're iterating through all cryptodev
> > devices of the current source disk. In case we are told to use a detached header
> > we now just set a read callback function that replaces whatever we did read with
> > the contents of the detached header. I think that this code could definitely use
> > some comments to explain what the idea behind this is to clarify it a bit for
> > future readers.
> 
> Yes, in my words, I'd say: we're iterating through the registered
> cryptodisk device backends, checking if a certain source disk can be
> opened by that backend (ie scan returns non-NULL). If we've been given
> a detached header to use, we set a read hook, which will redirect any
> reads on the source disk to a read from the header file at the same
> offset as it would've been to the source disk. Its an artifact of the
> disk read hook mechanism that the source disk will be read from and
> then the read data will be overwritten by the read hook with the read
> data from the header file. I'm not sure this need be documented. Do you
> think so?

It would likely help reading the code when you ain't got the original
context of the commit.

> I agree some comments would be good. Where would've you liked to see
> comments when you were reading this and what do you think would be
> helpful? Since I'm removing the prev_read_data code, no need to comment
> that. I'm thinking above the cryptodisk_read_hook definition with
> something like: "This read hook is used read a detached header file of
> a cryptodisk device instead of reading the header from the device.
> Currently, it can only be used when the header is located at the start
> of the volume, as is the case for LUKS1 and LUKS2, but not GELI."

What took me longest to figure out was the lifetime of the hook. I was
confused that it wasn't unset after we return, which is fine in case we
know that the caller would destroy the disk anyway. But it's not
obvious, and for code hygiene I'd in fact change that assumption so that
any future changes don't break that assumption.

Furthermore, I was confused at first that the hook is invoked multiple
times and how that can in fact work. Until I realized that it is fine
for the hook to be called as many times as we want to, as long as every
single read really only reads in the area where we expect the header to
be.

I think this loop here would also be the best location to document on a
comparatively high level what the hook does, what the basic assumption
is, and how the called functions are impacted. If we continue to not
reset the hook, then we should also document why so that the reader
doesn't have to figure it out by themself.

> > It took me some thinking, but ultimately this does seem to do the right thing.
> > And as you said, it's nice in that the actual backends don't need any changes at
> > all.
> > 
> > It seems to me like we're not unsetting the hook on the source disk after this
> > function return though. We do conditionally restore the previous read hook, but
> > in case there was none we don't do anything. It's likely not a good idea to leak
> > the hook to outside callers given that the disk will now essentially be backed
> > by the file.
> 
> Yes, this is an inconsistency. I didn't unset the hook because the
> source disk is closed right away in both callers of
> grub_cryptodisk_scan_device_real. But then it doesn't make sense to do
> the prev_read_hook because the disk has just been opened (if we assume
> that opening a disk doesn't set any hooks). So I'm removing that code.
> 
> Glenn

Fair enough. As stated above, this is not at all obvious though when
only reading this function. So I'd either just unset it so that the
reader is satisfied and not left wondering why we don't. Or explain why
we don't need to reset it.

Personally, I'd lean towards just resetting the hook because it feels
tidier to me. Even if the caller changes we wouldn't leak the hook in
any way.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]