[PATCH 0/3] Refactor/improve cryptomount data passing to crypto modules

[PATCH 0/3] Refactor/improve cryptomount data passing to crypto modules

[Glenn Washburn]

This patch series refactors the way cryptomount passes data to the crypto
modules. Currently, the method has been by global variable and function call
argument, neither of which are ideal. This method passes data via a
grub_cryptomount_args struct, which can be added to over time as opposed to
continually adding arguments to the cryptodisk recover_key (as is being
proposed in the keyfile and detached header patches).

The infrastructure is implemented in patch #1 along with adding a new -p
parameter to cryptomount partly as an example to show how a password would be
passed to the crypto module backends. The backends do nothing with this data
in this patch, but print a message saying that sending a password is
unimplemented.

Patch #2 takes advantage of this new data passing mechanism to refactor the
essentially duplicated code in each crypto backend module for inputting the
password and puts that functionality in the cryptodisk code. Conceptually,
the crypto backends should not be getting user input anyway.

Finally patch #3, gets rid of some long time globals in cryptodisk, moving them
into the passed struct.

My intention is for this patch series to lay the foundation for an improved
patch series providing detached header and keyfile support (I already have
the series updated and ready to send once this is accepted). I also believe
tha this will somewhat simplify the patch series by James Bottomley in
passing secrets to the crypto backends.

Glenn

Glenn Washburn (3):
  cryptodisk: Add infrastructure to pass data from cryptomount to
    cryptodisk modules
  cryptodisk: Refactor password input out of crypto dev modules into
    cryptodisk
  cryptodisk: Move global variables into grub_cryptomount_args struct

 grub-core/disk/cryptodisk.c | 109 ++++++++++++++++++++++++------------
 grub-core/disk/geli.c       |  24 ++------
 grub-core/disk/luks.c       |  25 ++-------
 grub-core/disk/luks2.c      |  24 ++------
 include/grub/cryptodisk.h   |  12 ++++
 5 files changed, 102 insertions(+), 92 deletions(-)

-- 
2.27.0

[PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Glenn Washburn]

As an example, passing a password as a cryptomount argument is implemented.
However, the backends are not implemented, so testing this will return a not
implemented error.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++---------
 grub-core/disk/geli.c       |  4 ++++
 grub-core/disk/luks.c       |  4 ++++
 grub-core/disk/luks2.c      |  4 ++++
 include/grub/cryptodisk.h   |  8 ++++++++
 5 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..b966b19ab 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
     /* TRANSLATORS: It's still restricted to cryptodisks only.  */
     {"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},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
 }
 
 static grub_err_t
-grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
+grub_cryptodisk_scan_device_real (const char *name,
+				  grub_disk_t source,
+				  grub_cryptomount_args_t cargs)
 {
   grub_err_t err;
   grub_cryptodisk_t dev;
@@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
     if (!dev)
       continue;
     
+    *dev->cargs = *cargs;
     err = cr->recover_key (source, dev);
+    *dev->cargs = NULL;
     if (err)
     {
       cryptodisk_close (dev);
@@ -1080,10 +1085,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
 
 static int
 grub_cryptodisk_scan_device (const char *name,
-			     void *data __attribute__ ((unused)))
+			     void *data)
 {
   grub_err_t err;
   grub_disk_t source;
+  grub_cryptomount_args_t cargs = data;
 
   /* Try to open disk.  */
   source = grub_disk_open (name);
@@ -1093,7 +1099,7 @@ grub_cryptodisk_scan_device (const char *name,
       return 0;
     }
 
-  err = grub_cryptodisk_scan_device_real (name, source);
+  err = grub_cryptodisk_scan_device_real (name, source, cargs);
 
   grub_disk_close (source);
   
@@ -1106,12 +1112,19 @@ static grub_err_t
 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)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
+  if (state[3].set) /* password */
+    {
+      cargs.key_data = (grub_uint8_t *) state[3].arg;
+      cargs.key_len = grub_strlen(state[3].arg);
+    }
+
   have_it = 0;
-  if (state[0].set)
+  if (state[0].set) /* uuid */
     {
       grub_cryptodisk_t dev;
 
@@ -1125,18 +1138,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
       check_boot = state[2].set;
       search_uuid = args[0];
-      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
       search_uuid = NULL;
 
       if (!have_it)
 	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
       return GRUB_ERR_NONE;
     }
-  else if (state[1].set || (argc == 0 && state[2].set))
+  else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
     {
       search_uuid = NULL;
       check_boot = state[2].set;
-      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
       search_uuid = NULL;
       return GRUB_ERR_NONE;
     }
@@ -1178,7 +1191,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      err = grub_cryptodisk_scan_device_real (diskname, disk);
+      err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
 
       grub_disk_close (disk);
       if (disklast)
@@ -1317,7 +1330,7 @@ GRUB_MOD_INIT (cryptodisk)
 {
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-			      N_("SOURCE|-u UUID|-a|-b"),
+			      N_("[-p password] <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 2f34a35e6..0a7bd90da 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -414,6 +414,10 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
   grub_disk_addr_t sector;
   grub_err_t err;
 
+  /* Keyfiles are not implemented yet */
+  if (dev->cargs->key_data || dev->cargs->key_len)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
     return grub_error (GRUB_ERR_BUG, "cipher block is too long");
 
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 13103ea6a..e2a4a3bf5 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
   grub_size_t max_stripes = 1;
   char *tmp;
 
+  /* Keyfiles are not implemented yet */
+  if (dev->cargs->key_data || dev->cargs->key_len)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   err = grub_disk_read (source, 0, 0, sizeof (header), &header);
   if (err)
     return err;
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..e0de902c9 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -556,6 +556,10 @@ luks2_recover_key (grub_disk_t source,
   grub_json_t *json = NULL, keyslots;
   grub_err_t ret;
 
+  /* Keyfiles are not implemented yet */
+  if (crypt->cargs->key_data || crypt->cargs->key_len)
+     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
   ret = luks2_read_header (source, &header);
   if (ret)
     return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index dcf17fbb3..433c75426 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -66,6 +66,13 @@ typedef gcry_err_code_t
 (*grub_cryptodisk_rekey_func_t) (struct grub_cryptodisk *dev,
 				 grub_uint64_t zoneno);
 
+struct grub_cryptomount_args
+{
+  grub_uint8_t *key_data;
+  grub_size_t key_len;
+};
+typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
+
 struct grub_cryptodisk
 {
   struct grub_cryptodisk *next;
@@ -109,6 +116,7 @@ struct grub_cryptodisk
   grub_uint64_t last_rekey;
   int rekey_derived_size;
   grub_disk_addr_t partition_start;
+  grub_cryptomount_args_t cargs;
 };
 typedef struct grub_cryptodisk *grub_cryptodisk_t;
 
-- 
2.27.0

[PATCH 2/3] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk

[Glenn Washburn]

The crypto device modules should only be setting up the crypto devices and
not getting user input. This has the added benefit of simplifying the code
such that three essentially duplicate pieces of code are merged into one.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++-------
 grub-core/disk/geli.c       | 26 ++++---------------
 grub-core/disk/luks.c       | 27 +++----------------
 grub-core/disk/luks2.c      | 26 ++++---------------
 include/grub/cryptodisk.h   |  1 +
 5 files changed, 57 insertions(+), 75 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b966b19ab..b6cf1835d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
 				  grub_disk_t source,
 				  grub_cryptomount_args_t cargs)
 {
-  grub_err_t err;
+  grub_err_t ret = GRUB_ERR_NONE;
   grub_cryptodisk_t dev;
   grub_cryptodisk_dev_t cr;
+  int askpass = 0;
+  char *part = NULL;
 
   dev = grub_cryptodisk_get_by_source_disk (source);
 
@@ -1017,23 +1019,53 @@ grub_cryptodisk_scan_device_real (const char *name,
       return grub_errno;
     if (!dev)
       continue;
-    
+
+    if (cargs->key_len == 0)
+      {
+	/* Get the passphrase from the user, if no key data. */
+	askpass = 1;
+	if (source->partition)
+	  part = grub_partition_get_name (source->partition);
+	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+		     source->partition ? "," : "", part ? : "",
+		     dev->uuid);
+	grub_free (part);
+
+	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+	if (!cargs->key_data)
+	  return grub_errno;
+
+	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+	  {
+	    ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
+	    goto error;
+	  }
+	cargs->key_len = grub_strlen((char *) cargs->key_data);
+      }
+
     *dev->cargs = *cargs;
-    err = cr->recover_key (source, dev);
+    ret = cr->recover_key (source, dev);
     *dev->cargs = NULL;
-    if (err)
-    {
-      cryptodisk_close (dev);
-      return err;
-    }
+    if (ret)
+      goto error;
 
     grub_cryptodisk_insert (dev, name, source);
 
     have_it = 1;
 
-    return GRUB_ERR_NONE;
+    goto cleanup;
   }
-  return GRUB_ERR_NONE;
+  goto cleanup;
+
+error:
+  cryptodisk_close (dev);
+cleanup:
+  if (askpass)
+    {
+      cargs->key_len = 0;
+      grub_free(cargs->key_data);
+    }
+  return ret;
 }
 
 #ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 0a7bd90da..b4525ed48 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -135,8 +135,6 @@ const char *algorithms[] = {
   [0x16] = "aes"
 };
 
-#define MAX_PASSPHRASE 256
-
 static gcry_err_code_t
 geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno)
 {
@@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
   grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN];
   grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE];
   grub_uint8_t geli_cipher_key[64];
-  char passphrase[MAX_PASSPHRASE] = "";
   unsigned i;
   gcry_err_code_t gcry_err;
   struct grub_geli_phdr header;
-  char *tmp;
   grub_disk_addr_t sector;
   grub_err_t err;
 
-  /* Keyfiles are not implemented yet */
-  if (dev->cargs->key_data || dev->cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+  if (dev->cargs->key_data == NULL || dev->cargs->key_len == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
 
   if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
     return grub_error (GRUB_ERR_BUG, "cipher block is too long");
@@ -438,23 +433,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
 
   grub_puts_ (N_("Attempting to decrypt master key..."));
 
-  /* Get the passphrase from the user.  */
-  tmp = NULL;
-  if (source->partition)
-    tmp = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-		source->partition ? "," : "", tmp ? : "",
-		dev->uuid);
-  grub_free (tmp);
-  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
-    return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-
   /* Calculate the PBKDF2 of the user supplied passphrase.  */
   if (grub_le_to_cpu32 (header.niter) != 0)
     {
       grub_uint8_t pbkdf_key[64];
-      gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
-				     grub_strlen (passphrase),
+      gcry_err = grub_crypto_pbkdf2 (dev->hash, dev->cargs->key_data,
+				     dev->cargs->key_len,
 				     header.salt,
 				     sizeof (header.salt),
 				     grub_le_to_cpu32 (header.niter),
@@ -477,7 +461,7 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
 	return grub_crypto_gcry_error (GPG_ERR_OUT_OF_MEMORY);
 
       grub_crypto_hmac_write (hnd, header.salt, sizeof (header.salt));
-      grub_crypto_hmac_write (hnd, passphrase, grub_strlen (passphrase));
+      grub_crypto_hmac_write (hnd, dev->cargs->key_data, dev->cargs->key_len);
 
       gcry_err = grub_crypto_hmac_fini (hnd, geomkey);
       if (gcry_err)
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index e2a4a3bf5..e6ff0631e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -29,8 +29,6 @@
 
 GRUB_MOD_LICENSE ("GPLv3+");
 
-#define MAX_PASSPHRASE 256
-
 #define LUKS_KEY_ENABLED  0x00AC71F3
 
 /* On disk LUKS header */
@@ -157,17 +155,14 @@ luks_recover_key (grub_disk_t source,
   struct grub_luks_phdr header;
   grub_size_t keysize;
   grub_uint8_t *split_key = NULL;
-  char passphrase[MAX_PASSPHRASE] = "";
   grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
   unsigned i;
   grub_size_t length;
   grub_err_t err;
   grub_size_t max_stripes = 1;
-  char *tmp;
 
-  /* Keyfiles are not implemented yet */
-  if (dev->cargs->key_data || dev->cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+  if (dev->cargs->key_data == NULL || dev->cargs->key_len == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
 
   err = grub_disk_read (source, 0, 0, sizeof (header), &header);
   if (err)
@@ -187,20 +182,6 @@ luks_recover_key (grub_disk_t source,
   if (!split_key)
     return grub_errno;
 
-  /* Get the passphrase from the user.  */
-  tmp = NULL;
-  if (source->partition)
-    tmp = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-	       source->partition ? "," : "", tmp ? : "",
-	       dev->uuid);
-  grub_free (tmp);
-  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
-    {
-      grub_free (split_key);
-      return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-    }
-
   /* Try to recover master key from each active keyslot.  */
   for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
     {
@@ -215,8 +196,8 @@ luks_recover_key (grub_disk_t source,
       grub_dprintf ("luks", "Trying keyslot %d\n", i);
 
       /* Calculate the PBKDF2 of the user supplied passphrase.  */
-      gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
-				     grub_strlen (passphrase),
+      gcry_err = grub_crypto_pbkdf2 (dev->hash, dev->cargs->key_data,
+				     dev->cargs->key_len,
 				     header.keyblock[i].passwordSalt,
 				     sizeof (header.keyblock[i].passwordSalt),
 				     grub_be_to_cpu32 (header.keyblock[i].
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e0de902c9..e4afa4156 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -35,8 +35,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
 #define LUKS_MAGIC_1ST "LUKS\xBA\xBE"
 #define LUKS_MAGIC_2ND "SKUL\xBA\xBE"
 
-#define MAX_PASSPHRASE 256
-
 enum grub_luks2_kdf_type
 {
   LUKS2_KDF_TYPE_ARGON2I,
@@ -545,8 +543,8 @@ luks2_recover_key (grub_disk_t source,
 		   grub_cryptodisk_t crypt)
 {
   grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
-  char passphrase[MAX_PASSPHRASE], cipher[32];
-  char *json_header = NULL, *part = NULL, *ptr;
+  char cipher[32];
+  char *json_header = NULL, *ptr;
   grub_size_t candidate_key_len = 0, json_idx, size;
   grub_luks2_header_t header;
   grub_luks2_keyslot_t keyslot;
@@ -556,9 +554,8 @@ luks2_recover_key (grub_disk_t source,
   grub_json_t *json = NULL, keyslots;
   grub_err_t ret;
 
-  /* Keyfiles are not implemented yet */
-  if (crypt->cargs->key_data || crypt->cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+  if (crypt->cargs->key_data == NULL || crypt->cargs->key_len == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
 
   ret = luks2_read_header (source, &header);
   if (ret)
@@ -585,18 +582,6 @@ luks2_recover_key (grub_disk_t source,
       goto err;
     }
 
-  /* Get the passphrase from the user. */
-  if (source->partition)
-    part = grub_partition_get_name (source->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
-		source->partition ? "," : "", part ? : "",
-		crypt->uuid);
-  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
-    {
-      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-      goto err;
-    }
-
   if (grub_json_getvalue (&keyslots, json, "keyslots") ||
       grub_json_getsize (&size, &keyslots))
     {
@@ -721,7 +706,7 @@ luks2_recover_key (grub_disk_t source,
 	}
 
       ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
-			       (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
+			       crypt->cargs->key_data, crypt->cargs->key_len);
       if (ret)
 	{
 	  grub_dprintf ("luks2", "Decryption with keyslot \"%" PRIuGRUB_UINT64_T "\" failed: %s\n",
@@ -773,7 +758,6 @@ luks2_recover_key (grub_disk_t source,
     }
 
  err:
-  grub_free (part);
   grub_free (json_header);
   grub_json_free (json);
   return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 433c75426..1070140d9 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -59,6 +59,7 @@ typedef enum
 #define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE - 3)
 #define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
 #define GRUB_CRYPTODISK_MAX_KEYLEN 128
+#define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
 
 struct grub_cryptodisk;
 
-- 
2.27.0

[PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
 include/grub/cryptodisk.h   |  3 +++
 2 files changed, 12 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b6cf1835d..00a671a59 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
 
 #endif
 
-static int check_boot, have_it;
-static char *search_uuid;
-
 static void
 cryptodisk_close (grub_cryptodisk_t dev)
 {
@@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
 
   FOR_CRYPTODISK_DEVS (cr)
   {
-    dev = cr->scan (source, search_uuid, check_boot);
+    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char *name,
 
     grub_cryptodisk_insert (dev, name, source);
 
-    have_it = 1;
+    cargs->found_uuid = 1;
 
     goto cleanup;
   }
@@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
 
   FOR_CRYPTODISK_DEVS (cr)
   {
-    dev = cr->scan (source, search_uuid, check_boot);
+    dev = cr->scan (source, NULL, 0);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
   
   if (err)
     grub_print_error ();
-  return have_it && search_uuid ? 1 : 0;
+  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
 }
 
 static grub_err_t
@@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       cargs.key_len = grub_strlen(state[3].arg);
     }
 
-  have_it = 0;
   if (state[0].set) /* uuid */
     {
       grub_cryptodisk_t dev;
@@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      check_boot = state[2].set;
-      search_uuid = args[0];
+      cargs.check_boot = state[2].set;
+      cargs.search_uuid = args[0];
       grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
-      search_uuid = NULL;
 
-      if (!have_it)
+      if (!cargs.found_uuid)
 	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
       return GRUB_ERR_NONE;
     }
   else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
     {
-      search_uuid = NULL;
-      check_boot = state[2].set;
+      cargs.check_boot = state[2].set;
       grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
-      search_uuid = NULL;
       return GRUB_ERR_NONE;
     }
   else
@@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       char *disklast = NULL;
       grub_size_t len;
 
-      search_uuid = NULL;
-      check_boot = state[2].set;
+      cargs.check_boot = state[2].set;
       diskname = args[0];
       len = grub_strlen (diskname);
       if (len && diskname[0] == '(' && diskname[len - 1] == ')')
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 1070140d9..11062f43a 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -69,6 +69,9 @@ typedef gcry_err_code_t
 
 struct grub_cryptomount_args
 {
+  grub_uint32_t check_boot : 1;
+  grub_uint32_t found_uuid : 1;
+  char *search_uuid;
   grub_uint8_t *key_data;
   grub_size_t key_len;
 };
-- 
2.27.0

Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Patrick Steinhardt]

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

On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
> 
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++---------
>  grub-core/disk/geli.c       |  4 ++++
>  grub-core/disk/luks.c       |  4 ++++
>  grub-core/disk/luks2.c      |  4 ++++
>  include/grub/cryptodisk.h   |  8 ++++++++
>  5 files changed, 42 insertions(+), 9 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..b966b19ab 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
>      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
>      {"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},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
>  }
>  
>  static grub_err_t
> -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> +grub_cryptodisk_scan_device_real (const char *name,
> +				  grub_disk_t source,
> +				  grub_cryptomount_args_t cargs)
>  {
>    grub_err_t err;
>    grub_cryptodisk_t dev;
> @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
>      if (!dev)
>        continue;
>      
> +    *dev->cargs = *cargs;
>      err = cr->recover_key (source, dev);
> +    *dev->cargs = NULL;
>      if (err)
>      {
>        cryptodisk_close (dev);

Is there any specific reason why you choose to set `cargs` as a struct
field? Seeing uses like this makes me wonder whether it wouldn't be
better to pass in `cargs` as explicit arguments whenever required. This
would also automatically answer any lifetime questions which arise.

[snip]
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 13103ea6a..e2a4a3bf5 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
>    grub_size_t max_stripes = 1;
>    char *tmp;
>  
> +  /* Keyfiles are not implemented yet */
> +  if (dev->cargs->key_data || dev->cargs->key_len)
> +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
>      return err;

Hum. This makes me wonder whether we'll have to adjust all backends
whenever we add a new parameter to `cargs` to return `NOT_IMPLEMENTED`.
I fear that it won't scale nicely, and that it is a recipe for passing
unsupported arguments to backends even though they don't know to handle
them.

Not sure about a nice alternative though. The only idea I have right now
is something like a capabilities bitfield exposed by backends: only if a
specific bit is set will we pass the corresponding field to such a
backend. This would allow us to easily centralize the logic into a
single function which only receives both the capabilities bitset and the
`cargs` struct.

Other than that I really like where this is going.

Patrick

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

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Patrick Steinhardt]

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

On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
>  include/grub/cryptodisk.h   |  3 +++
>  2 files changed, 12 insertions(+), 17 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index b6cf1835d..00a671a59 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  
>  #endif
>  
> -static int check_boot, have_it;
> -static char *search_uuid;
> -
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
>  {
> @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>  
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> -    dev = cr->scan (source, search_uuid, check_boot);
> +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>  
>      grub_cryptodisk_insert (dev, name, source);
>  
> -    have_it = 1;
> +    cargs->found_uuid = 1;
>  
>      goto cleanup;
>    }
> @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
>  
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> -    dev = cr->scan (source, search_uuid, check_boot);
> +    dev = cr->scan (source, NULL, 0);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
>    
>    if (err)
>      grub_print_error ();
> -  return have_it && search_uuid ? 1 : 0;
> +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
>  }
>  
>  static grub_err_t
> @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        cargs.key_len = grub_strlen(state[3].arg);
>      }
>  
> -  have_it = 0;
>    if (state[0].set) /* uuid */
>      {
>        grub_cryptodisk_t dev;
> @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>  	  return GRUB_ERR_NONE;
>  	}
>  
> -      check_boot = state[2].set;
> -      search_uuid = args[0];
> +      cargs.check_boot = state[2].set;
> +      cargs.search_uuid = args[0];
>        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> -      search_uuid = NULL;
>  
> -      if (!have_it)
> +      if (!cargs.found_uuid)
>  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>        return GRUB_ERR_NONE;
>      }
>    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
>      {
> -      search_uuid = NULL;
> -      check_boot = state[2].set;
> +      cargs.check_boot = state[2].set;
>        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> -      search_uuid = NULL;
>        return GRUB_ERR_NONE;
>      }
>    else
> @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        char *disklast = NULL;
>        grub_size_t len;
>  
> -      search_uuid = NULL;
> -      check_boot = state[2].set;
> +      cargs.check_boot = state[2].set;
>        diskname = args[0];
>        len = grub_strlen (diskname);
>        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 1070140d9..11062f43a 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -69,6 +69,9 @@ typedef gcry_err_code_t
>  
>  struct grub_cryptomount_args
>  {
> +  grub_uint32_t check_boot : 1;
> +  grub_uint32_t found_uuid : 1;
> +  char *search_uuid;
>    grub_uint8_t *key_data;
>    grub_size_t key_len;
>  };

Aren't these parameters in a different scope than the key data? These
are only used for device discovery via `scan()`, while the other ones
are for decrypting the key. Do we want to split those up into two
different structs?

Patrick

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

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

On Mon, 30 Aug 2021 20:02:26 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> >  include/grub/cryptodisk.h   |  3 +++
> >  2 files changed, 12 insertions(+), 17 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >  
> >  #endif
> >  
> > -static int check_boot, have_it;
> > -static char *search_uuid;
> > -
> >  static void
> >  cryptodisk_close (grub_cryptodisk_t dev)
> >  {
> > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > *name, 
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > -    dev = cr->scan (source, search_uuid, check_boot);
> > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > *name, 
> >      grub_cryptodisk_insert (dev, name, source);
> >  
> > -    have_it = 1;
> > +    cargs->found_uuid = 1;
> >  
> >      goto cleanup;
> >    }
> > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > *sourcedev, const char *cheat) 
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > -    dev = cr->scan (source, search_uuid, check_boot);
> > +    dev = cr->scan (source, NULL, 0);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> >    
> >    if (err)
> >      grub_print_error ();
> > -  return have_it && search_uuid ? 1 : 0;
> > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> >  }
> >  
> >  static grub_err_t
> > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) cargs.key_len =
> > grub_strlen(state[3].arg); }
> >  
> > -  have_it = 0;
> >    if (state[0].set) /* uuid */
> >      {
> >        grub_cryptodisk_t dev;
> > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> >  	}
> >  
> > -      check_boot = state[2].set;
> > -      search_uuid = args[0];
> > +      cargs.check_boot = state[2].set;
> > +      cargs.search_uuid = args[0];
> >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > -      search_uuid = NULL;
> >  
> > -      if (!have_it)
> > +      if (!cargs.found_uuid)
> >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > cryptodisk found"); return GRUB_ERR_NONE;
> >      }
> >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> >      {
> > -      search_uuid = NULL;
> > -      check_boot = state[2].set;
> > +      cargs.check_boot = state[2].set;
> >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > -      search_uuid = NULL;
> >        return GRUB_ERR_NONE;
> >      }
> >    else
> > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > ctxt, int argc, char **args) char *disklast = NULL;
> >        grub_size_t len;
> >  
> > -      search_uuid = NULL;
> > -      check_boot = state[2].set;
> > +      cargs.check_boot = state[2].set;
> >        diskname = args[0];
> >        len = grub_strlen (diskname);
> >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 1070140d9..11062f43a 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> >  
> >  struct grub_cryptomount_args
> >  {
> > +  grub_uint32_t check_boot : 1;
> > +  grub_uint32_t found_uuid : 1;
> > +  char *search_uuid;
> >    grub_uint8_t *key_data;
> >    grub_size_t key_len;
> >  };
> 
> Aren't these parameters in a different scope than the key data? These
> are only used for device discovery via `scan()`, while the other ones
> are for decrypting the key. Do we want to split those up into two
> different structs?

This struct is meant to be used for any data passed to the crypto
backend from cryptomount. All of those members are affected by
cryptomount options. So this struct isn't about anything in particular,
just a common set of data passed to the crypto backends via
cryptomount. So I don't think two structs would improve anything here.
Am I missing something?

Glenn

Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Glenn Washburn]

On Mon, 30 Aug 2021 19:55:59 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
> > As an example, passing a password as a cryptomount argument is
> > implemented. However, the backends are not implemented, so testing
> > this will return a not implemented error.
> > 
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 31 ++++++++++++++++++++++---------
> >  grub-core/disk/geli.c       |  4 ++++
> >  grub-core/disk/luks.c       |  4 ++++
> >  grub-core/disk/luks2.c      |  4 ++++
> >  include/grub/cryptodisk.h   |  8 ++++++++
> >  5 files changed, 42 insertions(+), 9 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c
> > b/grub-core/disk/cryptodisk.c index 90f82b2d3..b966b19ab 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> >      /* TRANSLATORS: It's still restricted to cryptodisks only.  */
> >      {"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}, {0, 0, 0, 0, 0, 0}
> >    };
> >  
> > @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> >  }
> >  
> >  static grub_err_t
> > -grub_cryptodisk_scan_device_real (const char *name, grub_disk_t
> > source) +grub_cryptodisk_scan_device_real (const char *name,
> > +				  grub_disk_t source,
> > +				  grub_cryptomount_args_t cargs)
> >  {
> >    grub_err_t err;
> >    grub_cryptodisk_t dev;
> > @@ -1015,7 +1018,9 @@ grub_cryptodisk_scan_device_real (const char
> > *name, grub_disk_t source) if (!dev)
> >        continue;
> >      
> > +    *dev->cargs = *cargs;
> >      err = cr->recover_key (source, dev);
> > +    *dev->cargs = NULL;
> >      if (err)
> >      {
> >        cryptodisk_close (dev);
> 
> Is there any specific reason why you choose to set `cargs` as a struct
> field? Seeing uses like this makes me wonder whether it wouldn't be
> better to pass in `cargs` as explicit arguments whenever required.
> This would also automatically answer any lifetime questions which
> arise.

I'm not opposed to this. One of my original goals was to try and not
change the function interfaces between cryptomount and the backends. I
also was originally going to have the dev->cargs stick around for the
lifetime of the dev, but I'm not remembering a good use case for that.
I'll send another series with cargs being passed as an argument.

> [snip]
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 13103ea6a..e2a4a3bf5 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
> >    grub_size_t max_stripes = 1;
> >    char *tmp;
> >  
> > +  /* Keyfiles are not implemented yet */
> > +  if (dev->cargs->key_data || dev->cargs->key_len)
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> >      return err;
> 
> Hum. This makes me wonder whether we'll have to adjust all backends
> whenever we add a new parameter to `cargs` to return
> `NOT_IMPLEMENTED`. I fear that it won't scale nicely, and that it is
> a recipe for passing unsupported arguments to backends even though
> they don't know to handle them.
> 
> Not sure about a nice alternative though. The only idea I have right
> now is something like a capabilities bitfield exposed by backends:
> only if a specific bit is set will we pass the corresponding field to
> such a backend. This would allow us to easily centralize the logic
> into a single function which only receives both the capabilities
> bitset and the `cargs` struct.
> 
> Other than that I really like where this is going.

I see your concern, and it would be nice to have an elegant solution to
it. The capability bitfield idea seems workable. I don't think this
needs to be solved right now though. This is a problem with all
proposed approaches. I think that this *could* lead to scalability
issues, but that's likely way down the road (considering the rate at
which we're adding args to cryptomount). Also, I don't think this patch
series hampers any such solution. So I think we can punt on this for
now. Does that sound reasonable?

Glenn

Re: [PATCH 1/3] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Patrick Steinhardt]

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

On Tue, Sep 07, 2021 at 04:43:18AM +0000, Glenn Washburn wrote:
> On Mon, 30 Aug 2021 19:55:59 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> > On Thu, Aug 26, 2021 at 12:08:50AM -0500, Glenn Washburn wrote:
[snip]
> > > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > > index 13103ea6a..e2a4a3bf5 100644
> > > --- a/grub-core/disk/luks.c
> > > +++ b/grub-core/disk/luks.c
> > > @@ -165,6 +165,10 @@ luks_recover_key (grub_disk_t source,
> > >    grub_size_t max_stripes = 1;
> > >    char *tmp;
> > >  
> > > +  /* Keyfiles are not implemented yet */
> > > +  if (dev->cargs->key_data || dev->cargs->key_len)
> > > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > > +
> > >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> > >    if (err)
> > >      return err;
> > 
> > Hum. This makes me wonder whether we'll have to adjust all backends
> > whenever we add a new parameter to `cargs` to return
> > `NOT_IMPLEMENTED`. I fear that it won't scale nicely, and that it is
> > a recipe for passing unsupported arguments to backends even though
> > they don't know to handle them.
> > 
> > Not sure about a nice alternative though. The only idea I have right
> > now is something like a capabilities bitfield exposed by backends:
> > only if a specific bit is set will we pass the corresponding field to
> > such a backend. This would allow us to easily centralize the logic
> > into a single function which only receives both the capabilities
> > bitset and the `cargs` struct.
> > 
> > Other than that I really like where this is going.
> 
> I see your concern, and it would be nice to have an elegant solution to
> it. The capability bitfield idea seems workable. I don't think this
> needs to be solved right now though. This is a problem with all
> proposed approaches. I think that this *could* lead to scalability
> issues, but that's likely way down the road (considering the rate at
> which we're adding args to cryptomount). Also, I don't think this patch
> series hampers any such solution. So I think we can punt on this for
> now. Does that sound reasonable?
> 
> Glenn

Yeah, that does sound reasonable to me.

Patrick

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

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Patrick Steinhardt]

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

On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> On Mon, 30 Aug 2021 20:02:26 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > >  include/grub/cryptodisk.h   |  3 +++
> > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/grub-core/disk/cryptodisk.c
> > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > >  
> > >  #endif
> > >  
> > > -static int check_boot, have_it;
> > > -static char *search_uuid;
> > > -
> > >  static void
> > >  cryptodisk_close (grub_cryptodisk_t dev)
> > >  {
> > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > *name, 
> > >    FOR_CRYPTODISK_DEVS (cr)
> > >    {
> > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > >      if (grub_errno)
> > >        return grub_errno;
> > >      if (!dev)
> > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > *name, 
> > >      grub_cryptodisk_insert (dev, name, source);
> > >  
> > > -    have_it = 1;
> > > +    cargs->found_uuid = 1;
> > >  
> > >      goto cleanup;
> > >    }
> > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > *sourcedev, const char *cheat) 
> > >    FOR_CRYPTODISK_DEVS (cr)
> > >    {
> > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > +    dev = cr->scan (source, NULL, 0);
> > >      if (grub_errno)
> > >        return grub_errno;
> > >      if (!dev)
> > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > >    
> > >    if (err)
> > >      grub_print_error ();
> > > -  return have_it && search_uuid ? 1 : 0;
> > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > >  }
> > >  
> > >  static grub_err_t
> > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > ctxt, int argc, char **args) cargs.key_len =
> > > grub_strlen(state[3].arg); }
> > >  
> > > -  have_it = 0;
> > >    if (state[0].set) /* uuid */
> > >      {
> > >        grub_cryptodisk_t dev;
> > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > >  	}
> > >  
> > > -      check_boot = state[2].set;
> > > -      search_uuid = args[0];
> > > +      cargs.check_boot = state[2].set;
> > > +      cargs.search_uuid = args[0];
> > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > -      search_uuid = NULL;
> > >  
> > > -      if (!have_it)
> > > +      if (!cargs.found_uuid)
> > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > cryptodisk found"); return GRUB_ERR_NONE;
> > >      }
> > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > >      {
> > > -      search_uuid = NULL;
> > > -      check_boot = state[2].set;
> > > +      cargs.check_boot = state[2].set;
> > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > -      search_uuid = NULL;
> > >        return GRUB_ERR_NONE;
> > >      }
> > >    else
> > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > ctxt, int argc, char **args) char *disklast = NULL;
> > >        grub_size_t len;
> > >  
> > > -      search_uuid = NULL;
> > > -      check_boot = state[2].set;
> > > +      cargs.check_boot = state[2].set;
> > >        diskname = args[0];
> > >        len = grub_strlen (diskname);
> > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > index 1070140d9..11062f43a 100644
> > > --- a/include/grub/cryptodisk.h
> > > +++ b/include/grub/cryptodisk.h
> > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > >  
> > >  struct grub_cryptomount_args
> > >  {
> > > +  grub_uint32_t check_boot : 1;
> > > +  grub_uint32_t found_uuid : 1;
> > > +  char *search_uuid;
> > >    grub_uint8_t *key_data;
> > >    grub_size_t key_len;
> > >  };
> > 
> > Aren't these parameters in a different scope than the key data? These
> > are only used for device discovery via `scan()`, while the other ones
> > are for decrypting the key. Do we want to split those up into two
> > different structs?
> 
> This struct is meant to be used for any data passed to the crypto
> backend from cryptomount. All of those members are affected by
> cryptomount options. So this struct isn't about anything in particular,
> just a common set of data passed to the crypto backends via
> cryptomount. So I don't think two structs would improve anything here.
> Am I missing something?
> 
> Glenn

I'm mostly wondering about lifetimes of these parameters. They are used
in different phases of the cryptomount: some are used only at the time
of discovery, while others are used at decryption time, where it's not
immediately clear which parameters are used when without having a look
at the cryptodisk implementations. That's why I was thinking it might
make more sense to split them up by those phases such that this becomes
explicit.

Patrick

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

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

On Sun, 12 Sep 2021 13:17:29 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > On Mon, 30 Aug 2021 20:02:26 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
> > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > >  include/grub/cryptodisk.h   |  3 +++
> > > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/grub-core/disk/cryptodisk.c
> > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > > --- a/grub-core/disk/cryptodisk.c
> > > > +++ b/grub-core/disk/cryptodisk.c
> > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > >  
> > > >  #endif
> > > >  
> > > > -static int check_boot, have_it;
> > > > -static char *search_uuid;
> > > > -
> > > >  static void
> > > >  cryptodisk_close (grub_cryptodisk_t dev)
> > > >  {
> > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > *name, 
> > > >    FOR_CRYPTODISK_DEVS (cr)
> > > >    {
> > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > > >      if (grub_errno)
> > > >        return grub_errno;
> > > >      if (!dev)
> > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > *name, 
> > > >      grub_cryptodisk_insert (dev, name, source);
> > > >  
> > > > -    have_it = 1;
> > > > +    cargs->found_uuid = 1;
> > > >  
> > > >      goto cleanup;
> > > >    }
> > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > > *sourcedev, const char *cheat) 
> > > >    FOR_CRYPTODISK_DEVS (cr)
> > > >    {
> > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > +    dev = cr->scan (source, NULL, 0);
> > > >      if (grub_errno)
> > > >        return grub_errno;
> > > >      if (!dev)
> > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > > >    
> > > >    if (err)
> > > >      grub_print_error ();
> > > > -  return have_it && search_uuid ? 1 : 0;
> > > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > > >  }
> > > >  
> > > >  static grub_err_t
> > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > ctxt, int argc, char **args) cargs.key_len =
> > > > grub_strlen(state[3].arg); }
> > > >  
> > > > -  have_it = 0;
> > > >    if (state[0].set) /* uuid */
> > > >      {
> > > >        grub_cryptodisk_t dev;
> > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > > >  	}
> > > >  
> > > > -      check_boot = state[2].set;
> > > > -      search_uuid = args[0];
> > > > +      cargs.check_boot = state[2].set;
> > > > +      cargs.search_uuid = args[0];
> > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > -      search_uuid = NULL;
> > > >  
> > > > -      if (!have_it)
> > > > +      if (!cargs.found_uuid)
> > > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > > cryptodisk found"); return GRUB_ERR_NONE;
> > > >      }
> > > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > > >      {
> > > > -      search_uuid = NULL;
> > > > -      check_boot = state[2].set;
> > > > +      cargs.check_boot = state[2].set;
> > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > -      search_uuid = NULL;
> > > >        return GRUB_ERR_NONE;
> > > >      }
> > > >    else
> > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > ctxt, int argc, char **args) char *disklast = NULL;
> > > >        grub_size_t len;
> > > >  
> > > > -      search_uuid = NULL;
> > > > -      check_boot = state[2].set;
> > > > +      cargs.check_boot = state[2].set;
> > > >        diskname = args[0];
> > > >        len = grub_strlen (diskname);
> > > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > index 1070140d9..11062f43a 100644
> > > > --- a/include/grub/cryptodisk.h
> > > > +++ b/include/grub/cryptodisk.h
> > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > >  
> > > >  struct grub_cryptomount_args
> > > >  {
> > > > +  grub_uint32_t check_boot : 1;
> > > > +  grub_uint32_t found_uuid : 1;
> > > > +  char *search_uuid;
> > > >    grub_uint8_t *key_data;
> > > >    grub_size_t key_len;
> > > >  };
> > > 
> > > Aren't these parameters in a different scope than the key data? These
> > > are only used for device discovery via `scan()`, while the other ones
> > > are for decrypting the key. Do we want to split those up into two
> > > different structs?
> > 
> > This struct is meant to be used for any data passed to the crypto
> > backend from cryptomount. All of those members are affected by
> > cryptomount options. So this struct isn't about anything in particular,
> > just a common set of data passed to the crypto backends via
> > cryptomount. So I don't think two structs would improve anything here.
> > Am I missing something?
> > 
> > Glenn
> 
> I'm mostly wondering about lifetimes of these parameters. They are used
> in different phases of the cryptomount: some are used only at the time
> of discovery, while others are used at decryption time, where it's not
> immediately clear which parameters are used when without having a look
> at the cryptodisk implementations. That's why I was thinking it might
> make more sense to split them up by those phases such that this becomes
> explicit.
> 
> Patrick

Okay, I think I see what you're saying. Essentially, as someone wanting
to write a new crypto-backend, when I'm writing by scan function, how
do I know what fields in the cargs struct are valid (have been
assigned)? The answer would be to check if they are not NULL, and
otherwise they're available. I don't really see an issue.

Would your objection be alleviated by having cargs be redefined like so:

struct grub_cryptomount_args
{
  struct {
    grub_uint32_t check_boot : 1;
    char *search_uuid;
  } scan;
  struct {
    grub_uint8_t *key_data;
    grub_size_t key_len;
  } recover_key;
  struct {
    ...
  } common;
};

Then in scan you know you can only use scan and common structs. I have
a common because the detached header changes will be such that scan and
recover_key need to be provided with detached header.  I think this way
is more than is needed at the present moment. If I'm still not getting
it, can you be a little more concrete in what is problematic and how
you think it could be changed to be better?

Glenn

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Patrick Steinhardt]

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

On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote:
> On Sun, 12 Sep 2021 13:17:29 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > > On Mon, 30 Aug 2021 20:02:26 +0200
> > > Patrick Steinhardt <ps@pks.im> wrote:
> > > 
> > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > > ---
> > > > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > > >  include/grub/cryptodisk.h   |  3 +++
> > > > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > > > 
> > > > > diff --git a/grub-core/disk/cryptodisk.c
> > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > > > --- a/grub-core/disk/cryptodisk.c
> > > > > +++ b/grub-core/disk/cryptodisk.c
> > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > > >  
> > > > >  #endif
> > > > >  
> > > > > -static int check_boot, have_it;
> > > > > -static char *search_uuid;
> > > > > -
> > > > >  static void
> > > > >  cryptodisk_close (grub_cryptodisk_t dev)
> > > > >  {
> > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > *name, 
> > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > >    {
> > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > > > >      if (grub_errno)
> > > > >        return grub_errno;
> > > > >      if (!dev)
> > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > *name, 
> > > > >      grub_cryptodisk_insert (dev, name, source);
> > > > >  
> > > > > -    have_it = 1;
> > > > > +    cargs->found_uuid = 1;
> > > > >  
> > > > >      goto cleanup;
> > > > >    }
> > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > > > *sourcedev, const char *cheat) 
> > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > >    {
> > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > +    dev = cr->scan (source, NULL, 0);
> > > > >      if (grub_errno)
> > > > >        return grub_errno;
> > > > >      if (!dev)
> > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > > > >    
> > > > >    if (err)
> > > > >      grub_print_error ();
> > > > > -  return have_it && search_uuid ? 1 : 0;
> > > > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > > > >  }
> > > > >  
> > > > >  static grub_err_t
> > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > ctxt, int argc, char **args) cargs.key_len =
> > > > > grub_strlen(state[3].arg); }
> > > > >  
> > > > > -  have_it = 0;
> > > > >    if (state[0].set) /* uuid */
> > > > >      {
> > > > >        grub_cryptodisk_t dev;
> > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > > > >  	}
> > > > >  
> > > > > -      check_boot = state[2].set;
> > > > > -      search_uuid = args[0];
> > > > > +      cargs.check_boot = state[2].set;
> > > > > +      cargs.search_uuid = args[0];
> > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > -      search_uuid = NULL;
> > > > >  
> > > > > -      if (!have_it)
> > > > > +      if (!cargs.found_uuid)
> > > > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > > > cryptodisk found"); return GRUB_ERR_NONE;
> > > > >      }
> > > > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > > > >      {
> > > > > -      search_uuid = NULL;
> > > > > -      check_boot = state[2].set;
> > > > > +      cargs.check_boot = state[2].set;
> > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > -      search_uuid = NULL;
> > > > >        return GRUB_ERR_NONE;
> > > > >      }
> > > > >    else
> > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > ctxt, int argc, char **args) char *disklast = NULL;
> > > > >        grub_size_t len;
> > > > >  
> > > > > -      search_uuid = NULL;
> > > > > -      check_boot = state[2].set;
> > > > > +      cargs.check_boot = state[2].set;
> > > > >        diskname = args[0];
> > > > >        len = grub_strlen (diskname);
> > > > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > > index 1070140d9..11062f43a 100644
> > > > > --- a/include/grub/cryptodisk.h
> > > > > +++ b/include/grub/cryptodisk.h
> > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > > >  
> > > > >  struct grub_cryptomount_args
> > > > >  {
> > > > > +  grub_uint32_t check_boot : 1;
> > > > > +  grub_uint32_t found_uuid : 1;
> > > > > +  char *search_uuid;
> > > > >    grub_uint8_t *key_data;
> > > > >    grub_size_t key_len;
> > > > >  };
> > > > 
> > > > Aren't these parameters in a different scope than the key data? These
> > > > are only used for device discovery via `scan()`, while the other ones
> > > > are for decrypting the key. Do we want to split those up into two
> > > > different structs?
> > > 
> > > This struct is meant to be used for any data passed to the crypto
> > > backend from cryptomount. All of those members are affected by
> > > cryptomount options. So this struct isn't about anything in particular,
> > > just a common set of data passed to the crypto backends via
> > > cryptomount. So I don't think two structs would improve anything here.
> > > Am I missing something?
> > > 
> > > Glenn
> > 
> > I'm mostly wondering about lifetimes of these parameters. They are used
> > in different phases of the cryptomount: some are used only at the time
> > of discovery, while others are used at decryption time, where it's not
> > immediately clear which parameters are used when without having a look
> > at the cryptodisk implementations. That's why I was thinking it might
> > make more sense to split them up by those phases such that this becomes
> > explicit.
> > 
> > Patrick
> 
> Okay, I think I see what you're saying. Essentially, as someone wanting
> to write a new crypto-backend, when I'm writing by scan function, how
> do I know what fields in the cargs struct are valid (have been
> assigned)? The answer would be to check if they are not NULL, and
> otherwise they're available. I don't really see an issue.
> 
> Would your objection be alleviated by having cargs be redefined like so:
> 
> struct grub_cryptomount_args
> {
>   struct {
>     grub_uint32_t check_boot : 1;
>     char *search_uuid;
>   } scan;
>   struct {
>     grub_uint8_t *key_data;
>     grub_size_t key_len;
>   } recover_key;
>   struct {
>     ...
>   } common;
> };
> 
> Then in scan you know you can only use scan and common structs. I have
> a common because the detached header changes will be such that scan and
> recover_key need to be provided with detached header.  I think this way
> is more than is needed at the present moment. If I'm still not getting
> it, can you be a little more concrete in what is problematic and how
> you think it could be changed to be better?
> 
> Glenn

Sorry, took me quite some time to get to this. I like your proposed
approach, and if we sprinkle in some comments about when those structs
should be used then I think it would be easy enough to understand. It
does raise the question whether we should instead define
`grub_cryptomount_args_common` and then embed it in
`grub_cryptomount_args_{scan,recover_key}` structs such that it is
impossible to use args in the wrong phase (e.g. `recover_key` args
during scan). But I feels a bit like bikeshedding, so please feel free
to go with your preferred style.

Patrick

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

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

On Mon, 4 Oct 2021 10:55:21 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote:
> > On Sun, 12 Sep 2021 13:17:29 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
> > > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > > > On Mon, 30 Aug 2021 20:02:26 +0200
> > > > Patrick Steinhardt <ps@pks.im> wrote:
> > > > 
> > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > > > ---
> > > > > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > > > >  include/grub/cryptodisk.h   |  3 +++
> > > > > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > diff --git a/grub-core/disk/cryptodisk.c
> > > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > > > > --- a/grub-core/disk/cryptodisk.c
> > > > > > +++ b/grub-core/disk/cryptodisk.c
> > > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > > > >  
> > > > > >  #endif
> > > > > >  
> > > > > > -static int check_boot, have_it;
> > > > > > -static char *search_uuid;
> > > > > > -
> > > > > >  static void
> > > > > >  cryptodisk_close (grub_cryptodisk_t dev)
> > > > > >  {
> > > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > *name, 
> > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > >    {
> > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > > > > >      if (grub_errno)
> > > > > >        return grub_errno;
> > > > > >      if (!dev)
> > > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > *name, 
> > > > > >      grub_cryptodisk_insert (dev, name, source);
> > > > > >  
> > > > > > -    have_it = 1;
> > > > > > +    cargs->found_uuid = 1;
> > > > > >  
> > > > > >      goto cleanup;
> > > > > >    }
> > > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > > > > *sourcedev, const char *cheat) 
> > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > >    {
> > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > +    dev = cr->scan (source, NULL, 0);
> > > > > >      if (grub_errno)
> > > > > >        return grub_errno;
> > > > > >      if (!dev)
> > > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > > > > >    
> > > > > >    if (err)
> > > > > >      grub_print_error ();
> > > > > > -  return have_it && search_uuid ? 1 : 0;
> > > > > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > > > > >  }
> > > > > >  
> > > > > >  static grub_err_t
> > > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) cargs.key_len =
> > > > > > grub_strlen(state[3].arg); }
> > > > > >  
> > > > > > -  have_it = 0;
> > > > > >    if (state[0].set) /* uuid */
> > > > > >      {
> > > > > >        grub_cryptodisk_t dev;
> > > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > > > > >  	}
> > > > > >  
> > > > > > -      check_boot = state[2].set;
> > > > > > -      search_uuid = args[0];
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > > +      cargs.search_uuid = args[0];
> > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > -      search_uuid = NULL;
> > > > > >  
> > > > > > -      if (!have_it)
> > > > > > +      if (!cargs.found_uuid)
> > > > > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > > > > cryptodisk found"); return GRUB_ERR_NONE;
> > > > > >      }
> > > > > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > > > > >      {
> > > > > > -      search_uuid = NULL;
> > > > > > -      check_boot = state[2].set;
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > -      search_uuid = NULL;
> > > > > >        return GRUB_ERR_NONE;
> > > > > >      }
> > > > > >    else
> > > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) char *disklast = NULL;
> > > > > >        grub_size_t len;
> > > > > >  
> > > > > > -      search_uuid = NULL;
> > > > > > -      check_boot = state[2].set;
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > >        diskname = args[0];
> > > > > >        len = grub_strlen (diskname);
> > > > > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > > > index 1070140d9..11062f43a 100644
> > > > > > --- a/include/grub/cryptodisk.h
> > > > > > +++ b/include/grub/cryptodisk.h
> > > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > > > >  
> > > > > >  struct grub_cryptomount_args
> > > > > >  {
> > > > > > +  grub_uint32_t check_boot : 1;
> > > > > > +  grub_uint32_t found_uuid : 1;
> > > > > > +  char *search_uuid;
> > > > > >    grub_uint8_t *key_data;
> > > > > >    grub_size_t key_len;
> > > > > >  };
> > > > > 
> > > > > Aren't these parameters in a different scope than the key data? These
> > > > > are only used for device discovery via `scan()`, while the other ones
> > > > > are for decrypting the key. Do we want to split those up into two
> > > > > different structs?
> > > > 
> > > > This struct is meant to be used for any data passed to the crypto
> > > > backend from cryptomount. All of those members are affected by
> > > > cryptomount options. So this struct isn't about anything in particular,
> > > > just a common set of data passed to the crypto backends via
> > > > cryptomount. So I don't think two structs would improve anything here.
> > > > Am I missing something?
> > > > 
> > > > Glenn
> > > 
> > > I'm mostly wondering about lifetimes of these parameters. They are used
> > > in different phases of the cryptomount: some are used only at the time
> > > of discovery, while others are used at decryption time, where it's not
> > > immediately clear which parameters are used when without having a look
> > > at the cryptodisk implementations. That's why I was thinking it might
> > > make more sense to split them up by those phases such that this becomes
> > > explicit.
> > > 
> > > Patrick
> > 
> > Okay, I think I see what you're saying. Essentially, as someone wanting
> > to write a new crypto-backend, when I'm writing by scan function, how
> > do I know what fields in the cargs struct are valid (have been
> > assigned)? The answer would be to check if they are not NULL, and
> > otherwise they're available. I don't really see an issue.
> > 
> > Would your objection be alleviated by having cargs be redefined like so:
> > 
> > struct grub_cryptomount_args
> > {
> >   struct {
> >     grub_uint32_t check_boot : 1;
> >     char *search_uuid;
> >   } scan;
> >   struct {
> >     grub_uint8_t *key_data;
> >     grub_size_t key_len;
> >   } recover_key;
> >   struct {
> >     ...
> >   } common;
> > };
> > 
> > Then in scan you know you can only use scan and common structs. I have
> > a common because the detached header changes will be such that scan and
> > recover_key need to be provided with detached header.  I think this way
> > is more than is needed at the present moment. If I'm still not getting
> > it, can you be a little more concrete in what is problematic and how
> > you think it could be changed to be better?
> > 
> > Glenn
> 
> Sorry, took me quite some time to get to this. I like your proposed
> approach, and if we sprinkle in some comments about when those structs
> should be used then I think it would be easy enough to understand. It
> does raise the question whether we should instead define
> `grub_cryptomount_args_common` and then embed it in
> `grub_cryptomount_args_{scan,recover_key}` structs such that it is
> impossible to use args in the wrong phase (e.g. `recover_key` args
> during scan). But I feels a bit like bikeshedding, so please feel free
> to go with your preferred style.
> 
> Patrick

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

On Mon, 4 Oct 2021 10:55:21 +0200
Patrick Steinhardt <ps@pks.im> wrote:

> On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote:
> > On Sun, 12 Sep 2021 13:17:29 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> > 
> > > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > > > On Mon, 30 Aug 2021 20:02:26 +0200
> > > > Patrick Steinhardt <ps@pks.im> wrote:
> > > > 
> > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > > > ---
> > > > > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > > > >  include/grub/cryptodisk.h   |  3 +++
> > > > > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > > > > 
> > > > > > diff --git a/grub-core/disk/cryptodisk.c
> > > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > > > > --- a/grub-core/disk/cryptodisk.c
> > > > > > +++ b/grub-core/disk/cryptodisk.c
> > > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > > > >  
> > > > > >  #endif
> > > > > >  
> > > > > > -static int check_boot, have_it;
> > > > > > -static char *search_uuid;
> > > > > > -
> > > > > >  static void
> > > > > >  cryptodisk_close (grub_cryptodisk_t dev)
> > > > > >  {
> > > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > *name, 
> > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > >    {
> > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > > > > >      if (grub_errno)
> > > > > >        return grub_errno;
> > > > > >      if (!dev)
> > > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > *name, 
> > > > > >      grub_cryptodisk_insert (dev, name, source);
> > > > > >  
> > > > > > -    have_it = 1;
> > > > > > +    cargs->found_uuid = 1;
> > > > > >  
> > > > > >      goto cleanup;
> > > > > >    }
> > > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > > > > *sourcedev, const char *cheat) 
> > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > >    {
> > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > +    dev = cr->scan (source, NULL, 0);
> > > > > >      if (grub_errno)
> > > > > >        return grub_errno;
> > > > > >      if (!dev)
> > > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > > > > >    
> > > > > >    if (err)
> > > > > >      grub_print_error ();
> > > > > > -  return have_it && search_uuid ? 1 : 0;
> > > > > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > > > > >  }
> > > > > >  
> > > > > >  static grub_err_t
> > > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) cargs.key_len =
> > > > > > grub_strlen(state[3].arg); }
> > > > > >  
> > > > > > -  have_it = 0;
> > > > > >    if (state[0].set) /* uuid */
> > > > > >      {
> > > > > >        grub_cryptodisk_t dev;
> > > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > > > > >  	}
> > > > > >  
> > > > > > -      check_boot = state[2].set;
> > > > > > -      search_uuid = args[0];
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > > +      cargs.search_uuid = args[0];
> > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > -      search_uuid = NULL;
> > > > > >  
> > > > > > -      if (!have_it)
> > > > > > +      if (!cargs.found_uuid)
> > > > > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > > > > cryptodisk found"); return GRUB_ERR_NONE;
> > > > > >      }
> > > > > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > > > > >      {
> > > > > > -      search_uuid = NULL;
> > > > > > -      check_boot = state[2].set;
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > -      search_uuid = NULL;
> > > > > >        return GRUB_ERR_NONE;
> > > > > >      }
> > > > > >    else
> > > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > ctxt, int argc, char **args) char *disklast = NULL;
> > > > > >        grub_size_t len;
> > > > > >  
> > > > > > -      search_uuid = NULL;
> > > > > > -      check_boot = state[2].set;
> > > > > > +      cargs.check_boot = state[2].set;
> > > > > >        diskname = args[0];
> > > > > >        len = grub_strlen (diskname);
> > > > > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > > > index 1070140d9..11062f43a 100644
> > > > > > --- a/include/grub/cryptodisk.h
> > > > > > +++ b/include/grub/cryptodisk.h
> > > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > > > >  
> > > > > >  struct grub_cryptomount_args
> > > > > >  {
> > > > > > +  grub_uint32_t check_boot : 1;
> > > > > > +  grub_uint32_t found_uuid : 1;
> > > > > > +  char *search_uuid;
> > > > > >    grub_uint8_t *key_data;
> > > > > >    grub_size_t key_len;
> > > > > >  };
> > > > > 
> > > > > Aren't these parameters in a different scope than the key data? These
> > > > > are only used for device discovery via `scan()`, while the other ones
> > > > > are for decrypting the key. Do we want to split those up into two
> > > > > different structs?
> > > > 
> > > > This struct is meant to be used for any data passed to the crypto
> > > > backend from cryptomount. All of those members are affected by
> > > > cryptomount options. So this struct isn't about anything in particular,
> > > > just a common set of data passed to the crypto backends via
> > > > cryptomount. So I don't think two structs would improve anything here.
> > > > Am I missing something?
> > > > 
> > > > Glenn
> > > 
> > > I'm mostly wondering about lifetimes of these parameters. They are used
> > > in different phases of the cryptomount: some are used only at the time
> > > of discovery, while others are used at decryption time, where it's not
> > > immediately clear which parameters are used when without having a look
> > > at the cryptodisk implementations. That's why I was thinking it might
> > > make more sense to split them up by those phases such that this becomes
> > > explicit.
> > > 
> > > Patrick
> > 
> > Okay, I think I see what you're saying. Essentially, as someone wanting
> > to write a new crypto-backend, when I'm writing by scan function, how
> > do I know what fields in the cargs struct are valid (have been
> > assigned)? The answer would be to check if they are not NULL, and
> > otherwise they're available. I don't really see an issue.
> > 
> > Would your objection be alleviated by having cargs be redefined like so:
> > 
> > struct grub_cryptomount_args
> > {
> >   struct {
> >     grub_uint32_t check_boot : 1;
> >     char *search_uuid;
> >   } scan;
> >   struct {
> >     grub_uint8_t *key_data;
> >     grub_size_t key_len;
> >   } recover_key;
> >   struct {
> >     ...
> >   } common;
> > };
> > 
> > Then in scan you know you can only use scan and common structs. I have
> > a common because the detached header changes will be such that scan and
> > recover_key need to be provided with detached header.  I think this way
> > is more than is needed at the present moment. If I'm still not getting
> > it, can you be a little more concrete in what is problematic and how
> > you think it could be changed to be better?
> > 
> > Glenn
> 
> Sorry, took me quite some time to get to this. I like your proposed
> approach, and if we sprinkle in some comments about when those structs
> should be used then I think it would be easy enough to understand. It
> does raise the question whether we should instead define
> `grub_cryptomount_args_common` and then embed it in
> `grub_cryptomount_args_{scan,recover_key}` structs such that it is
> impossible to use args in the wrong phase (e.g. `recover_key` args
> during scan). But I feels a bit like bikeshedding, so please feel free
> to go with your preferred style.

I'm gathering that the issue with partitioning the cargs struct is
based on a concern about the proper use of the data. Perhaps the reason
I don't see the value in partitioning the struct is because I also
don't see how the data can be used in "the wrong phase". I'm guessing
this is mainly a concern with key_data and key_len and that they might
not be set during the scan phase. In that case key_len should be 0 and
key_data a NULL pointer. If the scan phase wants to use them, I don't
see why not, just make sure to check the values for validity first. Is
the concern that a crypto-backend author will try to "unlock" the
encrypted volume in scan? Perhaps I should add a couple of comments in
the grub_cryptodisk_dev struct briefly describing scan and recover_key
instead.

I don't really like the nested struct solution I proposed
because then the backend has to use both the phase-specific child
struct and the common one, needing to remember which member is in which
struct. I don't like embedding the common members in two different
structs for, one for each phase, because then we have duplication of
data. Or is there a way I'm not thinking of which can define the
structs such that they share the common members?

What about a compromise, which would be to document in the member
comment which phase its intented to be used in?

Glenn

Re: [PATCH 3/3] cryptodisk: Move global variables into grub_cryptomount_args struct

[Patrick Steinhardt]

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

On Mon, Oct 04, 2021 at 11:51:33PM -0500, Glenn Washburn wrote:
> On Mon, 4 Oct 2021 10:55:21 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
> 
> > On Mon, Sep 13, 2021 at 09:05:15PM +0000, Glenn Washburn wrote:
> > > On Sun, 12 Sep 2021 13:17:29 +0200
> > > Patrick Steinhardt <ps@pks.im> wrote:
> > > 
> > > > On Tue, Sep 07, 2021 at 02:34:30AM +0000, Glenn Washburn wrote:
> > > > > On Mon, 30 Aug 2021 20:02:26 +0200
> > > > > Patrick Steinhardt <ps@pks.im> wrote:
> > > > > 
> > > > > > On Thu, Aug 26, 2021 at 12:08:52AM -0500, Glenn Washburn wrote:
> > > > > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > > > > ---
> > > > > > >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > > > > >  include/grub/cryptodisk.h   |  3 +++
> > > > > > >  2 files changed, 12 insertions(+), 17 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/grub-core/disk/cryptodisk.c
> > > > > > > b/grub-core/disk/cryptodisk.c index b6cf1835d..00a671a59 100644
> > > > > > > --- a/grub-core/disk/cryptodisk.c
> > > > > > > +++ b/grub-core/disk/cryptodisk.c
> > > > > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > > > > >  
> > > > > > >  #endif
> > > > > > >  
> > > > > > > -static int check_boot, have_it;
> > > > > > > -static char *search_uuid;
> > > > > > > -
> > > > > > >  static void
> > > > > > >  cryptodisk_close (grub_cryptodisk_t dev)
> > > > > > >  {
> > > > > > > @@ -1014,7 +1011,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > > *name, 
> > > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > > >    {
> > > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > > +    dev = cr->scan (source, cargs->search_uuid, cargs->check_boot);
> > > > > > >      if (grub_errno)
> > > > > > >        return grub_errno;
> > > > > > >      if (!dev)
> > > > > > > @@ -1051,7 +1048,7 @@ grub_cryptodisk_scan_device_real (const char
> > > > > > > *name, 
> > > > > > >      grub_cryptodisk_insert (dev, name, source);
> > > > > > >  
> > > > > > > -    have_it = 1;
> > > > > > > +    cargs->found_uuid = 1;
> > > > > > >  
> > > > > > >      goto cleanup;
> > > > > > >    }
> > > > > > > @@ -1093,7 +1090,7 @@ grub_cryptodisk_cheat_mount (const char
> > > > > > > *sourcedev, const char *cheat) 
> > > > > > >    FOR_CRYPTODISK_DEVS (cr)
> > > > > > >    {
> > > > > > > -    dev = cr->scan (source, search_uuid, check_boot);
> > > > > > > +    dev = cr->scan (source, NULL, 0);
> > > > > > >      if (grub_errno)
> > > > > > >        return grub_errno;
> > > > > > >      if (!dev)
> > > > > > > @@ -1137,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
> > > > > > >    
> > > > > > >    if (err)
> > > > > > >      grub_print_error ();
> > > > > > > -  return have_it && search_uuid ? 1 : 0;
> > > > > > > +  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > > > > > >  }
> > > > > > >  
> > > > > > >  static grub_err_t
> > > > > > > @@ -1155,7 +1152,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > > ctxt, int argc, char **args) cargs.key_len =
> > > > > > > grub_strlen(state[3].arg); }
> > > > > > >  
> > > > > > > -  have_it = 0;
> > > > > > >    if (state[0].set) /* uuid */
> > > > > > >      {
> > > > > > >        grub_cryptodisk_t dev;
> > > > > > > @@ -1168,21 +1164,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > > ctxt, int argc, char **args) return GRUB_ERR_NONE;
> > > > > > >  	}
> > > > > > >  
> > > > > > > -      check_boot = state[2].set;
> > > > > > > -      search_uuid = args[0];
> > > > > > > +      cargs.check_boot = state[2].set;
> > > > > > > +      cargs.search_uuid = args[0];
> > > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > > -      search_uuid = NULL;
> > > > > > >  
> > > > > > > -      if (!have_it)
> > > > > > > +      if (!cargs.found_uuid)
> > > > > > >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such
> > > > > > > cryptodisk found"); return GRUB_ERR_NONE;
> > > > > > >      }
> > > > > > >    else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
> > > > > > >      {
> > > > > > > -      search_uuid = NULL;
> > > > > > > -      check_boot = state[2].set;
> > > > > > > +      cargs.check_boot = state[2].set;
> > > > > > >        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > > > > > > -      search_uuid = NULL;
> > > > > > >        return GRUB_ERR_NONE;
> > > > > > >      }
> > > > > > >    else
> > > > > > > @@ -1194,8 +1187,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t
> > > > > > > ctxt, int argc, char **args) char *disklast = NULL;
> > > > > > >        grub_size_t len;
> > > > > > >  
> > > > > > > -      search_uuid = NULL;
> > > > > > > -      check_boot = state[2].set;
> > > > > > > +      cargs.check_boot = state[2].set;
> > > > > > >        diskname = args[0];
> > > > > > >        len = grub_strlen (diskname);
> > > > > > >        if (len && diskname[0] == '(' && diskname[len - 1] == ')')
> > > > > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > > > > index 1070140d9..11062f43a 100644
> > > > > > > --- a/include/grub/cryptodisk.h
> > > > > > > +++ b/include/grub/cryptodisk.h
> > > > > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > > > > >  
> > > > > > >  struct grub_cryptomount_args
> > > > > > >  {
> > > > > > > +  grub_uint32_t check_boot : 1;
> > > > > > > +  grub_uint32_t found_uuid : 1;
> > > > > > > +  char *search_uuid;
> > > > > > >    grub_uint8_t *key_data;
> > > > > > >    grub_size_t key_len;
> > > > > > >  };
> > > > > > 
> > > > > > Aren't these parameters in a different scope than the key data? These
> > > > > > are only used for device discovery via `scan()`, while the other ones
> > > > > > are for decrypting the key. Do we want to split those up into two
> > > > > > different structs?
> > > > > 
> > > > > This struct is meant to be used for any data passed to the crypto
> > > > > backend from cryptomount. All of those members are affected by
> > > > > cryptomount options. So this struct isn't about anything in particular,
> > > > > just a common set of data passed to the crypto backends via
> > > > > cryptomount. So I don't think two structs would improve anything here.
> > > > > Am I missing something?
> > > > > 
> > > > > Glenn
> > > > 
> > > > I'm mostly wondering about lifetimes of these parameters. They are used
> > > > in different phases of the cryptomount: some are used only at the time
> > > > of discovery, while others are used at decryption time, where it's not
> > > > immediately clear which parameters are used when without having a look
> > > > at the cryptodisk implementations. That's why I was thinking it might
> > > > make more sense to split them up by those phases such that this becomes
> > > > explicit.
> > > > 
> > > > Patrick
> > > 
> > > Okay, I think I see what you're saying. Essentially, as someone wanting
> > > to write a new crypto-backend, when I'm writing by scan function, how
> > > do I know what fields in the cargs struct are valid (have been
> > > assigned)? The answer would be to check if they are not NULL, and
> > > otherwise they're available. I don't really see an issue.
> > > 
> > > Would your objection be alleviated by having cargs be redefined like so:
> > > 
> > > struct grub_cryptomount_args
> > > {
> > >   struct {
> > >     grub_uint32_t check_boot : 1;
> > >     char *search_uuid;
> > >   } scan;
> > >   struct {
> > >     grub_uint8_t *key_data;
> > >     grub_size_t key_len;
> > >   } recover_key;
> > >   struct {
> > >     ...
> > >   } common;
> > > };
> > > 
> > > Then in scan you know you can only use scan and common structs. I have
> > > a common because the detached header changes will be such that scan and
> > > recover_key need to be provided with detached header.  I think this way
> > > is more than is needed at the present moment. If I'm still not getting
> > > it, can you be a little more concrete in what is problematic and how
> > > you think it could be changed to be better?
> > > 
> > > Glenn
> > 
> > Sorry, took me quite some time to get to this. I like your proposed
> > approach, and if we sprinkle in some comments about when those structs
> > should be used then I think it would be easy enough to understand. It
> > does raise the question whether we should instead define
> > `grub_cryptomount_args_common` and then embed it in
> > `grub_cryptomount_args_{scan,recover_key}` structs such that it is
> > impossible to use args in the wrong phase (e.g. `recover_key` args
> > during scan). But I feels a bit like bikeshedding, so please feel free
> > to go with your preferred style.
> 
> I'm gathering that the issue with partitioning the cargs struct is
> based on a concern about the proper use of the data. Perhaps the reason
> I don't see the value in partitioning the struct is because I also
> don't see how the data can be used in "the wrong phase". I'm guessing
> this is mainly a concern with key_data and key_len and that they might
> not be set during the scan phase. In that case key_len should be 0 and
> key_data a NULL pointer. If the scan phase wants to use them, I don't
> see why not, just make sure to check the values for validity first. Is
> the concern that a crypto-backend author will try to "unlock" the
> encrypted volume in scan? Perhaps I should add a couple of comments in
> the grub_cryptodisk_dev struct briefly describing scan and recover_key
> instead.

It's rather about passing data that we know to be  completely irrelevant
to the function at this point in time, even if it's unset. But as I said
above, this really isn't much of a strong concern, but rather feels like
a small inconsistency in the API design to me.

> I don't really like the nested struct solution I proposed
> because then the backend has to use both the phase-specific child
> struct and the common one, needing to remember which member is in which
> struct. I don't like embedding the common members in two different
> structs for, one for each phase, because then we have duplication of
> data. Or is there a way I'm not thinking of which can define the
> structs such that they share the common members?
> 
> What about a compromise, which would be to document in the member
> comment which phase its intented to be used in?

That's fine with me. I don't want to spend too much time discussing this
point: in the end it won't really matter anyway given that we only got
so many backends, and regardless of which way we go with, the end result
is cleaner than what we've got right now.

So again, please don't take my criticism on this point as a blocker.

Patrick

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