Re: [PATCH v9 4/7] cryptodisk: Add support for LUKS1 detached headers

[Daniel Kiper <dkiper@net-space.pl>]

On Mon, Apr 11, 2022 at 06:40:25AM +0000, Glenn Washburn wrote:
> From: John Lane <john@lane.uk.net>
>
> cryptsetup supports having a detached header through the --header command
> line argument for both LUKS1 and LUKS2. Allow the LUKS1 backend to use a
> given file as the LUKS1 header (aka detached header) instead of looking for
> the header on the disk.
>
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> development@efficientek.com: rebase, improve commit message
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/luks.c | 48 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 34 insertions(+), 14 deletions(-)
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index fe1655c3c2..195c4bb8da 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -23,6 +23,7 @@
>  #include <grub/dl.h>
>  #include <grub/err.h>
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/partition.h>
>  #include <grub/i18n.h>
> @@ -73,17 +74,23 @@ luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>    char ciphername[sizeof (header.cipherName) + 1];
>    char ciphermode[sizeof (header.cipherMode) + 1];
>    char hashspec[sizeof (header.hashSpec) + 1];
> -  grub_err_t err;
> -
> -  /* Detached headers are not implemented yet */
> -  if (cargs->hdr_file)
> -    return NULL;
> +  grub_err_t err = GRUB_ERR_NONE;
>
>    if (cargs->check_boot)
>      return NULL;
>
>    /* Read the LUKS header.  */
> -  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +  if (cargs->hdr_file)

I would prefer if you do "if (cargs->hdr_file != NULL)". Please fix this
in other places/patches too.

> +    {
> +      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
> +	return NULL;
> +
> +      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
> +	return NULL;
> +    }
> +  else
> +    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +
>    if (err)
>      {
>        if (err == GRUB_ERR_OUT_OF_RANGE)
> @@ -162,17 +169,24 @@ luks_recover_key (grub_disk_t source,
>    grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
>    unsigned i;
>    grub_size_t length;
> -  grub_err_t err;
> +  grub_err_t err = GRUB_ERR_NONE;
>    grub_size_t max_stripes = 1;
> +  grub_uint32_t sector;
>
>    if (cargs->key_data == NULL || cargs->key_len == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
>
> -  /* Detached headers are not implemented yet */
>    if (cargs->hdr_file)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +    {
> +      if (grub_file_seek (cargs->hdr_file, 0) == (grub_off_t) -1)
> +	return grub_errno;
> +
> +      if (grub_file_read (cargs->hdr_file, &header, sizeof (header)) != sizeof (header))
> +	return grub_errno;
> +    }
> +  else
> +    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>
> -  err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
>      return err;
>
> @@ -227,13 +241,19 @@ luks_recover_key (grub_disk_t source,
>  	  return grub_crypto_gcry_error (gcry_err);
>  	}
>
> +      sector = grub_be_to_cpu32 (header.keyblock[i].keyMaterialOffset);
>        length = (keysize * grub_be_to_cpu32 (header.keyblock[i].stripes));
>
>        /* Read and decrypt the key material from the disk.  */
> -      err = grub_disk_read (source,
> -			    grub_be_to_cpu32 (header.keyblock
> -					      [i].keyMaterialOffset), 0,
> -			    length, split_key);
> +      if (cargs->hdr_file)
> +      {
> +        if (grub_file_seek (cargs->hdr_file, sector * 512) == (grub_off_t) -1)

s/512/1 << GRUB_LUKS1_LOG_SECTOR_SIZE/?

Otherwise Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel