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

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

[Glenn Washburn]

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

[PATCH v4 1/7] 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 v4 2/7] 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.

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 | 29 ++++++++++++++++++-----------
 include/grub/err.h          |  1 +
 2 files changed, 19 insertions(+), 11 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..9224105ac 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
@@ -1005,7 +1005,12 @@ 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;
+    {
+      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
+	return GRUB_ERR_NONE;
+      else
+	return GRUB_ERR_EXISTS;
+    }
 
   FOR_CRYPTODISK_DEVS (cr)
   {
@@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
 
     grub_cryptodisk_insert (dev, name, source);
 
-    have_it = 1;
-
     return GRUB_ERR_NONE;
   }
-  return GRUB_ERR_NONE;
+  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
 }
 
 #ifdef GRUB_UTIL
@@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
   err = grub_cryptodisk_scan_device_real (name, source);
 
   grub_disk_close (source);
-  
-  if (err)
+
+  /*
+   * 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)
     grub_print_error ();
-  return have_it && search_uuid ? 1 : 0;
+  return (err == GRUB_ERR_NONE && search_uuid != NULL);
 }
 
 static grub_err_t
@@ -1110,9 +1117,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 +1132,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;
     }
diff --git a/include/grub/err.h b/include/grub/err.h
index b08d5d0de..55219cdcc 100644
--- a/include/grub/err.h
+++ b/include/grub/err.h
@@ -57,6 +57,7 @@ typedef enum
     GRUB_ERR_MENU,
     GRUB_ERR_TIMEOUT,
     GRUB_ERR_IO,
+    GRUB_ERR_EXISTS,
     GRUB_ERR_ACCESS_DENIED,
     GRUB_ERR_EXTRACTOR,
     GRUB_ERR_NET_BAD_ADDRESS,
-- 
2.27.0

[PATCH v4 3/7] 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.

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.

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

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 9224105ac..aa086837d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1100,11 +1100,15 @@ grub_cryptodisk_scan_device (const char *name,
 
   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)
+    /* Push error onto stack to save for cryptomount */
+    grub_error_push();
+  else
     grub_print_error ();
   return (err == GRUB_ERR_NONE && search_uuid != NULL);
 }
@@ -1117,6 +1121,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;
@@ -1135,9 +1142,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, perhaps a needed disk or cryptodisk module is not loaded");
+	}
+      return grub_errno;
     }
   else if (state[1].set || (argc == 0 && state[2].set))
     {
-- 
2.27.0

[PATCH v4 4/7] 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 aa086837d..28b61bcb8 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
     /* TRANSLATORS: It's still restricted to cryptodisks only.  */
     {"all", 'a', 0, N_("Mount all."), 0, 0},
     {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
+    {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
 }
 
 static grub_err_t
-grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
+grub_cryptodisk_scan_device_real (const char *name,
+				  grub_disk_t source,
+				  grub_cryptomount_args_t cargs)
 {
   grub_err_t err;
   grub_cryptodisk_t dev;
@@ -1020,7 +1023,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);
@@ -1083,10 +1086,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
 
 static int
 grub_cryptodisk_scan_device (const char *name,
-			     void *data __attribute__ ((unused)))
+			     void *data)
 {
   grub_err_t err;
   grub_disk_t source;
+  grub_cryptomount_args_t cargs = data;
 
   /* Try to open disk.  */
   source = grub_disk_open (name);
@@ -1096,7 +1100,7 @@ grub_cryptodisk_scan_device (const char *name,
       return 0;
     }
 
-  err = grub_cryptodisk_scan_device_real (name, source);
+  err = grub_cryptodisk_scan_device_real (name, source, cargs);
 
   grub_disk_close (source);
 
@@ -1117,6 +1121,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");
@@ -1124,7 +1129,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;
@@ -1139,7 +1150,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)
@@ -1156,11 +1167,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;
     }
@@ -1202,7 +1213,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 	  return GRUB_ERR_NONE;
 	}
 
-      err = grub_cryptodisk_scan_device_real (diskname, disk);
+      err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
 
       grub_disk_close (disk);
       if (disklast)
@@ -1341,7 +1352,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 v4 5/7] 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 | 55 ++++++++++++++++++++++++++++++-------
 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(+), 78 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 28b61bcb8..b1f127e83 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);
 
@@ -1022,19 +1024,52 @@ grub_cryptodisk_scan_device_real (const char *name,
       return grub_errno;
     if (!dev)
       continue;
-    
-    err = cr->recover_key (source, dev, cargs);
-    if (err)
-    {
-      cryptodisk_close (dev);
-      return err;
-    }
+
+    if (!cargs->key_len)
+      {
+	/* 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 grub_errno;
+
+	if (!grub_password_get ((char *) cargs->key_data, GRUB_CRYPTODISK_MAX_PASSPHRASE))
+	  {
+	    ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "passphrase not supplied");
+	    goto error;
+	  }
+	cargs->key_len = grub_strlen ((char *) cargs->key_data);
+      }
+
+    ret = cr->recover_key (source, dev, cargs);
+    if (ret != GRUB_ERR_NONE)
+      goto error;
 
     grub_cryptodisk_insert (dev, name, source);
 
-    return GRUB_ERR_NONE;
+    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");
+  goto cleanup;
+
+ error:
+  cryptodisk_close (dev);
+
+ cleanup:
+  if (askpass)
+    {
+      cargs->key_len = 0;
+      grub_free (cargs->key_data);
+    }
+  return ret;
 }
 
 #ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 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 v4 6/7] 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 b1f127e83..ad08240ff 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)
 {
@@ -1011,7 +1008,7 @@ 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;
@@ -1019,7 +1016,7 @@ grub_cryptodisk_scan_device_real (const char *name,
 
   FOR_CRYPTODISK_DEVS (cr)
   {
-    dev = cr->scan (source, search_uuid, check_boot);
+    dev = cr->scan (source, cargs);
     if (grub_errno)
       return grub_errno;
     if (!dev)
@@ -1081,6 +1078,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);
@@ -1097,7 +1095,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)
@@ -1149,7 +1147,7 @@ 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);
 }
 
 static grub_err_t
@@ -1183,10 +1181,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;
@@ -1204,10 +1201,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
@@ -1219,8 +1214,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 v4 7/7] 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 ad08240ff..d50b3033e 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1026,11 +1026,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 v4 1/7] luks2: Add debug message to align with luks and geli modules

[Daniel Kiper]

On Sat, Dec 04, 2021 at 01:15:44AM -0600, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>

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

Daniel

Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global

[Daniel Kiper]

On Sat, Dec 04, 2021 at 01:15:45AM -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.
>
> 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 | 29 ++++++++++++++++++-----------
>  include/grub/err.h          |  1 +
>  2 files changed, 19 insertions(+), 11 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..9224105ac 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
> @@ -1005,7 +1005,12 @@ 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;
> +    {
> +      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> +	return GRUB_ERR_NONE;
> +      else
> +	return GRUB_ERR_EXISTS;

Hmmm... This looks strange. Could you explain why you return
GRUB_ERR_EXISTS if UUIDs do not match?

> +    }
>
>    FOR_CRYPTODISK_DEVS (cr)
>    {
> @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
>
>      grub_cryptodisk_insert (dev, name, source);
>
> -    have_it = 1;
> -
>      return GRUB_ERR_NONE;
>    }
> -  return GRUB_ERR_NONE;
> +  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");

Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?

>  }
>
>  #ifdef GRUB_UTIL
> @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
>    err = grub_cryptodisk_scan_device_real (name, source);
>
>    grub_disk_close (source);
> -
> -  if (err)
> +
> +  /*
> +   * 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)
>      grub_print_error ();
> -  return have_it && search_uuid ? 1 : 0;
> +  return (err == GRUB_ERR_NONE && search_uuid != NULL);
>  }
>
>  static grub_err_t
> @@ -1110,9 +1117,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 +1132,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;
>      }
> diff --git a/include/grub/err.h b/include/grub/err.h
> index b08d5d0de..55219cdcc 100644
> --- a/include/grub/err.h
> +++ b/include/grub/err.h
> @@ -57,6 +57,7 @@ typedef enum
>      GRUB_ERR_MENU,
>      GRUB_ERR_TIMEOUT,
>      GRUB_ERR_IO,
> +    GRUB_ERR_EXISTS,
>      GRUB_ERR_ACCESS_DENIED,
>      GRUB_ERR_EXTRACTOR,
>      GRUB_ERR_NET_BAD_ADDRESS,

Daniel

Re: [PATCH v4 3/7] cryptodisk: Improve error messaging in cryptomount invocations

[Daniel Kiper]

On Sat, Dec 04, 2021 at 01:15:46AM -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.
>
> 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.

Is it possible to split this patch into 3 separate patches?

Daniel

Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global

[Glenn Washburn]

On Wed, 8 Dec 2021 17:37:19 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sat, Dec 04, 2021 at 01:15:45AM -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.
> >
> > 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 | 29 ++++++++++++++++++-----------
> >  include/grub/err.h          |  1 +
> >  2 files changed, 19 insertions(+), 11 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 90f82b2d3..9224105ac 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
> > @@ -1005,7 +1005,12 @@ 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;
> > +    {
> > +      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> > +	return GRUB_ERR_NONE;
> > +      else
> > +	return GRUB_ERR_EXISTS;
> 
> Hmmm... This looks strange. Could you explain why you return
> GRUB_ERR_EXISTS if UUIDs do not match?

I've re-defined success (that is return == GRUB_ERR_NONE) in
grub_cryptodisk_scan_device_real to be such that, for the case where we
are looking for a particular UUID, either source we're given matches
that UUID is already opened or we successfully open it. If a UUID is
not being searched for, then the criteria is essentially the same,
except for the UUID check.

When searching, GRUB_ERR_EXISTS is returned when the source is
associated with an unlocked crypto device, but is not the UUID that is
being searched for. This in turn tells grub_cryptodisk_scan_device to
not return true (ie. do not stop searching for the requested UUID).

Looking at this again, I'm thinking it might be clearer if
grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
either opens the source or source is associated with an already opened
crypto device, and otehrwise return NULL. If the caller receives a
non-NULL, and does the UUID check itself, if it cares/needs to.

I'm also noticing that at a minimum, I think the if statement with the
UUID check needs to be updated to "search_uuid == NULL ||
grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
like being sent a NULL pointer. Though, I'm confused why this didn't
crash in my testing.

> > +    }
> >
> >    FOR_CRYPTODISK_DEVS (cr)
> >    {
> > @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> >
> >      grub_cryptodisk_insert (dev, name, source);
> >
> > -    have_it = 1;
> > -
> >      return GRUB_ERR_NONE;
> >    }
> > -  return GRUB_ERR_NONE;
> > +  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
> 
> Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?

If this return is reached, it means that all cryptodisk backend modules
(eg. luks, luks2, geli) have tried unsuccessfully to open source. We
need to return an error here does that grub_cryptodisk_scan_device does
not confuse a success here with meaning that the source was
successfully opened.

This was not needed before because "have_it" was being used to
determine if source was or has been successfully opened. With these
changes it is not the return code of grub_cryptodisk_scan_device_real
that is being used for this.

> >  }
> >
> >  #ifdef GRUB_UTIL
> > @@ -1096,10 +1099,14 @@ grub_cryptodisk_scan_device (const char *name,
> >    err = grub_cryptodisk_scan_device_real (name, source);
> >
> >    grub_disk_close (source);
> > -
> > -  if (err)
> > +
> > +  /*
> > +   * 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)
> >      grub_print_error ();
> > -  return have_it && search_uuid ? 1 : 0;
> > +  return (err == GRUB_ERR_NONE && search_uuid != NULL);
> >  }
> >
> >  static grub_err_t
> > @@ -1110,9 +1117,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 +1132,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;
> >      }
> > diff --git a/include/grub/err.h b/include/grub/err.h
> > index b08d5d0de..55219cdcc 100644
> > --- a/include/grub/err.h
> > +++ b/include/grub/err.h
> > @@ -57,6 +57,7 @@ typedef enum
> >      GRUB_ERR_MENU,
> >      GRUB_ERR_TIMEOUT,
> >      GRUB_ERR_IO,
> > +    GRUB_ERR_EXISTS,
> >      GRUB_ERR_ACCESS_DENIED,
> >      GRUB_ERR_EXTRACTOR,
> >      GRUB_ERR_NET_BAD_ADDRESS,
> 
> Daniel

Glenn

Re: [PATCH v4 3/7] cryptodisk: Improve error messaging in cryptomount invocations

[Glenn Washburn]

On Wed, 8 Dec 2021 17:41:32 +0100
Daniel Kiper <dkiper@net-space.pl> wrote:

> On Sat, Dec 04, 2021 at 01:15:46AM -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.
> >
> > 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.
> 
> Is it possible to split this patch into 3 separate patches?

I think the first and last hunks will be difficult to untangle, which
is part of what I expect you're asking for. The commit message has
three sections, which can be broken in to three patches easily, but two
of the patches would be fairly trivial (one is the second hunk and the
other is changing an error string to be more descriptive. Can you
clarify that I've understood you correctly and if not, give more detail
in what you're looking for?

Glenn

Re: [PATCH v4 3/7] cryptodisk: Improve error messaging in cryptomount invocations

[Daniel Kiper]

On Wed, Dec 08, 2021 at 12:28:56PM -0600, Glenn Washburn wrote:
> On Wed, 8 Dec 2021 17:41:32 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Sat, Dec 04, 2021 at 01:15:46AM -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.
> > >
> > > 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.
> >
> > Is it possible to split this patch into 3 separate patches?
>
> I think the first and last hunks will be difficult to untangle, which
> is part of what I expect you're asking for. The commit message has
> three sections, which can be broken in to three patches easily, but two
> of the patches would be fairly trivial (one is the second hunk and the
> other is changing an error string to be more descriptive. Can you
> clarify that I've understood you correctly and if not, give more detail
> in what you're looking for?

Yes, please split this patch into two or three patches. The general
rule is: one logical change per patch. Exceptions should not happen
often...

Daniel

Re: [PATCH v4 2/7] cryptodisk: Refactor to discard have_it global

[Daniel Kiper]

On Wed, Dec 08, 2021 at 12:18:13PM -0600, Glenn Washburn wrote:
> On Wed, 8 Dec 2021 17:37:19 +0100
> Daniel Kiper <dkiper@net-space.pl> wrote:
>
> > On Sat, Dec 04, 2021 at 01:15:45AM -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.
> > >
> > > 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 | 29 ++++++++++++++++++-----------
> > >  include/grub/err.h          |  1 +
> > >  2 files changed, 19 insertions(+), 11 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 90f82b2d3..9224105ac 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
> > > @@ -1005,7 +1005,12 @@ 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;
> > > +    {
> > > +      if (grub_strcasecmp (search_uuid, dev->uuid) == 0)
> > > +	return GRUB_ERR_NONE;
> > > +      else
> > > +	return GRUB_ERR_EXISTS;
> >
> > Hmmm... This looks strange. Could you explain why you return
> > GRUB_ERR_EXISTS if UUIDs do not match?
>
> I've re-defined success (that is return == GRUB_ERR_NONE) in
> grub_cryptodisk_scan_device_real to be such that, for the case where we
> are looking for a particular UUID, either source we're given matches
> that UUID is already opened or we successfully open it. If a UUID is
> not being searched for, then the criteria is essentially the same,
> except for the UUID check.
>
> When searching, GRUB_ERR_EXISTS is returned when the source is
> associated with an unlocked crypto device, but is not the UUID that is
> being searched for. This in turn tells grub_cryptodisk_scan_device to
> not return true (ie. do not stop searching for the requested UUID).
>
> Looking at this again, I'm thinking it might be clearer if
> grub_cryptodisk_scan_device_real returns a grub_cryptodisk_t if it
> either opens the source or source is associated with an already opened
> crypto device, and otehrwise return NULL. If the caller receives a
> non-NULL, and does the UUID check itself, if it cares/needs to.
>
> I'm also noticing that at a minimum, I think the if statement with the
> UUID check needs to be updated to "search_uuid == NULL ||
> grub_strcasecmp (search_uuid, dev->uuid) == 0". grub_strcasecmp doesn't
> like being sent a NULL pointer. Though, I'm confused why this didn't
> crash in my testing.

OK, please fix the patch and add a comment if you do some not obvious
things like here.

> > > +    }
> > >
> > >    FOR_CRYPTODISK_DEVS (cr)
> > >    {
> > > @@ -1024,11 +1029,9 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> > >
> > >      grub_cryptodisk_insert (dev, name, source);
> > >
> > > -    have_it = 1;
> > > -
> > >      return GRUB_ERR_NONE;
> > >    }
> > > -  return GRUB_ERR_NONE;
> > > +  return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk module can handle this device");
> >
> > Could you enlighten me why GRUB_ERR_NONE changes to GRUB_ERR_BAD_MODULE?
>
> If this return is reached, it means that all cryptodisk backend modules
> (eg. luks, luks2, geli) have tried unsuccessfully to open source. We
> need to return an error here does that grub_cryptodisk_scan_device does
> not confuse a success here with meaning that the source was
> successfully opened.
>
> This was not needed before because "have_it" was being used to
> determine if source was or has been successfully opened. With these
> changes it is not the return code of grub_cryptodisk_scan_device_real
> that is being used for this.

I think this begs for a comment too...

Daniel