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 <p...@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
_______________________________________________ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel