grub-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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


From: Glenn Washburn
Subject: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules
Date: Thu, 9 Dec 2021 11:14:49 -0600

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

---
This patch series refactors the way cryptomount passes data to the crypto
modules. Currently, the method has been by global variable and function call
argument, neither of which are ideal. This method passes data via a
grub_cryptomount_args struct, which can be added to over time as opposed to
continually adding arguments to the cryptodisk recover_key (as is being
proposed in the keyfile and detached header patches).

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

Then, the infrastructure for passing argument data to cryptodisk backends is
implemented in patch #6 along with adding a new -p parameter to cryptomount
partly as an example to show how a password would be passed to the crypto
module backends. The backends do nothing with this data in this patch, but
print a message saying that sending a password is unimplemented.

Patch #7 takes advantage of this new data passing mechanism to refactor the
essentially duplicated code in each crypto backend module for inputting the
password and puts that functionality in the cryptodisk code. Conceptually,
the crypto backends should not be getting user input anyway.

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

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

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

Glenn

Glenn Washburn (9):
  luks2: Add debug message to align with luks and geli modules
  cryptodisk: Refactor to discard have_it global
  cryptodisk: Return failure in cryptomount when no cryptodisk modules
    are loaded
  cryptodisk: Improve error messaging in cryptomount invocations
  cryptodisk: Improve cryptomount -u error message
  cryptodisk: Add infrastructure to pass data from cryptomount to
    cryptodisk modules
  cryptodisk: Refactor password input out of crypto dev modules into
    cryptodisk
  cryptodisk: Move global variables into grub_cryptomount_args struct
  cryptodisk: Improve handling of partition name in cryptomount password
    prompt

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

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




reply via email to

[Prev in Thread] Current Thread [Next in Thread]