[PATCH 0/3] Cryptomount detached headers

[PATCH 0/3] Cryptomount detached headers

[Glenn Washburn]

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. It takes
care of the case where there is already a previous read hook on the source
device and will call that read hook after modifying the read buffer. I don't
believe this is strictly necessary currently because I don't think there
ever is a read hook already set since the disk was just created with a
grub_disk_open(). I'm not opposed to getting rid of this code. The one
benefit I see if a bit of future proofing.

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.

Glenn

Glenn Washburn (3):
  disk: Allow read hook callback to take read buffer to potentially
    modify it
  cryptodisk: Add --header option to cryptomount to support detached
    headers
  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    | 68 +++++++++++++++++++++++++++++++++-
 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, 125 insertions(+), 35 deletions(-)

-- 
2.34.1

[PATCH 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 88ea49fee..22f4fff44 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 2/3] cryptodisk: Add --header option to cryptomount to support detached headers

[Glenn Washburn]

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

[PATCH 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 8d1536c4d..9437b46a5 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4356,19 +4356,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 0/3] Cryptomount detached headers

[Daniel Kiper]

On Tue, May 10, 2022 at 11:53:06PM -0500, Glenn Washburn wrote:
> 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. It takes
> care of the case where there is already a previous read hook on the source
> device and will call that read hook after modifying the read buffer. I don't
> believe this is strictly necessary currently because I don't think there
> ever is a read hook already set since the disk was just created with a
> grub_disk_open(). I'm not opposed to getting rid of this code. The one
> benefit I see if a bit of future proofing.

I would get rid of this code. The first question which comes to my mind
is: which hook has to process the data first? If we do not know potential
users of that "multi-hook" feature I would not introduce it to not
create a feeling the hook interface is well defined. So, at this point
I would suggest to stick with one hook only.

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

I am OK with the idea though I would like to hear Patrick's opinion here.

Daniel

Re: [PATCH 0/3] Cryptomount detached headers

[Glenn Washburn]

On Fri, 13 May 2022 13:24:12 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, May 10, 2022 at 11:53:06PM -0500, Glenn Washburn wrote:
> > 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. It takes
> > care of the case where there is already a previous read hook on the source
> > device and will call that read hook after modifying the read buffer. I don't
> > believe this is strictly necessary currently because I don't think there
> > ever is a read hook already set since the disk was just created with a
> > grub_disk_open(). I'm not opposed to getting rid of this code. The one
> > benefit I see if a bit of future proofing.
> 
> I would get rid of this code. The first question which comes to my mind
> is: which hook has to process the data first? If we do not know potential
> users of that "multi-hook" feature I would not introduce it to not
> create a feeling the hook interface is well defined. So, at this point
> I would suggest to stick with one hook only.

Agreed. The problem that I was trying to solve was not about future
hook users, but current ones. I was not 100% sure that there wouldn't
be a state where the source disk might currently have a hook on it. No
current hooks can modify the read buffer because this patch series is
what allows for hooks that can modify the read buffer. And I believe
that none have any side effects that are consequential for the
functioning of GRUB (eg. only used for informational purposes). And
since none even look at the read buffer, because they couldn't before
this series) it doesn't matter whether they go before or after. But it
also probably doesn't matter if they get called at all (a tiny less
accurate progress indicator isn't a big deal). I wanted to call
previous hooks just for completeness.

> > 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.
> 
> I am OK with the idea though I would like to hear Patrick's opinion here.

A couple ideas are coming to me. With GELI the last 512 bytes of the
device are the header. So I could have the backends specify header
offset in the grub_cryptodisk_dev and by default have the value 0. This
would allow for GELI using the hook mechanism. Instead of or in
addition to this, there could be a flag field in the
grub_cryptodisk_dev which has a flag indicating not to use the read
hook. The backend gets the header file in the cargs struct, so it can
then decide how it wants to implement detached headers itself.

What do you think about having either of these ideas in the next
version of the series? Or just wait until need arises?

Glenn

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

[Patrick Steinhardt]

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

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 #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/3] Cryptomount detached headers

[Patrick Steinhardt]

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

On Fri, May 13, 2022 at 01:24:12PM +0200, Daniel Kiper wrote:
> On Tue, May 10, 2022 at 11:53:06PM -0500, Glenn Washburn wrote:
> > 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. It takes
> > care of the case where there is already a previous read hook on the source
> > device and will call that read hook after modifying the read buffer. I don't
> > believe this is strictly necessary currently because I don't think there
> > ever is a read hook already set since the disk was just created with a
> > grub_disk_open(). I'm not opposed to getting rid of this code. The one
> > benefit I see if a bit of future proofing.
> 
> I would get rid of this code. The first question which comes to my mind
> is: which hook has to process the data first? If we do not know potential
> users of that "multi-hook" feature I would not introduce it to not
> create a feeling the hook interface is well defined. So, at this point
> I would suggest to stick with one hook only.
> 
> > 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.
> 
> I am OK with the idea though I would like to hear Patrick's opinion here.
> 
> Daniel

It's rather intimate with how the backends are working right now, but
has the big advantage that it's backend-agnostic except for GELI. I feel
like the code warrants some more comments to explain what the underlying
idea is, but overall I like it.

Patrick

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

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

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

[Patrick Steinhardt]

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

On Mon, May 16, 2022 at 09:26:39AM -0500, Glenn Washburn wrote:
> 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:
[snip]
> > > +      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?

It would likely help reading the code when you ain't got the original
context of the commit.

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

What took me longest to figure out was the lifetime of the hook. I was
confused that it wasn't unset after we return, which is fine in case we
know that the caller would destroy the disk anyway. But it's not
obvious, and for code hygiene I'd in fact change that assumption so that
any future changes don't break that assumption.

Furthermore, I was confused at first that the hook is invoked multiple
times and how that can in fact work. Until I realized that it is fine
for the hook to be called as many times as we want to, as long as every
single read really only reads in the area where we expect the header to
be.

I think this loop here would also be the best location to document on a
comparatively high level what the hook does, what the basic assumption
is, and how the called functions are impacted. If we continue to not
reset the hook, then we should also document why so that the reader
doesn't have to figure it out by themself.

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

Fair enough. As stated above, this is not at all obvious though when
only reading this function. So I'd either just unset it so that the
reader is satisfied and not left wondering why we don't. Or explain why
we don't need to reset it.

Personally, I'd lean towards just resetting the hook because it feels
tidier to me. Even if the caller changes we wouldn't leak the hook in
any way.

Patrick

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

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

[Glenn Washburn]

On Mon, 16 May 2022 17:39:01 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, May 16, 2022 at 09:26:39AM -0500, Glenn Washburn wrote:
> > 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:
> [snip]
> > > > +      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?
> 
> It would likely help reading the code when you ain't got the original
> context of the commit.

Ok, I'll add this in.

> > 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."
> 
> What took me longest to figure out was the lifetime of the hook. I was
> confused that it wasn't unset after we return, which is fine in case we
> know that the caller would destroy the disk anyway. But it's not
> obvious, and for code hygiene I'd in fact change that assumption so that
> any future changes don't break that assumption.

Yep, I agree.

> Furthermore, I was confused at first that the hook is invoked multiple
> times and how that can in fact work. Until I realized that it is fine
> for the hook to be called as many times as we want to, as long as every
> single read really only reads in the area where we expect the header to
> be.

Correct. The LUKS detached header implementation doesn't care or even
know that its using a detached header (ie. nothing about the detached
header file indicates its a detached header as opposed to a normal
volume). If there are other cryptodisk backends where a detached header
is differently formatted, this transparent approach will not work.

> I think this loop here would also be the best location to document on a
> comparatively high level what the hook does, what the basic assumption
> is, and how the called functions are impacted. If we continue to not
> reset the hook, then we should also document why so that the reader
> doesn't have to figure it out by themself.

By "this loop", I think you mean the cryptodisk dev loop. I think it
makes more sense to document this when setting the hook above the loop
in the if statement. Does that sound alright?

> > > 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
> 
> Fair enough. As stated above, this is not at all obvious though when
> only reading this function. So I'd either just unset it so that the
> reader is satisfied and not left wondering why we don't. Or explain why
> we don't need to reset it.
>
> Personally, I'd lean towards just resetting the hook because it feels
> tidier to me. Even if the caller changes we wouldn't leak the hook in
> any way.

Agreed. Thanks for the feedback. I'll incorporate this in the next
version.

Glenn