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

[Patrick Steinhardt]

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