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

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

[Glenn Washburn]

Updates since v2:
* Add space after function name in function calls
* Add comments for members of struct grub_cryptomount_args
* Correct commit message for patch #4

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

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

Patch #4 removes the found_uuid flag from the cargs struct, which is not
needed because the same information can be obtained from the return value
of grub_device_iterate.

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 (4):
  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
  cryptodisk: Remove unneeded found_uuid from cryptomount args

 grub-core/disk/cryptodisk.c | 108 ++++++++++++++++++++++++------------
 grub-core/disk/geli.c       |  35 ++++--------
 grub-core/disk/luks.c       |  37 ++++--------
 grub-core/disk/luks2.c      |  33 ++++-------
 include/grub/cryptodisk.h   |  19 ++++++-
 5 files changed, 120 insertions(+), 112 deletions(-)

Range-diff against v2:
1:  475bf7e9e ! 1:  83112518f cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
    @@ grub-core/disk/cryptodisk.c: static grub_err_t
     +  if (state[3].set) /* password */
     +    {
     +      cargs.key_data = (grub_uint8_t *) state[3].arg;
    -+      cargs.key_len = grub_strlen(state[3].arg);
    ++      cargs.key_len = grub_strlen (state[3].arg);
     +    }
     +
        have_it = 0;
2:  a0c0694fc ! 2:  65a18c5e8 cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     +	    ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
     +	    goto error;
     +	  }
    -+	cargs->key_len = grub_strlen((char *) cargs->key_data);
    ++	cargs->key_len = grub_strlen ((char *) cargs->key_data);
     +      }
     +
     +    ret = cr->recover_key (source, dev, cargs);
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     +  if (askpass)
     +    {
     +      cargs->key_len = 0;
    -+      grub_free(cargs->key_data);
    ++      grub_free (cargs->key_data);
     +    }
     +  return ret;
      }
3:  419f114d8 ! 3:  fed38b3dc cryptodisk: Move global variables into grub_cryptomount_args struct
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
      
      static grub_err_t
     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
    -       cargs.key_len = grub_strlen(state[3].arg);
    +       cargs.key_len = grub_strlen (state[3].arg);
          }
      
     -  have_it = 0;
    @@ include/grub/cryptodisk.h: typedef gcry_err_code_t
      
      struct grub_cryptomount_args
      {
    ++  /* scan: Flag to indicate that only bootable volumes should be decrypted */
     +  grub_uint32_t check_boot : 1;
     +  grub_uint32_t found_uuid : 1;
    ++  /* scan: Only volumes matching this UUID should be decrpyted */
     +  char *search_uuid;
    ++  /* recover_key: Key data used to decrypt voume */
        grub_uint8_t *key_data;
    ++  /* recover_key: Length of key_data */
        grub_size_t key_len;
      };
    + typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
     @@ include/grub/cryptodisk.h: struct grub_cryptodisk_dev
        struct grub_cryptodisk_dev *next;
        struct grub_cryptodisk_dev **prev;
4:  e6e1e8bc3 ! 4:  854709929 cryptodisk: Remove unneeded found_uuid from cryptomount args
    @@ Commit message
         iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
         will only return 1 if a search_uuid has been specified and a cryptodisk was
         successfully setup by a crypto-backend. So the return value of
    -    grub_cryptodisk_scan_device is equivalent to found_uuid.
    +    grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
    +    exception of the case where a mount is requested or an already opened
    +    crypto device.
    +
    +    With this change grub_device_iterate will return 1 when
    +    grub_cryptodisk_scan_device when a crypto device is successfully decrypted
    +    or when the source device has already been successfully opened. Prior to
    +    this change, trying to mount an already successfully opened device would
    +    trigger an error with the message "no such cryptodisk found", which is at
    +    best misleading. The mount should silently succeed in this case, which is
    +    what happens with this patch.
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
          }
     
      ## include/grub/cryptodisk.h ##
    -@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
    - struct grub_cryptomount_args
    +@@ include/grub/cryptodisk.h: struct grub_cryptomount_args
      {
    +   /* scan: Flag to indicate that only bootable volumes should be decrypted */
        grub_uint32_t check_boot : 1;
     -  grub_uint32_t found_uuid : 1;
    +   /* scan: Only volumes matching this UUID should be decrpyted */
        char *search_uuid;
    -   grub_uint8_t *key_data;
    -   grub_size_t key_len;
    +   /* recover_key: Key data used to decrypt voume */
-- 
2.27.0

[PATCH v3 1/4] 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       |  6 +++++-
 grub-core/disk/luks.c       |  7 ++++++-
 grub-core/disk/luks2.c      |  7 ++++++-
 include/grub/cryptodisk.h   |  9 ++++++++-
 5 files changed, 46 insertions(+), 14 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..577942088 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,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
     if (!dev)
       continue;
     
-    err = cr->recover_key (source, dev);
+    err = cr->recover_key (source, dev, cargs);
     if (err)
     {
       cryptodisk_close (dev);
@@ -1080,10 +1083,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 +1097,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 +1110,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 +1136,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 +1189,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 +1328,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..4e8c377e7 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 }
 
 static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev)
+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];
@@ -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 (cargs->key_data || 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..0462edc6e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
 
 static grub_err_t
 luks_recover_key (grub_disk_t source,
-		  grub_cryptodisk_t dev)
+		  grub_cryptodisk_t dev,
+		  grub_cryptomount_args_t cargs)
 {
   struct grub_luks_phdr header;
   grub_size_t keysize;
@@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
   grub_size_t max_stripes = 1;
   char *tmp;
 
+  /* Keyfiles are not implemented yet */
+  if (cargs->key_data || 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..455a78cb0 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 
 static grub_err_t
 luks2_recover_key (grub_disk_t source,
-		   grub_cryptodisk_t crypt)
+		   grub_cryptodisk_t crypt,
+		   grub_cryptomount_args_t cargs)
 {
   grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
   char passphrase[MAX_PASSPHRASE], cipher[32];
@@ -556,6 +557,10 @@ luks2_recover_key (grub_disk_t source,
   grub_json_t *json = NULL, keyslots;
   grub_err_t ret;
 
+  /* Keyfiles are not implemented yet */
+  if (cargs->key_data || 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..282f8ac45 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;
@@ -119,7 +126,7 @@ struct grub_cryptodisk_dev
 
   grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
 			     int boot_only);
-  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
+  grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
 
-- 
2.27.0

[PATCH v3 2/4] 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
       return grub_errno;
     if (!dev)
       continue;
-    
-    err = cr->recover_key (source, dev, cargs);
-    if (err)
-    {
-      cryptodisk_close (dev);
-      return err;
-    }
+
+    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);
+      }
+
+    ret = cr->recover_key (source, dev, cargs);
+    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 4e8c377e7..32f34d5c3 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_cryptomount_args_t
   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 (cargs->key_data || cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+  if (cargs->key_data == NULL || 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_cryptomount_args_t
 
   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, cargs->key_data,
+				     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, grub_cryptomount_args_t
 	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, cargs->key_data, 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 0462edc6e..51646cefe 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 */
@@ -158,17 +156,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 (cargs->key_data || cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+ if (cargs->key_data == NULL || 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)
@@ -188,20 +183,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++)
     {
@@ -216,8 +197,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, cargs->key_data,
+				     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 455a78cb0..c77380cbb 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,
@@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
 		   grub_cryptomount_args_t cargs)
 {
   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;
@@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source,
   grub_json_t *json = NULL, keyslots;
   grub_err_t ret;
 
-  /* Keyfiles are not implemented yet */
-  if (cargs->key_data || cargs->key_len)
-     return GRUB_ERR_NOT_IMPLEMENTED_YET;
+  if (cargs->key_data == NULL || cargs->key_len == 0)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
 
   ret = luks2_read_header (source, &header);
   if (ret)
@@ -586,18 +583,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))
     {
@@ -722,7 +707,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));
+			       cargs->key_data, cargs->key_len);
       if (ret)
 	{
 	  grub_dprintf ("luks2", "Decryption with keyslot \"%" PRIuGRUB_UINT64_T "\" failed: %s\n",
@@ -774,7 +759,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 282f8ac45..5bd970692 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 v3 3/4] 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 +++++++++-----------------
 grub-core/disk/geli.c       |  9 ++++-----
 grub-core/disk/luks.c       | 11 +++++------
 grub-core/disk/luks2.c      |  6 +++---
 include/grub/cryptodisk.h   | 10 ++++++++--
 5 files changed, 29 insertions(+), 33 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index a5f7b860c..5b38606ed 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);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
 
     grub_cryptodisk_insert (dev, name, source);
 
-    have_it = 1;
+    cargs->found_uuid = 1;
 
     goto cleanup;
   }
@@ -1091,7 +1088,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);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1135,7 +1132,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
@@ -1153,7 +1150,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;
@@ -1166,21 +1162,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
@@ -1192,8 +1185,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/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 32f34d5c3..32d35521b 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
 #endif
 
 static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
-		   int boot_only)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   struct grub_geli_phdr header;
@@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       return NULL;
     }
 
-  if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
+  if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
     {
       grub_dprintf ("geli", "not a boot volume\n");
       return NULL;
@@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
       return NULL;
     }
 
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
     {
-      grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
+      grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
       return NULL;
     }
 
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 51646cefe..6ced312c7 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,8 +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, const char *check_uuid,
-		   int check_boot)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t newdev;
   const char *iptr;
@@ -76,7 +75,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
   char hashspec[sizeof (header.hashSpec) + 1];
   grub_err_t err;
 
-  if (check_boot)
+  if (cargs->check_boot)
     return NULL;
 
   /* Read the LUKS header.  */
@@ -103,9 +102,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
     }
   *optr = 0;
 
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
     {
-      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
+      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
       return NULL;
     }
 
@@ -162,7 +161,7 @@ luks_recover_key (grub_disk_t source,
   grub_err_t err;
   grub_size_t max_stripes = 1;
 
- if (cargs->key_data == NULL || cargs->key_len == 0)
+  if (cargs->key_data == NULL || cargs->key_len == 0)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
 
   err = grub_disk_read (source, 0, 0, sizeof (header), &header);
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index c77380cbb..28fad54aa 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -346,14 +346,14 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
 }
 
 static grub_cryptodisk_t
-luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
+luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
 {
   grub_cryptodisk_t cryptodisk;
   grub_luks2_header_t header;
   char uuid[sizeof (header.uuid) + 1];
   grub_size_t i, j;
 
-  if (check_boot)
+  if (cargs->check_boot)
     return NULL;
 
   if (luks2_read_header (disk, &header))
@@ -367,7 +367,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
       uuid[j++] = header.uuid[i];
   uuid[j] = '\0';
 
-  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
     return NULL;
 
   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 5bd970692..2823fa80f 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -69,7 +69,14 @@ typedef gcry_err_code_t
 
 struct grub_cryptomount_args
 {
+  /* scan: Flag to indicate that only bootable volumes should be decrypted */
+  grub_uint32_t check_boot : 1;
+  grub_uint32_t found_uuid : 1;
+  /* scan: Only volumes matching this UUID should be decrpyted */
+  char *search_uuid;
+  /* recover_key: Key data used to decrypt voume */
   grub_uint8_t *key_data;
+  /* recover_key: Length of key_data */
   grub_size_t key_len;
 };
 typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
@@ -125,8 +132,7 @@ struct grub_cryptodisk_dev
   struct grub_cryptodisk_dev *next;
   struct grub_cryptodisk_dev **prev;
 
-  grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
-			     int boot_only);
+  grub_cryptodisk_t (*scan) (grub_disk_t disk, grub_cryptomount_args_t cargs);
   grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
-- 
2.27.0

[PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Glenn Washburn]

The member found_uuid was never used by the crypto-backends, but was used to
determine if a crypto-backend successfully mounted a cryptodisk with a given
uuid. This is not needed however, because grub_device_iterate will return 1
iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
will only return 1 if a search_uuid has been specified and a cryptodisk was
successfully setup by a crypto-backend. So the return value of
grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
exception of the case where a mount is requested or an already opened
crypto device.

With this change grub_device_iterate will return 1 when
grub_cryptodisk_scan_device when a crypto device is successfully decrypted
or when the source device has already been successfully opened. Prior to
this change, trying to mount an already successfully opened device would
trigger an error with the message "no such cryptodisk found", which is at
best misleading. The mount should silently succeed in this case, which is
what happens with this patch.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5b38606ed..8e5277314 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
 
     grub_cryptodisk_insert (dev, name, source);
 
-    cargs->found_uuid = 1;
-
     goto cleanup;
   }
   goto cleanup;
@@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
   
   if (err)
     grub_print_error ();
-  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
+  return (!err && cargs->search_uuid) ? 1 : 0;
 }
 
 static grub_err_t
@@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
   if (state[0].set) /* uuid */
     {
+      int found_uuid = 0;
       grub_cryptodisk_t dev;
 
       dev = grub_cryptodisk_get_by_uuid (args[0]);
@@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
       cargs.check_boot = state[2].set;
       cargs.search_uuid = args[0];
-      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
+      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
 
-      if (!cargs.found_uuid)
+      if (!found_uuid)
 	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
       return GRUB_ERR_NONE;
     }
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 2823fa80f..c6524c9ea 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -71,7 +71,6 @@ struct grub_cryptomount_args
 {
   /* scan: Flag to indicate that only bootable volumes should be decrypted */
   grub_uint32_t check_boot : 1;
-  grub_uint32_t found_uuid : 1;
   /* scan: Only volumes matching this UUID should be decrpyted */
   char *search_uuid;
   /* recover_key: Key data used to decrypt voume */
-- 
2.27.0

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

[Patrick Steinhardt]

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

On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
>  grub-core/disk/geli.c       |  9 ++++-----
>  grub-core/disk/luks.c       | 11 +++++------
>  grub-core/disk/luks2.c      |  6 +++---
>  include/grub/cryptodisk.h   | 10 ++++++++--
>  5 files changed, 29 insertions(+), 33 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index a5f7b860c..5b38606ed 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);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>  
>      grub_cryptodisk_insert (dev, name, source);
>  
> -    have_it = 1;
> +    cargs->found_uuid = 1;
>  
>      goto cleanup;
>    }
> @@ -1091,7 +1088,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);

If I didn't get this wrong, then all scan implementations
unconditionally dereference the `grub_cryptomount_args_t` pointer.
So why does this work, and shouldn't we pass down a struct which has the
`search_uuid` and `check_boot` parameters properly set up?

Patrick

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

Re: [PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto modules

[Patrick Steinhardt]

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

On Tue, Oct 12, 2021 at 06:26:25PM -0500, Glenn Washburn wrote:
> Updates since v2:
> * Add space after function name in function calls
> * Add comments for members of struct grub_cryptomount_args
> * Correct commit message for patch #4
> 
> ---
> 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.
> 
> Patch #3 gets rid of some long time globals in cryptodisk, moving them
> into the passed struct.
> 
> Patch #4 removes the found_uuid flag from the cargs struct, which is not
> needed because the same information can be obtained from the return value
> of grub_device_iterate.
> 
> 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

A single question for patch 3/4, but all the others look good to me.
Feel free to add "Reviewed-by: Patrick Steinhardt <ps@pks.im>" to those.

Patrick

> Glenn Washburn (4):
>   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
>   cryptodisk: Remove unneeded found_uuid from cryptomount args
> 
>  grub-core/disk/cryptodisk.c | 108 ++++++++++++++++++++++++------------
>  grub-core/disk/geli.c       |  35 ++++--------
>  grub-core/disk/luks.c       |  37 ++++--------
>  grub-core/disk/luks2.c      |  33 ++++-------
>  include/grub/cryptodisk.h   |  19 ++++++-
>  5 files changed, 120 insertions(+), 112 deletions(-)
> 
> Range-diff against v2:
> 1:  475bf7e9e ! 1:  83112518f cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
>     @@ grub-core/disk/cryptodisk.c: static grub_err_t
>      +  if (state[3].set) /* password */
>      +    {
>      +      cargs.key_data = (grub_uint8_t *) state[3].arg;
>     -+      cargs.key_len = grub_strlen(state[3].arg);
>     ++      cargs.key_len = grub_strlen (state[3].arg);
>      +    }
>      +
>         have_it = 0;
> 2:  a0c0694fc ! 2:  65a18c5e8 cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
>     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
>      +	    ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
>      +	    goto error;
>      +	  }
>     -+	cargs->key_len = grub_strlen((char *) cargs->key_data);
>     ++	cargs->key_len = grub_strlen ((char *) cargs->key_data);
>      +      }
>      +
>      +    ret = cr->recover_key (source, dev, cargs);
>     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
>      +  if (askpass)
>      +    {
>      +      cargs->key_len = 0;
>     -+      grub_free(cargs->key_data);
>     ++      grub_free (cargs->key_data);
>      +    }
>      +  return ret;
>       }
> 3:  419f114d8 ! 3:  fed38b3dc cryptodisk: Move global variables into grub_cryptomount_args struct
>     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
>       
>       static grub_err_t
>      @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>     -       cargs.key_len = grub_strlen(state[3].arg);
>     +       cargs.key_len = grub_strlen (state[3].arg);
>           }
>       
>      -  have_it = 0;
>     @@ include/grub/cryptodisk.h: typedef gcry_err_code_t
>       
>       struct grub_cryptomount_args
>       {
>     ++  /* scan: Flag to indicate that only bootable volumes should be decrypted */
>      +  grub_uint32_t check_boot : 1;
>      +  grub_uint32_t found_uuid : 1;
>     ++  /* scan: Only volumes matching this UUID should be decrpyted */
>      +  char *search_uuid;
>     ++  /* recover_key: Key data used to decrypt voume */
>         grub_uint8_t *key_data;
>     ++  /* recover_key: Length of key_data */
>         grub_size_t key_len;
>       };
>     + typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
>      @@ include/grub/cryptodisk.h: struct grub_cryptodisk_dev
>         struct grub_cryptodisk_dev *next;
>         struct grub_cryptodisk_dev **prev;
> 4:  e6e1e8bc3 ! 4:  854709929 cryptodisk: Remove unneeded found_uuid from cryptomount args
>     @@ Commit message
>          iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
>          will only return 1 if a search_uuid has been specified and a cryptodisk was
>          successfully setup by a crypto-backend. So the return value of
>     -    grub_cryptodisk_scan_device is equivalent to found_uuid.
>     +    grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
>     +    exception of the case where a mount is requested or an already opened
>     +    crypto device.
>     +
>     +    With this change grub_device_iterate will return 1 when
>     +    grub_cryptodisk_scan_device when a crypto device is successfully decrypted
>     +    or when the source device has already been successfully opened. Prior to
>     +    this change, trying to mount an already successfully opened device would
>     +    trigger an error with the message "no such cryptodisk found", which is at
>     +    best misleading. The mount should silently succeed in this case, which is
>     +    what happens with this patch.
>      
>       ## grub-core/disk/cryptodisk.c ##
>      @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
>     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
>           }
>      
>       ## include/grub/cryptodisk.h ##
>     -@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
>     - struct grub_cryptomount_args
>     +@@ include/grub/cryptodisk.h: struct grub_cryptomount_args
>       {
>     +   /* scan: Flag to indicate that only bootable volumes should be decrypted */
>         grub_uint32_t check_boot : 1;
>      -  grub_uint32_t found_uuid : 1;
>     +   /* scan: Only volumes matching this UUID should be decrpyted */
>         char *search_uuid;
>     -   grub_uint8_t *key_data;
>     -   grub_size_t key_len;
>     +   /* recover_key: Key data used to decrypt voume */
> -- 
> 2.27.0
> 

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

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

[Daniel Kiper]

On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.

I am not very happy with that. Splitting this into separate patch or
merging with patch #2 probably would not help either.

> However, the backends are not implemented, so testing this will return a not
> implemented error.

The commit message lacks of explanation why this change is needed.
I think you can copy part of the cover letter here.

> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
>  grub-core/disk/geli.c       |  6 +++++-
>  grub-core/disk/luks.c       |  7 ++++++-
>  grub-core/disk/luks2.c      |  7 ++++++-
>  include/grub/cryptodisk.h   |  9 ++++++++-
>  5 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..577942088 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},

Should not you update docs/grub.texi as well?

>      {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,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
>      if (!dev)
>        continue;
>
> -    err = cr->recover_key (source, dev);
> +    err = cr->recover_key (source, dev, cargs);
>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -1080,10 +1083,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 +1097,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 +1110,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 */

Nice but if you want to do that please put that change into separate patch.
Or at least advise in the commit message you are doing this on occasion.

>      {
>        grub_cryptodisk_t dev;
>
> @@ -1125,18 +1136,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 */

Ditto.

>      {
>        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 +1189,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 +1328,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>"),

s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?

>  			      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..4e8c377e7 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>  }
>
>  static grub_err_t
> -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> +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];
> @@ -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 (cargs->key_data || cargs->key_len)

if (cargs->key_data != NULL || 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..0462edc6e 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>
>  static grub_err_t
>  luks_recover_key (grub_disk_t source,
> -		  grub_cryptodisk_t dev)
> +		  grub_cryptodisk_t dev,
> +		  grub_cryptomount_args_t cargs)
>  {
>    struct grub_luks_phdr header;
>    grub_size_t keysize;
> @@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
>    grub_size_t max_stripes = 1;
>    char *tmp;
>
> +  /* Keyfiles are not implemented yet */
> +  if (cargs->key_data || cargs->key_len)

Ditto.

> +     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..455a78cb0 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>
>  static grub_err_t
>  luks2_recover_key (grub_disk_t source,
> -		   grub_cryptodisk_t crypt)
> +		   grub_cryptodisk_t crypt,
> +		   grub_cryptomount_args_t cargs)
>  {
>    grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
>    char passphrase[MAX_PASSPHRASE], cipher[32];
> @@ -556,6 +557,10 @@ luks2_recover_key (grub_disk_t source,
>    grub_json_t *json = NULL, keyslots;
>    grub_err_t ret;
>
> +  /* Keyfiles are not implemented yet */
> +  if (cargs->key_data || cargs->key_len)

Ditto.

> +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
>    ret = luks2_read_header (source, &header);
>    if (ret)
>      return ret;

Daniel

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

[Daniel Kiper]

On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> 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.

Mostly nitpicks below...

> 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
>        return grub_errno;
>      if (!dev)
>        continue;
> -
> -    err = cr->recover_key (source, dev, cargs);
> -    if (err)
> -    {
> -      cryptodisk_close (dev);
> -      return err;
> -    }
> +
> +    if (cargs->key_len == 0)

I am OK with "if (!cargs->key_len)" for all kinds of numeric values...

> +      {
> +	/* Get the passphrase from the user, if no key data. */
> +	askpass = 1;
> +	if (source->partition)

...but not for the pointers and enums.

s/if (source->partition)/if (source->partition != NULL)/

> +	  part = grub_partition_get_name (source->partition);
> +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +		     source->partition ? "," : "", part ? : "",

s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/

s/part ? : ""/part != NULL ? part : "NO NAME"/?

> +		     dev->uuid);
> +	grub_free (part);
> +
> +	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> +	if (!cargs->key_data)

Ditto and below please...

> +	  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");

All errors printed by grub_error() should start with lower case...

> +	    goto error;
> +	  }
> +	cargs->key_len = grub_strlen ((char *) cargs->key_data);
> +      }
> +
> +    ret = cr->recover_key (source, dev, cargs);
> +    if (ret)

if (ret != GRUB_ERR_NONE)

> +      goto error;
>
>      grub_cryptodisk_insert (dev, name, source);
>
>      have_it = 1;
>
> -    return GRUB_ERR_NONE;
> +    goto cleanup;
>    }
> -  return GRUB_ERR_NONE;
> +  goto cleanup;
> +
> +error:

Please put space before labels.

> +  cryptodisk_close (dev);

I would add empty line here.

> +cleanup:

Please put space before labels.

> +  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 4e8c377e7..32f34d5c3 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_cryptomount_args_t
>    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 (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +  if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

All errors printed by grub_error() should start with lower case...

>    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_cryptomount_args_t
>
>    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, cargs->key_data,
> +				     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, grub_cryptomount_args_t
>  	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, cargs->key_data, 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 0462edc6e..51646cefe 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 */
> @@ -158,17 +156,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 (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> + if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

Same as above...

>    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
>    if (err)
> @@ -188,20 +183,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++)
>      {
> @@ -216,8 +197,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, cargs->key_data,
> +				     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 455a78cb0..c77380cbb 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,
> @@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
>  		   grub_cryptomount_args_t cargs)
>  {
>    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];

If you change that could you add a comment why cipher length is equal 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;
> @@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source,
>    grub_json_t *json = NULL, keyslots;
>    grub_err_t ret;
>
> -  /* Keyfiles are not implemented yet */
> -  if (cargs->key_data || cargs->key_len)
> -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +  if (cargs->key_data == NULL || cargs->key_len == 0)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");

Same as above...

Daniel

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

[Daniel Kiper]

On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
>  grub-core/disk/geli.c       |  9 ++++-----
>  grub-core/disk/luks.c       | 11 +++++------
>  grub-core/disk/luks2.c      |  6 +++---
>  include/grub/cryptodisk.h   | 10 ++++++++--
>  5 files changed, 29 insertions(+), 33 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index a5f7b860c..5b38606ed 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);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
>
>      grub_cryptodisk_insert (dev, name, source);
>
> -    have_it = 1;
> +    cargs->found_uuid = 1;

Please say in the commit message you are changing variable/member name too.
Or maybe it would be better if you do this in separate patch.

>      goto cleanup;
>    }
> @@ -1091,7 +1088,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);
>      if (grub_errno)
>        return grub_errno;
>      if (!dev)
> @@ -1135,7 +1132,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;

I think this should be enough:
  return (cargs->found_uuid && cargs->search_uuid != NULL)

>  }
>
>  static grub_err_t
> @@ -1153,7 +1150,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        cargs.key_len = grub_strlen (state[3].arg);
>      }
>
> -  have_it = 0;

cargs->found_uuid = 0?

>    if (state[0].set) /* uuid */
>      {
>        grub_cryptodisk_t dev;
> @@ -1166,21 +1162,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;

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;

Ditto. If this is correct then I think it should be shortly explained in
the commit message why you can drop these assignments.

>        return GRUB_ERR_NONE;
>      }
>    else
> @@ -1192,8 +1185,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        char *disklast = NULL;
>        grub_size_t len;
>
> -      search_uuid = NULL;

Ditto.

> -      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/grub-core/disk/geli.c b/grub-core/disk/geli.c
> index 32f34d5c3..32d35521b 100644
> --- a/grub-core/disk/geli.c
> +++ b/grub-core/disk/geli.c
> @@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
>  #endif
>
>  static grub_cryptodisk_t
> -configure_ciphers (grub_disk_t disk, const char *check_uuid,
> -		   int boot_only)
> +configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
>  {
>    grub_cryptodisk_t newdev;
>    struct grub_geli_phdr header;
> @@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>        return NULL;
>      }
>
> -  if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
> +  if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
>      {
>        grub_dprintf ("geli", "not a boot volume\n");
>        return NULL;
> @@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>        return NULL;
>      }
>
> -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
>      {
> -      grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
> +      grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
>        return NULL;
>      }
>
> diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> index 51646cefe..6ced312c7 100644
> --- a/grub-core/disk/luks.c
> +++ b/grub-core/disk/luks.c
> @@ -63,8 +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, const char *check_uuid,
> -		   int check_boot)
> +configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
>  {
>    grub_cryptodisk_t newdev;
>    const char *iptr;
> @@ -76,7 +75,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>    char hashspec[sizeof (header.hashSpec) + 1];
>    grub_err_t err;
>
> -  if (check_boot)
> +  if (cargs->check_boot)
>      return NULL;
>
>    /* Read the LUKS header.  */
> @@ -103,9 +102,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
>      }
>    *optr = 0;
>
> -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)

if (cargs->search_uuid != NULL...

Please fix similar things below too.

>      {
> -      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
> +      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
>        return NULL;
>      }
>
> @@ -162,7 +161,7 @@ luks_recover_key (grub_disk_t source,
>    grub_err_t err;
>    grub_size_t max_stripes = 1;
>
> - if (cargs->key_data == NULL || cargs->key_len == 0)
> +  if (cargs->key_data == NULL || cargs->key_len == 0)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
>    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> index c77380cbb..28fad54aa 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -346,14 +346,14 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
>  }
>
>  static grub_cryptodisk_t
> -luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
> +luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
>  {
>    grub_cryptodisk_t cryptodisk;
>    grub_luks2_header_t header;
>    char uuid[sizeof (header.uuid) + 1];
>    grub_size_t i, j;
>
> -  if (check_boot)
> +  if (cargs->check_boot)
>      return NULL;
>
>    if (luks2_read_header (disk, &header))
> @@ -367,7 +367,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
>        uuid[j++] = header.uuid[i];
>    uuid[j] = '\0';
>
> -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
>      return NULL;
>
>    cryptodisk = grub_zalloc (sizeof (*cryptodisk));
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 5bd970692..2823fa80f 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -69,7 +69,14 @@ typedef gcry_err_code_t
>
>  struct grub_cryptomount_args
>  {
> +  /* scan: Flag to indicate that only bootable volumes should be decrypted */
> +  grub_uint32_t check_boot : 1;

Could you add a comment what found_uuid flags means too?

> +  grub_uint32_t found_uuid : 1;
> +  /* scan: Only volumes matching this UUID should be decrpyted */
> +  char *search_uuid;
> +  /* recover_key: Key data used to decrypt voume */
>    grub_uint8_t *key_data;
> +  /* recover_key: Length of key_data */
>    grub_size_t key_len;
>  };

Please address Patrick's concern expressed in the other email too.

Daniel

Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Daniel Kiper]

On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> The member found_uuid was never used by the crypto-backends, but was used to

Ha! Could you make this patch second in this patch series? Then we could
avoid carrying over have_it/found_uuid cruft over succeeding patches.

> determine if a crypto-backend successfully mounted a cryptodisk with a given
> uuid. This is not needed however, because grub_device_iterate will return 1
> iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device

s/iff/if/

> will only return 1 if a search_uuid has been specified and a cryptodisk was
> successfully setup by a crypto-backend. So the return value of
> grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
> exception of the case where a mount is requested or an already opened
> crypto device.
>
> With this change grub_device_iterate will return 1 when

I like if you add "()" to function names in comments, etc.

> grub_cryptodisk_scan_device when a crypto device is successfully decrypted

I think one "when" is redundant here. Or something else is wrong.

> or when the source device has already been successfully opened. Prior to
> this change, trying to mount an already successfully opened device would
> trigger an error with the message "no such cryptodisk found", which is at
> best misleading. The mount should silently succeed in this case, which is
> what happens with this patch.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
>  grub-core/disk/cryptodisk.c | 9 ++++-----
>  include/grub/cryptodisk.h   | 1 -
>  2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 5b38606ed..8e5277314 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
>
>      grub_cryptodisk_insert (dev, name, source);
>
> -    cargs->found_uuid = 1;
> -
>      goto cleanup;
>    }
>    goto cleanup;
> @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
>
>    if (err)
>      grub_print_error ();
> -  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> +  return (!err && cargs->search_uuid) ? 1 : 0;

err == GRUB_ERR_NONE && cargs->search_uuid != NULL

>  }
>
>  static grub_err_t
> @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>
>    if (state[0].set) /* uuid */
>      {
> +      int found_uuid = 0;

Zero initialization seems redundant here.

>        grub_cryptodisk_t dev;
>
>        dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>
>        cargs.check_boot = state[2].set;
>        cargs.search_uuid = args[0];
> -      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
>
> -      if (!cargs.found_uuid)
> +      if (!found_uuid)
>  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
>        return GRUB_ERR_NONE;
>      }

Daniel

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

[Glenn Washburn]

On Wed, 17 Nov 2021 18:29:36 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> > As an example, passing a password as a cryptomount argument is implemented.
> 
> I am not very happy with that. Splitting this into separate patch or
> merging with patch #2 probably would not help either.

Its not clear to me what action you're desiring. I don't particularly
like it either, but haven't thought of something better. Ideas?

> > However, the backends are not implemented, so testing this will return a not
> > implemented error.
> 
> The commit message lacks of explanation why this change is needed.
> I think you can copy part of the cover letter here.

I can add an explanation.

> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
> >  grub-core/disk/geli.c       |  6 +++++-
> >  grub-core/disk/luks.c       |  7 ++++++-
> >  grub-core/disk/luks2.c      |  7 ++++++-
> >  include/grub/cryptodisk.h   |  9 ++++++++-
> >  5 files changed, 46 insertions(+), 14 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..577942088 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},
> 
> Should not you update docs/grub.texi as well?

Yep, good catch. I think the doc change should be in patch #2 because
that's where the option actually becomes useful. What do you think?

> >      {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,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> >      if (!dev)
> >        continue;
> >
> > -    err = cr->recover_key (source, dev);
> > +    err = cr->recover_key (source, dev, cargs);
> >      if (err)
> >      {
> >        cryptodisk_close (dev);
> > @@ -1080,10 +1083,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 +1097,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 +1110,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 */
> 
> Nice but if you want to do that please put that change into separate patch.
> Or at least advise in the commit message you are doing this on occasion.

I'll add a sentence to the commit message.

> >      {
> >        grub_cryptodisk_t dev;
> >
> > @@ -1125,18 +1136,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 */
> 
> Ditto.
> 
> >      {
> >        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 +1189,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 +1328,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>"),
> 
> s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?

The change you're suggesting indicates that "cryptomount -u UUID -p
password" is not correct usage of the command, only "cryptomount -p
password". My version doesn't support that either, but it does indicate
that "cryptomount -p password -u UUID" is an option. The idea behind my
version is that "-p password" is optional and can be used with any of
the other options, but not alone. So I don't believe the suggestion is
correct.

> >  			      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..4e8c377e7 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >  }
> >
> >  static grub_err_t
> > -recover_key (grub_disk_t source, grub_cryptodisk_t dev)
> > +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];
> > @@ -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 (cargs->key_data || cargs->key_len)
> 
> if (cargs->key_data != NULL || 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..0462edc6e 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >
> >  static grub_err_t
> >  luks_recover_key (grub_disk_t source,
> > -		  grub_cryptodisk_t dev)
> > +		  grub_cryptodisk_t dev,
> > +		  grub_cryptomount_args_t cargs)
> >  {
> >    struct grub_luks_phdr header;
> >    grub_size_t keysize;
> > @@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
> >    grub_size_t max_stripes = 1;
> >    char *tmp;
> >
> > +  /* Keyfiles are not implemented yet */
> > +  if (cargs->key_data || cargs->key_len)
> 
> Ditto.
> 
> > +     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..455a78cb0 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
> >
> >  static grub_err_t
> >  luks2_recover_key (grub_disk_t source,
> > -		   grub_cryptodisk_t crypt)
> > +		   grub_cryptodisk_t crypt,
> > +		   grub_cryptomount_args_t cargs)
> >  {
> >    grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
> >    char passphrase[MAX_PASSPHRASE], cipher[32];
> > @@ -556,6 +557,10 @@ luks2_recover_key (grub_disk_t source,
> >    grub_json_t *json = NULL, keyslots;
> >    grub_err_t ret;
> >
> > +  /* Keyfiles are not implemented yet */
> > +  if (cargs->key_data || cargs->key_len)
> 
> Ditto.
> 
> > +     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +
> >    ret = luks2_read_header (source, &header);
> >    if (ret)
> >      return ret;
> 
> Daniel

Glenn

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

[Glenn Washburn]

On Wed, 17 Nov 2021 20:10:21 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > 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.
> 
> Mostly nitpicks below...
> 
> > 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> >        return grub_errno;
> >      if (!dev)
> >        continue;
> > -
> > -    err = cr->recover_key (source, dev, cargs);
> > -    if (err)
> > -    {
> > -      cryptodisk_close (dev);
> > -      return err;
> > -    }
> > +
> > +    if (cargs->key_len == 0)
> 
> I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> 
> > +      {
> > +	/* Get the passphrase from the user, if no key data. */
> > +	askpass = 1;
> > +	if (source->partition)
> 
> ...but not for the pointers and enums.
> 
> s/if (source->partition)/if (source->partition != NULL)/
> 
> > +	  part = grub_partition_get_name (source->partition);
> > +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > +		     source->partition ? "," : "", part ? : "",
> 
> s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
> 
> s/part ? : ""/part != NULL ? part : "NO NAME"/?

Ok, when moving code, I generally don't like to change it unless
necesary. I'll add these changes.

> > +		     dev->uuid);
> > +	grub_free (part);
> > +
> > +	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > +	if (!cargs->key_data)
> 
> Ditto and below please...
> 
> > +	  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");
> 
> All errors printed by grub_error() should start with lower case...

Ok, I'll try to keep that in mind. There's quite a few grub_error()
calls in cryptodisk that do not conform to that (and I expect else
where in the source).

> > +	    goto error;
> > +	  }
> > +	cargs->key_len = grub_strlen ((char *) cargs->key_data);
> > +      }
> > +
> > +    ret = cr->recover_key (source, dev, cargs);
> > +    if (ret)
> 
> if (ret != GRUB_ERR_NONE)
> 
> > +      goto error;
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> >      have_it = 1;
> >
> > -    return GRUB_ERR_NONE;
> > +    goto cleanup;
> >    }
> > -  return GRUB_ERR_NONE;
> > +  goto cleanup;
> > +
> > +error:
> 
> Please put space before labels.

Are you saying you want the line to be " error:"? There are labels in
the source which are preceeded by whitespace, but they seem to be in
the minority. What's the rationale for this? or is it just personal
preference? I don't mind making this change, but I don't understand it.

> 
> > +  cryptodisk_close (dev);
> 
> I would add empty line here.
> 
> > +cleanup:
> 
> Please put space before labels.
> 
> > +  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 4e8c377e7..32f34d5c3 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_cryptomount_args_t
> >    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 (cargs->key_data || cargs->key_len)
> > -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +  if (cargs->key_data == NULL || cargs->key_len == 0)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
> 
> All errors printed by grub_error() should start with lower case...
> 
> >    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_cryptomount_args_t
> >
> >    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, cargs->key_data,
> > +				     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, grub_cryptomount_args_t
> >  	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, cargs->key_data, 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 0462edc6e..51646cefe 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 */
> > @@ -158,17 +156,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 (cargs->key_data || cargs->key_len)
> > -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > + if (cargs->key_data == NULL || cargs->key_len == 0)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
> 
> Same as above...
> 
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> >    if (err)
> > @@ -188,20 +183,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++)
> >      {
> > @@ -216,8 +197,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, cargs->key_data,
> > +				     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 455a78cb0..c77380cbb 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,
> > @@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
> >  		   grub_cryptomount_args_t cargs)
> >  {
> >    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];
> 
> If you change that could you add a comment why cipher length is equal 32?

I'm not sure why. I think that's a question for Patrick. I'd guess he
figured it was a resonable upper bound on the length of the cipher
string.

> > +  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;
> > @@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source,
> >    grub_json_t *json = NULL, keyslots;
> >    grub_err_t ret;
> >
> > -  /* Keyfiles are not implemented yet */
> > -  if (cargs->key_data || cargs->key_len)
> > -     return GRUB_ERR_NOT_IMPLEMENTED_YET;
> > +  if (cargs->key_data == NULL || cargs->key_len == 0)
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
> 
> Same as above...
> 
> Daniel

Glenn

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

[Glenn Washburn]

On Sun, 14 Nov 2021 10:56:15 +0100
Patrick Steinhardt <ps@pks.im> wrote:

> On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> >  grub-core/disk/geli.c       |  9 ++++-----
> >  grub-core/disk/luks.c       | 11 +++++------
> >  grub-core/disk/luks2.c      |  6 +++---
> >  include/grub/cryptodisk.h   | 10 ++++++++--
> >  5 files changed, 29 insertions(+), 33 deletions(-)
> > 
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index a5f7b860c..5b38606ed 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);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >  
> >      grub_cryptodisk_insert (dev, name, source);
> >  
> > -    have_it = 1;
> > +    cargs->found_uuid = 1;
> >  
> >      goto cleanup;
> >    }
> > @@ -1091,7 +1088,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);
> 
> If I didn't get this wrong, then all scan implementations
> unconditionally dereference the `grub_cryptomount_args_t` pointer.
> So why does this work, and shouldn't we pass down a struct which has the
> `search_uuid` and `check_boot` parameters properly set up?

I'm not sure that this does work, good catch. I don't have a setup to
test GRUB utils in conjunction with cryptodisk. If you do (or anyone
else does), testing would be greatly appreciated. Regardless, I believe
this is an issue.

My reading of the previous code is that search_uuid and check_boot are
not explicitly initialized here. The only reasonable conclusion I come
to is that they are initialized to zero by default by the compiler. So
I'll create a cargs struct with all fields initalized to zero and pass
that in. Does this sound good?

Glenn

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

[Glenn Washburn]

On Thu, 18 Nov 2021 15:06:56 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:28PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> >  grub-core/disk/geli.c       |  9 ++++-----
> >  grub-core/disk/luks.c       | 11 +++++------
> >  grub-core/disk/luks2.c      |  6 +++---
> >  include/grub/cryptodisk.h   | 10 ++++++++--
> >  5 files changed, 29 insertions(+), 33 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index a5f7b860c..5b38606ed 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);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> > -    have_it = 1;
> > +    cargs->found_uuid = 1;
> 
> Please say in the commit message you are changing variable/member name too.
> Or maybe it would be better if you do this in separate patch.

I'll see about moving the next patch such that this is not a concern.

> >      goto cleanup;
> >    }
> > @@ -1091,7 +1088,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);
> >      if (grub_errno)
> >        return grub_errno;
> >      if (!dev)
> > @@ -1135,7 +1132,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;
> 
> I think this should be enough:
>   return (cargs->found_uuid && cargs->search_uuid != NULL)
> 
> >  }
> >
> >  static grub_err_t
> > @@ -1153,7 +1150,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >        cargs.key_len = grub_strlen (state[3].arg);
> >      }
> >
> > -  have_it = 0;
> 
> cargs->found_uuid = 0?
> 
> >    if (state[0].set) /* uuid */
> >      {
> >        grub_cryptodisk_t dev;
> > @@ -1166,21 +1162,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;
> 
> cargs.search_uuid = NULL?

The initializer used for the struct declaration should already take
care of this.

> 
> > -      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;
> 
> Ditto. If this is correct then I think it should be shortly explained in
> the commit message why you can drop these assignments.

Since the storage for search_uuid is not global anymore, the struct
initializer tkes care of this. I'll add a line in the commit message.

> >        return GRUB_ERR_NONE;
> >      }
> >    else
> > @@ -1192,8 +1185,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >        char *disklast = NULL;
> >        grub_size_t len;
> >
> > -      search_uuid = NULL;
> 
> Ditto.
> 
> > -      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/grub-core/disk/geli.c b/grub-core/disk/geli.c
> > index 32f34d5c3..32d35521b 100644
> > --- a/grub-core/disk/geli.c
> > +++ b/grub-core/disk/geli.c
> > @@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
> >  #endif
> >
> >  static grub_cryptodisk_t
> > -configure_ciphers (grub_disk_t disk, const char *check_uuid,
> > -		   int boot_only)
> > +configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >  {
> >    grub_cryptodisk_t newdev;
> >    struct grub_geli_phdr header;
> > @@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >        return NULL;
> >      }
> >
> > -  if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
> > +  if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
> >      {
> >        grub_dprintf ("geli", "not a boot volume\n");
> >        return NULL;
> > @@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >        return NULL;
> >      }
> >
> > -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> > +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> >      {
> > -      grub_dprintf ("geli", "%s != %s\n", uuid, check_uuid);
> > +      grub_dprintf ("geli", "%s != %s\n", uuid, cargs->search_uuid);
> >        return NULL;
> >      }
> >
> > diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
> > index 51646cefe..6ced312c7 100644
> > --- a/grub-core/disk/luks.c
> > +++ b/grub-core/disk/luks.c
> > @@ -63,8 +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, const char *check_uuid,
> > -		   int check_boot)
> > +configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >  {
> >    grub_cryptodisk_t newdev;
> >    const char *iptr;
> > @@ -76,7 +75,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >    char hashspec[sizeof (header.hashSpec) + 1];
> >    grub_err_t err;
> >
> > -  if (check_boot)
> > +  if (cargs->check_boot)
> >      return NULL;
> >
> >    /* Read the LUKS header.  */
> > @@ -103,9 +102,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
> >      }
> >    *optr = 0;
> >
> > -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> > +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> 
> if (cargs->search_uuid != NULL...
> 
> Please fix similar things below too.
> 
> >      {
> > -      grub_dprintf ("luks", "%s != %s\n", uuid, check_uuid);
> > +      grub_dprintf ("luks", "%s != %s\n", uuid, cargs->search_uuid);
> >        return NULL;
> >      }
> >
> > @@ -162,7 +161,7 @@ luks_recover_key (grub_disk_t source,
> >    grub_err_t err;
> >    grub_size_t max_stripes = 1;
> >
> > - if (cargs->key_data == NULL || cargs->key_len == 0)
> > +  if (cargs->key_data == NULL || cargs->key_len == 0)
> >      return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
> >    err = grub_disk_read (source, 0, 0, sizeof (header), &header);
> > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > index c77380cbb..28fad54aa 100644
> > --- a/grub-core/disk/luks2.c
> > +++ b/grub-core/disk/luks2.c
> > @@ -346,14 +346,14 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
> >  }
> >
> >  static grub_cryptodisk_t
> > -luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
> > +luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
> >  {
> >    grub_cryptodisk_t cryptodisk;
> >    grub_luks2_header_t header;
> >    char uuid[sizeof (header.uuid) + 1];
> >    grub_size_t i, j;
> >
> > -  if (check_boot)
> > +  if (cargs->check_boot)
> >      return NULL;
> >
> >    if (luks2_read_header (disk, &header))
> > @@ -367,7 +367,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
> >        uuid[j++] = header.uuid[i];
> >    uuid[j] = '\0';
> >
> > -  if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
> > +  if (cargs->search_uuid && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
> >      return NULL;
> >
> >    cryptodisk = grub_zalloc (sizeof (*cryptodisk));
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 5bd970692..2823fa80f 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -69,7 +69,14 @@ typedef gcry_err_code_t
> >
> >  struct grub_cryptomount_args
> >  {
> > +  /* scan: Flag to indicate that only bootable volumes should be decrypted */
> > +  grub_uint32_t check_boot : 1;
> 
> Could you add a comment what found_uuid flags means too?
> 
> > +  grub_uint32_t found_uuid : 1;
> > +  /* scan: Only volumes matching this UUID should be decrpyted */
> > +  char *search_uuid;
> > +  /* recover_key: Key data used to decrypt voume */
> >    grub_uint8_t *key_data;
> > +  /* recover_key: Length of key_data */
> >    grub_size_t key_len;
> >  };
> 
> Please address Patrick's concern expressed in the other email too.
> 
> Daniel

Glenn

Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Glenn Washburn]

On Thu, 18 Nov 2021 15:25:44 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> > The member found_uuid was never used by the crypto-backends, but was used to
> 
> Ha! Could you make this patch second in this patch series? Then we could
> avoid carrying over have_it/found_uuid cruft over succeeding patches.

Sure, I was avoiding do that work, but since you've requested it, I'll
take a stab at it. I'm thinking I'll make it the first patch though, so
its independent from the rest of the series.

> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate will return 1
> > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
> 
> s/iff/if/

"iff" is short hand for "if and only if". I'll expand it.

> > will only return 1 if a search_uuid has been specified and a cryptodisk was
> > successfully setup by a crypto-backend. So the return value of
> > grub_cryptodisk_scan_device is almost equivalent to found_uuid, with the
> > exception of the case where a mount is requested or an already opened
> > crypto device.
> >
> > With this change grub_device_iterate will return 1 when
> 
> I like if you add "()" to function names in comments, etc.
> 
> > grub_cryptodisk_scan_device when a crypto device is successfully decrypted
> 
> I think one "when" is redundant here. Or something else is wrong.

Indeed, I'll fix this.

> > or when the source device has already been successfully opened. Prior to
> > this change, trying to mount an already successfully opened device would
> > trigger an error with the message "no such cryptodisk found", which is at
> > best misleading. The mount should silently succeed in this case, which is
> > what happens with this patch.
> >
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> >  grub-core/disk/cryptodisk.c | 9 ++++-----
> >  include/grub/cryptodisk.h   | 1 -
> >  2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 5b38606ed..8e5277314 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> > -    cargs->found_uuid = 1;
> > -
> >      goto cleanup;
> >    }
> >    goto cleanup;
> > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
> >
> >    if (err)
> >      grub_print_error ();
> > -  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > +  return (!err && cargs->search_uuid) ? 1 : 0;
> 
> err == GRUB_ERR_NONE && cargs->search_uuid != NULL
> 
> >  }
> >
> >  static grub_err_t
> > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >
> >    if (state[0].set) /* uuid */
> >      {
> > +      int found_uuid = 0;
> 
> Zero initialization seems redundant here.

It is. I'll remove the initializer.

> >        grub_cryptodisk_t dev;
> >
> >        dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >
> >        cargs.check_boot = state[2].set;
> >        cargs.search_uuid = args[0];
> > -      grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> > +      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> >
> > -      if (!cargs.found_uuid)
> > +      if (!found_uuid)
> >  	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> >        return GRUB_ERR_NONE;
> >      }

Glenn

Re: [PATCH v3 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args

[Daniel Kiper]

On Thu, Dec 02, 2021 at 12:51:09AM -0600, Glenn Washburn wrote:
> On Thu, 18 Nov 2021 15:25:44 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Oct 12, 2021 at 06:26:29PM -0500, Glenn Washburn wrote:
> > > The member found_uuid was never used by the crypto-backends, but was used to
> >
> > Ha! Could you make this patch second in this patch series? Then we could
> > avoid carrying over have_it/found_uuid cruft over succeeding patches.
>
> Sure, I was avoiding do that work, but since you've requested it, I'll
> take a stab at it. I'm thinking I'll make it the first patch though, so
> its independent from the rest of the series.

If you want it to be the first I am OK with it.

> > > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > > uuid. This is not needed however, because grub_device_iterate will return 1
> > > iff grub_cryptodisk_scan_device returns 1. And grub_cryptodisk_scan_device
> >
> > s/iff/if/
>
> "iff" is short hand for "if and only if". I'll expand it.

Yeah, that would be better.

Daniel

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

[Daniel Kiper]

On Wed, Dec 01, 2021 at 03:18:06PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 18:29:36 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> > > As an example, passing a password as a cryptomount argument is implemented.
> >
> > I am not very happy with that. Splitting this into separate patch or
> > merging with patch #2 probably would not help either.
>
> Its not clear to me what action you're desiring. I don't particularly
> like it either, but haven't thought of something better. Ideas?

No, I do not have better one now. I am afraid we have to live with it.

> > > However, the backends are not implemented, so testing this will return a not
> > > implemented error.
> >
> > The commit message lacks of explanation why this change is needed.
> > I think you can copy part of the cover letter here.
>
> I can add an explanation.

Cool! Thanks!

> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > >  grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
> > >  grub-core/disk/geli.c       |  6 +++++-
> > >  grub-core/disk/luks.c       |  7 ++++++-
> > >  grub-core/disk/luks2.c      |  7 ++++++-
> > >  include/grub/cryptodisk.h   |  9 ++++++++-
> > >  5 files changed, 46 insertions(+), 14 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 90f82b2d3..577942088 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},
> >
> > Should not you update docs/grub.texi as well?
>
> Yep, good catch. I think the doc change should be in patch #2 because
> that's where the option actually becomes useful. What do you think?

Not perfect but I am OK with it.

[...]

> > > @@ -1317,7 +1328,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>"),
> >
> > s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?
>
> The change you're suggesting indicates that "cryptomount -u UUID -p
> password" is not correct usage of the command, only "cryptomount -p
> password". My version doesn't support that either, but it does indicate
> that "cryptomount -p password -u UUID" is an option. The idea behind my
> version is that "-p password" is optional and can be used with any of
> the other options, but not alone. So I don't believe the suggestion is
> correct.

OK, let's use your version.

Daniel

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

[Daniel Kiper]

On Wed, Dec 01, 2021 at 03:48:40PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 20:10:21 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > > 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.
> >
> > Mostly nitpicks below...
> >
> > > 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> > >        return grub_errno;
> > >      if (!dev)
> > >        continue;
> > > -
> > > -    err = cr->recover_key (source, dev, cargs);
> > > -    if (err)
> > > -    {
> > > -      cryptodisk_close (dev);
> > > -      return err;
> > > -    }
> > > +
> > > +    if (cargs->key_len == 0)
> >
> > I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> >
> > > +      {
> > > +	/* Get the passphrase from the user, if no key data. */
> > > +	askpass = 1;
> > > +	if (source->partition)
> >
> > ...but not for the pointers and enums.
> >
> > s/if (source->partition)/if (source->partition != NULL)/
> >
> > > +	  part = grub_partition_get_name (source->partition);
> > > +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > > +		     source->partition ? "," : "", part ? : "",
> >
> > s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
> >
> > s/part ? : ""/part != NULL ? part : "NO NAME"/?
>
> Ok, when moving code, I generally don't like to change it unless
> necesary. I'll add these changes.

Yeah, I agree but I would make an exception here.

> > > +		     dev->uuid);
> > > +	grub_free (part);
> > > +
> > > +	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> > > +	if (!cargs->key_data)
> >
> > Ditto and below please...
> >
> > > +	  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");
> >
> > All errors printed by grub_error() should start with lower case...
>
> Ok, I'll try to keep that in mind. There's quite a few grub_error()
> calls in cryptodisk that do not conform to that (and I expect else
> where in the source).

Yeah, it would be nice to fix it. If you do not mind please make a patch
and I will take it.

> > > +	    goto error;
> > > +	  }
> > > +	cargs->key_len = grub_strlen ((char *) cargs->key_data);
> > > +      }
> > > +
> > > +    ret = cr->recover_key (source, dev, cargs);
> > > +    if (ret)
> >
> > if (ret != GRUB_ERR_NONE)
> >
> > > +      goto error;
> > >
> > >      grub_cryptodisk_insert (dev, name, source);
> > >
> > >      have_it = 1;
> > >
> > > -    return GRUB_ERR_NONE;
> > > +    goto cleanup;
> > >    }
> > > -  return GRUB_ERR_NONE;
> > > +  goto cleanup;
> > > +
> > > +error:
> >
> > Please put space before labels.
>
> Are you saying you want the line to be " error:"? There are labels in
> the source which are preceeded by whitespace, but they seem to be in
> the minority. What's the rationale for this? or is it just personal
> preference? I don't mind making this change, but I don't understand it.

It differentiates a bit labels from e.g. functions names which starts in
the first column. I got used to it when I was working on the Linux
kernel and Xen. I can agree this is not perfect but...

[...]

> > > diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
> > > index 455a78cb0..c77380cbb 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,
> > > @@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
> > >  		   grub_cryptomount_args_t cargs)
> > >  {
> > >    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];
> >
> > If you change that could you add a comment why cipher length is equal 32?
>
> I'm not sure why. I think that's a question for Patrick. I'd guess he
> figured it was a resonable upper bound on the length of the cipher
> string.

Patrick, could you chime in?

Daniel

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

[Glenn Washburn]

On Wed, 17 Nov 2021 20:10:21 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > 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.
> 
> Mostly nitpicks below...
> 
> > 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> >        return grub_errno;
> >      if (!dev)
> >        continue;
> > -
> > -    err = cr->recover_key (source, dev, cargs);
> > -    if (err)
> > -    {
> > -      cryptodisk_close (dev);
> > -      return err;
> > -    }
> > +
> > +    if (cargs->key_len == 0)
> 
> I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> 
> > +      {
> > +	/* Get the passphrase from the user, if no key data. */
> > +	askpass = 1;
> > +	if (source->partition)
> 
> ...but not for the pointers and enums.
> 
> s/if (source->partition)/if (source->partition != NULL)/
> 
> > +	  part = grub_partition_get_name (source->partition);
> > +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > +		     source->partition ? "," : "", part ? : "",
> 
> s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
> 
> s/part ? : ""/part != NULL ? part : "NO NAME"/?

For completeness, I missed a part of your suggestion in my original
response. I don't believe we should use "NO NAME" because a part ==
NULL means that the source device is not a partition (or that
grub_partition_get_name fails, which only happens if grub_malloc
fails). So the empty string is more appropriate. I could add an extra
tertiary operator to select "NO NAME" (or better "UNKNOWN"?) if
source->partition != NULL. I think this should be done in a different
patch though.

Glenn

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

[Daniel Kiper]

On Fri, Dec 03, 2021 at 03:04:36PM -0600, Glenn Washburn wrote:
> On Wed, 17 Nov 2021 20:10:21 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > > 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.
> >
> > Mostly nitpicks below...
> >
> > > 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> > >        return grub_errno;
> > >      if (!dev)
> > >        continue;
> > > -
> > > -    err = cr->recover_key (source, dev, cargs);
> > > -    if (err)
> > > -    {
> > > -      cryptodisk_close (dev);
> > > -      return err;
> > > -    }
> > > +
> > > +    if (cargs->key_len == 0)
> >
> > I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> >
> > > +      {
> > > +	/* Get the passphrase from the user, if no key data. */
> > > +	askpass = 1;
> > > +	if (source->partition)
> >
> > ...but not for the pointers and enums.
> >
> > s/if (source->partition)/if (source->partition != NULL)/
> >
> > > +	  part = grub_partition_get_name (source->partition);
> > > +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > > +		     source->partition ? "," : "", part ? : "",
> >
> > s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
> >
> > s/part ? : ""/part != NULL ? part : "NO NAME"/?
>
> For completeness, I missed a part of your suggestion in my original
> response. I don't believe we should use "NO NAME" because a part ==
> NULL means that the source device is not a partition (or that
> grub_partition_get_name fails, which only happens if grub_malloc
> fails). So the empty string is more appropriate. I could add an extra

... but the problem with empty string is it is invisible for the user.
That is why I do not like it.

> tertiary operator to select "NO NAME" (or better "UNKNOWN"?) if

I concur.

> source->partition != NULL. I think this should be done in a different
> patch though.

If you wish go ahead.

Daniel

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

[Glenn Washburn]

On Fri, 3 Dec 2021 22:35:11 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Fri, Dec 03, 2021 at 03:04:36PM -0600, Glenn Washburn wrote:
> > On Wed, 17 Nov 2021 20:10:21 +0100
> > Daniel Kiper <dkiper@net-space.pl> wrote:
> >
> > > On Tue, Oct 12, 2021 at 06:26:27PM -0500, Glenn Washburn wrote:
> > > > 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.
> > >
> > > Mostly nitpicks below...
> > >
> > > > 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 577942088..a5f7b860c 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,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> > > >        return grub_errno;
> > > >      if (!dev)
> > > >        continue;
> > > > -
> > > > -    err = cr->recover_key (source, dev, cargs);
> > > > -    if (err)
> > > > -    {
> > > > -      cryptodisk_close (dev);
> > > > -      return err;
> > > > -    }
> > > > +
> > > > +    if (cargs->key_len == 0)
> > >
> > > I am OK with "if (!cargs->key_len)" for all kinds of numeric values...
> > >
> > > > +      {
> > > > +	/* Get the passphrase from the user, if no key data. */
> > > > +	askpass = 1;
> > > > +	if (source->partition)
> > >
> > > ...but not for the pointers and enums.
> > >
> > > s/if (source->partition)/if (source->partition != NULL)/
> > >
> > > > +	  part = grub_partition_get_name (source->partition);
> > > > +	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> > > > +		     source->partition ? "," : "", part ? : "",
> > >
> > > s/source->partition ? "," : ""/source->partition != NULL ? "," : ""/
> > >
> > > s/part ? : ""/part != NULL ? part : "NO NAME"/?
> >
> > For completeness, I missed a part of your suggestion in my original
> > response. I don't believe we should use "NO NAME" because a part ==
> > NULL means that the source device is not a partition (or that
> > grub_partition_get_name fails, which only happens if grub_malloc
> > fails). So the empty string is more appropriate. I could add an extra
> 
> ... but the problem with empty string is it is invisible for the user.
> That is why I do not like it.

That's the whole point though. For the majority of cases where part ==
NULL a whole disk is being attempted to be decrypted, so there is no
partition. In this case, you want the third %s to be empty. Only in the
extremely rare case that a partition is being attempted and we have a
grub_malloc failure in grub_partition_get_name does the empty string
for the third %s become undesirable.

Anyway, the next version will address this.

> > tertiary operator to select "NO NAME" (or better "UNKNOWN"?) if
> 
> I concur.
> 
> > source->partition != NULL. I think this should be done in a different
> > patch though.
> 
> If you wish go ahead.
> 
> Daniel

Glenn