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

[Patrick Steinhardt]

On Mon, Jun 06, 2022 at 12:11:39PM -0500, Glenn Washburn wrote:
> 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

I think we misunderstood. The two patches already _are_ in the `master`
branch:

    - ee12785f7 (luks2: Strip dashes off of the UUID, 2020-05-30)
    - 1066336dc (luks: Fix out-of-bounds copy of UUID, 2020-09-07)

So there's nothing that needs to be done here anymore.

Patrick

signature.asc
Description: PGP signature