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