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

[Patrick Steinhardt <ps@pks.im>]

[-- Attachment #1: Type: text/plain, Size: 3739 bytes --]

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.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]