Re: [PATCH v2 3/3] grub-core/kern/disk.c: handle LUKS2 devices

[Glenn Washburn]

On Mon, 09 May 2022 22:27:30 +0200
Josselin Poiret <dev@jpoiret.xyz> wrote:

> Hello everyone,
> 
> Glenn Washburn <development@efficientek.com> writes:
> 
> > I don't really like this, but it gets the job done and is a work-around
> > for a peculiarity of the LUKS2 backend. The cheat mount code for
> > cryptodisk does only calls scan() and not recover_key(). For LUKS1 scan
> > will return a grub_cryptodisk_t with log_sector_size set, but LUKS2
> > will not. This is because for LUKS1 the log_sector_size is constant
> > (LUKS1 also sets most of the other properties of the cryptodisk device,
> > like crypto algorithm, because they are in the binary header). However,
> > for LUKS2 the sector size (along with other properties) is in the json
> > header, which isn't getting parsed in scan(). 
> >
> > For single segment LUKS2 containers, scan() could get the sector size
> > from the json segment object. The LUKS2 spec says that normal LUKS2
> > devices are single segment[1], so this should work in the the cases the
> > care about (currently). scan() would not be able to fill in the other
> > properties, like crypto algorithm, because that depends on the keyslot
> > used, which needs key recovery to be determined. To avoid parsing the
> > json data twice, once in scan() and once in recover_key(), which should
> > be avoided, the parsed json object could be put in the grub_cryptodisk_t
> > in scan(), and used and freed in recover_key(). We'd probably also want
> > to add a way for grub_cryptodisk_t objects to get cleaned up by the
> > backend using them, so that the json object could be freed even if
> > recover_key() is never called.
> >
> > I think the above is the real fix, a moderate amount more work, and not
> > something I'd expect Pierre-Louis to take up. So if we're not going to
> > do this to get this functionality to work, we'll need a hack to get it
> > working. However, I'd prefer a different one.
> >
> > I've not tested this, but it seems to me that we can set the
> > log_sector_size field to GRUB_DISK_SECTOR_BITS _if_ it equals zero in
> > grub_cryptodisk_cheat_insert(). This limits the hack to only GRUB
> > host/user-space code.
> 
> Regarding these last lines, it's also possible to directly ask dm for
> the actual sector size when cheatmounting, as well as the crypto
> algorithm, bypassing the whole issue of parsing the json and finding the
> right slot.  This is roughly what's done in patch 2 of [1], maybe this
> workaround would be more to your liking?

Hah! Yes, thanks for reminding me. I forgot I'd suggested this and its
a better approach than what I suggested above. And probably the one I'd
support, but I need to more thoroughly take a look at it.

> I've distributed this patch to several people that were having issues on
> GNU Guix and they've been happily using LUKS2 with GRUB with it.

Yes, this does work and too much of a hack as-is. Regardless, your
contribution is appreciated. I'd like to get your patch with the GRUB fs
tests merged. Do you want to make the changes I suggested and send a new
patch here? If not, at some point I'll probably make them myself and
submit it to the list.

> [1] https://lists.gnu.org/archive/html/grub-devel/2021-12/msg00079.html
> (20211211122945.6326-1-dev@jpoiret.xyz)

Glenn