On Tue, Oct 12, 2021 at 06:26:26PM -0500, Glenn Washburn wrote:
> As an example, passing a password as a cryptomount argument is implemented.
I am not very happy with that. Splitting this into separate patch or
merging with patch #2 probably would not help either.
> However, the backends are not implemented, so testing this will return a not
> implemented error.
The commit message lacks of explanation why this change is needed.
I think you can copy part of the cover letter here.
> Signed-off-by: Glenn Washburn <developm...@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..577942088 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},
Should not you update docs/grub.texi as well?
> {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 */
Nice but if you want to do that please put that change into separate patch.
Or at least advise in the commit message you are doing this on occasion.
> {
> 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 */
Ditto.
> {
> 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>"),
s/[-p password] <SOURCE|-u UUID|-a|-b>/<SOURCE|-u UUID|-a|-b|-p password>/?
> 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)
if (cargs->key_data != NULL || 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)
Ditto.
> + 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)
Ditto.
> + return GRUB_ERR_NOT_IMPLEMENTED_YET;
> +
> ret = luks2_read_header (source, &header);
> if (ret)
> return ret;
Daniel
_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel
