[PATCH v3 0/5] Cryptomount keyfile support

[PATCH v3 0/5] Cryptomount keyfile support

[Glenn Washburn]

Uupdates from v2:
 * Use one error message for all string to interger conversion errors,
   suggested by Daniel
 * Move placement of keyfile_size == 0 check

Updates from v1:
 * Make some changes suggested by Daniel
 * Improve error message for grub_strtoull() failures
 * Add patch to use enum constants to index parsed option array

Glenn

Denis 'GNUtoo' Carikli (2):
  cryptodisk: luks: Unify grub_cryptodisk_dev function names
  cryptodisk: geli: Unify grub_cryptodisk_dev function names

Glenn Washburn (2):
  cryptodisk: Use enum constants as indexes into cryptomount option
    array
  docs: Add documentation on keyfile option to cryptomount

John Lane (1):
  cryptodisk: Add options to cryptomount to support keyfiles

 docs/grub.texi              |  14 +++--
 grub-core/disk/cryptodisk.c | 110 ++++++++++++++++++++++++++++++++----
 grub-core/disk/geli.c       |   8 +--
 grub-core/disk/luks.c       |   4 +-
 include/grub/cryptodisk.h   |   2 +
 include/grub/file.h         |   2 +
 6 files changed, 119 insertions(+), 21 deletions(-)

Interdiff against v2:
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index ecbda7ce9..e2b8636e4 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1188,7 +1188,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
   if (state[OPTION_KEYFILE].set) /* keyfile */
     {
-      char tmp_errmsg[GRUB_MAX_ERRMSG];
       const char *p = NULL;
       grub_file_t keyfile;
       unsigned long long keyfile_offset = 0, keyfile_size = 0;
@@ -1199,20 +1198,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  keyfile_offset = grub_strtoull (state[OPTION_KEYFILE_OFFSET].arg, &p, 0);
 
 	  if (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0')
-	    {
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_strncpy (tmp_errmsg, grub_errmsg, GRUB_MAX_ERRMSG);
-		  return grub_error (grub_errno,
-				     N_("non-numeric or invalid keyfile offset `%s': %s"),
-				     state[OPTION_KEYFILE_OFFSET].arg, tmp_errmsg);
-		}
-	      else
-		return grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   N_("invalid keyfile offset `%s': non-numeric"
-				      " characters at end of number"),
-				   state[OPTION_KEYFILE_OFFSET].arg);
-	    }
+	    return grub_error (grub_errno,
+			       N_("non-numeric or invalid keyfile offset `%s'"),
+			       state[OPTION_KEYFILE_OFFSET].arg);
 	}
 
       if (state[OPTION_KEYFILE_SIZE].set) /* keyfile-size */
@@ -1221,28 +1209,17 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  keyfile_size = grub_strtoull (state[OPTION_KEYFILE_SIZE].arg, &p, 0);
 
 	  if (state[OPTION_KEYFILE_SIZE].arg[0] == '\0' || *p != '\0')
-	    {
-	      if (grub_errno != GRUB_ERR_NONE)
-		{
-		  grub_strncpy (tmp_errmsg, grub_errmsg, GRUB_MAX_ERRMSG);
-		  return grub_error (grub_errno,
-				     N_("non-numeric or invalid keyfile offset `%s': %s"),
-				     state[OPTION_KEYFILE_SIZE].arg, tmp_errmsg);
-		}
-	      else
-		return grub_error (GRUB_ERR_BAD_ARGUMENT,
-				   N_("invalid keyfile offset `%s': non-numeric"
-				      " characters at end of number"),
-				   state[OPTION_KEYFILE_SIZE].arg);
-	    }
+	    return grub_error (grub_errno,
+			       N_("non-numeric or invalid keyfile size `%s'"),
+			       state[OPTION_KEYFILE_SIZE].arg);
+
+	  if (keyfile_size == 0)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
 
 	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
 	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
 			       N_("key file size exceeds maximum (%d)"),
 			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
-
-	  if (keyfile_size == 0)
-	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
 	}
 
       keyfile = grub_file_open (state[OPTION_KEYFILE].arg,
-- 
2.34.1

[PATCH v3 1/5] cryptodisk: luks: Unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/disk/luks.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 46ae734ef..7f837d52c 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,7 +63,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
 			  grub_size_t blocknumbers);
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+luks_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   const char *iptr;
@@ -297,7 +297,7 @@ luks_recover_key (grub_disk_t source,
 }
 
 struct grub_cryptodisk_dev luks_crypto = {
-  .scan = configure_ciphers,
+  .scan = luks_scan,
   .recover_key = luks_recover_key
 };
 
-- 
2.34.1

[PATCH v3 2/5] cryptodisk: geli: Unify grub_cryptodisk_dev function names

[Glenn Washburn]

From: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>

Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
Reviewed-by: Patrick Steinhardt <ps@pks.im>
Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 grub-core/disk/geli.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 445a66878..91eb10122 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,7 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
 #endif
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
+geli_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   struct grub_geli_phdr header;
@@ -395,7 +395,7 @@ configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 }
 
 static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
+geli_recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -567,8 +567,8 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
 }
 
 struct grub_cryptodisk_dev geli_crypto = {
-  .scan = configure_ciphers,
-  .recover_key = recover_key
+  .scan = geli_scan,
+  .recover_key = geli_recover_key
 };
 
 GRUB_MOD_INIT (geli)
-- 
2.34.1

[PATCH v3 3/5] cryptodisk: Add options to cryptomount to support keyfiles

[Glenn Washburn]

From: John Lane <john@lane.uk.net>

Add the options --key-file, --keyfile-offset, and --keyfile-size to
cryptomount and code to put read the requested key file data and pass
via the cargs struct. Note, key file data is for all intents and purposes
equivalent to a password given to cryptomount. So there is no need to
enable support for key files in the various crypto backends (eg. LUKS1)
because the key data is passed just as if it were a password.

Signed-off-by: John Lane <john@lane.uk.net>
GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
development@efficientek.com: rebase and rework to use cryptomount arg passing,
  minor fixes, improve commit message
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 81 ++++++++++++++++++++++++++++++++++++-
 include/grub/cryptodisk.h   |  2 +
 include/grub/file.h         |  2 +
 3 files changed, 84 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9f5dc7acb..1198cb0e6 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -42,6 +42,9 @@ static const struct grub_arg_option options[] =
     {"all", 'a', 0, N_("Mount all."), 0, 0},
     {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
     {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
+    {"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},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -1172,6 +1175,80 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       cargs.key_len = grub_strlen (state[3].arg);
     }
 
+  if (state[4].set) /* keyfile */
+    {
+      const char *p = NULL;
+      grub_file_t keyfile;
+      unsigned long long keyfile_offset = 0, keyfile_size = 0;
+
+      if (state[5].set) /* keyfile-offset */
+	{
+	  grub_errno = GRUB_ERR_NONE;
+	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
+
+	  if (state[5].arg[0] == '\0' || *p != '\0')
+	    return grub_error (grub_errno,
+			       N_("non-numeric or invalid keyfile offset `%s'"),
+			       state[5].arg);
+	}
+
+      if (state[6].set) /* keyfile-size */
+	{
+	  grub_errno = GRUB_ERR_NONE;
+	  keyfile_size = grub_strtoull (state[6].arg, &p, 0);
+
+	  if (state[6].arg[0] == '\0' || *p != '\0')
+	    return grub_error (grub_errno,
+			       N_("non-numeric or invalid keyfile size `%s'"),
+			       state[6].arg);
+
+	  if (keyfile_size == 0)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
+
+	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
+	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			       N_("key file size exceeds maximum (%d)"),
+			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
+	}
+
+      keyfile = grub_file_open (state[4].arg,
+				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
+      if (keyfile == NULL)
+	return grub_errno;
+
+      if (keyfile_offset > keyfile->size)
+	return grub_error (GRUB_ERR_OUT_OF_RANGE,
+			   N_("Keyfile offset, %llu, is greater than"
+			      "keyfile size, %" PRIuGRUB_UINT64_T),
+			   keyfile_offset, keyfile->size);
+
+      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == (grub_off_t) -1)
+	return grub_errno;
+
+      if (keyfile_size != 0)
+	{
+	  if (keyfile_size > (keyfile->size - keyfile_offset))
+	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
+			       N_("keyfile is too small: requested %llu bytes,"
+				  " but the file only has %" PRIuGRUB_UINT64_T
+				  " bytes left at offset %llu"),
+			       keyfile_size,
+			       (grub_size_t) (keyfile->size - keyfile_offset),
+			       keyfile_offset);
+
+	  cargs.key_len = keyfile_size;
+	}
+      else
+	cargs.key_len = keyfile->size - keyfile_offset;
+
+      cargs.key_data = grub_malloc (cargs.key_len);
+      if (cargs.key_data == NULL)
+	return GRUB_ERR_OUT_OF_MEMORY;
+
+      if (grub_file_read (keyfile, cargs.key_data, cargs.key_len) != (grub_ssize_t) cargs.key_len)
+	return grub_error (GRUB_ERR_FILE_READ_ERROR, (N_("failed to read key file")));
+    }
+
   if (state[0].set) /* uuid */
     {
       int found_uuid;
@@ -1384,7 +1461,9 @@ GRUB_MOD_INIT (cryptodisk)
 {
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-			      N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
+			      N_("[ [-p password] | [-k keyfile"
+				 " [-O keyoffset] [-S keysize] ] ]"
+				 " <SOURCE|-u UUID|-a|-b>"),
 			      N_("Mount a crypto device."), options);
   grub_procfs_register ("luks_script", &luks_script);
 }
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index c6524c9ea..467065f00 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -61,6 +61,8 @@ typedef enum
 #define GRUB_CRYPTODISK_MAX_KEYLEN 128
 #define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
 
+#define GRUB_CRYPTODISK_MAX_KEYFILE_SIZE 8192
+
 struct grub_cryptodisk;
 
 typedef gcry_err_code_t
diff --git a/include/grub/file.h b/include/grub/file.h
index 31567483c..d53ee8edd 100644
--- a/include/grub/file.h
+++ b/include/grub/file.h
@@ -90,6 +90,8 @@ enum grub_file_type
     GRUB_FILE_TYPE_FONT,
     /* File holding encryption key for encrypted ZFS.  */
     GRUB_FILE_TYPE_ZFS_ENCRYPTION_KEY,
+    /* File holding the encryption key */
+    GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY,
     /* File we open n grub-fstest.  */
     GRUB_FILE_TYPE_FSTEST,
     /* File we open n grub-mount.  */
-- 
2.34.1

[PATCH v3 4/5] cryptodisk: Use enum constants as indexes into cryptomount option array

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 49 +++++++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 19 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 1198cb0e6..e2b8636e4 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -35,6 +35,17 @@ GRUB_MOD_LICENSE ("GPLv3+");
 
 grub_cryptodisk_dev_t grub_cryptodisk_list;
 
+enum
+  {
+    OPTION_UUID,
+    OPTION_ALL,
+    OPTION_BOOT,
+    OPTION_PASSWORD,
+    OPTION_KEYFILE,
+    OPTION_KEYFILE_OFFSET,
+    OPTION_KEYFILE_SIZE
+  };
+
 static const struct grub_arg_option options[] =
   {
     {"uuid", 'u', 0, N_("Mount by UUID."), 0, 0},
@@ -1163,44 +1174,44 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
   struct grub_arg_list *state = ctxt->state;
   struct grub_cryptomount_args cargs = {0};
 
-  if (argc < 1 && !state[1].set && !state[2].set)
+  if (argc < 1 && !state[OPTION_ALL].set && !state[OPTION_BOOT].set)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
   if (grub_cryptodisk_list == NULL)
     return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
 
-  if (state[3].set) /* password */
+  if (state[OPTION_PASSWORD].set) /* password */
     {
-      cargs.key_data = (grub_uint8_t *) state[3].arg;
-      cargs.key_len = grub_strlen (state[3].arg);
+      cargs.key_data = (grub_uint8_t *) state[OPTION_PASSWORD].arg;
+      cargs.key_len = grub_strlen (state[OPTION_PASSWORD].arg);
     }
 
-  if (state[4].set) /* keyfile */
+  if (state[OPTION_KEYFILE].set) /* keyfile */
     {
       const char *p = NULL;
       grub_file_t keyfile;
       unsigned long long keyfile_offset = 0, keyfile_size = 0;
 
-      if (state[5].set) /* keyfile-offset */
+      if (state[OPTION_KEYFILE_OFFSET].set) /* keyfile-offset */
 	{
 	  grub_errno = GRUB_ERR_NONE;
-	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
+	  keyfile_offset = grub_strtoull (state[OPTION_KEYFILE_OFFSET].arg, &p, 0);
 
-	  if (state[5].arg[0] == '\0' || *p != '\0')
+	  if (state[OPTION_KEYFILE_OFFSET].arg[0] == '\0' || *p != '\0')
 	    return grub_error (grub_errno,
 			       N_("non-numeric or invalid keyfile offset `%s'"),
-			       state[5].arg);
+			       state[OPTION_KEYFILE_OFFSET].arg);
 	}
 
-      if (state[6].set) /* keyfile-size */
+      if (state[OPTION_KEYFILE_SIZE].set) /* keyfile-size */
 	{
 	  grub_errno = GRUB_ERR_NONE;
-	  keyfile_size = grub_strtoull (state[6].arg, &p, 0);
+	  keyfile_size = grub_strtoull (state[OPTION_KEYFILE_SIZE].arg, &p, 0);
 
-	  if (state[6].arg[0] == '\0' || *p != '\0')
+	  if (state[OPTION_KEYFILE_SIZE].arg[0] == '\0' || *p != '\0')
 	    return grub_error (grub_errno,
 			       N_("non-numeric or invalid keyfile size `%s'"),
-			       state[6].arg);
+			       state[OPTION_KEYFILE_SIZE].arg);
 
 	  if (keyfile_size == 0)
 	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
@@ -1211,7 +1222,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
 	}
 
-      keyfile = grub_file_open (state[4].arg,
+      keyfile = grub_file_open (state[OPTION_KEYFILE].arg,
 				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
       if (keyfile == NULL)
 	return grub_errno;
@@ -1249,7 +1260,7 @@ 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[0].set) /* uuid */
+  if (state[OPTION_UUID].set) /* uuid */
     {
       int found_uuid;
       grub_cryptodisk_t dev;
@@ -1262,7 +1273,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      cargs.check_boot = state[2].set;
+      cargs.check_boot = state[OPTION_BOOT].set;
       cargs.search_uuid = args[0];
       found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
 
@@ -1280,9 +1291,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	}
       return grub_errno;
     }
-  else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
+  else if (state[OPTION_ALL].set || (argc == 0 && state[OPTION_BOOT].set)) /* -a|-b */
     {
-      cargs.check_boot = state[2].set;
+      cargs.check_boot = state[OPTION_BOOT].set;
       grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
       return GRUB_ERR_NONE;
     }
@@ -1294,7 +1305,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       char *disklast = NULL;
       grub_size_t len;
 
-      cargs.check_boot = state[2].set;
+      cargs.check_boot = state[OPTION_BOOT].set;
       diskname = args[0];
       len = grub_strlen (diskname);
       if (len && diskname[0] == '(' && diskname[len - 1] == ')')
-- 
2.34.1

[PATCH v3 5/5] docs: Add documentation on keyfile option to cryptomount

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 docs/grub.texi | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 50ef28edd..0a8057482 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4490,11 +4490,15 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See command @command{hashsum}
 @node cryptomount
 @subsection cryptomount
 
-@deffn Command cryptomount [@option{-p} password] device|@option{-u} uuid|@option{-a}|@option{-b}
-Setup access to encrypted device. If @option{-p} is not given, a passphrase
-is requested interactively. Otherwise, the given @var{password} will be used and
-no passphrase will be requested interactively.
-Option @var{device} configures specific grub device
+@deffn Command cryptomount [ [@option{-p} password] | [@option{-k} keyfile [@option{-O} keyoffset] [@option{-S} keysize] ] ] 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.
+
+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.
-- 
2.34.1

Re: [PATCH v3 3/5] cryptodisk: Add options to cryptomount to support keyfiles

[Daniel Kiper]

On Fri, May 20, 2022 at 02:32:17PM -0500, Glenn Washburn wrote:
> From: John Lane <john@lane.uk.net>
>
> Add the options --key-file, --keyfile-offset, and --keyfile-size to
> cryptomount and code to put read the requested key file data and pass
> via the cargs struct. Note, key file data is for all intents and purposes
> equivalent to a password given to cryptomount. So there is no need to
> enable support for key files in the various crypto backends (eg. LUKS1)
> because the key data is passed just as if it were a password.
>
> Signed-off-by: John Lane <john@lane.uk.net>
> GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> development@efficientek.com: rebase and rework to use cryptomount arg passing,
>   minor fixes, improve commit message
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 81 ++++++++++++++++++++++++++++++++++++-
>  include/grub/cryptodisk.h   |  2 +
>  include/grub/file.h         |  2 +
>  3 files changed, 84 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 9f5dc7acb..1198cb0e6 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,9 @@ static const struct grub_arg_option options[] =
>      {"all", 'a', 0, N_("Mount all."), 0, 0},
>      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
>      {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
> +    {"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},
>      {0, 0, 0, 0, 0, 0}
>    };
>
> @@ -1172,6 +1175,80 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        cargs.key_len = grub_strlen (state[3].arg);
>      }
>
> +  if (state[4].set) /* keyfile */
> +    {
> +      const char *p = NULL;
> +      grub_file_t keyfile;
> +      unsigned long long keyfile_offset = 0, keyfile_size = 0;
> +
> +      if (state[5].set) /* keyfile-offset */
> +	{
> +	  grub_errno = GRUB_ERR_NONE;
> +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> +
> +	  if (state[5].arg[0] == '\0' || *p != '\0')
> +	    return grub_error (grub_errno,
> +			       N_("non-numeric or invalid keyfile offset `%s'"),
> +			       state[5].arg);
> +	}
> +
> +      if (state[6].set) /* keyfile-size */
> +	{
> +	  grub_errno = GRUB_ERR_NONE;
> +	  keyfile_size = grub_strtoull (state[6].arg, &p, 0);
> +
> +	  if (state[6].arg[0] == '\0' || *p != '\0')
> +	    return grub_error (grub_errno,
> +			       N_("non-numeric or invalid keyfile size `%s'"),
> +			       state[6].arg);
> +
> +	  if (keyfile_size == 0)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
> +
> +	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +			       N_("key file size exceeds maximum (%d)"),
> +			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> +	}
> +
> +      keyfile = grub_file_open (state[4].arg,
> +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> +      if (keyfile == NULL)
> +	return grub_errno;
> +
> +      if (keyfile_offset > keyfile->size)
> +	return grub_error (GRUB_ERR_OUT_OF_RANGE,
> +			   N_("Keyfile offset, %llu, is greater than"
> +			      "keyfile size, %" PRIuGRUB_UINT64_T),
> +			   keyfile_offset, keyfile->size);
> +
> +      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == (grub_off_t) -1)
> +	return grub_errno;
> +
> +      if (keyfile_size != 0)
> +	{
> +	  if (keyfile_size > (keyfile->size - keyfile_offset))
> +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> +			       N_("keyfile is too small: requested %llu bytes,"
> +				  " but the file only has %" PRIuGRUB_UINT64_T

If you cast subtraction result to grub_size_t below then PRIuGRUB_UINT64_T
should be replaced with PRIuGRUB_SIZE. Or maybe it would be safer if we
do s/grub_size_t/grub_off_t/ instead of changing format string. This does
not matter today but...

Otherwise all patches LGTM -> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
So, I can fix the issue mentioned above for you before push.

Daniel

Re: [PATCH v3 3/5] cryptodisk: Add options to cryptomount to support keyfiles

[Glenn Washburn]

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

> On Fri, May 20, 2022 at 02:32:17PM -0500, Glenn Washburn wrote:
> > From: John Lane <john@lane.uk.net>
> >
> > Add the options --key-file, --keyfile-offset, and --keyfile-size to
> > cryptomount and code to put read the requested key file data and pass
> > via the cargs struct. Note, key file data is for all intents and purposes
> > equivalent to a password given to cryptomount. So there is no need to
> > enable support for key files in the various crypto backends (eg. LUKS1)
> > because the key data is passed just as if it were a password.
> >
> > Signed-off-by: John Lane <john@lane.uk.net>
> > GNUtoo@cyberdimension.org: rebase, patch split, small fixes, commit message
> > Signed-off-by: Denis 'GNUtoo' Carikli <GNUtoo@cyberdimension.org>
> > development@efficientek.com: rebase and rework to use cryptomount arg passing,
> >   minor fixes, improve commit message
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 81 ++++++++++++++++++++++++++++++++++++-
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  3 files changed, 84 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 9f5dc7acb..1198cb0e6 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -42,6 +42,9 @@ static const struct grub_arg_option options[] =
> >      {"all", 'a', 0, N_("Mount all."), 0, 0},
> >      {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> >      {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
> > +    {"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},
> >      {0, 0, 0, 0, 0, 0}
> >    };
> >
> > @@ -1172,6 +1175,80 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >        cargs.key_len = grub_strlen (state[3].arg);
> >      }
> >
> > +  if (state[4].set) /* keyfile */
> > +    {
> > +      const char *p = NULL;
> > +      grub_file_t keyfile;
> > +      unsigned long long keyfile_offset = 0, keyfile_size = 0;
> > +
> > +      if (state[5].set) /* keyfile-offset */
> > +	{
> > +	  grub_errno = GRUB_ERR_NONE;
> > +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> > +
> > +	  if (state[5].arg[0] == '\0' || *p != '\0')
> > +	    return grub_error (grub_errno,
> > +			       N_("non-numeric or invalid keyfile offset `%s'"),
> > +			       state[5].arg);
> > +	}
> > +
> > +      if (state[6].set) /* keyfile-size */
> > +	{
> > +	  grub_errno = GRUB_ERR_NONE;
> > +	  keyfile_size = grub_strtoull (state[6].arg, &p, 0);
> > +
> > +	  if (state[6].arg[0] == '\0' || *p != '\0')
> > +	    return grub_error (grub_errno,
> > +			       N_("non-numeric or invalid keyfile size `%s'"),
> > +			       state[6].arg);
> > +
> > +	  if (keyfile_size == 0)
> > +	    return grub_error (GRUB_ERR_OUT_OF_RANGE, N_("key file size is 0"));
> > +
> > +	  if (keyfile_size > GRUB_CRYPTODISK_MAX_KEYFILE_SIZE)
> > +	    return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > +			       N_("key file size exceeds maximum (%d)"),
> > +			       GRUB_CRYPTODISK_MAX_KEYFILE_SIZE);
> > +	}
> > +
> > +      keyfile = grub_file_open (state[4].arg,
> > +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> > +      if (keyfile == NULL)
> > +	return grub_errno;
> > +
> > +      if (keyfile_offset > keyfile->size)
> > +	return grub_error (GRUB_ERR_OUT_OF_RANGE,
> > +			   N_("Keyfile offset, %llu, is greater than"
> > +			      "keyfile size, %" PRIuGRUB_UINT64_T),
> > +			   keyfile_offset, keyfile->size);
> > +
> > +      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == (grub_off_t) -1)
> > +	return grub_errno;
> > +
> > +      if (keyfile_size != 0)
> > +	{
> > +	  if (keyfile_size > (keyfile->size - keyfile_offset))
> > +	    return grub_error (GRUB_ERR_FILE_READ_ERROR,
> > +			       N_("keyfile is too small: requested %llu bytes,"
> > +				  " but the file only has %" PRIuGRUB_UINT64_T
> 
> If you cast subtraction result to grub_size_t below then PRIuGRUB_UINT64_T
> should be replaced with PRIuGRUB_SIZE. Or maybe it would be safer if we
> do s/grub_size_t/grub_off_t/ instead of changing format string. This does
> not matter today but...
> 
> Otherwise all patches LGTM -> Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>
> So, I can fix the issue mentioned above for you before push.

Sounds great. Glad to finally get this series off my plate.

Glenn