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

[Glenn Washburn]

On Mon, 16 May 2022 17:39:01 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> 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.

Ok, I'll add this in.

> > 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.

Yep, I agree.

> 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.

Correct. The LUKS detached header implementation doesn't care or even
know that its using a detached header (ie. nothing about the detached
header file indicates its a detached header as opposed to a normal
volume). If there are other cryptodisk backends where a detached header
is differently formatted, this transparent approach will not work.

> 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.

By "this loop", I think you mean the cryptodisk dev loop. I think it
makes more sense to document this when setting the hook above the loop
in the if statement. Does that sound alright?

> > > 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.

Agreed. Thanks for the feedback. I'll incorporate this in the next
version.

Glenn