[PATCH v3 0/4] Refactor/improve cryptomount data passing to crypto modules

[Glenn Washburn]

Updates since v2:
* Add space after function name in function calls
* Add comments for members of struct grub_cryptomount_args
* Correct commit message for patch #4

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

The infrastructure is implemented in patch #1 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 #2 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 #3 gets rid of some long time globals in cryptodisk, moving them
into the passed struct.

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.

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 (4):
  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: Remove unneeded found_uuid from cryptomount args

 grub-core/disk/cryptodisk.c | 108 ++++++++++++++++++++++++------------
 grub-core/disk/geli.c       |  35 ++++--------
 grub-core/disk/luks.c       |  37 ++++--------
 grub-core/disk/luks2.c      |  33 ++++-------
 include/grub/cryptodisk.h   |  19 ++++++-
 5 files changed, 120 insertions(+), 112 deletions(-)

Range-diff against v2:
1:  475bf7e9e ! 1:  83112518f cryptodisk: Add infrastructure to pass data from 
cryptomount to cryptodisk modules
    @@ grub-core/disk/cryptodisk.c: static grub_err_t
     +  if (state[3].set) /* password */
     +    {
     +      cargs.key_data = (grub_uint8_t *) state[3].arg;
    -+      cargs.key_len = grub_strlen(state[3].arg);
    ++      cargs.key_len = grub_strlen (state[3].arg);
     +    }
     +
        have_it = 0;
2:  a0c0694fc ! 2:  65a18c5e8 cryptodisk: Refactor password input out of crypto 
dev modules into cryptodisk
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const 
char *name,
     +      ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
     +      goto error;
     +    }
    -+  cargs->key_len = grub_strlen((char *) cargs->key_data);
    ++  cargs->key_len = grub_strlen ((char *) cargs->key_data);
     +      }
     +
     +    ret = cr->recover_key (source, dev, cargs);
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const 
char *name,
     +  if (askpass)
     +    {
     +      cargs->key_len = 0;
    -+      grub_free(cargs->key_data);
    ++      grub_free (cargs->key_data);
     +    }
     +  return ret;
      }
3:  419f114d8 ! 3:  fed38b3dc cryptodisk: Move global variables into 
grub_cryptomount_args struct
    @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device (const char 
*name,
      
      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);
    +       cargs.key_len = grub_strlen (state[3].arg);
          }
      
     -  have_it = 0;
    @@ include/grub/cryptodisk.h: 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;
     +  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 */
        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;
     @@ include/grub/cryptodisk.h: struct grub_cryptodisk_dev
        struct grub_cryptodisk_dev *next;
        struct grub_cryptodisk_dev **prev;
4:  e6e1e8bc3 ! 4:  854709929 cryptodisk: Remove unneeded found_uuid from 
cryptomount args
    @@ Commit message
         iff grub_cryptodisk_scan_device returns 1. And 
grub_cryptodisk_scan_device
         will only return 1 if a search_uuid has been specified and a 
cryptodisk was
         successfully setup by a crypto-backend. So the return value of
    -    grub_cryptodisk_scan_device is equivalent to found_uuid.
    +    grub_cryptodisk_scan_device is almost equivalent to found_uuid, with 
the
    +    exception of the case where a mount is requested or an already opened
    +    crypto device.
    +
    +    With this change grub_device_iterate will return 1 when
    +    grub_cryptodisk_scan_device 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.
     
      ## grub-core/disk/cryptodisk.c ##
     @@ grub-core/disk/cryptodisk.c: grub_cryptodisk_scan_device_real (const 
char *name,
    @@ grub-core/disk/cryptodisk.c: grub_cmd_cryptomount (grub_extcmd_context_t 
ctxt, i
          }
     
      ## include/grub/cryptodisk.h ##
    -@@ include/grub/cryptodisk.h: typedef gcry_err_code_t
    - struct grub_cryptomount_args
    +@@ include/grub/cryptodisk.h: struct grub_cryptomount_args
      {
    +   /* 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;
    -   grub_uint8_t *key_data;
    -   grub_size_t key_len;
    +   /* recover_key: Key data used to decrypt voume */
-- 
2.27.0