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

[Patrick Steinhardt]

On Sun, Jun 05, 2022 at 01:43:18PM -0500, Glenn Washburn wrote:
> 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.

I also noted that my own two orthogonal patches to fix the out-of-bounds
copy and dash-stripping have landed in `master` already. So I agree that
a combination of the other two series would be best.

Patrick

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

signature.asc
Description: PGP signature