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

[Glenn Washburn]

On Sun, 29 May 2022 08:58:20 +0200
Patrick Steinhardt <ps@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.bonicoli@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