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

[Patrick Steinhardt]

On Tue, Oct 12, 2021 at 06:26:25PM -0500, Glenn Washburn wrote:
> 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

A single question for patch 3/4, but all the others look good to me.
Feel free to add "Reviewed-by: Patrick Steinhardt <ps@pks.im>" to those.

Patrick

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

signature.asc
Description: PGP signature