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

[Glenn Washburn <development@efficientek.com>]

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