From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from list by lists.gnu.org with archive (Exim 4.90_1) id 1mvN0q-0002Ef-4v for mharc-grub-devel@gnu.org; Thu, 09 Dec 2021 12:15:21 -0500 Received: from eggs.gnu.org ([209.51.188.92]:44806) by lists.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.90_1) (envelope-from ) id 1mvN0n-0002Cm-H3 for grub-devel@gnu.org; Thu, 09 Dec 2021 12:15:17 -0500 Received: from [2607:f8b0:4864:20::f2b] (port=42617 helo=mail-qv1-xf2b.google.com) by eggs.gnu.org with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.90_1) (envelope-from ) id 1mvN0l-00055z-1O for grub-devel@gnu.org; Thu, 09 Dec 2021 12:15:17 -0500 Received: by mail-qv1-xf2b.google.com with SMTP id p3so5668405qvj.9 for ; Thu, 09 Dec 2021 09:15:13 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=efficientek-com.20210112.gappssmtp.com; s=20210112; h=from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=TpqsRb04HcSy0ZMiTfZ2NnVLFc5vObtrkmDVPWafcj8=; b=cN8KQKHYFy5zvLdgk6aQcbsGEldPOv9b2xUWIYTSrbgK0Ecq6Y5lj4VRUxywL6uThJ MDW77Bm8Jhzjl5x6OwzWarK1pho7Ry8mY55TaMqA5GH9nId1Qo2N/oTpv85oFK/TH/Wu Noc0WAaY6itbHaF8LfuhgxBue7zOG7biP//pmeCLJCvk6H6YiWq6D2l5Hm/ARfiv6nnv dKXLoABMERLMpD9KCruoXd53MG9/NKDF5z2bgCKfSaBCPyjw1P5b+k2RPMj8boz8OgUc 8NMBT43NCFR30ddAIEgRwUauqkjCCMOLUb893bns7hgdsjXO9PVZDKxBodsIQ2/n/uwu WOZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:from:to:cc:subject:date:message-id:mime-version :content-transfer-encoding; bh=TpqsRb04HcSy0ZMiTfZ2NnVLFc5vObtrkmDVPWafcj8=; b=aBw5k+OcLpKBU2sceEF9h3Ht3rIDT/ZUpPVbpODfAOYQWKs7BhnG8blk6GJ8D+qVaw AoKK4YgToslgsYFe8/Pq6yQ6l0lAfmk/tZy0cXfCoibajY/5/vVb6Bg8QuwToMwIGRB5 Hrx8jzTPUciKGAZLhHnvVXg2A9+Gn3/aGSgbcu/eIOLlfHgT7GbwjoqdB1wVP0uTadPk NGEc0GrXM8eV88QQ+HkaHxC8nGLNkTTYnlFyC9/D2378pH0nrHfv45nEuAH179KufTHT RK2M+301Q3xEt809gV4+RanjbiZUDEv2JfpzhwQZ9ZIZKaH7vr3UB/DtXdHXhQGMAKcX vi9Q== X-Gm-Message-State: AOAM533gvoTX0746vJD0P3QP2ucIOP3+625Au+uteLqlf131BU1LF+JV dS5zCSszlTumEbbXrUPAyRlqBA== X-Google-Smtp-Source: ABdhPJzVDPpW5zuo0miJv1mbOBq3JVdkIR69juq7lvBDKbeF4sjHZ7NTVN3CNQHI2lYg8wq07iRVtg== X-Received: by 2002:ad4:5e87:: with SMTP id jl7mr18352109qvb.19.1639070113232; Thu, 09 Dec 2021 09:15:13 -0800 (PST) Received: from localhost.localdomain ([37.218.244.251]) by smtp.gmail.com with ESMTPSA id m9sm151839qkn.59.2021.12.09.09.15.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Thu, 09 Dec 2021 09:15:12 -0800 (PST) From: Glenn Washburn To: Daniel Kiper , grub-devel@gnu.org Cc: Denis 'GNUtoo' Carikli , Patrick Steinhardt , James Bottomley , Glenn Washburn Subject: [PATCH v5 0/9] Refactor/improve cryptomount data passing to crypto modules Date: Thu, 9 Dec 2021 11:14:49 -0600 Message-Id: X-Mailer: git-send-email 2.27.0 MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Host-Lookup-Failed: Reverse DNS lookup failed for 2607:f8b0:4864:20::f2b (failed) Received-SPF: pass client-ip=2607:f8b0:4864:20::f2b; envelope-from=development@efficientek.com; helo=mail-qv1-xf2b.google.com X-Spam_score_int: -10 X-Spam_score: -1.1 X-Spam_bar: - X-Spam_report: (-1.1 / 5.0 requ) BAYES_00=-1.9, DKIM_SIGNED=0.1, DKIM_VALID=-0.1, PDS_HP_HELO_NORDNS=0.001, RCVD_IN_DNSWL_NONE=-0.0001, RDNS_NONE=0.793, SPF_HELO_NONE=0.001, SPF_PASS=-0.001 autolearn=no autolearn_force=no X-Spam_action: no action X-BeenThere: grub-devel@gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: The development of GNU GRUB List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 09 Dec 2021 17:15:17 -0000 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