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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 5770 bytes --]

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.

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]