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

[Glenn Washburn <development@efficientek.com>]

Updates since v3:
* Many updates based on feedback from Daniel and Patrick
* Make removal of global "have_it" happen before rearchitecting cryptomount
  arg passing
* Add changes that improve cryptomount error messaging

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

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.

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, patch #3
improves some long standing issues in cryptomount error messaging.

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

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

Glenn

Glenn Washburn (7):
  luks2: Add debug message to align with luks and geli modules
  cryptodisk: Refactor to discard have_it global
  cryptodisk: Improve error messaging in cryptomount invocations
  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 | 147 +++++++++++++++++++++++++-----------
 grub-core/disk/geli.c       |  35 +++------
 grub-core/disk/luks.c       |  37 +++------
 grub-core/disk/luks2.c      |  38 ++++------
 include/grub/cryptodisk.h   |  19 ++++-
 include/grub/err.h          |   1 +
 7 files changed, 165 insertions(+), 121 deletions(-)

Range-diff against v3:
-:  --------- > 1:  c71461896 luks2: Add debug message to align with luks and geli modules
-:  --------- > 2:  37c2adcf5 cryptodisk: Refactor to discard have_it global
-:  --------- > 3:  675aaf68c cryptodisk: Improve error messaging in cryptomount invocations
1:  ef344591f ! 4:  6def57f22 cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
    @@ Metadata
      ## Commit message ##
         cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
     
    +    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.
    +
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: static const struct grub_arg_option options[] =
          /* TRANSLATORS: It's still restricted to cryptodisks only.  */
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
     +  err = grub_cryptodisk_scan_device_real (name, source, cargs);
      
        grub_disk_close (source);
    -   
    + 
     @@ 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: static grub_err_t
      
        if (argc < 1 && !state[1].set && !state[2].set)
          return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
    +@@ grub-core/disk/cryptodisk.c: 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);
     +    }
     +
    -   have_it = 0;
    --  if (state[0].set)
     +  if (state[0].set) /* uuid */
          {
    +       int found_uuid;
            grub_cryptodisk_t dev;
    - 
     @@ grub-core/disk/cryptodisk.c: 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);
    +-      found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
    ++      found_uuid = 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;
    +       if (found_uuid)
    +@@ grub-core/disk/cryptodisk.c: 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 */
    @@ grub-core/disk/geli.c: recover_key (grub_disk_t source, grub_cryptodisk_t dev)
        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)
    @@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
        char *tmp;
      
     +  /* 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;
     +
        err = grub_disk_read (source, 0, 0, sizeof (header), &header);
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
        grub_err_t ret;
      
     +  /* 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;
     +
        ret = luks2_read_header (source, &header);
2:  cfeb864ce ! 5:  ef6394f1e cryptodisk: Refactor password input out of crypto dev modules into cryptodisk
    @@ Commit message
         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.
    +
    + ## docs/grub.texi ##
    +@@ docs/grub.texi: 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
    +
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
      				  grub_disk_t source,
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
     -      return err;
     -    }
     +
    -+    if (cargs->key_len == 0)
    ++    if (!cargs->key_len)
     +      {
     +	/* Get the passphrase from the user, if no key data. */
     +	askpass = 1;
    -+	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 ? : "",
    ++		     source->partition != NULL ? "," : "",
    ++		     part != NULL ? part : "",
     +		     dev->uuid);
     +	grub_free (part);
     +
     +	cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
    -+	if (!cargs->key_data)
    ++	if (cargs->key_data == NULL)
     +	  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");
    ++	    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)
    ++    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;
    +-  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");
     +  goto cleanup;
     +
    -+error:
    ++ error:
     +  cryptodisk_close (dev);
    -+cleanup:
    ++
    ++ cleanup:
     +  if (askpass)
     +    {
     +      cargs->key_len = 0;
    @@ grub-core/disk/geli.c: recover_key (grub_disk_t source, grub_cryptodisk_t dev, g
        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 (cargs->key_data == NULL || cargs->key_len == 0)
    -+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
    ++    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");
    @@ grub-core/disk/luks.c: luks_recover_key (grub_disk_t source,
     -  char *tmp;
      
     -  /* 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 (cargs->key_data == NULL || cargs->key_len == 0)
    -+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
    ++  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)
    @@ grub-core/disk/luks2.c: luks2_recover_key (grub_disk_t source,
        grub_err_t ret;
      
     -  /* 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 (cargs->key_data == NULL || cargs->key_len == 0)
    -+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "No key data");
    ++    return grub_error (GRUB_ERR_BAD_ARGUMENT, "no key data");
      
        ret = luks2_read_header (source, &header);
        if (ret)
3:  bfe1a2708 ! 6:  242ee2798 cryptodisk: Move global variables into grub_cryptomount_args struct
    @@ Metadata
      ## Commit message ##
         cryptodisk: Move global variables into grub_cryptomount_args struct
     
    +    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.
    +
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: 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
    @@ 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);
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
          if (grub_errno)
            return grub_errno;
          if (!dev)
    -@@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const char *name,
    - 
    -     grub_cryptodisk_insert (dev, name, source);
    - 
    --    have_it = 1;
    -+    cargs->found_uuid = 1;
    +@@ grub-core/disk/cryptodisk.c: 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};
      
    -     goto cleanup;
    -   }
    +   /* Try to open disk.  */
    +   source = grub_disk_open (sourcedev);
     @@ grub-core/disk/cryptodisk.c: 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);
    ++    dev = cr->scan (source, &cargs);
          if (grub_errno)
            return grub_errno;
          if (!dev)
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char *name,
    -   
    -   if (err)
    +     grub_error_push();
    +   else
          grub_print_error ();
    --  return have_it && search_uuid ? 1 : 0;
    -+  return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
    +-  return (err == GRUB_ERR_NONE && search_uuid != NULL);
    ++  return (err == GRUB_ERR_NONE && cargs->search_uuid != NULL);
      }
      
      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);
    -     }
    - 
    --  have_it = 0;
    -   if (state[0].set) /* uuid */
    -     {
    -       grub_cryptodisk_t dev;
     @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
      	  return GRUB_ERR_NONE;
      	}
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, i
     -      search_uuid = args[0];
     +      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);
     -      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;
    +       if (found_uuid)
    + 	return GRUB_ERR_NONE;
    +@@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
          }
        else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
          {
    @@ grub-core/disk/geli.c: configure_ciphers (grub_disk_t disk, const char *check_uu
          }
      
     -  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 && 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);
    @@ grub-core/disk/luks.c: configure_ciphers (grub_disk_t disk, const char *check_uu
        *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 && 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;
          }
      
    -@@ grub-core/disk/luks.c: 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);
     
      ## grub-core/disk/luks2.c ##
     @@ grub-core/disk/luks2.c: luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
    @@ grub-core/disk/luks2.c: luks2_scan (grub_disk_t disk, const char *check_uuid, in
        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;
    ++  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;
    +     }
      
    -   cryptodisk = grub_zalloc (sizeof (*cryptodisk));
     
      ## include/grub/cryptodisk.h ##
     @@ include/grub/cryptodisk.h: typedef gcry_err_code_t
    @@ include/grub/cryptodisk.h: typedef gcry_err_code_t
      {
     +  /* 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 */
4:  157e08487 < -:  --------- cryptodisk: Remove unneeded found_uuid from cryptomount args
-:  --------- > 7:  40a6f2d1b cryptodisk: Improve handling of partition name in cryptomount password prompt
-- 
2.27.0