grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 2/3] cryptodisk: Add --header option to cryptomount to suppor


From: Patrick Steinhardt
Subject: Re: [PATCH 2/3] cryptodisk: Add --header option to cryptomount to support detached headers
Date: Sun, 15 May 2022 18:47:47 +0200

On Tue, May 10, 2022 at 11:53:08PM -0500, Glenn Washburn wrote:
> Add a --header (short -H) option to cryptomount which takes a file argument.
> Using the improved read hook, setup a read hook on the source device which
> will read from the given header file during the scan and recovery cryptodisk
> backend functions. This makes supporting detached headers transparent to the
> backend, so long as the attached header is located at the start of the
> source disk. This is not the case for GELI volumes, which have the header at
> the end of the volume. So GELI will return an error if a detached header is
> specified.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 68 ++++++++++++++++++++++++++++++++++++-
>  grub-core/disk/geli.c       |  4 +++
>  include/grub/cryptodisk.h   |  2 ++
>  include/grub/file.h         |  2 ++
>  4 files changed, 75 insertions(+), 1 deletion(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 19af4fa49..4307b723b 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -45,9 +45,18 @@ static const struct grub_arg_option options[] =
>      {"key-file", 'k', 0, N_("Key file"), 0, ARG_TYPE_STRING},
>      {"keyfile-offset", 'O', 0, N_("Key file offset (bytes)"), 0, 
> ARG_TYPE_INT},
>      {"keyfile-size", 'S', 0, N_("Key file data size (bytes)"), 0, 
> ARG_TYPE_INT},
> +    {"header", 'H', 0, N_("Read header from file"), 0, ARG_TYPE_STRING},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> +struct cryptodisk_read_hook_ctx
> +{
> +  grub_disk_read_hook_t prev_read_hook;
> +  void *prev_read_hook_data;
> +  grub_file_t hdr_file;
> +};
> +typedef struct cryptodisk_read_hook_ctx *cryptodisk_read_hook_ctx_t;
> +
>  /* Our irreducible polynom is x^128+x^7+x^2+x+1. Lowest byte of it is:  */
>  #define GF_POLYNOM 0x87
>  static inline int GF_PER_SECTOR (const struct grub_cryptodisk *dev)
> @@ -993,6 +1002,31 @@ cryptodisk_close (grub_cryptodisk_t dev)
>    grub_free (dev);
>  }
>  
> +static grub_err_t
> +cryptodisk_read_hook (grub_disk_addr_t sector, unsigned offset,
> +                   unsigned length, char *buf, void *data)
> +{
> +  grub_err_t ret = GRUB_ERR_NONE;
> +  cryptodisk_read_hook_ctx_t ctx = data;
> +
> +  if (ctx->hdr_file == NULL)
> +    return GRUB_ERR_NONE;

If you intend to keep support for `prev_read_hook`, then this case
should likely also execute it, shouldn't it?

Furthermore, should we return an error in case `hdr_file == NULL`? The only
function that sets this up sets a header file, and it doesn't make a lot of
sense to have the hook set up without one.

> +  if (grub_file_seek (ctx->hdr_file,
> +                   (sector * GRUB_DISK_SECTOR_SIZE) + offset)
> +      == (grub_off_t) -1)
> +    return grub_errno;
> +
> +  if (grub_file_read (ctx->hdr_file, buf, length) != (grub_ssize_t) length)
> +    return grub_errno;
> +
> +  if (ctx->prev_read_hook != NULL)
> +    ret = ctx->prev_read_hook(sector, offset, length, buf,
> +                           ctx->prev_read_hook_data);
> +
> +  return ret;
> +}
> +
>  static grub_cryptodisk_t
>  grub_cryptodisk_scan_device_real (const char *name,
>                                 grub_disk_t source,
> @@ -1001,6 +1035,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>    grub_err_t ret = GRUB_ERR_NONE;
>    grub_cryptodisk_t dev;
>    grub_cryptodisk_dev_t cr;
> +  struct cryptodisk_read_hook_ctx read_hook_data = {0};
>    int askpass = 0;
>    char *part = NULL;
>  
> @@ -1009,6 +1044,20 @@ grub_cryptodisk_scan_device_real (const char *name,
>    if (dev)
>      return dev;
>  
> +  if (cargs->hdr_file != NULL)
> +    {
> +      read_hook_data.hdr_file = cargs->hdr_file;
> +
> +      if (source->read_hook != NULL)
> +     {
> +       read_hook_data.prev_read_hook = source->read_hook;
> +       read_hook_data.prev_read_hook_data = source->read_hook_data;
> +     }

This part wouldn't need to be conditional. If it was `NULL` before, then
the `prev_read_hook` would be `NULL` anyway even without this condition,
rendering it essentially pointless.

> +      source->read_hook = cryptodisk_read_hook;
> +      source->read_hook_data = (void *) &read_hook_data;
> +    }
> +
>    FOR_CRYPTODISK_DEVS (cr)
>    {
>      dev = cr->scan (source, cargs);
> @@ -1058,6 +1107,11 @@ grub_cryptodisk_scan_device_real (const char *name,
>    dev = NULL;
>  
>   cleanup:
> +  if (read_hook_data.prev_read_hook != NULL)
> +    {
> +      source->read_hook = read_hook_data.prev_read_hook;
> +      source->read_hook_data = read_hook_data.prev_read_hook_data;
> +    }

So let me check whether I get this right. We're iterating through all cryptodev
devices of the current source disk. In case we are told to use a detached header
we now just set a read callback function that replaces whatever we did read with
the contents of the detached header. I think that this code could definitely use
some comments to explain what the idea behind this is to clarify it a bit for
future readers.

It took me some thinking, but ultimately this does seem to do the right thing.
And as you said, it's nice in that the actual backends don't need any changes at
all.

It seems to me like we're not unsetting the hook on the source disk after this
function return though. We do conditionally restore the previous read hook, but
in case there was none we don't do anything. It's likely not a good idea to leak
the hook to outside callers given that the disk will now essentially be backed
by the file.

Patrick

>    if (askpass)
>      {
>        cargs->key_len = 0;
> @@ -1254,6 +1308,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int 
> argc, char **args)
>       return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
>      }
>  
> +  if (state[7].set) /* header */
> +    {
> +      if (state[0].set)
> +     return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +                        N_("cannot use UUID lookup with detached header"));
> +
> +      cargs.hdr_file = grub_file_open (state[7].arg,
> +                         GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
> +      if (cargs.hdr_file == NULL)
> +     return grub_errno;
> +    }
> +
>    if (state[0].set) /* uuid */
>      {
>        int found_uuid;
> @@ -1467,7 +1533,7 @@ GRUB_MOD_INIT (cryptodisk)
>    grub_disk_dev_register (&grub_cryptodisk_dev);
>    cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
>                             N_("[ [-p password] | [-k keyfile"
> -                              " [-O keyoffset] [-S keysize] ] ]"
> +                              " [-O keyoffset] [-S keysize] ] ] [-H file]"
>                                " <SOURCE|-u UUID|-a|-b>"),
>                             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 91eb10122..b3c9bbd80 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -252,6 +252,10 @@ geli_scan (grub_disk_t disk, grub_cryptomount_args_t 
> cargs)
>    grub_disk_addr_t sector;
>    grub_err_t err;
>  
> +  /* Detached headers are not implemented yet */
> +  if (cargs->hdr_file != NULL)
> +    return NULL;
> +
>    if (2 * GRUB_MD_SHA256->mdlen + 1 > GRUB_CRYPTODISK_MAX_UUID_LENGTH)
>      return NULL;
>  
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 467065f00..d94df68b6 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -20,6 +20,7 @@
>  #define GRUB_CRYPTODISK_HEADER       1
>  
>  #include <grub/disk.h>
> +#include <grub/file.h>
>  #include <grub/crypto.h>
>  #include <grub/list.h>
>  #ifdef GRUB_UTIL
> @@ -79,6 +80,7 @@ struct grub_cryptomount_args
>    grub_uint8_t *key_data;
>    /* recover_key: Length of key_data */
>    grub_size_t key_len;
> +  grub_file_t hdr_file;
>  };
>  typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
>  
> diff --git a/include/grub/file.h b/include/grub/file.h
> index d53ee8edd..43b47bff5 100644
> --- a/include/grub/file.h
> +++ b/include/grub/file.h
> @@ -92,6 +92,8 @@ enum grub_file_type
>      GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
>      /* File holding the encryption key */
>      GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY,
> +    /* File holding the encryption metadata header */
> +    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER,
>      /* File we open n grub-fstest.  */
>      GRUB_FILE_TYPE_FSTEST,
>      /* File we open n grub-mount.  */
> -- 
> 2.34.1
> 

Attachment: signature.asc
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]