Re: [PATCH 2/3] cryptodisk: Add --header option to cryptomount to support detached headers

[Glenn Washburn]

On Sun, 15 May 2022 18:47:47 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> 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?

This is a good point. I'm going to get rid of the prev_read_hook.

> 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.

This is a good idea. I wasn't thinking of hdr_file == NULL as being an
error condition, but you're right. I think from a users debugging
perspective it would be nice to add in a grub_dprintf so we have an
idea that the problem occured here and not somewhere else in the read
processing.

> > +  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;

Thinking about this again, I realized that in the case that the header
file is too small, grub_file_read() could return here, but
grub_errno == GRUB_ERR_NONE, which doesn't properly propagate an error
condition. I don't like occluding the real error either, so I'll set
grub_errno only if its not already set.

> > +
> > +  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.

I was thinking it might save a memory write, but yeah not necessary.

> > +      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.

Yes, in my words, I'd say: we're iterating through the registered
cryptodisk device backends, checking if a certain source disk can be
opened by that backend (ie scan returns non-NULL). If we've been given
a detached header to use, we set a read hook, which will redirect any
reads on the source disk to a read from the header file at the same
offset as it would've been to the source disk. Its an artifact of the
disk read hook mechanism that the source disk will be read from and
then the read data will be overwritten by the read hook with the read
data from the header file. I'm not sure this need be documented. Do you
think so?

I agree some comments would be good. Where would've you liked to see
comments when you were reading this and what do you think would be
helpful? Since I'm removing the prev_read_data code, no need to comment
that. I'm thinking above the cryptodisk_read_hook definition with
something like: "This read hook is used read a detached header file of
a cryptodisk device instead of reading the header from the device.
Currently, it can only be used when the header is located at the start
of the volume, as is the case for LUKS1 and LUKS2, but not GELI."

> 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.

Yes, this is an inconsistency. I didn't unset the hook because the
source disk is closed right away in both callers of
grub_cryptodisk_scan_device_real. But then it doesn't make sense to do
the prev_read_hook because the disk has just been opened (if we assume
that opening a disk doesn't set any hooks). So I'm removing that code.

Glenn

> 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
> >