[PATCH 0/4] Cryptomount keyfile support

[PATCH 0/4] Cryptomount keyfile support

[Glenn Washburn]

I'm breaking the keyfile and detached header patch series into two series.
I think that the detached header patches can be improved and I don't want
to hold up the more trivial keyfile support patches. This series is patches
#1, #2, #5, and a split of #7, the documentation patch.

The first two patches are unchanged. The third contains changes addressing
comments by Daniel on the v9 keyfile and detached header patch series. And
the last patch is the same #7 except removing reference to the detached
header option.

Glenn

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

Glenn Washburn (1):
  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 | 86 ++++++++++++++++++++++++++++++++++++-
 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, 104 insertions(+), 12 deletions(-)

Interdiff:
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 45f6d7231..19af4fa49 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1179,33 +1179,29 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
     {
       const char *p = NULL;
       grub_file_t keyfile;
-      int keyfile_offset;
-      grub_size_t keyfile_size = 0;
-
+      unsigned long long keyfile_offset = 0, keyfile_size = 0;
 
       if (state[5].set) /* keyfile-offset */
 	{
-	  keyfile_offset = grub_strtoul (state[5].arg, &p, 0);
+	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
 
 	  if (grub_errno != GRUB_ERR_NONE)
 	    return grub_errno;
 
-	  if (*p != '\0')
+	  if (state[5].arg[0] == '\0' || *p != '\0')
 	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
-			       N_("unrecognized number"));
-	}
-      else
-	{
-	  keyfile_offset = 0;
+			       N_("non-numeric or invalid keyfile offset `%s'"),
+			       state[5].arg);
 	}
 
       if (state[6].set) /* keyfile-size */
 	{
 	  keyfile_size = grub_strtoul (state[6].arg, &p, 0);
 
-	  if (*p != '\0')
+	  if (state[6].arg[0] == '\0' || *p != '\0')
 	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
-			       N_("unrecognized number"));
+			       N_("non-numeric or invalid keyfile size `%s'"),
+			       state[6].arg);
 
 	  if (grub_errno != GRUB_ERR_NONE)
 	    return grub_errno;
@@ -1224,16 +1220,23 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       if (keyfile == NULL)
 	return grub_errno;
 
-      if (grub_file_seek (keyfile, keyfile_offset) == (grub_off_t)-1)
+      if (keyfile_offset > keyfile->size)
+	{
+	  keyfile_offset = keyfile->size;
+	  grub_dprintf ("cryptodisk","Keyfile offset, %llu, is greater than"
+				     "keyfile size, %" PRIuGRUB_UINT64_T "\n",
+				     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 %" PRIuGRUB_SIZE " bytes, "
-				  "but the file only has %" PRIuGRUB_UINT64_T
+			       N_("keyfile is too small: requested %llu bytes,"
+				  " but the file only has %" PRIuGRUB_UINT64_T
 				  " bytes"),
 			       keyfile_size,
 			       keyfile->size);
@@ -1241,9 +1244,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  cargs.key_len = keyfile_size;
 	}
       else
-	{
-	  cargs.key_len = keyfile->size - keyfile_offset;
-	}
+	cargs.key_len = keyfile->size - keyfile_offset;
 
       cargs.key_data = grub_malloc (cargs.key_len);
       if (cargs.key_data == NULL)
-- 
2.34.1

[PATCH 1/4] 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>
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 2/4] 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>
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 3/4] 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 | 86 ++++++++++++++++++++++++++++++++++++-
 include/grub/cryptodisk.h   |  2 +
 include/grub/file.h         |  2 +
 3 files changed, 89 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9f5dc7acb..19af4fa49 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,85 @@ 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 */
+	{
+	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  if (state[5].arg[0] == '\0' || *p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("non-numeric or invalid keyfile offset `%s'"),
+			       state[5].arg);
+	}
+
+      if (state[6].set) /* keyfile-size */
+	{
+	  keyfile_size = grub_strtoul (state[6].arg, &p, 0);
+
+	  if (state[6].arg[0] == '\0' || *p != '\0')
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+			       N_("non-numeric or invalid keyfile size `%s'"),
+			       state[6].arg);
+
+	  if (grub_errno != GRUB_ERR_NONE)
+	    return grub_errno;
+
+	  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[4].arg,
+				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
+      if (keyfile == NULL)
+	return grub_errno;
+
+      if (keyfile_offset > keyfile->size)
+	{
+	  keyfile_offset = keyfile->size;
+	  grub_dprintf ("cryptodisk","Keyfile offset, %llu, is greater than"
+				     "keyfile size, %" PRIuGRUB_UINT64_T "\n",
+				     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"),
+			       keyfile_size,
+			       keyfile->size);
+
+	  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_("reading key file")));
+    }
+
   if (state[0].set) /* uuid */
     {
       int found_uuid;
@@ -1384,7 +1466,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 4/4] docs: Add documentation on keyfile option to cryptomount

[Glenn Washburn]

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

diff --git a/docs/grub.texi b/docs/grub.texi
index 5de94d062..3f01871f7 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4356,11 +4356,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 3/4] cryptodisk: Add options to cryptomount to support keyfiles

[Daniel Kiper]

On Fri, May 06, 2022 at 03:45:59AM -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 | 86 ++++++++++++++++++++++++++++++++++++-
>  include/grub/cryptodisk.h   |  2 +
>  include/grub/file.h         |  2 +
>  3 files changed, 89 insertions(+), 1 deletion(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 9f5dc7acb..19af4fa49 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,85 @@ 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 */
> +	{
> +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);

Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?

> +
> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;

Could you explain why do you think this is needed? I think this check is
redundant. "man strtoul" says:

  If endptr is not NULL, strtoul() stores the address of the first invalid
  character in *endptr. If there were no digits at all, strtoul() stores the
  original value of nptr in *endptr (and returns 0). In particular, if *nptr
  is not '\0' but **endptr is '\0' on return, the entire string is valid.

> +	  if (state[5].arg[0] == '\0' || *p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("non-numeric or invalid keyfile offset `%s'"),
> +			       state[5].arg);
> +	}
> +
> +      if (state[6].set) /* keyfile-size */
> +	{
> +	  keyfile_size = grub_strtoul (state[6].arg, &p, 0);
> +
> +	  if (state[6].arg[0] == '\0' || *p != '\0')
> +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> +			       N_("non-numeric or invalid keyfile size `%s'"),
> +			       state[6].arg);
> +
> +	  if (grub_errno != GRUB_ERR_NONE)
> +	    return grub_errno;

Ditto.

> +	  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[4].arg,
> +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> +      if (keyfile == NULL)
> +	return grub_errno;
> +
> +      if (keyfile_offset > keyfile->size)
> +	{
> +	  keyfile_offset = keyfile->size;

I do not understand this keyfile_offset clamp. Could you enlighten me?
In general I think you should fail here.

> +	  grub_dprintf ("cryptodisk","Keyfile offset, %llu, is greater than"
> +				     "keyfile size, %" PRIuGRUB_UINT64_T "\n",
> +				     keyfile_offset, keyfile->size);

Both keyfile_offset and keyfile->size are the same here.

> +	}
> +
> +      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == (grub_off_t) -1)
> +	return grub_errno;
> +
> +      if (keyfile_size > 0)

I think "!= 0" would be more natural here.

> +	{
> +	  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"),
> +			       keyfile_size,
> +			       keyfile->size);

This error message is confusing. I think "keyfile->size" should be "keyfile->size - 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_("reading key file")));

s/reading key file/cannot read key file/?

Daniel

Re: [PATCH 4/4] docs: Add documentation on keyfile option to cryptomount

[Daniel Kiper]

On Fri, May 06, 2022 at 03:46:00AM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>

Reviewed-by: Daniel Kiper <daniel.kiper@oracle.com>

Daniel

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

[Glenn Washburn]

On Thu, 12 May 2022 19:45:48 +0200
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, May 06, 2022 at 03:45:59AM -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 | 86 ++++++++++++++++++++++++++++++++++++-
> >  include/grub/cryptodisk.h   |  2 +
> >  include/grub/file.h         |  2 +
> >  3 files changed, 89 insertions(+), 1 deletion(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 9f5dc7acb..19af4fa49 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,85 @@ 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 */
> > +	{
> > +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> 
> Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?

I was thinking the allowing the largest possible offset might be useful
to allow for using unallocated space at the end of large block devices.
This could still be done with smaller offsets, by using the blocklist
syntax to create a "file" that starts near the end and then use a much
smaller offset. Why don't you like it as is?

> > +
> > +	  if (grub_errno != GRUB_ERR_NONE)
> > +	    return grub_errno;
> 
> Could you explain why do you think this is needed? I think this check is
> redundant. "man strtoul" says:
> 
>   If endptr is not NULL, strtoul() stores the address of the first invalid
>   character in *endptr. If there were no digits at all, strtoul() stores the
>   original value of nptr in *endptr (and returns 0). In particular, if *nptr
>   is not '\0' but **endptr is '\0' on return, the entire string is valid.

Yep, just checked the implementation, you're right, this isn't needed.
I didn't realize that endptr would be set in the case of an overflow of
an otherwise valid number.

> > +	  if (state[5].arg[0] == '\0' || *p != '\0')
> > +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +			       N_("non-numeric or invalid keyfile offset `%s'"),
> > +			       state[5].arg);
> > +	}
> > +
> > +      if (state[6].set) /* keyfile-size */
> > +	{
> > +	  keyfile_size = grub_strtoul (state[6].arg, &p, 0);
> > +
> > +	  if (state[6].arg[0] == '\0' || *p != '\0')
> > +	    return grub_error (GRUB_ERR_BAD_ARGUMENT,
> > +			       N_("non-numeric or invalid keyfile size `%s'"),
> > +			       state[6].arg);
> > +
> > +	  if (grub_errno != GRUB_ERR_NONE)
> > +	    return grub_errno;
> 
> Ditto.
> 
> > +	  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[4].arg,
> > +				GRUB_FILE_TYPE_CRYPTODISK_ENCRYPTION_KEY);
> > +      if (keyfile == NULL)
> > +	return grub_errno;
> > +
> > +      if (keyfile_offset > keyfile->size)
> > +	{
> > +	  keyfile_offset = keyfile->size;
> 
> I do not understand this keyfile_offset clamp. Could you enlighten me?
> In general I think you should fail here.

Hmm, yes, the clamp just makes the keyfile effectively zero bytes long,
but that makes little sense.

> > +	  grub_dprintf ("cryptodisk","Keyfile offset, %llu, is greater than"
> > +				     "keyfile size, %" PRIuGRUB_UINT64_T "\n",
> > +				     keyfile_offset, keyfile->size);
> 
> Both keyfile_offset and keyfile->size are the same here.

This should've been before the clamp.

> > +	}
> > +
> > +      if (grub_file_seek (keyfile, (grub_off_t) keyfile_offset) == (grub_off_t) -1)
> > +	return grub_errno;
> > +
> > +      if (keyfile_size > 0)
> 
> I think "!= 0" would be more natural here.

Sounds good.

> > +	{
> > +	  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"),
> > +			       keyfile_size,
> > +			       keyfile->size);
> 
> This error message is confusing. I think "keyfile->size" should be "keyfile->size - keyfile_offset".

Good catch. I think it could be still confusing. I'll think of a better
way to say this.

> > +	  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_("reading key file")));
> 
> s/reading key file/cannot read key file/?

Sure.

Glenn

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

[Daniel Kiper]

On Thu, May 12, 2022 at 01:53:31PM -0500, Glenn Washburn wrote:
> On Thu, 12 May 2022 19:45:48 +0200
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Fri, May 06, 2022 at 03:45:59AM -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 | 86 ++++++++++++++++++++++++++++++++++++-
> > >  include/grub/cryptodisk.h   |  2 +
> > >  include/grub/file.h         |  2 +
> > >  3 files changed, 89 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 9f5dc7acb..19af4fa49 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,85 @@ 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;

I think keyfile_size should be "unsigned long" if you use grub_strtoul() below.

> > > +
> > > +      if (state[5].set) /* keyfile-offset */
> > > +	{
> > > +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> >
> > Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?
>
> I was thinking the allowing the largest possible offset might be useful
> to allow for using unallocated space at the end of large block devices.
> This could still be done with smaller offsets, by using the blocklist
> syntax to create a "file" that starts near the end and then use a much
> smaller offset. Why don't you like it as is?

I would not say I do not like it. I just want to understand discrepancy
between this conversion and one below. If you need "unsigned long long"
go ahead with it. Though I would suggest to check the value does not
exceed maximum value of target type. I think GRUB_TYPE_U_MAX() can be
useful here. Same applies to conversion below.

Daniel

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

[Glenn Washburn]

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

> On Thu, May 12, 2022 at 01:53:31PM -0500, Glenn Washburn wrote:
> > On Thu, 12 May 2022 19:45:48 +0200
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Fri, May 06, 2022 at 03:45:59AM -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 | 86 ++++++++++++++++++++++++++++++++++++-
> > > >  include/grub/cryptodisk.h   |  2 +
> > > >  include/grub/file.h         |  2 +
> > > >  3 files changed, 89 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > > index 9f5dc7acb..19af4fa49 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,85 @@ 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;
> 
> I think keyfile_size should be "unsigned long" if you use grub_strtoul() below.

I made them both unsigned long long because I wanted both using
grub_strtoull().

> > > > +
> > > > +      if (state[5].set) /* keyfile-offset */
> > > > +	{
> > > > +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> > >
> > > Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?
> >
> > I was thinking the allowing the largest possible offset might be useful
> > to allow for using unallocated space at the end of large block devices.
> > This could still be done with smaller offsets, by using the blocklist
> > syntax to create a "file" that starts near the end and then use a much
> > smaller offset. Why don't you like it as is?
> 
> I would not say I do not like it. I just want to understand discrepancy
> between this conversion and one below. If you need "unsigned long long"
> go ahead with it. Though I would suggest to check the value does not
> exceed maximum value of target type. I think GRUB_TYPE_U_MAX() can be
> useful here. Same applies to conversion below.

Ahh, now I see what you're getting at. I hadn't noticed the difference.
I meant to change the other one to grub_strtoull, but forgot. I'll do
that.

Glenn

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

[Glenn Washburn]

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

> On Thu, May 12, 2022 at 01:53:31PM -0500, Glenn Washburn wrote:
> > On Thu, 12 May 2022 19:45:48 +0200
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Fri, May 06, 2022 at 03:45:59AM -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 | 86 ++++++++++++++++++++++++++++++++++++-
> > > >  include/grub/cryptodisk.h   |  2 +
> > > >  include/grub/file.h         |  2 +
> > > >  3 files changed, 89 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > > index 9f5dc7acb..19af4fa49 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,85 @@ 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;
> 
> I think keyfile_size should be "unsigned long" if you use grub_strtoul() below.
> 
> > > > +
> > > > +      if (state[5].set) /* keyfile-offset */
> > > > +	{
> > > > +	  keyfile_offset = grub_strtoull (state[5].arg, &p, 0);
> > >
> > > Hmmm... Could not you use grub_strtoul() instead of grub_strtoull()?
> >
> > I was thinking the allowing the largest possible offset might be useful
> > to allow for using unallocated space at the end of large block devices.
> > This could still be done with smaller offsets, by using the blocklist
> > syntax to create a "file" that starts near the end and then use a much
> > smaller offset. Why don't you like it as is?
> 
> I would not say I do not like it. I just want to understand discrepancy
> between this conversion and one below. If you need "unsigned long long"
> go ahead with it. Though I would suggest to check the value does not
> exceed maximum value of target type. I think GRUB_TYPE_U_MAX() can be
> useful here. Same applies to conversion below.

I sent the last response too hastily. In case its not clear, I'm using
unsigned long long because that is the return type of grub_strtoull().
Once I change from grub_strtoul() -> grub_strtoull(), I don't think need
the suggested check. I'll submit the updated code and see if you think
otherwise.

Glenn