[PATCH v2 0/3] Cryptomount detached headers

[PATCH v2 0/3] Cryptomount detached headers

[Glenn Washburn]

Note: Description here has changed since last time, the last few sentences,
which describe a method for allowing backends to bypass the read hook on
the source disk.

Updates since v1:
 * Add marge comment block describing at a high level the read hook operation
   and assumptions.
 * Add code to unset read hook on source disk when exiting
   grub_cryptodisk_scan_device_real(). A couple return calls were converted
   into gotos so that logical flows to one exit point.

This patch series is, I believe, a better approach to supporting detached
headers for cryptomount and backends. This series will probably not apply
cleanly without the changes from the recent series entitled "[PATCH 0/4]
Cryptomount keyfile support". But its only because they touch code in the
same vicinity, not because there's any real dependency.

Conceptually the approach is different than the previous detach header
series because this one uses the disk objects read hook to hook reads to
the disk in scan() and recover_key() such that if there is an associated
header file, the hook will cause the read to return data from the header
file instead of from the source disk.

For this the read hook implementation needed to be upgaded because prior
it didn't get the read buffer sent from the read caller and so could not
modify its contents. Patch #1 updates the hook accordingly and all instances
of its use, but doesn't functionally change how GRUB operates.

The second patch adds the header option to cryptomount and the read hook
to the source disk during the scan() and recover_key() stages.
The benefit of this approach is its simpler/less code and does not require
the modification of backends, except to potentially cause a failure in
scan() if the backend doesn't support the current implementation of detached
headers, as with the GELI backend. This implementation requires that the
crypto volume header reside at the beginning of the volume. GELI has its
header at the end, which is why it can not currently be supported. In
theory, GELI could be supported if extra assumptions about its source
access pattern during scan() and recovery_key() were made. I don't use GELI,
no one seems to be asking for GELI detached headers, and I don't think that
GELI even support detached headers with the official tools. So for me, not
supporting crypto volumes with headers at the end of the disk is not a big
deal. With the patch series each backend gets the header file through cargs,
so it can implement detached headers by solely operating on that file instead
of the hooked source disk. In the the future, a flag can be added to the
cryptodisk_dev_t that backends can sent when registering to enabled/disable
the use of the read hook, if the backend needs to read from both the detached
header file and the source disk.

Glenn

Glenn Washburn (3):
  disk: Allow read hook callback to take read buffer to potentially
    modify it
  cryptodisk: Add support for using detached header files
  docs: Add documentation on detached header option to cryptomount

 docs/grub.texi                 | 13 +++--
 grub-core/commands/blocklist.c | 10 ++--
 grub-core/commands/loadenv.c   |  8 +--
 grub-core/commands/testload.c  |  4 +-
 grub-core/disk/cryptodisk.c    | 93 ++++++++++++++++++++++++++++++++--
 grub-core/disk/geli.c          |  4 ++
 grub-core/fs/hfspluscomp.c     |  4 +-
 grub-core/fs/ntfscomp.c        | 14 ++---
 grub-core/kern/disk.c          | 12 ++---
 grub-core/lib/progress.c       | 11 ++--
 grub-core/net/net.c            |  2 +-
 include/grub/cryptodisk.h      |  2 +
 include/grub/disk.h            |  6 +--
 include/grub/file.h            |  2 +
 14 files changed, 147 insertions(+), 38 deletions(-)

Range-diff against v1:
1:  b3672a4cf = 1:  ad4a48225 disk: Allow read hook callback to take read buffer to potentially modify it
2:  a3ffa4738 ! 2:  25db3635f cryptodisk: Add --header option to cryptomount to support detached headers
    @@ Metadata
     Author: Glenn Washburn <development@efficientek.com>
     
      ## Commit message ##
    -    cryptodisk: Add --header option to cryptomount to support detached headers
    +    cryptodisk: Add support for using detached header files
     
    -    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.
    +    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.
     
      ## grub-core/disk/cryptodisk.c ##
    +@@ grub-core/disk/cryptodisk.c: enum
    +     OPTION_PASSWORD,
    +     OPTION_KEYFILE,
    +     OPTION_KEYFILE_OFFSET,
    +-    OPTION_KEYFILE_SIZE
    ++    OPTION_KEYFILE_SIZE,
    ++    OPTION_HEADER
    +   };
    + 
    + static const struct grub_arg_option options[] =
     @@ grub-core/disk/cryptodisk.c: 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},
    @@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option options[] =
      
     +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;
    @@ grub-core/disk/cryptodisk.c: cryptodisk_close (grub_cryptodisk_t dev)
     +  cryptodisk_read_hook_ctx_t ctx = data;
     +
     +  if (ctx->hdr_file == NULL)
    -+    return GRUB_ERR_NONE;
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("header file not found"));
     +
     +  if (grub_file_seek (ctx->hdr_file,
     +		      (sector * GRUB_DISK_SECTOR_SIZE) + offset)
    @@ grub-core/disk/cryptodisk.c: cryptodisk_close (grub_cryptodisk_t dev)
     +    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);
    ++    {
    ++      if (grub_errno == GRUB_ERR_NONE)
    ++	grub_error (GRUB_ERR_OUT_OF_RANGE, N_("header file too small"));
    ++      return grub_errno;
    ++    }
     +
     +  return ret;
     +}
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
      
     +  if (cargs->hdr_file != NULL)
     +    {
    ++      /*
    ++       * Set read hook to read header from a file instead of the source disk.
    ++       * 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). 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.
    ++       *
    ++       * This hook needs only be set once and 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.
    ++       */
     +      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;
    -+	}
    -+
     +      source->read_hook = cryptodisk_read_hook;
     +      source->read_hook_data = (void *) &read_hook_data;
     +    }
     +
        FOR_CRYPTODISK_DEVS (cr)
        {
    ++    /*
    ++     * Loop through each cryptodisk backend that is registered (ie. loaded).
    ++     * If the scan returns NULL, then the backend being tested does not
    ++     * recognize the source disk, so move on to the next backend.
    ++     */
          dev = cr->scan (source, cargs);
    +     if (grub_errno)
    +-      return NULL;
    ++      goto error_no_close;
    +     if (!dev)
    +       continue;
    + 
    +@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
    + 
    + 	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
    + 	if (cargs->key_data == NULL)
    +-	  return NULL;
    ++	  goto error_no_close;
    + 
    + 	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
    + 	  {
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
    + 
    +  error:
    +   cryptodisk_close (dev);
    ++ error_no_close:
        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;
    -+    }
    ++  if (cargs->hdr_file != NULL)
    ++    source->read_hook = NULL;
    ++
        if (askpass)
          {
            cargs->key_len = 0;
     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
    - 	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("reading key file")));
    + 	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("failed to read key file")));
          }
      
    -+  if (state[7].set) /* header */
    ++  if (state[OPTION_HEADER].set) /* header */
     +    {
    -+      if (state[0].set)
    ++      if (state[OPTION_UUID].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,
    ++      cargs.hdr_file = grub_file_open (state[OPTION_HEADER].arg,
     +			    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
     +      if (cargs.hdr_file == NULL)
     +	return grub_errno;
     +    }
     +
    -   if (state[0].set) /* uuid */
    +   if (state[OPTION_UUID].set) /* uuid */
          {
            int found_uuid;
     @@ grub-core/disk/cryptodisk.c: GRUB_MOD_INIT (cryptodisk)
3:  01c30f477 = 3:  a0bd5337e docs: Add documentation on detached header option to cryptomount
-- 
2.34.1

[PATCH v2 1/3] disk: Allow read hook callback to take read buffer to potentially modify it

[Glenn Washburn]

It will be desirable in the future to allow having the read hook modify the
data passed back from a read function call on a disk or file. This adds that
infrustructure and has no impact on code flow for existing uses of the read
hook. Also changed is that now when the read hook callback is called it can
also indicate what error code should be sent back to the read caller.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/commands/blocklist.c | 10 ++++++----
 grub-core/commands/loadenv.c   |  8 +++++---
 grub-core/commands/testload.c  |  4 +++-
 grub-core/fs/hfspluscomp.c     |  4 ++--
 grub-core/fs/ntfscomp.c        | 14 +++++++-------
 grub-core/kern/disk.c          | 12 ++++++------
 grub-core/lib/progress.c       | 11 +++++++----
 grub-core/net/net.c            |  2 +-
 include/grub/disk.h            |  6 +++---
 9 files changed, 40 insertions(+), 31 deletions(-)

diff --git a/grub-core/commands/blocklist.c b/grub-core/commands/blocklist.c
index 944449b77..48fd85a33 100644
--- a/grub-core/commands/blocklist.c
+++ b/grub-core/commands/blocklist.c
@@ -53,9 +53,9 @@ print_blocklist (grub_disk_addr_t sector, unsigned num,
 }
 
 /* Helper for grub_cmd_blocklist.  */
-static void
+static grub_err_t
 read_blocklist (grub_disk_addr_t sector, unsigned offset, unsigned length,
-		void *data)
+		char *buf __attribute__ ((unused)), void *data)
 {
   struct blocklist_ctx *ctx = data;
 
@@ -70,7 +70,7 @@ read_blocklist (grub_disk_addr_t sector, unsigned offset, unsigned length,
 	}
 
       if (!length)
-	return;
+	return GRUB_ERR_NONE;
       print_blocklist (ctx->start_sector, ctx->num_sectors, 0, 0, ctx);
       ctx->num_sectors = 0;
     }
@@ -87,7 +87,7 @@ read_blocklist (grub_disk_addr_t sector, unsigned offset, unsigned length,
     }
 
   if (!length)
-    return;
+    return GRUB_ERR_NONE;
 
   if (length & (GRUB_DISK_SECTOR_SIZE - 1))
     {
@@ -103,6 +103,8 @@ read_blocklist (grub_disk_addr_t sector, unsigned offset, unsigned length,
       ctx->start_sector = sector;
       ctx->num_sectors = length >> GRUB_DISK_SECTOR_BITS;
     }
+
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
diff --git a/grub-core/commands/loadenv.c b/grub-core/commands/loadenv.c
index 3fd664aac..166445849 100644
--- a/grub-core/commands/loadenv.c
+++ b/grub-core/commands/loadenv.c
@@ -352,16 +352,16 @@ struct grub_cmd_save_env_ctx
 };
 
 /* Store blocklists in a linked list.  */
-static void
+static grub_err_t
 save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
-		    void *data)
+		    char *buf __attribute__ ((unused)), void *data)
 {
   struct grub_cmd_save_env_ctx *ctx = data;
   struct blocklist *block;
 
   block = grub_malloc (sizeof (*block));
   if (! block)
-    return;
+    return GRUB_ERR_NONE;
 
   block->sector = sector;
   block->offset = offset;
@@ -374,6 +374,8 @@ save_env_read_hook (grub_disk_addr_t sector, unsigned offset, unsigned length,
   ctx->tail = block;
   if (! ctx->head)
     ctx->head = block;
+
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
diff --git a/grub-core/commands/testload.c b/grub-core/commands/testload.c
index ff01a0516..76a8af94c 100644
--- a/grub-core/commands/testload.c
+++ b/grub-core/commands/testload.c
@@ -32,10 +32,11 @@
 GRUB_MOD_LICENSE ("GPLv3+");
 
 /* Helper for grub_cmd_testload.  */
-static void
+static grub_err_t
 read_progress (grub_disk_addr_t sector __attribute__ ((unused)),
 	       unsigned offset __attribute__ ((unused)),
 	       unsigned len,
+	       char *buf __attribute__ ((unused)),
 	       void *data __attribute__ ((unused)))
 {
   for (; len >= GRUB_DISK_SECTOR_SIZE; len -= GRUB_DISK_SECTOR_SIZE)
@@ -43,6 +44,7 @@ read_progress (grub_disk_addr_t sector __attribute__ ((unused)),
   if (len)
     grub_xputs (".");
   grub_refresh ();
+  return GRUB_ERR_NONE;
 }
 
 static grub_err_t
diff --git a/grub-core/fs/hfspluscomp.c b/grub-core/fs/hfspluscomp.c
index 095ea48c9..48ae438d8 100644
--- a/grub-core/fs/hfspluscomp.c
+++ b/grub-core/fs/hfspluscomp.c
@@ -123,7 +123,7 @@ hfsplus_read_compressed_real (struct grub_hfsplus_file *node,
     {
       grub_memcpy (buf, node->cbuf + pos, len);
       if (grub_file_progress_hook && node->file)
-	grub_file_progress_hook (0, 0, len, node->file);
+	grub_file_progress_hook (0, 0, len, NULL, node->file);
       return len;
     }
 
@@ -170,7 +170,7 @@ hfsplus_read_compressed_real (struct grub_hfsplus_file *node,
       grub_memcpy (buf, node->cbuf + (pos % HFSPLUS_COMPRESS_BLOCK_SIZE),
 		   curlen);
       if (grub_file_progress_hook && node->file)
-	grub_file_progress_hook (0, 0, curlen, node->file);
+	grub_file_progress_hook (0, 0, curlen, NULL, node->file);
       buf += curlen;
       pos += curlen;
       len -= curlen;
diff --git a/grub-core/fs/ntfscomp.c b/grub-core/fs/ntfscomp.c
index 3cd97d337..a009f2c2d 100644
--- a/grub-core/fs/ntfscomp.c
+++ b/grub-core/fs/ntfscomp.c
@@ -227,7 +227,7 @@ read_block (struct grub_ntfs_rlst *ctx, grub_uint8_t *buf, grub_size_t num)
 		  grub_memset (buf, 0, nn * GRUB_NTFS_COM_LEN);
 		  buf += nn * GRUB_NTFS_COM_LEN;
 		  if (grub_file_progress_hook && ctx->file)
-		    grub_file_progress_hook (0, 0, nn * GRUB_NTFS_COM_LEN,
+		    grub_file_progress_hook (0, 0, nn * GRUB_NTFS_COM_LEN, NULL,
 					     ctx->file);
 		}
 	    }
@@ -240,7 +240,7 @@ read_block (struct grub_ntfs_rlst *ctx, grub_uint8_t *buf, grub_size_t num)
 		  if (buf)
 		    buf += GRUB_NTFS_COM_LEN;
 		  if (grub_file_progress_hook && ctx->file)
-		    grub_file_progress_hook (0, 0, GRUB_NTFS_COM_LEN,
+		    grub_file_progress_hook (0, 0, GRUB_NTFS_COM_LEN, NULL,
 					     ctx->file);
 		  nn--;
 		}
@@ -271,7 +271,7 @@ read_block (struct grub_ntfs_rlst *ctx, grub_uint8_t *buf, grub_size_t num)
 		  if (grub_file_progress_hook && ctx->file)
 		    grub_file_progress_hook (0, 0,
 					     tt << (ctx->comp.log_spc
-						    + GRUB_NTFS_BLK_SHR),
+						    + GRUB_NTFS_BLK_SHR), NULL,
 					     ctx->file);
 		  buf += tt << (ctx->comp.log_spc + GRUB_NTFS_BLK_SHR);
 		}
@@ -294,7 +294,7 @@ read_block (struct grub_ntfs_rlst *ctx, grub_uint8_t *buf, grub_size_t num)
 		  if (grub_file_progress_hook && ctx->file)
 		    grub_file_progress_hook (0, 0,
 					     nn << (ctx->comp.log_spc
-						    + GRUB_NTFS_BLK_SHR),
+						    + GRUB_NTFS_BLK_SHR), NULL,
 					     ctx->file);
 		}
 	      ctx->target_vcn += nn;
@@ -323,7 +323,7 @@ ntfscomp (grub_uint8_t *dest, grub_disk_addr_t ofs,
 
 	  grub_memcpy (dest, ctx->attr->sbuf + ofs - ctx->attr->save_pos, n);
 	  if (grub_file_progress_hook && ctx->file)
-	    grub_file_progress_hook (0, 0, n, ctx->file);
+	    grub_file_progress_hook (0, 0, n, NULL, ctx->file);
 	  if (n == len)
 	    return 0;
 
@@ -390,7 +390,7 @@ ntfscomp (grub_uint8_t *dest, grub_disk_addr_t ofs,
 	n = len;
       grub_memcpy (dest, &ctx->attr->sbuf[o], n);
       if (grub_file_progress_hook && ctx->file)
-	grub_file_progress_hook (0, 0, n, ctx->file);
+	grub_file_progress_hook (0, 0, n, NULL, ctx->file);
       if (n == len)
 	goto quit;
       dest += n;
@@ -422,7 +422,7 @@ ntfscomp (grub_uint8_t *dest, grub_disk_addr_t ofs,
 
       grub_memcpy (dest, ctx->attr->sbuf, len);
       if (grub_file_progress_hook && file)
-	grub_file_progress_hook (0, 0, len, file);
+	grub_file_progress_hook (0, 0, len, NULL, file);
     }
 
 quit:
diff --git a/grub-core/kern/disk.c b/grub-core/kern/disk.c
index 3a42c007b..01190085e 100644
--- a/grub-core/kern/disk.c
+++ b/grub-core/kern/disk.c
@@ -402,10 +402,10 @@ grub_disk_read_small (grub_disk_t disk, grub_disk_addr_t sector,
   if (err)
     return err;
   if (disk->read_hook)
-    (disk->read_hook) (sector + (offset >> GRUB_DISK_SECTOR_BITS),
-		       offset & (GRUB_DISK_SECTOR_SIZE - 1),
-		       size, disk->read_hook_data);
-  return GRUB_ERR_NONE;
+    err = (disk->read_hook) (sector + (offset >> GRUB_DISK_SECTOR_BITS),
+			     offset & (GRUB_DISK_SECTOR_SIZE - 1),
+			     size, buf, disk->read_hook_data);
+  return err;
 }
 
 /* Read data from the disk.  */
@@ -501,7 +501,7 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector,
 
 	  if (disk->read_hook)
 	    (disk->read_hook) (sector, 0, agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS),
-			       disk->read_hook_data);
+			       buf, disk->read_hook_data);
 
 	  sector += agglomerate << GRUB_DISK_CACHE_BITS;
 	  size -= agglomerate << (GRUB_DISK_CACHE_BITS + GRUB_DISK_SECTOR_BITS);
@@ -513,7 +513,7 @@ grub_disk_read (grub_disk_t disk, grub_disk_addr_t sector,
 	{
 	  if (disk->read_hook)
 	    (disk->read_hook) (sector, 0, (GRUB_DISK_CACHE_SIZE << GRUB_DISK_SECTOR_BITS),
-			       disk->read_hook_data);
+			       buf, disk->read_hook_data);
 	  sector += GRUB_DISK_CACHE_SIZE;
 	  buf = (char *) buf + (GRUB_DISK_CACHE_SIZE << GRUB_DISK_SECTOR_BITS);
 	  size -= (GRUB_DISK_CACHE_SIZE << GRUB_DISK_SECTOR_BITS);
diff --git a/grub-core/lib/progress.c b/grub-core/lib/progress.c
index 4b7cbbca6..4f4389dd5 100644
--- a/grub-core/lib/progress.c
+++ b/grub-core/lib/progress.c
@@ -29,10 +29,11 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 #define UPDATE_INTERVAL 800
 
-static void
+static grub_err_t
 grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
                               unsigned offset __attribute__ ((unused)),
-                              unsigned length, void *data)
+                              unsigned length,
+			      char *buf __attribute__ ((unused)), void *data)
 {
   static int call_depth = 0;
   grub_uint64_t now;
@@ -42,11 +43,11 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
   file->progress_offset += length;
 
   if (call_depth)
-    return;
+    return GRUB_ERR_NONE;
 
   e = grub_env_get ("enable_progress_indicator");
   if (e && e[0] == '0') {
-    return;
+    return GRUB_ERR_NONE;
   }
 
   call_depth = 1;
@@ -132,6 +133,8 @@ grub_file_progress_hook_real (grub_disk_addr_t sector __attribute__ ((unused)),
       last_progress_update_time = now;
     }
   call_depth = 0;
+
+  return GRUB_ERR_NONE;
 }
 
 GRUB_MOD_INIT(progress)
diff --git a/grub-core/net/net.c b/grub-core/net/net.c
index 2b6771523..4796b344b 100644
--- a/grub-core/net/net.c
+++ b/grub-core/net/net.c
@@ -1661,7 +1661,7 @@ grub_net_fs_read_real (grub_file_t file, char *buf, grub_size_t len)
 	  total += amount;
 	  file->device->net->offset += amount;
 	  if (grub_file_progress_hook)
-	    grub_file_progress_hook (0, 0, amount, file);
+	    grub_file_progress_hook (0, 0, amount, NULL, file);
 	  if (buf)
 	    {
 	      grub_memcpy (ptr, nb->data, amount);
diff --git a/include/grub/disk.h b/include/grub/disk.h
index a17d257c3..25c141ea2 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -110,9 +110,9 @@ extern grub_disk_dev_t EXPORT_VAR (grub_disk_dev_list);
 
 struct grub_partition;
 
-typedef void (*grub_disk_read_hook_t) (grub_disk_addr_t sector,
-				       unsigned offset, unsigned length,
-				       void *data);
+typedef grub_err_t (*grub_disk_read_hook_t) (grub_disk_addr_t sector,
+					     unsigned offset, unsigned length,
+					     char *buf, void *data);
 
 /* Disk.  */
 struct grub_disk
-- 
2.34.1

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

[Glenn Washburn]

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;
+  cryptodisk_read_hook_ctx_t ctx = data;
+
+  if (ctx->hdr_file == NULL)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, N_("header file not found"));
+
+  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)
+    {
+      if (grub_errno == GRUB_ERR_NONE)
+	grub_error (GRUB_ERR_OUT_OF_RANGE, N_("header file too small"));
+      return grub_errno;
+    }
+
+  return ret;
+}
+
 static grub_cryptodisk_t
 grub_cryptodisk_scan_device_real (const char *name,
 				  grub_disk_t source,
@@ -1012,6 +1045,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;
 
@@ -1020,11 +1054,46 @@ grub_cryptodisk_scan_device_real (const char *name,
   if (dev)
     return dev;
 
+  if (cargs->hdr_file != NULL)
+    {
+      /*
+       * Set read hook to read header from a file instead of the source disk.
+       * 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). 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.
+       *
+       * This hook needs only be set once and 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.
+       */
+      read_hook_data.hdr_file = cargs->hdr_file;
+      source->read_hook = cryptodisk_read_hook;
+      source->read_hook_data = (void *) &read_hook_data;
+    }
+
   FOR_CRYPTODISK_DEVS (cr)
   {
+    /*
+     * Loop through each cryptodisk backend that is registered (ie. loaded).
+     * If the scan returns NULL, then the backend being tested does not
+     * recognize the source disk, so move on to the next backend.
+     */
     dev = cr->scan (source, cargs);
     if (grub_errno)
-      return NULL;
+      goto error_no_close;
     if (!dev)
       continue;
 
@@ -1041,7 +1110,7 @@ grub_cryptodisk_scan_device_real (const char *name,
 
 	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
 	if (cargs->key_data == NULL)
-	  return NULL;
+	  goto error_no_close;
 
 	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
 	  {
@@ -1066,9 +1135,13 @@ grub_cryptodisk_scan_device_real (const char *name,
 
  error:
   cryptodisk_close (dev);
+ error_no_close:
   dev = NULL;
 
  cleanup:
+  if (cargs->hdr_file != NULL)
+    source->read_hook = NULL;
+
   if (askpass)
     {
       cargs->key_len = 0;
@@ -1283,6 +1356,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("failed to read key file")));
     }
 
+  if (state[OPTION_HEADER].set) /* header */
+    {
+      if (state[OPTION_UUID].set)
+	return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			   N_("cannot use UUID lookup with detached header"));
+
+      cargs.hdr_file = grub_file_open (state[OPTION_HEADER].arg,
+			    GRUB_FILE_TYPE_CRYPTODISK_DETACHED_HEADER);
+      if (cargs.hdr_file == NULL)
+	return grub_errno;
+    }
+
   if (state[OPTION_UUID].set) /* uuid */
     {
       int found_uuid;
@@ -1496,7 +1581,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

[PATCH v2 3/3] docs: Add documentation on detached header option to cryptomount

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 docs/grub.texi | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 0a8057482..a741034dd 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4490,19 +4490,26 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See command @command{hashsum}
 @node cryptomount
 @subsection cryptomount
 
-@deffn Command cryptomount [ [@option{-p} password] | [@option{-k} keyfile [@option{-O} keyoffset] [@option{-S} keysize] ] ] device|@option{-u} uuid|@option{-a}|@option{-b}
+@deffn Command cryptomount [ [@option{-p} password] | [@option{-k} keyfile [@option{-O} keyoffset] [@option{-S} keysize] ] ] [@option{-H} file] device|@option{-u} uuid|@option{-a}|@option{-b}
 Setup access to encrypted device. A passphrase will be requested interactively,
 if neither the @option{-p} nor @option{-k} options are given. The option
 @option{-p} can be used to supply a passphrase (useful for scripts).
 Alternatively the @option{-k} option can be used to supply a keyfile with
 options @option{-O} and @option{-S} optionally supplying the offset and size,
-respectively, of the key data in the given key file.
-
+respectively, of the key data in the given key file. The @option{-H} options can
+be used to supply cryptomount backends with an alternative header file (aka
+detached header). Not all backends have headers nor support alternative header
+files (currently only LUKS1 and LUKS2 support them).
 Argument @var{device} configures specific grub device
 (@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
 with specified @var{uuid}; option @option{-a} configures all detected encrypted
 devices; option @option{-b} configures all geli containers that have boot flag set.
 
+Devices are not allowed to be given as key files nor as detached header files.
+However, this limitation can be worked around by using blocklist syntax. So
+for instance, @code{(hd1,gpt2)} can not be used, but @code{(hd1,gpt2)0+} will
+achieve the desired result.
+
 GRUB suports devices encrypted using LUKS, LUKS2 and geli. Note that necessary
 modules (@var{luks}, @var{luks2} and @var{geli}) have to be loaded manually
 before this command can be used. For LUKS2 only the PBKDF2 key derivation
-- 
2.34.1

Re: [PATCH v2 0/3] Cryptomount detached headers

[Daniel Kiper]

On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote:
> Note: Description here has changed since last time, the last few sentences,
> which describe a method for allowing backends to bypass the read hook on
> the source disk.
>
> Updates since v1:
>  * Add marge comment block describing at a high level the read hook operation
>    and assumptions.
>  * Add code to unset read hook on source disk when exiting
>    grub_cryptodisk_scan_device_real(). A couple return calls were converted
>    into gotos so that logical flows to one exit point.
>
> This patch series is, I believe, a better approach to supporting detached
> headers for cryptomount and backends. This series will probably not apply
> cleanly without the changes from the recent series entitled "[PATCH 0/4]
> Cryptomount keyfile support". But its only because they touch code in the
> same vicinity, not because there's any real dependency.

Does not this series require rebase on master after merging "Cryptomount
keyfile support" patch set? If yes I will review these patches after
rebase. I hope it will be quick turn around.

Daniel

Re: [PATCH v2 0/3] Cryptomount detached headers

[Glenn Washburn]

On Fri, 27 May 2022 16:06:42 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote:
> > Note: Description here has changed since last time, the last few sentences,
> > which describe a method for allowing backends to bypass the read hook on
> > the source disk.
> >
> > Updates since v1:
> >  * Add marge comment block describing at a high level the read hook operation
> >    and assumptions.
> >  * Add code to unset read hook on source disk when exiting
> >    grub_cryptodisk_scan_device_real(). A couple return calls were converted
> >    into gotos so that logical flows to one exit point.
> >
> > This patch series is, I believe, a better approach to supporting detached
> > headers for cryptomount and backends. This series will probably not apply
> > cleanly without the changes from the recent series entitled "[PATCH 0/4]
> > Cryptomount keyfile support". But its only because they touch code in the
> > same vicinity, not because there's any real dependency.
> 
> Does not this series require rebase on master after merging "Cryptomount
> keyfile support" patch set? If yes I will review these patches after
> rebase. I hope it will be quick turn around.

The keyfile changes haven't hit master yet, so I can't be sure. But, I
think this should apply cleanly. Randgediff shows no difference between
the patches in the branch for this series and the branch for the latest
keyfile series.

Glenn

Re: [PATCH v2 0/3] Cryptomount detached headers

[Daniel Kiper]

On Fri, May 27, 2022 at 03:20:26PM -0500, Glenn Washburn wrote:
> On Fri, 27 May 2022 16:06:42 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
> > On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote:
> > > Note: Description here has changed since last time, the last few sentences,
> > > which describe a method for allowing backends to bypass the read hook on
> > > the source disk.
> > >
> > > Updates since v1:
> > >  * Add marge comment block describing at a high level the read hook operation
> > >    and assumptions.
> > >  * Add code to unset read hook on source disk when exiting
> > >    grub_cryptodisk_scan_device_real(). A couple return calls were converted
> > >    into gotos so that logical flows to one exit point.
> > >
> > > This patch series is, I believe, a better approach to supporting detached
> > > headers for cryptomount and backends. This series will probably not apply
> > > cleanly without the changes from the recent series entitled "[PATCH 0/4]
> > > Cryptomount keyfile support". But its only because they touch code in the
> > > same vicinity, not because there's any real dependency.
> >
> > Does not this series require rebase on master after merging "Cryptomount
> > keyfile support" patch set? If yes I will review these patches after
> > rebase. I hope it will be quick turn around.
>
> The keyfile changes haven't hit master yet, so I can't be sure. But, I
> think this should apply cleanly. Randgediff shows no difference between
> the patches in the branch for this series and the branch for the latest
> keyfile series.

If there are no difference I will review these patches.

Thanks,

Daniel

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

[Patrick Steinhardt]

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

Re: [PATCH v2 0/3] Cryptomount detached headers

[Patrick Steinhardt]

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

On Mon, May 16, 2022 at 04:49:45PM -0500, Glenn Washburn wrote:
> Note: Description here has changed since last time, the last few sentences,
> which describe a method for allowing backends to bypass the read hook on
> the source disk.
> 
> Updates since v1:
>  * Add marge comment block describing at a high level the read hook operation
>    and assumptions.
>  * Add code to unset read hook on source disk when exiting
>    grub_cryptodisk_scan_device_real(). A couple return calls were converted
>    into gotos so that logical flows to one exit point.
[snip]

I've reviewed this patch series, and except for a single nit the changes
all look good to me. So please feel free to add my

    Reviewed-by: Patrick Steinhardt <ps@pks.im>

, regardless of whether you decide to apply my prosed nit in 2/3 or not.

Patrick

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

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