Re: [v5][ 4/6] cryptodisk: add support for LUKS1 detached headers

[Glenn Washburn]

Thanks Denis for taking the lead in trying to get these patches
included.  One issue I've found that make LUKS1 detached header support
unusable is below.

On Thu, 11 Jun 2020 18:18:05 +0200
Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org> 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.
> 
> This adds support for LUKS1 detached headers.
> 
> 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>
> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> ---
> ChangeLog since v4:
> - Added Reviewed-by tag
> ---
>  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 ffeb679d1..0b20908ac 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c

...

> @@ -240,13 +254,19 @@ luks_recover_key (grub_disk_t source,
> grub_cryptodisk_t dev, grub_file_t hdr) 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 (hdr)
> +      {
> +        if (grub_file_seek (hdr, sector * 512))

The return value is properly checked for all the other seeks but this
one.  Without this fixed, cryptomount with a header file on a LUKS
device and properly entered password will exit early with grub_errno
indicating success. However the key will not be properly setup, so the
blocks will decrypt to garbage.

> +          return grub_errno;
> +        if (grub_file_read (hdr, split_key, length) !=
> (grub_ssize_t)length)
> +          return grub_errno;
> +      }
> +      else
> +        err = grub_disk_read (source, sector, 0, length, split_key);
>        if (err)
>       {
>         grub_free (split_key);