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

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

[Glenn Washburn]

Updates since v4:
* Rework patch #2 to (hopefully) be easier to understand
* Add more commentary to patch #2 commit message
* Split previous patch #3 into three separate patches

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

To make thing simpler and easier to understand, the "have_it" global variable
is gotten rid of first in patch #2. Taking advantage of this change, patches
#3-5 improve some long standing issues in cryptomount error messaging.

Then, the infrastructure for passing argument data to cryptodisk backends is
implemented in patch #6 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 #7 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 #8 gets rid of some long time globals in cryptodisk, moving them
into the passed struct.

Patch #9 improves handling of partition name in cryptomount password prompt.

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 (9):
  luks2: Add debug message to align with luks and geli modules
  cryptodisk: Refactor to discard have_it global
  cryptodisk: Return failure in cryptomount when no cryptodisk modules
    are loaded
  cryptodisk: Improve error messaging in cryptomount invocations
  cryptodisk: Improve cryptomount -u error message
  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: Improve handling of partition name in cryptomount password
    prompt

 docs/grub.texi              |   9 +-
 grub-core/disk/cryptodisk.c | 164 +++++++++++++++++++++++++-----------
 grub-core/disk/geli.c       |  35 +++-----
 grub-core/disk/luks.c       |  37 +++-----
 grub-core/disk/luks2.c      |  38 ++++-----
 include/grub/cryptodisk.h   |  19 ++++-
 6 files changed, 175 insertions(+), 127 deletions(-)

Range-diff against v4:
1:  0ae554743 < -:  --------- cryptodisk: Refactor to discard have_it global
-:  --------- > 1:  5056163ca cryptodisk: Refactor to discard have_it global
-:  --------- > 2:  224d7f9bc cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded
2:  d5040065e ! 3:  a0dfaeb5a cryptodisk: Improve error messaging in cryptomount invocations
    @@ Commit message
         when an invalid passphrase is given and the most relevant error message
         will be displayed.
     
    -    Improve error message which is displayed when a UUID is specified, but no
    -    cryptodisk backends find a disk with that UUID.
    -
    -    Also, make cryptomount return failure when no cryptodisk modules are loaded,
    -    which allows an error to be displayed notifying the user that they'll want
    -    to load a backend module to make cryptomount useful.
    -
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
    +   if (grub_errno == GRUB_ERR_BAD_MODULE)
    +     grub_error_pop ();
      
    -   grub_disk_close (source);
    - 
    --  /*
    --   * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many unhelpful
    --   * error messages.
    --   */
    --  if (err != GRUB_ERR_NONE && err != GRUB_ERR_EXISTS && err != GRUB_ERR_BAD_MODULE)
    -+  if (err == GRUB_ERR_NONE || err == GRUB_ERR_EXISTS)
    -+    ; /* Success, skip the error handling */
    -+  else if (err == GRUB_ERR_BAD_MODULE)
    -+    /* Do nothing to avoid printing unhelpful error messages */
    -+    grub_errno = GRUB_ERR_NONE;
    -+  else if (cargs->search_uuid != NULL)
    +-  if (grub_errno != GRUB_ERR_NONE)
    ++  if (search_uuid != NULL)
     +    /* Push error onto stack to save for cryptomount */
    -+    grub_error_push();
    ++    grub_error_push ();
     +  else
          grub_print_error ();
    -   return (err == GRUB_ERR_NONE && search_uuid != NULL);
    - }
    -@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
    -   if (argc < 1 && !state[1].set && !state[2].set)
    -     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
      
    -+  if (grub_cryptodisk_list == NULL)
    -+    return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
    -+
    -   if (state[0].set)
    -     {
    -       int found_uuid;
    +  cleanup:
     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
            found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
            search_uuid = NULL;
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     +	   * Try to pop the next error on the stack. If there is not one, then
     +	   * no device matched the given UUID.
     +	   */
    -+	  grub_error_pop();
    ++	  grub_error_pop ();
     +	  if (grub_errno == GRUB_ERR_NONE)
    -+	    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found, perhaps a needed disk or cryptodisk module is not loaded");
    ++	    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
     +	}
     +      return grub_errno;
          }
-:  --------- > 4:  53a2d29b4 cryptodisk: Improve cryptomount -u error message
3:  4e7a9f135 ! 5:  9ab7601bd cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
    @@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option options[] =
     @@ grub-core/disk/cryptodisk.c: cryptodisk_close (grub_cryptodisk_t dev)
      }
      
    - static grub_err_t
    + static grub_cryptodisk_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-core/disk/cryptodisk.c: grub_cryptodisk_cheat_mount (const char *sourcedev,
     -			     void *data __attribute__ ((unused)))
     +			     void *data)
      {
    -   grub_err_t err;
    +   int ret = 0;
        grub_disk_t source;
    +   grub_cryptodisk_t dev;
     +  grub_cryptomount_args_t cargs = data;
    +   grub_errno = GRUB_ERR_NONE;
      
        /* Try to open disk.  */
    -   source = grub_disk_open (name);
     @@ grub-core/disk/cryptodisk.c: 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);
    - 
    +-  dev = grub_cryptodisk_scan_device_real (name, source);
    ++  dev = grub_cryptodisk_scan_device_real (name, source, cargs);
    +   if (dev)
    +     {
    +       ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
     @@ grub-core/disk/cryptodisk.c: static grub_err_t
      grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
      {
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
      	  return GRUB_ERR_NONE;
      	}
      
    --      err = grub_cryptodisk_scan_device_real (diskname, disk);
    -+      err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
    +-      dev = grub_cryptodisk_scan_device_real (diskname, disk);
    ++      dev = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
      
            grub_disk_close (disk);
            if (disklast)
4:  4149dcb56 ! 6:  9db80950c cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
        dev = grub_cryptodisk_get_by_source_disk (source);
      
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
    -       return grub_errno;
    +       return NULL;
          if (!dev)
            continue;
     -    
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     -    if (err)
     -    {
     -      cryptodisk_close (dev);
    --      return err;
    +-      return NULL;
     -    }
     +
     +    if (!cargs->key_len)
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     +
     +	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
     +	if (cargs->key_data == NULL)
    -+	  return grub_errno;
    ++	  return NULL;
     +
     +	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
     +	  {
    -+	    ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
    ++	    grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
     +	    goto error;
     +	  }
     +	cargs->key_len = grub_strlen ((char *) cargs->key_data);
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
      
          grub_cryptodisk_insert (dev, name, source);
      
    --    return GRUB_ERR_NONE;
    +-    return dev;
     +    goto cleanup;
        }
    --  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
    -+  ret = grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
    +-
    +   grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
    +-  return NULL;
     +  goto cleanup;
     +
     + error:
     +  cryptodisk_close (dev);
    ++  dev = NULL;
     +
     + cleanup:
     +  if (askpass)
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     +      cargs->key_len = 0;
     +      grub_free (cargs->key_data);
     +    }
    -+  return ret;
    ++  return dev;
      }
      
      #ifdef GRUB_UTIL
5:  d0cfd1b18 ! 7:  33385f215 cryptodisk: Move global variables into grub_cryptomount_args struct
    @@ grub-core/disk/cryptodisk.c: grub_util_cryptodisk_get_uuid (grub_disk_t disk)
      {
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
      
    -   if (dev)
    -     {
    --      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
    -+      if (grub_strcasecmp (cargs->search_uuid, dev->uuid) == 0)
    - 	return GRUB_ERR_NONE;
    -       else
    - 	return GRUB_ERR_EXISTS;
    -@@ grub-core/disk/cryptodisk.c: 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;
    +       return NULL;
          if (!dev)
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
        grub_cryptodisk_t dev;
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_cheat_mount (const char *sourcedev,
            return grub_errno;
          if (!dev)
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
    -     grub_error_push();
    -   else
    -     grub_print_error ();
    --  return (err == GRUB_ERR_NONE && search_uuid != NULL);
    -+  return (err == GRUB_ERR_NONE && cargs->search_uuid != NULL);
    - }
    +   dev = grub_cryptodisk_scan_device_real (name, source, cargs);
    +   if (dev)
    +     {
    +-      ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
    ++      ret = (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, dev->uuid) == 0);
    +       goto cleanup;
    +     }
      
    - static grub_err_t
    +@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
    +   if (grub_errno == GRUB_ERR_BAD_MODULE)
    +     grub_error_pop ();
    + 
    +-  if (search_uuid != NULL)
    ++  if (cargs->search_uuid != NULL)
    +     /* Push error onto stack to save for cryptomount */
    +     grub_error_push ();
    +   else
     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
      	  return GRUB_ERR_NONE;
      	}
6:  1e6641f0f = 8:  6835aa866 cryptodisk: Improve handling of partition name in cryptomount password prompt
-- 
2.27.0

[PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules

[Glenn Washburn]

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/luks2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..fea196dd4 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -370,7 +370,10 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
   uuid[j] = '\0';
 
   if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
-    return NULL;
+    {
+      grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
+      return NULL;
+    }
 
   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
   if (!cryptodisk)
-- 
2.27.0

[PATCH v5 2/9] cryptodisk: Refactor to discard have_it global

[Glenn Washburn]

The global "have_it" 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 if and only if grub_cryptodisk_scan_device() returns 1. And
grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
been specified and a cryptodisk was successfully setup by a crypto-backend or
a cryptodisk of the requested uuid is already open.

To implement this grub_cryptodisk_scan_device_real is modified to return a
cryptodisk or NULL on failure and having the appropriate grub_errno set to
indicated failure. Note that grub_cryptodisk_scan_device_real will fail now
with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
modules succeed in identifying the source disk.

With this change grub_device_iterate() will return 1 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 | 56 +++++++++++++++++++++++--------------
 1 file changed, 35 insertions(+), 21 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..9df3d310f 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -983,7 +983,7 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
 
 #endif
 
-static int check_boot, have_it;
+static int check_boot;
 static char *search_uuid;
 
 static void
@@ -995,7 +995,7 @@ cryptodisk_close (grub_cryptodisk_t dev)
   grub_free (dev);
 }
 
-static grub_err_t
+static grub_cryptodisk_t
 grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
 {
   grub_err_t err;
@@ -1005,13 +1005,13 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
   dev = grub_cryptodisk_get_by_source_disk (source);
 
   if (dev)
-    return GRUB_ERR_NONE;
+    return dev;
 
   FOR_CRYPTODISK_DEVS (cr)
   {
     dev = cr->scan (source, search_uuid, check_boot);
     if (grub_errno)
-      return grub_errno;
+      return NULL;
     if (!dev)
       continue;
     
@@ -1019,16 +1019,16 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
     if (err)
     {
       cryptodisk_close (dev);
-      return err;
+      return NULL;
     }
 
     grub_cryptodisk_insert (dev, name, source);
 
-    have_it = 1;
-
-    return GRUB_ERR_NONE;
+    return dev;
   }
-  return GRUB_ERR_NONE;
+
+  grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
+  return NULL;
 }
 
 #ifdef GRUB_UTIL
@@ -1082,8 +1082,10 @@ static int
 grub_cryptodisk_scan_device (const char *name,
 			     void *data __attribute__ ((unused)))
 {
-  grub_err_t err;
+  int ret = 0;
   grub_disk_t source;
+  grub_cryptodisk_t dev;
+  grub_errno = GRUB_ERR_NONE;
 
   /* Try to open disk.  */
   source = grub_disk_open (name);
@@ -1093,13 +1095,26 @@ grub_cryptodisk_scan_device (const char *name,
       return 0;
     }
 
-  err = grub_cryptodisk_scan_device_real (name, source);
+  dev = grub_cryptodisk_scan_device_real (name, source);
+  if (dev)
+    {
+      ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
+      goto cleanup;
+    }
 
-  grub_disk_close (source);
-  
-  if (err)
+  /*
+   * Do not print error when err is GRUB_ERR_BAD_MODULE to avoid many unhelpful
+   * error messages.
+   */
+  if (grub_errno == GRUB_ERR_BAD_MODULE)
+    grub_error_pop ();
+
+  if (grub_errno != GRUB_ERR_NONE)
     grub_print_error ();
-  return have_it && search_uuid ? 1 : 0;
+
+ cleanup:
+  grub_disk_close (source);
+  return ret;
 }
 
 static grub_err_t
@@ -1110,9 +1125,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
   if (argc < 1 && !state[1].set && !state[2].set)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
-  have_it = 0;
   if (state[0].set)
     {
+      int found_uuid;
       grub_cryptodisk_t dev;
 
       dev = grub_cryptodisk_get_by_uuid (args[0]);
@@ -1125,10 +1140,10 @@ 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);
+      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
       search_uuid = NULL;
 
-      if (!have_it)
+      if (!found_uuid)
 	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
       return GRUB_ERR_NONE;
     }
@@ -1142,7 +1157,6 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
     }
   else
     {
-      grub_err_t err;
       grub_disk_t disk;
       grub_cryptodisk_t dev;
       char *diskname;
@@ -1178,13 +1192,13 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      err = grub_cryptodisk_scan_device_real (diskname, disk);
+      dev = grub_cryptodisk_scan_device_real (diskname, disk);
 
       grub_disk_close (disk);
       if (disklast)
 	*disklast = ')';
 
-      return err;
+      return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
     }
 }
 
-- 
2.27.0

[PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded

[Glenn Washburn]

This displays an error notifying the user that they'll want to load a
backend module to make cryptomount useful.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9df3d310f..27491871a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1125,6 +1125,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
   if (argc < 1 && !state[1].set && !state[2].set)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
+  if (grub_cryptodisk_list == NULL)
+    return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
+
   if (state[0].set)
     {
       int found_uuid;
-- 
2.27.0

[PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations

[Glenn Washburn]

Update such that "cryptomount -u UUID" will not print two error messages
when an invalid passphrase is given and the most relevant error message
will be displayed.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 27491871a..f71bc62eb 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1109,7 +1109,10 @@ grub_cryptodisk_scan_device (const char *name,
   if (grub_errno == GRUB_ERR_BAD_MODULE)
     grub_error_pop ();
 
-  if (grub_errno != GRUB_ERR_NONE)
+  if (search_uuid != NULL)
+    /* Push error onto stack to save for cryptomount */
+    grub_error_push ();
+  else
     grub_print_error ();
 
  cleanup:
@@ -1146,9 +1149,19 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
       search_uuid = NULL;
 
-      if (!found_uuid)
-	return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
-      return GRUB_ERR_NONE;
+      if (found_uuid)
+	return GRUB_ERR_NONE;
+      else if (grub_errno == GRUB_ERR_NONE)
+	{
+	  /*
+	   * Try to pop the next error on the stack. If there is not one, then
+	   * no device matched the given UUID.
+	   */
+	  grub_error_pop ();
+	  if (grub_errno == GRUB_ERR_NONE)
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
+	}
+      return grub_errno;
     }
   else if (state[1].set || (argc == 0 && state[2].set))
     {
-- 
2.27.0

[PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message

[Glenn Washburn]

When a cryptmount is specified with a UUID, but no cryptodisk backends find
a disk with that UUID, return a more detailed message giving telling the
user that they might not have a needed cryptobackend module loaded.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 grub-core/disk/cryptodisk.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f71bc62eb..a5f1b0978 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1159,7 +1159,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	   */
 	  grub_error_pop ();
 	  if (grub_errno == GRUB_ERR_NONE)
-	    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
+	    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found, perhaps a needed disk or cryptodisk module is not loaded");
 	}
       return grub_errno;
     }
-- 
2.27.0

[PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Glenn Washburn]

Previously, the cryptomount arguments were passed by global variable and
function call argument, neither of which are ideal. This change passes data
via a grub_cryptomount_args struct, which can be added to over time as
opposed to continually adding arguments to the cryptodisk scan and
recover_key.

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.

Also, add comments to cryptomount argument parsing to make it more obvious
which argument states are being handled.

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 a5f1b0978..f1ccfbee3 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_cryptodisk_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,11 +1083,12 @@ 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)
 {
   int ret = 0;
   grub_disk_t source;
   grub_cryptodisk_t dev;
+  grub_cryptomount_args_t cargs = data;
   grub_errno = GRUB_ERR_NONE;
 
   /* Try to open disk.  */
@@ -1095,7 +1099,7 @@ grub_cryptodisk_scan_device (const char *name,
       return 0;
     }
 
-  dev = grub_cryptodisk_scan_device_real (name, source);
+  dev = grub_cryptodisk_scan_device_real (name, source, cargs);
   if (dev)
     {
       ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
@@ -1124,6 +1128,7 @@ 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");
@@ -1131,7 +1136,13 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
   if (grub_cryptodisk_list == NULL)
     return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
 
-  if (state[0].set)
+  if (state[3].set) /* password */
+    {
+      cargs.key_data = (grub_uint8_t *) state[3].arg;
+      cargs.key_len = grub_strlen (state[3].arg);
+    }
+
+  if (state[0].set) /* uuid */
     {
       int found_uuid;
       grub_cryptodisk_t dev;
@@ -1146,7 +1157,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
       check_boot = state[2].set;
       search_uuid = args[0];
-      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
       search_uuid = NULL;
 
       if (found_uuid)
@@ -1163,11 +1174,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	}
       return grub_errno;
     }
-  else if (state[1].set || (argc == 0 && state[2].set))
+  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;
     }
@@ -1208,7 +1219,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      dev = grub_cryptodisk_scan_device_real (diskname, disk);
+      dev = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
 
       grub_disk_close (disk);
       if (disklast)
@@ -1347,7 +1358,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..777da3a05 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 != 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..c5858fcf8 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 != NULL || 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 fea196dd4..2cbec8acc 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -545,7 +545,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];
@@ -559,6 +560,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 != NULL || 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 v5 7/9] 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.

Add documentation of passphrase option for cryptomount as it is now usable.

Signed-off-by: Glenn Washburn <development@efficientek.com>
---
 docs/grub.texi              |  9 ++++--
 grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++--------
 grub-core/disk/geli.c       | 26 ++++-------------
 grub-core/disk/luks.c       | 27 +++---------------
 grub-core/disk/luks2.c      | 26 ++++-------------
 include/grub/cryptodisk.h   |  1 +
 6 files changed, 66 insertions(+), 79 deletions(-)

diff --git a/docs/grub.texi b/docs/grub.texi
index 99d0a0149..e0a309574 100644
--- a/docs/grub.texi
+++ b/docs/grub.texi
@@ -4307,13 +4307,16 @@ Alias for @code{hashsum --hash crc32 arg @dots{}}. See command @command{hashsum}
 @node cryptomount
 @subsection cryptomount
 
-@deffn Command cryptomount device|@option{-u} uuid|@option{-a}|@option{-b}
-Setup access to encrypted device. If necessary, passphrase
-is requested interactively. Option @var{device} configures specific grub device
+@deffn Command cryptomount [@option{-p} password] device|@option{-u} uuid|@option{-a}|@option{-b}
+Setup access to encrypted device. If @option{-p} is not given, a passphrase
+is requested interactively. Otherwise, the given @var{password} will be used and
+no passphrase will be requested interactively.
+Option @var{device} configures specific grub device
 (@pxref{Naming convention}); option @option{-u} @var{uuid} configures device
 with specified @var{uuid}; option @option{-a} configures all detected encrypted
 devices; option @option{-b} configures all geli containers that have boot flag set.
 
+
 GRUB suports devices encrypted using LUKS, LUKS2 and geli. Note that necessary
 modules (@var{luks}, @var{luks2} and @var{geli}) have to be loaded manually
 before this command can be used. For LUKS2 only the PBKDF2 key derivation
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f1ccfbee3..f8f6e3e25 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,53 @@ grub_cryptodisk_scan_device_real (const char *name,
       return NULL;
     if (!dev)
       continue;
-    
-    err = cr->recover_key (source, dev, cargs);
-    if (err)
-    {
-      cryptodisk_close (dev);
-      return NULL;
-    }
+
+    if (!cargs->key_len)
+      {
+	/* Get the passphrase from the user, if no key data. */
+	askpass = 1;
+	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 != NULL ? "," : "",
+		     part != NULL ? part : "",
+		     dev->uuid);
+	grub_free (part);
+
+	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+	if (cargs->key_data == NULL)
+	  return NULL;
+
+	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+	  {
+	    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 != GRUB_ERR_NONE)
+      goto error;
 
     grub_cryptodisk_insert (dev, name, source);
 
-    return dev;
+    goto cleanup;
   }
-
   grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
-  return NULL;
+  goto cleanup;
+
+ error:
+  cryptodisk_close (dev);
+  dev = NULL;
+
+ cleanup:
+  if (askpass)
+    {
+      cargs->key_len = 0;
+      grub_free (cargs->key_data);
+    }
+  return dev;
 }
 
 #ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 777da3a05..7299a47d1 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 != NULL || 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 c5858fcf8..39a7af6a4 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 != NULL || 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 2cbec8acc..e6f9a22f8 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,
@@ -549,8 +547,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;
@@ -560,9 +558,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 != NULL || 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)
@@ -589,18 +586,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))
     {
@@ -725,7 +710,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",
@@ -777,7 +762,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 v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct

[Glenn Washburn]

Note that cargs.search_uuid does not need to be initialized in various parts
of the cryptomount argument parsing, just once when cargs is declared with a
struct initializer. The previous code used a global variable which would
retain the value across cryptomount invocations.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index f8f6e3e25..99265097a 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;
-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 NULL;
     if (!dev)
@@ -1077,6 +1074,7 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
   grub_cryptodisk_t dev;
   grub_cryptodisk_dev_t cr;
   grub_disk_t source;
+  struct grub_cryptomount_args cargs = {0};
 
   /* Try to open disk.  */
   source = grub_disk_open (sourcedev);
@@ -1093,7 +1091,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, &cargs);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1136,7 +1134,7 @@ grub_cryptodisk_scan_device (const char *name,
   dev = grub_cryptodisk_scan_device_real (name, source, cargs);
   if (dev)
     {
-      ret = (search_uuid != NULL && grub_strcasecmp (search_uuid, dev->uuid) == 0);
+      ret = (cargs->search_uuid != NULL && grub_strcasecmp (cargs->search_uuid, dev->uuid) == 0);
       goto cleanup;
     }
 
@@ -1147,7 +1145,7 @@ grub_cryptodisk_scan_device (const char *name,
   if (grub_errno == GRUB_ERR_BAD_MODULE)
     grub_error_pop ();
 
-  if (search_uuid != NULL)
+  if (cargs->search_uuid != NULL)
     /* Push error onto stack to save for cryptomount */
     grub_error_push ();
   else
@@ -1189,10 +1187,9 @@ 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];
       found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
-      search_uuid = NULL;
 
       if (found_uuid)
 	return GRUB_ERR_NONE;
@@ -1210,10 +1207,8 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
     }
   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
@@ -1224,8 +1219,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 7299a47d1..23789c43f 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 != NULL && 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 39a7af6a4..f0feb3844 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 != NULL && 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;
     }
 
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index e6f9a22f8..6d7d1f0d0 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,9 +367,9 @@ 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 != NULL && grub_strcasecmp (cargs->search_uuid, uuid) != 0)
     {
-      grub_dprintf ("luks2", "%s != %s\n", uuid, check_uuid);
+      grub_dprintf ("luks2", "%s != %s\n", uuid, cargs->search_uuid);
       return NULL;
     }
 
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 5bd970692..c6524c9ea 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -69,7 +69,13 @@ 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;
+  /* 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 +131,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 v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt

[Glenn Washburn]

Call grub_partition_get_name unconditionally to initialize the part
variable. Then part will only be NULL when grub_partition_get_name errors.
Note that when source->partition is NULL, then grub_partition_get_name
returns an allocated empty string. So no comma or partition will be printed,
as desired.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 99265097a..2381c3330 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1021,11 +1021,10 @@ grub_cryptodisk_scan_device_real (const char *name,
       {
 	/* Get the passphrase from the user, if no key data. */
 	askpass = 1;
-	if (source->partition != NULL)
-	  part = grub_partition_get_name (source->partition);
+	part = grub_partition_get_name (source->partition);
 	grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
 		     source->partition != NULL ? "," : "",
-		     part != NULL ? part : "",
+		     part != NULL ? part : N_("UNKNOWN"),
 		     dev->uuid);
 	grub_free (part);
 
-- 
2.27.0

Re: [PATCH v5 1/9] luks2: Add debug message to align with luks and geli modules

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:50AM -0600, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 2/9] cryptodisk: Refactor to discard have_it global

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:51AM -0600, Glenn Washburn wrote:
> The global "have_it" 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 if and only if grub_cryptodisk_scan_device() returns 1. And
> grub_cryptodisk_scan_device() will now only return 1 if a search_uuid has
> been specified and a cryptodisk was successfully setup by a crypto-backend or
> a cryptodisk of the requested uuid is already open.
>
> To implement this grub_cryptodisk_scan_device_real is modified to return a
> cryptodisk or NULL on failure and having the appropriate grub_errno set to
> indicated failure. Note that grub_cryptodisk_scan_device_real will fail now
> with a new errno GRUB_ERR_BAD_MODULE when none of the cryptodisk backend
> modules succeed in identifying the source disk.
>
> With this change grub_device_iterate() will return 1 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>

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

Daniel

Re: [PATCH v5 3/9] cryptodisk: Return failure in cryptomount when no cryptodisk modules are loaded

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:52AM -0600, Glenn Washburn wrote:
> This displays an error notifying the user that they'll want to load a
> backend module to make cryptomount useful.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 4/9] cryptodisk: Improve error messaging in cryptomount invocations

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:53AM -0600, Glenn Washburn wrote:
> Update such that "cryptomount -u UUID" will not print two error messages
> when an invalid passphrase is given and the most relevant error message
> will be displayed.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 5/9] cryptodisk: Improve cryptomount -u error message

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:54AM -0600, Glenn Washburn wrote:
> When a cryptmount is specified with a UUID, but no cryptodisk backends find
> a disk with that UUID, return a more detailed message giving telling the
> user that they might not have a needed cryptobackend module loaded.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 6/9] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:55AM -0600, Glenn Washburn wrote:
> Previously, the cryptomount arguments were passed by global variable and
> function call argument, neither of which are ideal. This change passes data
> via a grub_cryptomount_args struct, which can be added to over time as
> opposed to continually adding arguments to the cryptodisk scan and
> recover_key.
>
> 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.
>
> Also, add comments to cryptomount argument parsing to make it more obvious
> which argument states are being handled.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 7/9] cryptodisk: Refactor password input out of crypto dev modules into cryptodisk

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:56AM -0600, 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.
>
> Add documentation of passphrase option for cryptomount as it is now usable.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 8/9] cryptodisk: Move global variables into grub_cryptomount_args struct

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:57AM -0600, Glenn Washburn wrote:
> Note that cargs.search_uuid does not need to be initialized in various parts
> of the cryptomount argument parsing, just once when cargs is declared with a
> struct initializer. The previous code used a global variable which would
> retain the value across cryptomount invocations.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 9/9] cryptodisk: Improve handling of partition name in cryptomount password prompt

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:58AM -0600, Glenn Washburn wrote:
> Call grub_partition_get_name unconditionally to initialize the part
> variable. Then part will only be NULL when grub_partition_get_name errors.
> Note that when source->partition is NULL, then grub_partition_get_name
> returns an allocated empty string. So no comma or partition will be printed,
> as desired.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules

[Daniel Kiper]

On Thu, Dec 09, 2021 at 11:14:49AM -0600, Glenn Washburn wrote:
> Updates since v4:
> * Rework patch #2 to (hopefully) be easier to understand
> * Add more commentary to patch #2 commit message
> * Split previous patch #3 into three separate patches
>
> ---
> 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).

Whole patch series looks good to me. I am going to merge it no later
than on Thursday this week.

Patrick, could you do final check of these patches before I will apply them?

Daniel

Re: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules

[Daniel Kiper]

On Mon, Dec 20, 2021 at 11:40:10PM +0100, Daniel Kiper wrote:
> On Thu, Dec 09, 2021 at 11:14:49AM -0600, Glenn Washburn wrote:
> > Updates since v4:
> > * Rework patch #2 to (hopefully) be easier to understand
> > * Add more commentary to patch #2 commit message
> > * Split previous patch #3 into three separate patches
> >
> > ---
> > 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).
>
> Whole patch series looks good to me. I am going to merge it no later
> than on Thursday this week.

I have applied the patch set conditionally due to this Coverity report:

  *** CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  /grub-core/disk/cryptodisk.c: 1064 in grub_cryptodisk_scan_device_real()
  1058      cleanup:
  1059       if (askpass)
  1060         {
  1061           cargs->key_len = 0;
  1062           grub_free (cargs->key_data);
  1063         }
  >>>     CID 366905:  Memory - illegal accesses  (USE_AFTER_FREE)
  >>>     Using freed pointer "dev".
  1064       return dev;
  1065     }
  1066
  1067     #ifdef GRUB_UTIL
  1068     #include <grub/util/misc.h>
  1069     grub_err_t

Glenn, please fix this after holiday.

James, I think you can rabase your patch set on the latest master now.
The issue mentioned above should not impact your work.

Daniel