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

[Glenn Washburn]

On Sun, 29 May 2022 09:09:38 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Tue, May 10, 2022 at 10:55:52PM -0500, Glenn Washburn wrote:
> > 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
> 
> I very much agree that we should land the test-patches regardless of
> what happens to the rest: they cover an important test gap.
> 
> Other than that the patches look sane to me. The biggest question to me
> is which of the three patch series we want to include in the end:
> 
>     - Yours has the extra benefit of added tests, but these can go in
>       independently.
> 
>     - Josselin's patches [1] have the benefit that they try to derive a
>       "proper" sector size via device-mapper.
> 
>     - My own patches [2] include two additional patches: one to strip
>       dashes of the UUID so that findfs is easier to use and the same
>       across LUKS and LUKS2. And one out-of-bounds copy of the UUID in
>       LUKS. Both are kind of orthogonal though. One more thing I like
>       better about this patch series is that it clearly discerns LUKS
>       and LUKS2 devices.
> 
> So ultimately it feels like all of the patch series have their own
> advantages, but they should be combinable. The tests and my own
> orthogonal patches can be split out. And if we combined the approach in
> Josselin's patches to use DM to get the sector size with a proper
> conceptual split of LUKS and LUKS2 as in my own patches then I'd be more
> than happy.

I think[1] Fabian's patch[2] for getting sector size is actually better
and more general than using DM. My preference is for a combination of
Fabian's and Josselin's patches.

Your response made me question how we could get the correct luks module
to put in the grub config if we don't have a separate
GRUB_DEV_ABSTRACTION_LUKS2. But actually that code uses the command
"grub-probe --device $@ --target=abstraction, which calls
probe_abstraction() -> grub_util_cryptodisk_get_abstraction(), which
then uses the "modname" member of the grub_cryptodisk_t device. So
GRUB_DEV_ABSTRACTION_* is not used, nor needed in this case. When you
do make use of GRUB_DEV_ABSTRACTION_LUKS2, its just to duplicate
existing code. I think its best to not have a separate
GRUB_DEV_ABSTRACTION_LUKS2.

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2022-05/msg00184.html
[2] https://lists.gnu.org/archive/html/grub-devel/2021-08/msg00017.html

> 
> I may very well be biased here though given that one of the patch series
> is my own.
> 
> Patrick
> 
> [1]: <20220520182039.21654-1-dev@jpoiret.xyz>
> [2]: <cover.1590840835.git.ps@pks.im>