On Mon, Jun 06, 2022 at 12:11:39PM -0500, Glenn Washburn wrote: > On Mon, 6 Jun 2022 07:32:40 +0200 > Patrick Steinhardt <p...@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 <p...@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 <d...@jpoiret.xyz> wrote: > > > > > > > > > > > Hello everyone, > > > > > > > > > > > > Glenn Washburn <developm...@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-...@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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel