On Sun, 29 May 2022 08:58:20 +0200 Patrick Steinhardt <p...@pks.im> wrote:
> On Wed, May 04, 2022 at 04:47:08PM -0500, Glenn Washburn wrote: > > On Tue, 29 Mar 2022 12:31:58 +0200 > > Pierre-Louis Bonicoli <pierre-louis.bonic...@libregerbil.fr> wrote: > > > > > Unlike LUKS1, the sector size of LUKS2 devices isn't hardcoded. > > > > > > Regarding the probe command, the following values of --target switch > > > are affected: abstraction, arc_hints, baremetal_hints, bios_hints, > > > cryptodisk_uuid, drive, efi_hints, hints_string, ieee1275_hints, > > > zero_check. > > > > > > For example using the --target=drive option: > > > > > > # dd if=/dev/zero of=data count=10 bs=1M > > > # losetup --show -f data > > > /dev/loop4 > > > # echo -n pass | cryptsetup luksFormat -v --type luks2 /dev/loop4 > > > Key slot 0 created. > > > Command successful. > > > # echo -n pass | cryptsetup -v open /dev/loop4 test > > > No usable token is available. > > > Key slot 0 unlocked. > > > Command successful. > > > # grub-probe --device /dev/mapper/test --target=cryptodisk_uuid > > > grub-probe: error: disk `cryptouuid/f353c0f04a6a4c08bc53a0896130910f' > > > not found. > > > > > > The updated output: > > > > > > # grub-probe --device /dev/mapper/test --target=cryptodisk_uuid > > > f353c0f04a6a4c08bc53a0896130910f > > > --- > > > grub-core/kern/disk.c | 4 +++- > > > grub-core/osdep/devmapper/getroot.c | 3 ++- > > > 2 files changed, 5 insertions(+), 2 deletions(-) > > > > > > diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c > > > index 3a42c007b..fa3177bf0 100644 > > > --- a/grub-core/kern/disk.c > > > +++ b/grub-core/kern/disk.c > > > @@ -237,8 +237,10 @@ grub_disk_open (const char *name) > > > name); > > > goto fail; > > > } > > > - if (disk->log_sector_size > GRUB_DISK_CACHE_BITS + > > > GRUB_DISK_SECTOR_BITS > > > + if ((disk->log_sector_size > GRUB_DISK_CACHE_BITS + > > > GRUB_DISK_SECTOR_BITS > > > || disk->log_sector_size < GRUB_DISK_SECTOR_BITS) > > > + /* log_sector_size is unset for LUKS2 and that's ok */ > > > + && !(disk->log_sector_size == 0 && dev->id == > > > GRUB_DISK_DEVICE_CRYPTODISK_ID)) > > > > 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. > > > > [1] https://fossies.org/linux/cryptsetup/docs/on-disk-format-luks2.pdf, > > section 3.3 > > > > > { > > > grub_error (GRUB_ERR_NOT_IMPLEMENTED_YET, > > > "sector sizes of %d bytes aren't supported yet", > > > diff --git a/grub-core/osdep/devmapper/getroot.c > > > b/grub-core/osdep/devmapper/getroot.c > > > index 96781714c..4f51c113c 100644 > > > --- a/grub-core/osdep/devmapper/getroot.c > > > +++ b/grub-core/osdep/devmapper/getroot.c > > > @@ -180,7 +180,8 @@ grub_util_pull_devmapper (const char *os_dev) > > > grub_util_pull_device (subdev); > > > } > > > } > > > - if (uuid && strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - > > > 1) == 0 > > > + if (uuid && (strncmp (uuid, "CRYPT-LUKS1-", sizeof ("CRYPT-LUKS1-") - > > > 1) == 0 > > > + || strncmp (uuid, "CRYPT-LUKS2-", sizeof ("CRYPT-LUKS2-") - 1) == > > > 0) > > > > It seems better to me to not add another strncmp, but to only check for > > the prefix "CRYPT-LUKS". This way when LUKS3 comes out next decade we > > won't have to add another strncmp here. > > I'd actually argue the other way round: I'd rather be defensive and not > pretend that we can handle LUKS3, because chances are high that we won't > handle it correctly. I think its fine because there should be a failure or non-detection elsewhere. If a user has a LUKS3 volume and runs grub-probe with an installed GRUB that does not have support for LUKS3, then grub_cryptodisk_cheat_mount() will not call grub_cryptodisk_cheat_insert() because no cryptodisk backend modules are successfully scan the LUKS3 volume. But, I won't press this issue. Glenn > > Patrick > > > If we do want to keep this, I'd like to see '||' aligned with the start > > of "strncmp" of the line above, even though it will push the line past > > 80 chars by a few chars. At a minimum indent more than the line below. > > > > > && lastsubdev) > > > { > > > char *grdev = grub_util_get_grub_dev (lastsubdev); > > > > Glenn > > > > _______________________________________________ > > Grub-devel mailing list > > Grub-devel@gnu.org > > https://lists.gnu.org/mailman/listinfo/grub-devel _______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel