Re: [PATCH 1/2] Cryptomount support LUKS detached header

[Patrick Steinhardt]

On Fri, Feb 21, 2020 at 10:03:48PM +0100, Denis 'GNUtoo' Carikli wrote:
> From: John Lane <address@hidden>
> 
> Signed-off-by: John Lane <address@hidden>
> address@hidden: rebase
> Signed-off-by: Denis 'GNUtoo' Carikli <address@hidden>

We usually prefix commit subjects by the respective subsystem they're
touching. For example something like "cryptodisk: add support for
detached headers". It would also be nice to have the commit message
explain what you're doing in this commit in the future.

> ---
>  grub-core/disk/cryptodisk.c | 22 ++++++++++++++----
>  grub-core/disk/geli.c       |  7 ++++--
>  grub-core/disk/luks.c       | 45 ++++++++++++++++++++++++++++++-------
>  include/grub/cryptodisk.h   |  5 +++--
>  include/grub/file.h         |  2 ++
>  5 files changed, 65 insertions(+), 16 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 1897acc4b..6d4befc6f 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
>      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> +    {"header", 'H', 0, N_("Read LUKS header from file"), 0, ARG_TYPE_STRING},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -970,6 +971,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  
>  static int check_boot, have_it;
>  static char *search_uuid;
> +static grub_file_t hdr;
>  
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
> @@ -994,13 +996,13 @@ grub_cryptodisk_scan_device_real (const char *name, 
> grub_disk_t source)
>  
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> -    dev = cr->scan (source, search_uuid, check_boot);
> +    dev = cr->scan (source, search_uuid, check_boot, hdr);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
>        continue;
>      
> -    err = cr->recover_key (source, dev);
> +    err = cr->recover_key (source, dev, hdr);
>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -1041,7 +1043,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, 
> const char *cheat)
>  
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> -    dev = cr->scan (source, search_uuid, check_boot);
> +    dev = cr->scan (source, search_uuid, check_boot,0);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1095,6 +1097,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>    if (argc < 1 && !state[1].set && !state[2].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>  
> +  if (state[3].set) /* LUKS detached header */
> +    {
> +      if (state[0].set) /* Cannot use UUID lookup with detached header */
> +        return GRUB_ERR_BAD_ARGUMENT;

We should return a proper error to the user so that he knows that both
options are incompatible. E.g.

    return grub_error (GRUB_ERR_BAD_ARGUMENT,
                       "Cannot use UUID lookup with detached header");

That'd also make the comment obsolete.

> +
> +      hdr = grub_file_open (state[3].arg, 
> GRUB_FILE_TYPE_LUKS_DETACHED_HEADER);
> +      if (!hdr)
> +        return grub_errno;
> +    }
> +  else
> +    hdr = NULL;
> +
>    have_it = 0;
>    if (state[0].set)
>      {
> @@ -1302,7 +1316,7 @@ GRUB_MOD_INIT (cryptodisk)
>  {
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
> -                           N_("SOURCE|-u UUID|-a|-b"),
> +                           N_("SOURCE|-u UUID|-a|-b|-H file"),
>                             N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
> diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index e9d23299a..f4394eb42 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -52,6 +52,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>
> @@ -243,7 +244,8 @@ grub_util_get_geli_uuid (const char *dev)
>  
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -                int boot_only)
> +                int boot_only,
> +                grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_cryptodisk_t newdev;
>    struct grub_geli_phdr header;
> @@ -398,7 +400,8 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>  }
>  
>  static grub_err_t
> -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> +recover_key (grub_disk_t source, grub_cryptodisk_t dev,
> +          grub_file_t hdr __attribute__ ((unused)) )
>  {
>    grub_size_t keysize;
>    grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];

You've probably started working on an oldish commit, as we've gained
LUKS2 support in the meantime. Means that you'll also have to change
function signatures for LUKS2 as it will otherwise fail to compile.

> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 410cd6f84..950e89237 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>
> @@ -66,7 +67,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, 
> grub_uint8_t * src,
>  
>  static grub_cryptodisk_t
>  configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -                int check_boot)
> +                int check_boot, grub_file_t hdr)
>  {
>    grub_cryptodisk_t newdev;
>    const char *iptr;
> @@ -78,11 +79,21 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>    char hashspec[sizeof (header.hashSpec) + 1];
>    grub_err_t err;
>  
> +  err = GRUB_ERR_NONE;
> +

err could just be initialized directly with its declaration.

>    if (check_boot)
>      return NULL;
>  
>    /* Read the LUKS header.  */
> -  err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +  if (hdr)
> +  {
> +    grub_file_seek (hdr, 0);
> +    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
> +        err = GRUB_ERR_READ_ERROR;
> +  }

You should check the return value of `grub_file_seek()`. I also feel
like we should not clobber `err` with our own value, but instead we
should probably return error codes of `grub_file_seek()` and
`grub_file_read()` directly.

> +  else
> +    err = grub_disk_read (disk, 0, 0, sizeof (header), &header);
> +
>    if (err)
>      {
>        if (err == GRUB_ERR_OUT_OF_RANGE)
> @@ -146,12 +157,14 @@ configure_ciphers (grub_disk_t disk, const char 
> *check_uuid,
>      }
>  
>    COMPILE_TIME_ASSERT (sizeof (newdev->uuid) >= sizeof (uuid));
> +
>    return newdev;

Needless code churn :)

>  }
>  
>  static grub_err_t
>  luks_recover_key (grub_disk_t source,
> -               grub_cryptodisk_t dev)
> +               grub_cryptodisk_t dev,
> +               grub_file_t hdr)
>  {
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
> @@ -163,8 +176,19 @@ luks_recover_key (grub_disk_t source,
>    grub_err_t err;
>    grub_size_t max_stripes = 1;
>    char *tmp;
> +  grub_uint32_t sector;
> +
> +  err = GRUB_ERR_NONE;
> +
> +  if (hdr)
> +  {
> +    grub_file_seek (hdr, 0);
> +    if (grub_file_read (hdr, &header, sizeof (header)) != sizeof (header))
> +        err = GRUB_ERR_READ_ERROR;
> +  }

Same as above with regards to error handling.

The rest looks fine to me.

Patrick

signature.asc
Description: PGP signature