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