Re: [PATCH v2 2/3] cryptodisk: Add support for using detached header files

[Glenn Washburn]

On Sun, 29 May 2022 08:45:39 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, May 16, 2022 at 04:49:47PM -0500, Glenn Washburn wrote:
> > Using the disk read hook mechanism, setup a read hook on the source disk
> > which will read from the given header file during the scan and recovery
> > cryptodisk backend functions. Disk read hooks are executed after the data
> > has been read from the disk. This is okay, because the read hook is given
> > the read buffer before its sent back to the caller. In this case, the hook
> > can then overwrite the data read from the disk device with data from the
> > header file sent in as the read hook data. This is transparent to the
> > read caller. Since the callers of this function have just opened the
> > source disk, there are no current read hooks, so there's no need to
> > save/restore them nor consider if they should be called or not.
> > 
> > This hook assumes that the header is at the start of the volume, which
> > is not the case for some formats (eg. GELI). So GELI will return an
> > error if a detached header is specified. It also can only be used
> > with formats where the detached header file can be written to the
> > first blocks of the volume and the volume could still be unlocked.
> > So the header file can not be formatted differently from the on-disk
> > header. If these assumpts are not met, detached header file processing
> > must be specially handled in the cryptodisk backend module.
> > 
> > The hook will be called potentially many times by a backend. This is fine
> > because of the assumptions mentioned and the read hook reads from absolute
> > offsets and is stateless.
> > 
> > Also add a --header (short -H) option to cryptomount which takes a file
> > argument.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 93 +++++++++++++++++++++++++++++++++++--
> >  grub-core/disk/geli.c       |  4 ++
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  4 files changed, 97 insertions(+), 4 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index ecbda7ce9..fdb0461a8 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -43,7 +43,8 @@ enum
> >      OPTION_PASSWORD,
> >      OPTION_KEYFILE,
> >      OPTION_KEYFILE_OFFSET,
> > -    OPTION_KEYFILE_SIZE
> > +    OPTION_KEYFILE_SIZE,
> > +    OPTION_HEADER
> >    };
> >  
> >  static const struct grub_arg_option options[] =
> > @@ -56,9 +57,16 @@ 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_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)
> > @@ -1004,6 +1012,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;
> 
> Nit: `ret` is effectively unused. We only set it here and then return it
> at the end of this function. It prompted me to search whether I just
> missed it being set, but it's not. So I'd just remove this variable and
> return `GRUB_ERR_NONE` directly to simplify this.

Agreed. I think it was more useful in a prior version. I'll remove it.

Glenn