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

[Glenn Washburn]

On Mon, 6 Jun 2022 07:32:40 +0200
Patrick Steinhardt <ps@pks.im> wrote:

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

I think those should be a different series, since they are orthogonal.
I prefer my patch series for handling uuids[1], which allows searching
with dashes. This would allow the user to search using uuids that
contain dashes (or without dashes for backwards compatibility), which
would be consistent with filesystem uuid searching.

Glenn

[1] https://lists.gnu.org/archive/html/grub-devel/2022-02/msg00101.html

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