* [PATCH v2 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
2021-09-27 23:13 [PATCH v2 0/4] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
@ 2021-09-27 23:14 ` Glenn Washburn
2021-10-03 19:04 ` Patrick Steinhardt
2021-09-27 23:14 ` [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk Glenn Washburn
` (2 subsequent siblings)
3 siblings, 1 reply; 13+ messages in thread
From: Glenn Washburn @ 2021-09-27 23:14 UTC (permalink / raw)
To: grub-devel, Daniel Kiper
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
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.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
grub-core/disk/geli.c | 6 +++++-
grub-core/disk/luks.c | 7 ++++++-
grub-core/disk/luks2.c | 7 ++++++-
include/grub/cryptodisk.h | 9 ++++++++-
5 files changed, 46 insertions(+), 14 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..ca034859e 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
/* TRANSLATORS: It's still restricted to cryptodisks only. */
{"all", 'a', 0, N_("Mount all."), 0, 0},
{"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
+ {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
{0, 0, 0, 0, 0, 0}
};
@@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
}
static grub_err_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_cryptomount_args_t cargs)
{
grub_err_t err;
grub_cryptodisk_t dev;
@@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
if (!dev)
continue;
- err = cr->recover_key (source, dev);
+ err = cr->recover_key (source, dev, cargs);
if (err)
{
cryptodisk_close (dev);
@@ -1080,10 +1083,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
static int
grub_cryptodisk_scan_device (const char *name,
- void *data __attribute__ ((unused)))
+ void *data)
{
grub_err_t err;
grub_disk_t source;
+ grub_cryptomount_args_t cargs = data;
/* Try to open disk. */
source = grub_disk_open (name);
@@ -1093,7 +1097,7 @@ 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);
@@ -1106,12 +1110,19 @@ static grub_err_t
grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
{
struct grub_arg_list *state = ctxt->state;
+ struct grub_cryptomount_args cargs = {0};
if (argc < 1 && !state[1].set && !state[2].set)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
+ 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 */
{
grub_cryptodisk_t dev;
@@ -1125,18 +1136,18 @@ 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);
search_uuid = NULL;
if (!have_it)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
return GRUB_ERR_NONE;
}
- else if (state[1].set || (argc == 0 && state[2].set))
+ else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
search_uuid = NULL;
check_boot = state[2].set;
- grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+ grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
search_uuid = NULL;
return GRUB_ERR_NONE;
}
@@ -1178,7 +1189,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- err = grub_cryptodisk_scan_device_real (diskname, disk);
+ err = grub_cryptodisk_scan_device_real (diskname, disk, &cargs);
grub_disk_close (disk);
if (disklast)
@@ -1317,7 +1328,7 @@ GRUB_MOD_INIT (cryptodisk)
{
grub_disk_dev_register (&grub_cryptodisk_dev);
cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
- N_("SOURCE|-u UUID|-a|-b"),
+ N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
N_("Mount a crypto device."), options);
grub_procfs_register ("luks_script", &luks_script);
}
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 2f34a35e6..4e8c377e7 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -398,7 +398,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
}
static grub_err_t
-recover_key (grub_disk_t source, grub_cryptodisk_t dev)
+recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs)
{
grub_size_t keysize;
grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -414,6 +414,10 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
grub_disk_addr_t sector;
grub_err_t err;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
if (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
return grub_error (GRUB_ERR_BUG, "cipher block is too long");
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 13103ea6a..0462edc6e 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -152,7 +152,8 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
static grub_err_t
luks_recover_key (grub_disk_t source,
- grub_cryptodisk_t dev)
+ grub_cryptodisk_t dev,
+ grub_cryptomount_args_t cargs)
{
struct grub_luks_phdr header;
grub_size_t keysize;
@@ -165,6 +166,10 @@ luks_recover_key (grub_disk_t source,
grub_size_t max_stripes = 1;
char *tmp;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
return err;
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..455a78cb0 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -542,7 +542,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
static grub_err_t
luks2_recover_key (grub_disk_t source,
- grub_cryptodisk_t crypt)
+ grub_cryptodisk_t crypt,
+ grub_cryptomount_args_t cargs)
{
grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
char passphrase[MAX_PASSPHRASE], cipher[32];
@@ -556,6 +557,10 @@ luks2_recover_key (grub_disk_t source,
grub_json_t *json = NULL, keyslots;
grub_err_t ret;
+ /* Keyfiles are not implemented yet */
+ if (cargs->key_data || cargs->key_len)
+ return GRUB_ERR_NOT_IMPLEMENTED_YET;
+
ret = luks2_read_header (source, &header);
if (ret)
return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index dcf17fbb3..282f8ac45 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -66,6 +66,13 @@ typedef gcry_err_code_t
(*grub_cryptodisk_rekey_func_t) (struct grub_cryptodisk *dev,
grub_uint64_t zoneno);
+struct grub_cryptomount_args
+{
+ grub_uint8_t *key_data;
+ grub_size_t key_len;
+};
+typedef struct grub_cryptomount_args *grub_cryptomount_args_t;
+
struct grub_cryptodisk
{
struct grub_cryptodisk *next;
@@ -119,7 +126,7 @@ struct grub_cryptodisk_dev
grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
int boot_only);
- grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev);
+ grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules
2021-09-27 23:14 ` [PATCH v2 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
@ 2021-10-03 19:04 ` Patrick Steinhardt
0 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2021-10-03 19:04 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
[-- Attachment #1: Type: text/plain, Size: 3202 bytes --]
On Mon, Sep 27, 2021 at 06:14:00PM -0500, Glenn Washburn wrote:
> 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.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 31 +++++++++++++++++++++----------
> grub-core/disk/geli.c | 6 +++++-
> grub-core/disk/luks.c | 7 ++++++-
> grub-core/disk/luks2.c | 7 ++++++-
> include/grub/cryptodisk.h | 9 ++++++++-
> 5 files changed, 46 insertions(+), 14 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 90f82b2d3..ca034859e 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -41,6 +41,7 @@ static const struct grub_arg_option options[] =
> /* TRANSLATORS: It's still restricted to cryptodisks only. */
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> + {"password", 'p', 0, N_("Password to open volumes."), 0, ARG_TYPE_STRING},
> {0, 0, 0, 0, 0, 0}
> };
>
> @@ -996,7 +997,9 @@ cryptodisk_close (grub_cryptodisk_t dev)
> }
>
> static grub_err_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_cryptomount_args_t cargs)
> {
> grub_err_t err;
> grub_cryptodisk_t dev;
> @@ -1015,7 +1018,7 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
> if (!dev)
> continue;
>
> - err = cr->recover_key (source, dev);
> + err = cr->recover_key (source, dev, cargs);
> if (err)
> {
> cryptodisk_close (dev);
> @@ -1080,10 +1083,11 @@ grub_cryptodisk_cheat_mount (const char *sourcedev, const char *cheat)
>
> static int
> grub_cryptodisk_scan_device (const char *name,
> - void *data __attribute__ ((unused)))
> + void *data)
> {
> grub_err_t err;
> grub_disk_t source;
> + grub_cryptomount_args_t cargs = data;
>
> /* Try to open disk. */
> source = grub_disk_open (name);
> @@ -1093,7 +1097,7 @@ 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);
>
> @@ -1106,12 +1110,19 @@ static grub_err_t
> grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> {
> struct grub_arg_list *state = ctxt->state;
> + struct grub_cryptomount_args cargs = {0};
>
> if (argc < 1 && !state[1].set && !state[2].set)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>
> + if (state[3].set) /* password */
> + {
> + cargs.key_data = (grub_uint8_t *) state[3].arg;
> + cargs.key_len = grub_strlen(state[3].arg);
Nit: there's a missing space after the function name here. Other than
that, the patch looks good to me.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk
2021-09-27 23:13 [PATCH v2 0/4] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
2021-09-27 23:14 ` [PATCH v2 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
@ 2021-09-27 23:14 ` Glenn Washburn
2021-10-03 19:10 ` Patrick Steinhardt
2021-09-27 23:14 ` [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
2021-09-27 23:14 ` [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args Glenn Washburn
3 siblings, 1 reply; 13+ messages in thread
From: Glenn Washburn @ 2021-09-27 23:14 UTC (permalink / raw)
To: grub-devel, Daniel Kiper
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
The crypto device modules should only be setting up the crypto devices and
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.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++-------
grub-core/disk/geli.c | 26 ++++---------------
grub-core/disk/luks.c | 27 +++----------------
grub-core/disk/luks2.c | 26 ++++---------------
include/grub/cryptodisk.h | 1 +
5 files changed, 57 insertions(+), 75 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index ca034859e..86eaabe60 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_disk_t source,
grub_cryptomount_args_t cargs)
{
- grub_err_t err;
+ grub_err_t ret = GRUB_ERR_NONE;
grub_cryptodisk_t dev;
grub_cryptodisk_dev_t cr;
+ int askpass = 0;
+ char *part = NULL;
dev = grub_cryptodisk_get_by_source_disk (source);
@@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
return grub_errno;
if (!dev)
continue;
-
- err = cr->recover_key (source, dev, cargs);
- if (err)
- {
- cryptodisk_close (dev);
- return err;
- }
+
+ if (cargs->key_len == 0)
+ {
+ /* Get the passphrase from the user, if no key data. */
+ askpass = 1;
+ if (source->partition)
+ part = grub_partition_get_name (source->partition);
+ grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+ source->partition ? "," : "", part ? : "",
+ dev->uuid);
+ grub_free (part);
+
+ cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
+ if (!cargs->key_data)
+ 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");
+ goto error;
+ }
+ cargs->key_len = grub_strlen((char *) cargs->key_data);
+ }
+
+ ret = cr->recover_key (source, dev, cargs);
+ if (ret)
+ goto error;
grub_cryptodisk_insert (dev, name, source);
have_it = 1;
- return GRUB_ERR_NONE;
+ goto cleanup;
}
- return GRUB_ERR_NONE;
+ goto cleanup;
+
+error:
+ cryptodisk_close (dev);
+cleanup:
+ if (askpass)
+ {
+ cargs->key_len = 0;
+ grub_free(cargs->key_data);
+ }
+ return ret;
}
#ifdef GRUB_UTIL
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 4e8c377e7..32f34d5c3 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -135,8 +135,6 @@ const char *algorithms[] = {
[0x16] = "aes"
};
-#define MAX_PASSPHRASE 256
-
static gcry_err_code_t
geli_rekey (struct grub_cryptodisk *dev, grub_uint64_t zoneno)
{
@@ -406,17 +404,14 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
grub_uint8_t verify_key[GRUB_CRYPTO_MAX_MDLEN];
grub_uint8_t zero[GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE];
grub_uint8_t geli_cipher_key[64];
- char passphrase[MAX_PASSPHRASE] = "";
unsigned i;
gcry_err_code_t gcry_err;
struct grub_geli_phdr header;
- char *tmp;
grub_disk_addr_t sector;
grub_err_t err;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data || 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 (dev->cipher->cipher->blocksize > GRUB_CRYPTO_MAX_CIPHER_BLOCKSIZE)
return grub_error (GRUB_ERR_BUG, "cipher block is too long");
@@ -438,23 +433,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
grub_puts_ (N_("Attempting to decrypt master key..."));
- /* Get the passphrase from the user. */
- tmp = NULL;
- if (source->partition)
- tmp = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", tmp ? : "",
- dev->uuid);
- grub_free (tmp);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
-
/* Calculate the PBKDF2 of the user supplied passphrase. */
if (grub_le_to_cpu32 (header.niter) != 0)
{
grub_uint8_t pbkdf_key[64];
- gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
- grub_strlen (passphrase),
+ gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
+ cargs->key_len,
header.salt,
sizeof (header.salt),
grub_le_to_cpu32 (header.niter),
@@ -477,7 +461,7 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev, grub_cryptomount_args_t
return grub_crypto_gcry_error (GPG_ERR_OUT_OF_MEMORY);
grub_crypto_hmac_write (hnd, header.salt, sizeof (header.salt));
- grub_crypto_hmac_write (hnd, passphrase, grub_strlen (passphrase));
+ grub_crypto_hmac_write (hnd, cargs->key_data, cargs->key_len);
gcry_err = grub_crypto_hmac_fini (hnd, geomkey);
if (gcry_err)
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 0462edc6e..51646cefe 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -29,8 +29,6 @@
GRUB_MOD_LICENSE ("GPLv3+");
-#define MAX_PASSPHRASE 256
-
#define LUKS_KEY_ENABLED 0x00AC71F3
/* On disk LUKS header */
@@ -158,17 +156,14 @@ luks_recover_key (grub_disk_t source,
struct grub_luks_phdr header;
grub_size_t keysize;
grub_uint8_t *split_key = NULL;
- char passphrase[MAX_PASSPHRASE] = "";
grub_uint8_t candidate_digest[sizeof (header.mkDigest)];
unsigned i;
grub_size_t length;
grub_err_t err;
grub_size_t max_stripes = 1;
- char *tmp;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data || 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");
err = grub_disk_read (source, 0, 0, sizeof (header), &header);
if (err)
@@ -188,20 +183,6 @@ luks_recover_key (grub_disk_t source,
if (!split_key)
return grub_errno;
- /* Get the passphrase from the user. */
- tmp = NULL;
- if (source->partition)
- tmp = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", tmp ? : "",
- dev->uuid);
- grub_free (tmp);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- {
- grub_free (split_key);
- return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
- }
-
/* Try to recover master key from each active keyslot. */
for (i = 0; i < ARRAY_SIZE (header.keyblock); i++)
{
@@ -216,8 +197,8 @@ luks_recover_key (grub_disk_t source,
grub_dprintf ("luks", "Trying keyslot %d\n", i);
/* Calculate the PBKDF2 of the user supplied passphrase. */
- gcry_err = grub_crypto_pbkdf2 (dev->hash, (grub_uint8_t *) passphrase,
- grub_strlen (passphrase),
+ gcry_err = grub_crypto_pbkdf2 (dev->hash, cargs->key_data,
+ cargs->key_len,
header.keyblock[i].passwordSalt,
sizeof (header.keyblock[i].passwordSalt),
grub_be_to_cpu32 (header.keyblock[i].
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 455a78cb0..c77380cbb 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -35,8 +35,6 @@ GRUB_MOD_LICENSE ("GPLv3+");
#define LUKS_MAGIC_1ST "LUKS\xBA\xBE"
#define LUKS_MAGIC_2ND "SKUL\xBA\xBE"
-#define MAX_PASSPHRASE 256
-
enum grub_luks2_kdf_type
{
LUKS2_KDF_TYPE_ARGON2I,
@@ -546,8 +544,8 @@ luks2_recover_key (grub_disk_t source,
grub_cryptomount_args_t cargs)
{
grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
- char passphrase[MAX_PASSPHRASE], cipher[32];
- char *json_header = NULL, *part = NULL, *ptr;
+ char cipher[32];
+ char *json_header = NULL, *ptr;
grub_size_t candidate_key_len = 0, json_idx, size;
grub_luks2_header_t header;
grub_luks2_keyslot_t keyslot;
@@ -557,9 +555,8 @@ luks2_recover_key (grub_disk_t source,
grub_json_t *json = NULL, keyslots;
grub_err_t ret;
- /* Keyfiles are not implemented yet */
- if (cargs->key_data || 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");
ret = luks2_read_header (source, &header);
if (ret)
@@ -586,18 +583,6 @@ luks2_recover_key (grub_disk_t source,
goto err;
}
- /* Get the passphrase from the user. */
- if (source->partition)
- part = grub_partition_get_name (source->partition);
- grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
- source->partition ? "," : "", part ? : "",
- crypt->uuid);
- if (!grub_password_get (passphrase, MAX_PASSPHRASE))
- {
- ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
- goto err;
- }
-
if (grub_json_getvalue (&keyslots, json, "keyslots") ||
grub_json_getsize (&size, &keyslots))
{
@@ -722,7 +707,7 @@ luks2_recover_key (grub_disk_t source,
}
ret = luks2_decrypt_key (candidate_key, source, crypt, &keyslot,
- (const grub_uint8_t *) passphrase, grub_strlen (passphrase));
+ cargs->key_data, cargs->key_len);
if (ret)
{
grub_dprintf ("luks2", "Decryption with keyslot \"%" PRIuGRUB_UINT64_T "\" failed: %s\n",
@@ -774,7 +759,6 @@ luks2_recover_key (grub_disk_t source,
}
err:
- grub_free (part);
grub_free (json_header);
grub_json_free (json);
return ret;
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 282f8ac45..5bd970692 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -59,6 +59,7 @@ typedef enum
#define GRUB_CRYPTODISK_GF_LOG_BYTES (GRUB_CRYPTODISK_GF_LOG_SIZE - 3)
#define GRUB_CRYPTODISK_GF_BYTES (1U << GRUB_CRYPTODISK_GF_LOG_BYTES)
#define GRUB_CRYPTODISK_MAX_KEYLEN 128
+#define GRUB_CRYPTODISK_MAX_PASSPHRASE 256
struct grub_cryptodisk;
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk
2021-09-27 23:14 ` [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk Glenn Washburn
@ 2021-10-03 19:10 ` Patrick Steinhardt
0 siblings, 0 replies; 13+ messages in thread
From: Patrick Steinhardt @ 2021-10-03 19:10 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
[-- Attachment #1: Type: text/plain, Size: 2925 bytes --]
On Mon, Sep 27, 2021 at 06:14:01PM -0500, Glenn Washburn wrote:
> The crypto device modules should only be setting up the crypto devices and
> 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.
>
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 52 ++++++++++++++++++++++++++++++-------
> grub-core/disk/geli.c | 26 ++++---------------
> grub-core/disk/luks.c | 27 +++----------------
> grub-core/disk/luks2.c | 26 ++++---------------
> include/grub/cryptodisk.h | 1 +
> 5 files changed, 57 insertions(+), 75 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index ca034859e..86eaabe60 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1001,9 +1001,11 @@ grub_cryptodisk_scan_device_real (const char *name,
> grub_disk_t source,
> grub_cryptomount_args_t cargs)
> {
> - grub_err_t err;
> + grub_err_t ret = GRUB_ERR_NONE;
> grub_cryptodisk_t dev;
> grub_cryptodisk_dev_t cr;
> + int askpass = 0;
> + char *part = NULL;
>
> dev = grub_cryptodisk_get_by_source_disk (source);
>
> @@ -1017,21 +1019,51 @@ grub_cryptodisk_scan_device_real (const char *name,
> return grub_errno;
> if (!dev)
> continue;
> -
> - err = cr->recover_key (source, dev, cargs);
> - if (err)
> - {
> - cryptodisk_close (dev);
> - return err;
> - }
> +
> + if (cargs->key_len == 0)
> + {
> + /* Get the passphrase from the user, if no key data. */
> + askpass = 1;
> + if (source->partition)
> + part = grub_partition_get_name (source->partition);
> + grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> + source->partition ? "," : "", part ? : "",
> + dev->uuid);
> + grub_free (part);
> +
> + cargs->key_data = grub_malloc (GRUB_CRYPTODISK_MAX_PASSPHRASE);
> + if (!cargs->key_data)
> + 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");
> + goto error;
> + }
> + cargs->key_len = grub_strlen((char *) cargs->key_data);
Missing space.
> + }
> +
> + ret = cr->recover_key (source, dev, cargs);
> + if (ret)
> + goto error;
>
> grub_cryptodisk_insert (dev, name, source);
>
> have_it = 1;
>
> - return GRUB_ERR_NONE;
> + goto cleanup;
> }
> - return GRUB_ERR_NONE;
> + goto cleanup;
> +
> +error:
> + cryptodisk_close (dev);
> +cleanup:
> + if (askpass)
> + {
> + cargs->key_len = 0;
> + grub_free(cargs->key_data);
Here, too. Otherwise, the patch looks good to me.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-09-27 23:13 [PATCH v2 0/4] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
2021-09-27 23:14 ` [PATCH v2 1/4] cryptodisk: Add infrastructure to pass data from cryptomount to cryptodisk modules Glenn Washburn
2021-09-27 23:14 ` [PATCH v2 2/4] cryptodisk: Refactor password input from crypto dev modules into cryptodisk Glenn Washburn
@ 2021-09-27 23:14 ` Glenn Washburn
2021-10-03 19:16 ` Patrick Steinhardt
2021-09-27 23:14 ` [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args Glenn Washburn
3 siblings, 1 reply; 13+ messages in thread
From: Glenn Washburn @ 2021-09-27 23:14 UTC (permalink / raw)
To: grub-devel, Daniel Kiper
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
grub-core/disk/geli.c | 9 ++++-----
grub-core/disk/luks.c | 11 +++++------
grub-core/disk/luks2.c | 6 +++---
include/grub/cryptodisk.h | 6 ++++--
5 files changed, 25 insertions(+), 33 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 86eaabe60..5e153ee0a 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
#endif
-static int check_boot, have_it;
-static char *search_uuid;
-
static void
cryptodisk_close (grub_cryptodisk_t dev)
{
@@ -1014,7 +1011,7 @@ 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;
if (!dev)
@@ -1049,7 +1046,7 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_cryptodisk_insert (dev, name, source);
- have_it = 1;
+ cargs->found_uuid = 1;
goto cleanup;
}
@@ -1091,7 +1088,7 @@ 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);
if (grub_errno)
return grub_errno;
if (!dev)
@@ -1135,7 +1132,7 @@ grub_cryptodisk_scan_device (const char *name,
if (err)
grub_print_error ();
- return have_it && search_uuid ? 1 : 0;
+ return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
}
static grub_err_t
@@ -1153,7 +1150,6 @@ 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;
@@ -1166,21 +1162,18 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
return GRUB_ERR_NONE;
}
- check_boot = state[2].set;
- search_uuid = args[0];
+ cargs.check_boot = state[2].set;
+ cargs.search_uuid = args[0];
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;
}
else if (state[1].set || (argc == 0 && state[2].set)) /* -a|-b */
{
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
- search_uuid = NULL;
return GRUB_ERR_NONE;
}
else
@@ -1192,8 +1185,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
char *disklast = NULL;
grub_size_t len;
- search_uuid = NULL;
- check_boot = state[2].set;
+ cargs.check_boot = state[2].set;
diskname = args[0];
len = grub_strlen (diskname);
if (len && diskname[0] == '(' && diskname[len - 1] == ')')
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 32f34d5c3..32d35521b 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -240,8 +240,7 @@ grub_util_get_geli_uuid (const char *dev)
#endif
static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int boot_only)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t newdev;
struct grub_geli_phdr header;
@@ -289,7 +288,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (boot_only && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
+ if (cargs->check_boot && !(grub_le_to_cpu32 (header.flags) & GRUB_GELI_FLAGS_BOOT))
{
grub_dprintf ("geli", "not a boot volume\n");
return NULL;
@@ -302,9 +301,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
return NULL;
}
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid && 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);
return NULL;
}
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 51646cefe..6ced312c7 100644
--- a/grub-core/disk/luks.c
+++ b/grub-core/disk/luks.c
@@ -63,8 +63,7 @@ gcry_err_code_t AF_merge (const gcry_md_spec_t * hash, grub_uint8_t * src,
grub_size_t blocknumbers);
static grub_cryptodisk_t
-configure_ciphers (grub_disk_t disk, const char *check_uuid,
- int check_boot)
+configure_ciphers (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t newdev;
const char *iptr;
@@ -76,7 +75,7 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
char hashspec[sizeof (header.hashSpec) + 1];
grub_err_t err;
- if (check_boot)
+ if (cargs->check_boot)
return NULL;
/* Read the LUKS header. */
@@ -103,9 +102,9 @@ configure_ciphers (grub_disk_t disk, const char *check_uuid,
}
*optr = 0;
- if (check_uuid && grub_strcasecmp (check_uuid, uuid) != 0)
+ if (cargs->search_uuid && 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;
}
@@ -162,7 +161,7 @@ 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);
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index c77380cbb..28fad54aa 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -346,14 +346,14 @@ luks2_read_header (grub_disk_t disk, grub_luks2_header_t *outhdr)
}
static grub_cryptodisk_t
-luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
+luks2_scan (grub_disk_t disk, grub_cryptomount_args_t cargs)
{
grub_cryptodisk_t cryptodisk;
grub_luks2_header_t header;
char uuid[sizeof (header.uuid) + 1];
grub_size_t i, j;
- if (check_boot)
+ if (cargs->check_boot)
return NULL;
if (luks2_read_header (disk, &header))
@@ -367,7 +367,7 @@ luks2_scan (grub_disk_t disk, const char *check_uuid, int check_boot)
uuid[j++] = header.uuid[i];
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;
cryptodisk = grub_zalloc (sizeof (*cryptodisk));
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 5bd970692..230167ab0 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -69,6 +69,9 @@ typedef gcry_err_code_t
struct grub_cryptomount_args
{
+ grub_uint32_t check_boot : 1;
+ grub_uint32_t found_uuid : 1;
+ char *search_uuid;
grub_uint8_t *key_data;
grub_size_t key_len;
};
@@ -125,8 +128,7 @@ struct grub_cryptodisk_dev
struct grub_cryptodisk_dev *next;
struct grub_cryptodisk_dev **prev;
- grub_cryptodisk_t (*scan) (grub_disk_t disk, const char *check_uuid,
- int boot_only);
+ grub_cryptodisk_t (*scan) (grub_disk_t disk, grub_cryptomount_args_t cargs);
grub_err_t (*recover_key) (grub_disk_t disk, grub_cryptodisk_t dev, grub_cryptomount_args_t cargs);
};
typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-09-27 23:14 ` [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
@ 2021-10-03 19:16 ` Patrick Steinhardt
2021-10-03 23:57 ` Glenn Washburn
0 siblings, 1 reply; 13+ messages in thread
From: Patrick Steinhardt @ 2021-10-03 19:16 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
[-- Attachment #1: Type: text/plain, Size: 1855 bytes --]
On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote:
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> grub-core/disk/geli.c | 9 ++++-----
> grub-core/disk/luks.c | 11 +++++------
> grub-core/disk/luks2.c | 6 +++---
> include/grub/cryptodisk.h | 6 ++++--
> 5 files changed, 25 insertions(+), 33 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 86eaabe60..5e153ee0a 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> -static int check_boot, have_it;
We should really just get rid of `have_it`. It's only used in three
locations, and we only require it because
`grub_cryptodisk_scan_device_real` doesn't properly tell us whether it
was found or not. Using a parameter struct to pass back this value to
the caller feels like a code smell, and only papers over this weird
usage.
Anyway, your patch doesn't make it any worse, so we may just as well fix
it after this series has landed.
[snip]
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 5bd970692..230167ab0 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -69,6 +69,9 @@ typedef gcry_err_code_t
>
> struct grub_cryptomount_args
> {
> + grub_uint32_t check_boot : 1;
> + grub_uint32_t found_uuid : 1;
> + char *search_uuid;
> grub_uint8_t *key_data;
> grub_size_t key_len;
> };
Would be nice to have comments here which explain what these parameters
do. for `key_data` and `key_len` it's obvious enough, but as a reader I
wouldn't really know what `check_boot` ought to indicate.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-10-03 19:16 ` Patrick Steinhardt
@ 2021-10-03 23:57 ` Glenn Washburn
2021-10-04 8:43 ` Patrick Steinhardt
0 siblings, 1 reply; 13+ messages in thread
From: Glenn Washburn @ 2021-10-03 23:57 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
On Sun, 3 Oct 2021 21:16:09 +0200
Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote:
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > grub-core/disk/geli.c | 9 ++++-----
> > grub-core/disk/luks.c | 11 +++++------
> > grub-core/disk/luks2.c | 6 +++---
> > include/grub/cryptodisk.h | 6 ++++--
> > 5 files changed, 25 insertions(+), 33 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 86eaabe60..5e153ee0a 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> >
> > #endif
> >
> > -static int check_boot, have_it;
>
> We should really just get rid of `have_it`. It's only used in three
> locations, and we only require it because
> `grub_cryptodisk_scan_device_real` doesn't properly tell us whether it
> was found or not. Using a parameter struct to pass back this value to
> the caller feels like a code smell, and only papers over this weird
> usage.
>
> Anyway, your patch doesn't make it any worse, so we may just as well fix
> it after this series has landed.
I guess you wrote this before taknig a look at the next patch in this
series, which does get rid of have_it (renamed to found_uuid). The
intent of this patch was not to change the existing behavior, just get
rid of the globals. I could've moved the following patch before this
one, in which case have_it wouldn't exist here. But for historical
reasons it was easier not too.
>
> [snip]
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 5bd970692..230167ab0 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> >
> > struct grub_cryptomount_args
> > {
/* Flag to indicate that only bootable volumes should be decrypted */
> > + grub_uint32_t check_boot : 1;
> > + grub_uint32_t found_uuid : 1;
/* Only volumes matching this UUID should be decrpyted */
> > + char *search_uuid;
/* Key data used to decrypt voume */
> > grub_uint8_t *key_data;
/* Length of key_data */
> > grub_size_t key_len;
> > };
>
> Would be nice to have comments here which explain what these parameters
> do. for `key_data` and `key_len` it's obvious enough, but as a reader I
> wouldn't really know what `check_boot` ought to indicate.
I was trying to use the same names to provide continuity (except for
have_it which is badly named). check_boot might better be named as
only_boot. I can add some comments. I agree that as a reader I like to
see descriptions of struct fields. How do the comments above look? I
didn't add one for found_uuid because the next patch gets rid of it
anyway.
Glenn
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-10-03 23:57 ` Glenn Washburn
@ 2021-10-04 8:43 ` Patrick Steinhardt
2021-10-04 17:13 ` Glenn Washburn
0 siblings, 1 reply; 13+ messages in thread
From: Patrick Steinhardt @ 2021-10-04 8:43 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
[-- Attachment #1: Type: text/plain, Size: 3544 bytes --]
On Sun, Oct 03, 2021 at 06:57:45PM -0500, Glenn Washburn wrote:
> On Sun, 3 Oct 2021 21:16:09 +0200
> Patrick Steinhardt <ps@pks.im> wrote:
>
> > On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote:
> > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > ---
> > > grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > grub-core/disk/geli.c | 9 ++++-----
> > > grub-core/disk/luks.c | 11 +++++------
> > > grub-core/disk/luks2.c | 6 +++---
> > > include/grub/cryptodisk.h | 6 ++++--
> > > 5 files changed, 25 insertions(+), 33 deletions(-)
> > >
> > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > index 86eaabe60..5e153ee0a 100644
> > > --- a/grub-core/disk/cryptodisk.c
> > > +++ b/grub-core/disk/cryptodisk.c
> > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > >
> > > #endif
> > >
> > > -static int check_boot, have_it;
> >
> > We should really just get rid of `have_it`. It's only used in three
> > locations, and we only require it because
> > `grub_cryptodisk_scan_device_real` doesn't properly tell us whether it
> > was found or not. Using a parameter struct to pass back this value to
> > the caller feels like a code smell, and only papers over this weird
> > usage.
> >
> > Anyway, your patch doesn't make it any worse, so we may just as well fix
> > it after this series has landed.
>
> I guess you wrote this before taknig a look at the next patch in this
> series, which does get rid of have_it (renamed to found_uuid).
Indeed.
> The
> intent of this patch was not to change the existing behavior, just get
> rid of the globals. I could've moved the following patch before this
> one, in which case have_it wouldn't exist here. But for historical
> reasons it was easier not too.
I don't particularly mind the order, so your current version is good
enough for me.
> >
> > [snip]
> > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > index 5bd970692..230167ab0 100644
> > > --- a/include/grub/cryptodisk.h
> > > +++ b/include/grub/cryptodisk.h
> > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > >
> > > struct grub_cryptomount_args
> > > {
>
> /* Flag to indicate that only bootable volumes should be decrypted */
>
> > > + grub_uint32_t check_boot : 1;
> > > + grub_uint32_t found_uuid : 1;
>
> /* Only volumes matching this UUID should be decrpyted */
>
> > > + char *search_uuid;
>
> /* Key data used to decrypt voume */
This can either contain the user-provided password or contents of a key
file, right? Might be worthwhile to document that.
> > > grub_uint8_t *key_data;
>
> /* Length of key_data */
>
> > > grub_size_t key_len;
> > > };
> >
> > Would be nice to have comments here which explain what these parameters
> > do. for `key_data` and `key_len` it's obvious enough, but as a reader I
> > wouldn't really know what `check_boot` ought to indicate.
>
> I was trying to use the same names to provide continuity (except for
> have_it which is badly named). check_boot might better be named as
> only_boot. I can add some comments. I agree that as a reader I like to
> see descriptions of struct fields. How do the comments above look?
Thanks. Except for the one addendum above they look nice to me.
> I
> didn't add one for found_uuid because the next patch gets rid of it
> anyway.
>
> Glenn
Fair enough.
Patrick
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct
2021-10-04 8:43 ` Patrick Steinhardt
@ 2021-10-04 17:13 ` Glenn Washburn
0 siblings, 0 replies; 13+ messages in thread
From: Glenn Washburn @ 2021-10-04 17:13 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
On Mon, 4 Oct 2021 10:43:50 +0200
Patrick Steinhardt <ps@pks.im> wrote:
> On Sun, Oct 03, 2021 at 06:57:45PM -0500, Glenn Washburn wrote:
> > On Sun, 3 Oct 2021 21:16:09 +0200
> > Patrick Steinhardt <ps@pks.im> wrote:
> >
> > > On Mon, Sep 27, 2021 at 06:14:02PM -0500, Glenn Washburn wrote:
> > > > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > > > ---
> > > > grub-core/disk/cryptodisk.c | 26 +++++++++-----------------
> > > > grub-core/disk/geli.c | 9 ++++-----
> > > > grub-core/disk/luks.c | 11 +++++------
> > > > grub-core/disk/luks2.c | 6 +++---
> > > > include/grub/cryptodisk.h | 6 ++++--
> > > > 5 files changed, 25 insertions(+), 33 deletions(-)
> > > >
> > > > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > > > index 86eaabe60..5e153ee0a 100644
> > > > --- a/grub-core/disk/cryptodisk.c
> > > > +++ b/grub-core/disk/cryptodisk.c
> > > > @@ -984,9 +984,6 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
> > > >
> > > > #endif
> > > >
> > > > -static int check_boot, have_it;
> > >
> > > We should really just get rid of `have_it`. It's only used in three
> > > locations, and we only require it because
> > > `grub_cryptodisk_scan_device_real` doesn't properly tell us whether it
> > > was found or not. Using a parameter struct to pass back this value to
> > > the caller feels like a code smell, and only papers over this weird
> > > usage.
> > >
> > > Anyway, your patch doesn't make it any worse, so we may just as well fix
> > > it after this series has landed.
> >
> > I guess you wrote this before taknig a look at the next patch in this
> > series, which does get rid of have_it (renamed to found_uuid).
>
> Indeed.
>
> > The
> > intent of this patch was not to change the existing behavior, just get
> > rid of the globals. I could've moved the following patch before this
> > one, in which case have_it wouldn't exist here. But for historical
> > reasons it was easier not too.
>
> I don't particularly mind the order, so your current version is good
> enough for me.
>
> > >
> > > [snip]
> > > > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > > index 5bd970692..230167ab0 100644
> > > > --- a/include/grub/cryptodisk.h
> > > > +++ b/include/grub/cryptodisk.h
> > > > @@ -69,6 +69,9 @@ typedef gcry_err_code_t
> > > >
> > > > struct grub_cryptomount_args
> > > > {
> >
> > /* Flag to indicate that only bootable volumes should be decrypted */
> >
> > > > + grub_uint32_t check_boot : 1;
> > > > + grub_uint32_t found_uuid : 1;
> >
> > /* Only volumes matching this UUID should be decrpyted */
> >
> > > > + char *search_uuid;
> >
> > /* Key data used to decrypt voume */
>
> This can either contain the user-provided password or contents of a key
> file, right? Might be worthwhile to document that.
Make sense. I'll add that in. I'm seeing now that the comment as is
could lead one to believe that its the master key data, which its not.
Glenn
^ permalink raw reply [flat|nested] 13+ messages in thread
* [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args
2021-09-27 23:13 [PATCH v2 0/4] Refactor/improve cryptomount data passing to crypto modules Glenn Washburn
` (2 preceding siblings ...)
2021-09-27 23:14 ` [PATCH v2 3/4] cryptodisk: Move global variables into grub_cryptomount_args struct Glenn Washburn
@ 2021-09-27 23:14 ` Glenn Washburn
2021-10-03 20:22 ` Patrick Steinhardt
3 siblings, 1 reply; 13+ messages in thread
From: Glenn Washburn @ 2021-09-27 23:14 UTC (permalink / raw)
To: grub-devel, Daniel Kiper
Cc: Denis 'GNUtoo' Carikli, Patrick Steinhardt,
James Bottomley, Glenn Washburn
The member found_uuid was never used by the crypto-backends, but was used to
determine if a crypto-backend successfully mounted a cryptodisk with a given
uuid. This is not needed however, because grub_device_iterate will return 1
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.
Signed-off-by: Glenn Washburn <development@efficientek.com>
---
grub-core/disk/cryptodisk.c | 9 ++++-----
include/grub/cryptodisk.h | 1 -
2 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 5e153ee0a..033894257 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
grub_cryptodisk_insert (dev, name, source);
- cargs->found_uuid = 1;
-
goto cleanup;
}
goto cleanup;
@@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
if (err)
grub_print_error ();
- return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
+ return (!err && cargs->search_uuid) ? 1 : 0;
}
static grub_err_t
@@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
if (state[0].set) /* uuid */
{
+ int found_uuid = 0;
grub_cryptodisk_t dev;
dev = grub_cryptodisk_get_by_uuid (args[0]);
@@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
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);
- if (!cargs.found_uuid)
+ if (!found_uuid)
return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
return GRUB_ERR_NONE;
}
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index 230167ab0..f4afb9cbd 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -70,7 +70,6 @@ typedef gcry_err_code_t
struct grub_cryptomount_args
{
grub_uint32_t check_boot : 1;
- grub_uint32_t found_uuid : 1;
char *search_uuid;
grub_uint8_t *key_data;
grub_size_t key_len;
--
2.32.0
^ permalink raw reply related [flat|nested] 13+ messages in thread* Re: [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args
2021-09-27 23:14 ` [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args Glenn Washburn
@ 2021-10-03 20:22 ` Patrick Steinhardt
2021-10-03 23:57 ` Glenn Washburn
0 siblings, 1 reply; 13+ messages in thread
From: Patrick Steinhardt @ 2021-10-03 20:22 UTC (permalink / raw)
To: Glenn Washburn
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
[-- Attachment #1: Type: text/plain, Size: 3492 bytes --]
On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote:
> The member found_uuid was never used by the crypto-backends, but was used to
> determine if a crypto-backend successfully mounted a cryptodisk with a given
> uuid. This is not needed however, because grub_device_iterate will return 1
> 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.
Is that always the case though? Most notably, `scan_device_real` will
only set `have_it = 1` in case we have recovered the key. If the
cryptodisk existed before and was found via `get_by_source_disk`, then
we wouldn't set `have_it` at all. This essentially means that we'd only
set `have_it` in case we found a new cryptodisk, but not if we return a
preexisting one.
I don't know whether this behaviour is something we rely on. My gut
feeling says it's rather a bug in the current code, which seems to be
confirmed by the error message in the `if (!have_it)` case, which says
"no such cryptodisk found". We did find one, but it's not a new one.
So in total I think your patch makes sense and fixes a bug, but the
description doesn't quite match reality.
Patrick
> Signed-off-by: Glenn Washburn <development@efficientek.com>
> ---
> grub-core/disk/cryptodisk.c | 9 ++++-----
> include/grub/cryptodisk.h | 1 -
> 2 files changed, 4 insertions(+), 6 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 5e153ee0a..033894257 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
>
> grub_cryptodisk_insert (dev, name, source);
>
> - cargs->found_uuid = 1;
> -
> goto cleanup;
> }
> goto cleanup;
> @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
>
> if (err)
> grub_print_error ();
> - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> + return (!err && cargs->search_uuid) ? 1 : 0;
> }
>
> static grub_err_t
> @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>
> if (state[0].set) /* uuid */
> {
> + int found_uuid = 0;
> grub_cryptodisk_t dev;
>
> dev = grub_cryptodisk_get_by_uuid (args[0]);
> @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>
> 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);
>
> - if (!cargs.found_uuid)
> + if (!found_uuid)
> return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> return GRUB_ERR_NONE;
> }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index 230167ab0..f4afb9cbd 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -70,7 +70,6 @@ typedef gcry_err_code_t
> struct grub_cryptomount_args
> {
> grub_uint32_t check_boot : 1;
> - grub_uint32_t found_uuid : 1;
> char *search_uuid;
> grub_uint8_t *key_data;
> grub_size_t key_len;
> --
> 2.32.0
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 13+ messages in thread* Re: [PATCH v2 4/4] cryptodisk: Remove unneeded found_uuid from cryptomount args
2021-10-03 20:22 ` Patrick Steinhardt
@ 2021-10-03 23:57 ` Glenn Washburn
0 siblings, 0 replies; 13+ messages in thread
From: Glenn Washburn @ 2021-10-03 23:57 UTC (permalink / raw)
To: Patrick Steinhardt
Cc: grub-devel, Daniel Kiper, Denis 'GNUtoo' Carikli,
James Bottomley
On Sun, 3 Oct 2021 22:22:40 +0200
Patrick Steinhardt <ps@pks.im> wrote:
> On Mon, Sep 27, 2021 at 06:14:03PM -0500, Glenn Washburn wrote:
> > The member found_uuid was never used by the crypto-backends, but was used to
> > determine if a crypto-backend successfully mounted a cryptodisk with a given
> > uuid. This is not needed however, because grub_device_iterate will return 1
> > 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.
>
> Is that always the case though? Most notably, `scan_device_real` will
> only set `have_it = 1` in case we have recovered the key. If the
> cryptodisk existed before and was found via `get_by_source_disk`, then
> we wouldn't set `have_it` at all. This essentially means that we'd only
> set `have_it` in case we found a new cryptodisk, but not if we return a
> preexisting one.
>
> I don't know whether this behaviour is something we rely on. My gut
> feeling says it's rather a bug in the current code, which seems to be
> confirmed by the error message in the `if (!have_it)` case, which says
> "no such cryptodisk found". We did find one, but it's not a new one.
>
> So in total I think your patch makes sense and fixes a bug, but the
> description doesn't quite match reality.
Good catch, I actually hadn't thought about this case, and
inadvertently fixed this bug. I'll update the commit message to reflect
this. I don't believe the behavior of this "bug" is relied on anywhere
either.
Glenn
>
> Patrick
>
> > Signed-off-by: Glenn Washburn <development@efficientek.com>
> > ---
> > grub-core/disk/cryptodisk.c | 9 ++++-----
> > include/grub/cryptodisk.h | 1 -
> > 2 files changed, 4 insertions(+), 6 deletions(-)
> >
> > diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> > index 5e153ee0a..033894257 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1046,8 +1046,6 @@ grub_cryptodisk_scan_device_real (const char *name,
> >
> > grub_cryptodisk_insert (dev, name, source);
> >
> > - cargs->found_uuid = 1;
> > -
> > goto cleanup;
> > }
> > goto cleanup;
> > @@ -1132,7 +1130,7 @@ grub_cryptodisk_scan_device (const char *name,
> >
> > if (err)
> > grub_print_error ();
> > - return (cargs->found_uuid && cargs->search_uuid) ? 1 : 0;
> > + return (!err && cargs->search_uuid) ? 1 : 0;
> > }
> >
> > static grub_err_t
> > @@ -1152,6 +1150,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >
> > if (state[0].set) /* uuid */
> > {
> > + int found_uuid = 0;
> > grub_cryptodisk_t dev;
> >
> > dev = grub_cryptodisk_get_by_uuid (args[0]);
> > @@ -1164,9 +1163,9 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
> >
> > 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);
> >
> > - if (!cargs.found_uuid)
> > + if (!found_uuid)
> > return grub_error (GRUB_ERR_BAD_ARGUMENT, "no such cryptodisk found");
> > return GRUB_ERR_NONE;
> > }
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index 230167ab0..f4afb9cbd 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -70,7 +70,6 @@ typedef gcry_err_code_t
> > struct grub_cryptomount_args
> > {
> > grub_uint32_t check_boot : 1;
> > - grub_uint32_t found_uuid : 1;
> > char *search_uuid;
> > grub_uint8_t *key_data;
> > grub_size_t key_len;
> > --
> > 2.32.0
> >
^ permalink raw reply [flat|nested] 13+ messages in thread