[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 4/5] cryptodisk: Support key protectors

From: Hernan Gatta
Subject: Re: [PATCH 4/5] cryptodisk: Support key protectors
Date: Thu, 27 Jan 2022 06:26:14 -0800 (PST)

On Mon, 24 Jan 2022, Glenn Washburn wrote:

On Mon, 24 Jan 2022 06:12:17 -0800
Hernan Gatta <> wrote:

From: Hernan Gatta <>

Add a new parameter to cryptomount to support the key protectors framework: -k.
This parameter is accepted whenever the cryptomount command is used to mount a
specific disk either via a disk specification (e.g., (hd0,gpt1)) or by UUID, but
not when disks are mounted in bulk (i.e., via -a or -b). The parameter is used
to automatically retrieve a key from the specified key protector.

Why shouldn't key protectors be used to try to unlock all devices that
a backend identified as being able to unlock? Specifically, can a key
stored via the TPM be used for multiple devices? With respect to a
future unencrypted keyfile protector, it is certainly the case that it
can be applied to multiple devices.

I was wondering about this myself and was hoping to hear feedback on it. I'll remove the limitation as suggested.

Signed-off-by: <Hernan Gatta>
 Makefile.util.def           |  1 +
 grub-core/disk/cryptodisk.c | 21 ++++++++++++++++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

diff --git a/Makefile.util.def b/Makefile.util.def
index f8b356c..39b53b3 100644
--- a/Makefile.util.def
+++ b/Makefile.util.def
@@ -35,6 +35,7 @@ library = {
   common = grub-core/kern/list.c;
   common = grub-core/kern/misc.c;
   common = grub-core/kern/partition.c;
+  common = grub-core/kern/protectors.c;
   common = grub-core/lib/crypto.c;
   common = grub-core/lib/json/json.c;
   common = grub-core/disk/luks.c;
diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 4970973..176dd56 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -26,6 +26,7 @@
 #include <grub/file.h>
 #include <grub/procfs.h>
 #include <grub/partition.h>
+#include <grub/protector.h>

 #ifdef GRUB_UTIL
 #include <grub/emu/hostdisk.h>
@@ -42,6 +43,7 @@ static const struct grub_arg_option options[] =
     {"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},
+    {"protector", 'k', 0, N_("Unlock disk using the specified key 
protector."), 0, ARG_TYPE_STRING},

This conflicts with the keyfile and detached header patch series as is.
Ultimately, this is probably the right way to go and there will be an
protector for using unencrypted keyfiles.

Another option is to use -P for protectors and -k for keyfiles and have
two separate methods of using unencrypted keyfiles. I generally don't
like having two methods, the only benefit is that I believe the
--keyfile method is more intuitive. To use the keyfile protector the
user unfamiliar with these features will likely need to consult the

     {0, 0, 0, 0, 0, 0}

@@ -1160,6 +1162,7 @@ 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};
+  grub_err_t err;

   if (argc < 1 && !state[1].set && !state[2].set)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
@@ -1167,12 +1170,28 @@ 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[3].set && state[4].set) /* password and key protector */
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "a password and a key protector cannot both be set");
+  if (state[4].set && argc < 1) /* key protector */
+    return grub_error (GRUB_ERR_BAD_ARGUMENT,
+                       "key protectors require a device name or UUID");

Referring to the my first comment, I think we can get rid of this if

Will do.

   if (state[3].set) /* password */
       cargs.key_data = (grub_uint8_t *) state[3].arg;
       cargs.key_len = grub_strlen (state[3].arg);

+  if (state[4].set) /* key protector */
+    {
+      err = grub_key_protector_recover_key (state[4].arg, &cargs.key_data,
+                                                          &cargs.key_len);
+      if (err)
+        grub_printf_ (N_("Could not recover key from key protector.\n"));

Why is this not returning with an error here? Is it believed that
perhaps the device can be unlocked without recovering a key (even
though the user explicitly specified that a key should be used?)?

Yes, the idea was to let the user type in a passphrase in case something goes wrong with the key protector.

Another idea would be to have a passphrase key protector and have it prompt for input. Then, a user would type instead:

cryptomount -k tpm2:... -k passphrase

In this way, the user can be explicit about what the expectations are. cryptomount would try the first protector and, if that fails, then try the second, then the third, and so on.

If a user were then to specify:

cryptomount -k tpm2:...

but no "-k passphrase", no fallback would be initiated.


+    }
   if (state[0].set) /* uuid */
       int found_uuid;
@@ -1385,7 +1404,7 @@ GRUB_MOD_INIT (cryptodisk)
   grub_disk_dev_register (&grub_cryptodisk_dev);
   cmd = grub_register_extcmd ("cryptomount", grub_cmd_cryptomount, 0,
-                             N_("[-p password] <SOURCE|-u UUID|-a|-b>"),
+                             N_("[-p password] [-k protector[:args]] <SOURCE|-u 

This looks eerily similiar to what I proposed to James in response to
his SEV patches[1]. As such, I am in favor of this syntax and it looks
to me like a framework that James can use for his SEV series.


                              N_("Mount a crypto device."), options);
   grub_procfs_register ("luks_script", &luks_script);


Thank you,

reply via email to

[Prev in Thread] Current Thread [Next in Thread]