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

[Glenn Washburn]

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.

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