[PATCH v4 0/2] use confidential computing provisioned secrets for disk decryption

[PATCH v4 0/2] use confidential computing provisioned secrets for disk decryption

[James Bottomley]

From: James Bottomley <James.Bottomley@HansenPartnership.com>

v4: Update to new password passing API and fold in review comments
    original patch 1 (which contained a password passing API) is
    removed and patch 2 is updated and patch 3 largely unchanged.

v3: make password getter specify prompt requirement.  Update for TDX:
    Make name more generic and expand size of secret area

    https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba
    https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299

    Redo the cryptodisk secret handler to make it completely generic
    and pluggable using a list of named secret providers.  Also allow
    an optional additional argument for secret providers that may have
    more than one secret.

v2: update geli.c to use conditional prompt and add callback for
    variable message printing and secret destruction

To achieve encrypted disk images in the AMD SEV and other confidential
computing encrypted virtual machines, we need to add the ability for
grub to retrieve the disk passphrase from an OVMF provisioned
configuration table.

https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c

(this now needs additional patches to update for the change in flow in v4)

The patches in this series modify grub to look for the disk passphrase
in the secret configuration table and use it to decrypt any disks in
the system if they are found.  This is so an encrypted image with a
properly injected password will boot without any user intervention.

The three patches firstly modify the cryptodisk consumers to allow
arbitrary password getters instead of the current console based one.
The next patch adds a '-s module [id]' option to cryptodisk to allow
it to use plugin provided passwords and the final one adds a sevsecret
command to check for the secrets configuration table and provision the
disk passphrase from it if an entry is found.  With all this in place,
the sequence to boot an encrypted volume without user intervention is:

cryptomount -s efisecret -a
source (crypto0)/boot/grub.cfg

Assuming there's a standard Linux root partition.

James

---

James Bottomley (2):
  cryptodisk: add OS provided secret support
  efi: Add API for retrieving the EFI secret for cryptodisk

 grub-core/Makefile.core.def    |   8 ++
 grub-core/disk/cryptodisk.c    |  56 +++++++++++++-
 grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
 include/grub/cryptodisk.h      |  14 ++++
 include/grub/efi/api.h         |  15 ++++
 5 files changed, 220 insertions(+), 2 deletions(-)
 create mode 100644 grub-core/disk/efi/efisecret.c

-- 
2.34.1

[PATCH v4 1/2] cryptodisk: add OS provided secret support

[James Bottomley]

Make use of the new OS provided secrets API so that if the new '-s'
option is passed in we try to extract the secret from the API rather
than prompting for it.

The primary consumer of this is AMD SEV, which has been programmed to
provide an injectable secret to the encrypted virtual machine.  OVMF
provides the secret area and passes it into the EFI Configuration
Tables.  The grub EFI layer pulls the secret out and primes the
secrets API with it.  The upshot of all of this is that a SEV
protected VM can do an encrypted boot with a protected boot secret.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>

---

v2: add callback for after secret use
v3: update to use named module mechanism for secret loading
v4: update for new secret passing API
---
 grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++++++++--
 include/grub/cryptodisk.h   | 14 ++++++++++
 2 files changed, 68 insertions(+), 2 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 497097394..f9c86f958 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -42,6 +42,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},
+    {"secret", 's', 0, N_("Get secret passphrase from named module and optional identifier"), 0, 0},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
 
 #endif
 
+/* variable to hold the list of secret providers */
+static struct grub_secret_entry *secret_providers;
+
 static void
 cryptodisk_close (grub_cryptodisk_t dev)
 {
@@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
   return dev;
 }
 
+void
+grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e)
+{
+  grub_list_push(GRUB_AS_LIST_P (&secret_providers), GRUB_AS_LIST (e));
+}
+
+void
+grub_cryptodisk_remove_secret_provider  (struct grub_secret_entry *e)
+{
+  grub_list_remove (GRUB_AS_LIST (e));
+}
+
 #ifdef GRUB_UTIL
 #include <grub/util/misc.h>
 grub_err_t
@@ -1160,10 +1176,14 @@ 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};
+  struct grub_secret_entry *se = NULL;
 
-  if (argc < 1 && !state[1].set && !state[2].set)
+  if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
 
+  if (state[3].set && state[4].set)
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive options");
+
   if (grub_cryptodisk_list == NULL)
     return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
 
@@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       cargs.key_data = (grub_uint8_t *) state[3].arg;
       cargs.key_len = grub_strlen (state[3].arg);
     }
+  else if (state[4].set)	/* secret module */
+    {
+      grub_err_t rc;
+
+      if (argc < 1)
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must be specified");
+#ifndef GRUB_UTIL
+      grub_dl_load (args[0]);
+#endif
+      se = grub_named_list_find (GRUB_AS_NAMED_LIST (secret_providers), args[0]);
+      if (se == NULL)
+	return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret provider is found");
+
+      rc = se->get (args[1], &cargs.key_data);
+      if (rc)
+	return rc;
+      cargs.key_len = grub_strlen((char *) cargs.key_data);
+    }
 
   if (state[0].set) /* uuid */
     {
@@ -1190,6 +1228,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       cargs.search_uuid = args[0];
       found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
 
+      if (state[4].set)
+        {
+          se->put (args[1], found_uuid, &cargs.key_data);
+        }
+
       if (found_uuid)
 	return GRUB_ERR_NONE;
       else if (grub_errno == GRUB_ERR_NONE)
@@ -1208,6 +1251,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
     {
       cargs.check_boot = state[2].set;
       grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
+      if (state[4].set)
+        {
+          se->put (args[1], 1, &cargs.key_data);
+        }
       return GRUB_ERR_NONE;
     }
   else
@@ -1252,6 +1299,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       if (disklast)
 	*disklast = ')';
 
+      if (state[4].set)
+        {
+          se->put (args[1], dev != NULL, &cargs.key_data);
+        }
+
       return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
     }
 }
@@ -1385,7 +1437,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] <SOURCE|-u UUID|-a|-b|-s MOD [ID]>"),
 			      N_("Mount a crypto device."), options);
   grub_procfs_register ("luks_script", &luks_script);
 }
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index c6524c9ea..60249f1fc 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
 grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
 grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
 
+struct grub_secret_entry {
+  /* as named list */
+  struct grub_secret_entry *next;
+  struct grub_secret_entry **prev;
+  const char *name;
+
+  /* additional entries */
+  grub_err_t (*get) (const char *arg, grub_uint8_t **secret);
+  grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t **secret);
+};
+
+void grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e);
+void grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e);
+
 #endif
-- 
2.34.1

[PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk

[James Bottomley]

This module is designed to provide an efisecret provider which
interrogates the EFI configuration table to find the location of the
confidential computing secret and tries to register the secret with
the cryptodisk.

The secret is stored in a boot allocated area, usually a page in size.
The layout of the secret injection area is a header

|GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|

with entries of the form

|guid|len|data|

the guid corresponding to the disk encryption passphrase is
GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.
To get a high entropy string that doesn't need large numbers of
iterations, use a base64 encoding of 33 bytes of random data.

Signed-off-by: James Bottomley <jejb@linux.ibm.com>

---

v2: use callback to print failure message and destroy secret
v3: change to generic naming to use for TDX and SEV and use new mechanism
v4: review fixes
---
 grub-core/Makefile.core.def    |   8 ++
 grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
 include/grub/efi/api.h         |  15 ++++
 3 files changed, 152 insertions(+)
 create mode 100644 grub-core/disk/efi/efisecret.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 8022e1c0a..6293ddaa5 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -788,6 +788,14 @@ module = {
   enable = efi;
 };
 
+module = {
+  name = efisecret;
+
+  common = disk/efi/efisecret.c;
+
+  enable = efi;
+};
+
 module = {
   name = lsefimmap;
 
diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
new file mode 100644
index 000000000..4cecebbdc
--- /dev/null
+++ b/grub-core/disk/efi/efisecret.c
@@ -0,0 +1,129 @@
+#include <grub/err.h>
+#include <grub/misc.h>
+#include <grub/cryptodisk.h>
+#include <grub/efi/efi.h>
+#include <grub/efi/api.h>
+#include <grub/dl.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
+static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID;
+static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
+
+struct efi_secret {
+  grub_uint64_t base;
+  grub_uint64_t size;
+};
+
+struct secret_header {
+  grub_efi_packed_guid_t guid;
+  grub_uint32_t len;
+};
+
+struct secret_entry {
+  grub_efi_packed_guid_t guid;
+  grub_uint32_t len;
+  grub_uint8_t data[0];
+};
+
+static grub_err_t
+grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
+		     grub_uint8_t **ptr)
+{
+  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data);
+  int len = e->len;
+
+  /* destroy the secret */
+  grub_memset (e, 0, len);
+  /* put back the length to make sure the table is still traversable */
+  e->len = len;
+
+  *ptr = NULL;
+
+  if (have_it)
+    return GRUB_ERR_NONE;
+
+  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any volumes");
+}
+
+static grub_err_t
+grub_efi_secret_find (struct efi_secret *s, grub_uint8_t **secret_ptr)
+{
+  int len;
+  struct secret_header *h;
+  struct secret_entry *e;
+  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
+
+  /* the area must be big enough for a guid and a u32 length */
+  if (s->size < sizeof (*h))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small");
+
+  h = (struct secret_header *)ptr;
+  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not start with correct guid\n");
+  if (h->len < sizeof (*h))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small\n");
+
+  len = h->len - sizeof (*h);
+  ptr += sizeof (*h);
+
+  while (len >= (int)sizeof (*e)) {
+    e = (struct secret_entry *)ptr;
+    if (e->len < sizeof(*e) || e->len > (unsigned int)len)
+      return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is corrupt\n");
+
+    if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) {
+      int end = e->len - sizeof(*e);
+
+      /*
+       * the passphrase must be a zero terminated string because the
+       * password routines call grub_strlen () to find its size
+       */
+      if (end < 2 || e->data[end - 1] != '\0')
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is corrupt\n");
+
+      *secret_ptr = e->data;
+      return GRUB_ERR_NONE;
+    }
+    ptr += e->len;
+    len -= e->len;
+  }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not contain disk decryption password\n");
+}
+
+static grub_err_t
+grub_efi_secret_get (const char *arg __attribute__((unused)), grub_uint8_t **ptr)
+{
+  unsigned int i;
+
+  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
+    {
+      grub_efi_packed_guid_t *guid =
+	&grub_efi_system_table->configuration_table[i].vendor_guid;
+
+      if (! grub_memcmp (guid, &secret_guid, sizeof (grub_efi_packed_guid_t))) {
+	struct efi_secret *s =
+	  grub_efi_system_table->configuration_table[i].vendor_table;
+
+	return grub_efi_secret_find(s, ptr);
+      }
+    }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI configuration table");
+}
+
+static struct grub_secret_entry secret = {
+  .name = "efisecret",
+  .get = grub_efi_secret_get,
+  .put = grub_efi_secret_put,
+};
+
+GRUB_MOD_INIT(efisecret)
+{
+  grub_cryptodisk_add_secret_provider (&secret);
+}
+
+GRUB_MOD_FINI(efisecret)
+{
+  grub_cryptodisk_remove_secret_provider (&secret);
+}
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index f1a52210c..f33a8f2ab 100644
--- a/include/grub/efi/api.h
+++ b/include/grub/efi/api.h
@@ -299,6 +299,21 @@
     { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
   }
 
+#define GRUB_EFI_SECRET_TABLE_GUID \
+  { 0xadf956ad, 0xe98c, 0x484c, \
+    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
+  }
+
+#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \
+  { 0x1e74f542, 0x71dd, 0x4d66, \
+    { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \
+  }
+
+#define GRUB_EFI_DISKPASSWD_GUID \
+  { 0x736869e5, 0x84f0, 0x4973, \
+    { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \
+  }
+
 #define GRUB_EFI_ACPI_TABLE_GUID	\
   { 0xeb9d2d30, 0x2d88, 0x11d3, \
     { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
-- 
2.34.1

Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk

[Dr. David Alan Gilbert]

* James Bottomley (jejb@linux.ibm.com) wrote:
> This module is designed to provide an efisecret provider which
> interrogates the EFI configuration table to find the location of the
> confidential computing secret and tries to register the secret with
> the cryptodisk.
> 
> The secret is stored in a boot allocated area, usually a page in size.
> The layout of the secret injection area is a header
> 
> |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|
> 
> with entries of the form
> 
> |guid|len|data|
> 
> the guid corresponding to the disk encryption passphrase is
> GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.
> To get a high entropy string that doesn't need large numbers of
> iterations, use a base64 encoding of 33 bytes of random data.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: use callback to print failure message and destroy secret
> v3: change to generic naming to use for TDX and SEV and use new mechanism
> v4: review fixes
> ---
>  grub-core/Makefile.core.def    |   8 ++
>  grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h         |  15 ++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 grub-core/disk/efi/efisecret.c
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c0a..6293ddaa5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -788,6 +788,14 @@ module = {
>    enable = efi;
>  };
>  
> +module = {
> +  name = efisecret;
> +
> +  common = disk/efi/efisecret.c;
> +
> +  enable = efi;
> +};
> +
>  module = {
>    name = lsefimmap;
>  
> diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
> new file mode 100644
> index 000000000..4cecebbdc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include <grub/err.h>
> +#include <grub/misc.h>
> +#include <grub/cryptodisk.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +struct efi_secret {
> +  grub_uint64_t base;
> +  grub_uint64_t size;
> +};
> +
> +struct secret_header {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +};
> +
> +struct secret_entry {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +  grub_uint8_t data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> +		     grub_uint8_t **ptr)
> +{
> +  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data);

use offsetof ?

> +  int len = e->len;
> +
> +  /* destroy the secret */
> +  grub_memset (e, 0, len);
> +  /* put back the length to make sure the table is still traversable */
> +  e->len = len;
> +
> +  *ptr = NULL;
> +
> +  if (have_it)
> +    return GRUB_ERR_NONE;
> +
> +  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any volumes");
> +}
> +
> +static grub_err_t
> +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t **secret_ptr)
> +{
> +  int len;
> +  struct secret_header *h;
> +  struct secret_entry *e;
> +  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> +
> +  /* the area must be big enough for a guid and a u32 length */
> +  if (s->size < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small");
> +
> +  h = (struct secret_header *)ptr;
> +  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not start with correct guid\n");
> +  if (h->len < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small\n");

These grub_error calls (and a couple later) are terminated using a \n
which I don't think you want.

> +
> +  len = h->len - sizeof (*h);
> +  ptr += sizeof (*h);
> +
> +  while (len >= (int)sizeof (*e)) {
> +    e = (struct secret_entry *)ptr;
> +    if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is corrupt\n");
> +
> +    if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) {
> +      int end = e->len - sizeof(*e);
> +
> +      /*
> +       * the passphrase must be a zero terminated string because the
> +       * password routines call grub_strlen () to find its size
> +       */
> +      if (end < 2 || e->data[end - 1] != '\0')
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is corrupt\n");
> +
> +      *secret_ptr = e->data;
> +      return GRUB_ERR_NONE;
> +    }
> +    ptr += e->len;
> +    len -= e->len;
> +  }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not contain disk decryption password\n");
> +}
> +
> +static grub_err_t
> +grub_efi_secret_get (const char *arg __attribute__((unused)), grub_uint8_t **ptr)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +    {
> +      grub_efi_packed_guid_t *guid =
> +	&grub_efi_system_table->configuration_table[i].vendor_guid;
> +
> +      if (! grub_memcmp (guid, &secret_guid, sizeof (grub_efi_packed_guid_t))) {
> +	struct efi_secret *s =
> +	  grub_efi_system_table->configuration_table[i].vendor_table;
> +
> +	return grub_efi_secret_find(s, ptr);
> +      }
> +    }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI configuration table");
> +}
> +
> +static struct grub_secret_entry secret = {
> +  .name = "efisecret",
> +  .get = grub_efi_secret_get,
> +  .put = grub_efi_secret_put,
> +};
> +
> +GRUB_MOD_INIT(efisecret)
> +{
> +  grub_cryptodisk_add_secret_provider (&secret);
> +}
> +
> +GRUB_MOD_FINI(efisecret)
> +{
> +  grub_cryptodisk_remove_secret_provider (&secret);
> +}
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index f1a52210c..f33a8f2ab 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -299,6 +299,21 @@
>      { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
>    }
>  
> +#define GRUB_EFI_SECRET_TABLE_GUID \
> +  { 0xadf956ad, 0xe98c, 0x484c, \
> +    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
> +  }
> +
> +#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \
> +  { 0x1e74f542, 0x71dd, 0x4d66, \
> +    { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \
> +  }
> +
> +#define GRUB_EFI_DISKPASSWD_GUID \
> +  { 0x736869e5, 0x84f0, 0x4973, \
> +    { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \
> +  }
> +
>  #define GRUB_EFI_ACPI_TABLE_GUID	\
>    { 0xeb9d2d30, 0x2d88, 0x11d3, \
>      { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
> -- 
> 2.34.1
> 
-- 
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk

[James Bottomley]

On Mon, 2022-02-07 at 17:00 +0000, Dr. David Alan Gilbert wrote:
> * James Bottomley (jejb@linux.ibm.com) wrote:
[...]
> > +static grub_err_t
> > +grub_efi_secret_put (const char *arg __attribute__((unused)), int
> > have_it,
> > +		     grub_uint8_t **ptr)
> > +{
> > +  struct secret_entry *e = (struct secret_entry *)(*ptr -
> > (long)&((struct secret_entry *)0)->data);
> 
> use offsetof ?

Yes, Dov suggested that as well.  I don't really find either form more
or less readable, but I can certainly change it.

[...]
> > +static grub_err_t
> > +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t
> > **secret_ptr)
> > +{
> > +  int len;
> > +  struct secret_header *h;
> > +  struct secret_entry *e;
> > +  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> > +
> > +  /* the area must be big enough for a guid and a u32 length */
> > +  if (s->size < sizeof (*h))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small");
> > +
> > +  h = (struct secret_header *)ptr;
> > +  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area
> > does not start with correct guid\n");
> > +  if (h->len < sizeof (*h))
> > +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> > too small\n");
> 
> These grub_error calls (and a couple later) are terminated using a \n
> which I don't think you want.

That does seem to be consistent with the rest.  I note I didn't see an
extra \n in testing, but I bet something in grub eats it.

James

Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support

[Glenn Washburn]

On Mon,  7 Feb 2022 10:29:43 -0500
James Bottomley <jejb@linux.ibm.com> wrote:

> Make use of the new OS provided secrets API so that if the new '-s'
> option is passed in we try to extract the secret from the API rather
> than prompting for it.
> 
> The primary consumer of this is AMD SEV, which has been programmed to
> provide an injectable secret to the encrypted virtual machine.  OVMF
> provides the secret area and passes it into the EFI Configuration
> Tables.  The grub EFI layer pulls the secret out and primes the
> secrets API with it.  The upshot of all of this is that a SEV
> protected VM can do an encrypted boot with a protected boot secret.

I think I prefer the key protector framework proposed in the first
patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It feels
like a more generic mechanism (though admittedly very similar to yours)
because its not tied to cryptodisk. I'm not sure where we'd want to use
the secrets/protectors framework outside of cryptodisk, but it seems
like it could be useful. The advantage of this patch is that it allows
for the clearing of key data from memory.

I don't think we should have both a secrets and a key protectors
framework, as its seems they are aiming to accomplish basically the
same thing.

The name "secrets" seems a bit more generic than "protectors" because,
as in this case, the secret is not protected. There is something I
don't like about the word "secrets", but I don't have a better
suggestion, so this might be what sticks. One thing going for "secrets"
over "protectors", is that the cryptomount option "-s" works better
than "-p" for protectors because that's already taken by the password
option.

> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: add callback for after secret use
> v3: update to use named module mechanism for secret loading
> v4: update for new secret passing API
> ---
>  grub-core/disk/cryptodisk.c | 56 +++++++++++++++++++++++++++++++++++--
>  include/grub/cryptodisk.h   | 14 ++++++++++
>  2 files changed, 68 insertions(+), 2 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..f9c86f958 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,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},
> +    {"secret", 's', 0, N_("Get secret passphrase from named module and optional identifier"), 0, 0},
>      {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>  
>  #endif
>  
> +/* variable to hold the list of secret providers */
> +static struct grub_secret_entry *secret_providers;
> +
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
>  {
> @@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
>    return dev;
>  }
>  
> +void
> +grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e)
> +{
> +  grub_list_push(GRUB_AS_LIST_P (&secret_providers), GRUB_AS_LIST (e));
> +}
> +
> +void
> +grub_cryptodisk_remove_secret_provider  (struct grub_secret_entry *e)
> +{
> +  grub_list_remove (GRUB_AS_LIST (e));
> +}
> +
>  #ifdef GRUB_UTIL
>  #include <grub/util/misc.h>
>  grub_err_t
> @@ -1160,10 +1176,14 @@ 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};
> +  struct grub_secret_entry *se = NULL;
>  
> -  if (argc < 1 && !state[1].set && !state[2].set)
> +  if (argc < 1 && !state[1].set && !state[2].set && !state[3].set)
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "device name required");
>  
> +  if (state[3].set && state[4].set)
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive options");
> +
>    if (grub_cryptodisk_list == NULL)
>      return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
>  
> @@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        cargs.key_data = (grub_uint8_t *) state[3].arg;
>        cargs.key_len = grub_strlen (state[3].arg);
>      }
> +  else if (state[4].set)	/* secret module */
> +    {
> +      grub_err_t rc;
> +
> +      if (argc < 1)
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must be specified");
> +#ifndef GRUB_UTIL
> +      grub_dl_load (args[0]);
> +#endif
> +      se = grub_named_list_find (GRUB_AS_NAMED_LIST (secret_providers), args[0]);
> +      if (se == NULL)
> +	return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret provider is found");
> +
> +      rc = se->get (args[1], &cargs.key_data);
> +      if (rc)
> +	return rc;
> +      cargs.key_len = grub_strlen((char *) cargs.key_data);

It seems better to me to send a pointer to cargs.key_len to se->get()
because it already knows the length without having to do a strlen. And
this will allow NULLs in the key data.

> +    }
>  
>    if (state[0].set) /* uuid */
>      {
> @@ -1190,6 +1228,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        cargs.search_uuid = args[0];
>        found_uuid = grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
>  
> +      if (state[4].set)
> +        {
> +          se->put (args[1], found_uuid, &cargs.key_data);
> +        }
> +
>        if (found_uuid)
>  	return GRUB_ERR_NONE;
>        else if (grub_errno == GRUB_ERR_NONE)
> @@ -1208,6 +1251,10 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>      {
>        cargs.check_boot = state[2].set;
>        grub_device_iterate (&grub_cryptodisk_scan_device, &cargs);
> +      if (state[4].set)
> +        {
> +          se->put (args[1], 1, &cargs.key_data);
> +        }
>        return GRUB_ERR_NONE;
>      }
>    else
> @@ -1252,6 +1299,11 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
>        if (disklast)
>  	*disklast = ')';
>  
> +      if (state[4].set)
> +        {
> +          se->put (args[1], dev != NULL, &cargs.key_data);
> +        }
> +
>        return (dev == NULL) ? grub_errno : GRUB_ERR_NONE;
>      }
>  }
> @@ -1385,7 +1437,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] <SOURCE|-u UUID|-a|-b|-s MOD [ID]>"),

Seems like this should be:
 "[-p password|-s MOD [ID]] <SOURCE|-u UUID|-a|-b"

>  			      N_("Mount a crypto device."), options);
>    grub_procfs_register ("luks_script", &luks_script);
>  }
> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index c6524c9ea..60249f1fc 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
>  grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
>  grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t disk);
>  
> +struct grub_secret_entry {

s/grub_secret_entry/grub_secret_provider/

> +  /* as named list */
> +  struct grub_secret_entry *next;
> +  struct grub_secret_entry **prev;
> +  const char *name;
> +
> +  /* additional entries */
> +  grub_err_t (*get) (const char *arg, grub_uint8_t **secret);

I like having this first arg to pass some data to the secret provider.
I'm guessing this is to accomodate a keyfiles secrets provider. It seems
like it could also be used to differentiate between different elements
using the same secrets provider. Hypotheticaly say one had a USB device
with a bunch of key slots, a numerical value could be passed to choose
which slot to use. In light of that, perhaps the arg should be a void *,
to be more generic.

> +  grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t **secret);

I don't really like the names get and put. Something more descriptive
would be nice. I like "recover_key" or perhaps "recover_secret" for
"get", and "dispose" or "release" for "put". I'm open to other names
also.

> +};
> +
> +void grub_cryptodisk_add_secret_provider (struct grub_secret_entry *e);
> +void grub_cryptodisk_remove_secret_provider (struct grub_secret_entry *e);
> +
>  #endif

Glenn

Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk

[Glenn Washburn]

On Mon,  7 Feb 2022 10:29:44 -0500
James Bottomley <jejb@linux.ibm.com> wrote:

> This module is designed to provide an efisecret provider which
> interrogates the EFI configuration table to find the location of the
> confidential computing secret and tries to register the secret with
> the cryptodisk.
> 
> The secret is stored in a boot allocated area, usually a page in size.
> The layout of the secret injection area is a header
> 
> |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|
> 
> with entries of the form
> 
> |guid|len|data|
> 
> the guid corresponding to the disk encryption passphrase is
> GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.

Is the NULL termination requirement something specified in some
specification? Otherwise, its not clear to me why it must be so.

> To get a high entropy string that doesn't need large numbers of
> iterations, use a base64 encoding of 33 bytes of random data.

I won't reiterate the comments by David Gilbert, which I also agree
with.

> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: use callback to print failure message and destroy secret
> v3: change to generic naming to use for TDX and SEV and use new mechanism
> v4: review fixes
> ---
>  grub-core/Makefile.core.def    |   8 ++
>  grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h         |  15 ++++
>  3 files changed, 152 insertions(+)
>  create mode 100644 grub-core/disk/efi/efisecret.c
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c0a..6293ddaa5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -788,6 +788,14 @@ module = {
>    enable = efi;
>  };
>  
> +module = {
> +  name = efisecret;
> +
> +  common = disk/efi/efisecret.c;
> +
> +  enable = efi;
> +};
> +
>  module = {
>    name = lsefimmap;
>  
> diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
> new file mode 100644
> index 000000000..4cecebbdc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include <grub/err.h>
> +#include <grub/misc.h>
> +#include <grub/cryptodisk.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +struct efi_secret {

I find this name confusing. Perhaps a better name would be
"efi_secret_table_location"?

> +  grub_uint64_t base;
> +  grub_uint64_t size;
> +};
> +
> +struct secret_header {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +};
> +
> +struct secret_entry {
> +  grub_efi_packed_guid_t guid;
> +  grub_uint32_t len;
> +  grub_uint8_t data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> +		     grub_uint8_t **ptr)
> +{
> +  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data);
> +  int len = e->len;
> +
> +  /* destroy the secret */
> +  grub_memset (e, 0, len);
> +  /* put back the length to make sure the table is still traversable */
> +  e->len = len;
> +
> +  *ptr = NULL;
> +
> +  if (have_it)
> +    return GRUB_ERR_NONE;
> +
> +  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any volumes");

I'm not seeing why the "have_it" argument and returning an error here is
useful. Its seems a bit out of place. Wouldn't it be better to do this
in the caller?

> +}
> +
> +static grub_err_t
> +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t **secret_ptr)
> +{
> +  int len;
> +  struct secret_header *h;
> +  struct secret_entry *e;
> +  unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> +
> +  /* the area must be big enough for a guid and a u32 length */
> +  if (s->size < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small");
> +
> +  h = (struct secret_header *)ptr;
> +  if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not start with correct guid\n");
> +  if (h->len < sizeof (*h))
> +    return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too small\n");
> +
> +  len = h->len - sizeof (*h);
> +  ptr += sizeof (*h);
> +
> +  while (len >= (int)sizeof (*e)) {
> +    e = (struct secret_entry *)ptr;
> +    if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> +      return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is corrupt\n");
> +
> +    if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) {
> +      int end = e->len - sizeof(*e);
> +
> +      /*
> +       * the passphrase must be a zero terminated string because the
> +       * password routines call grub_strlen () to find its size
> +       */

I'm confused, what password routines are being referenced here? 

> +      if (end < 2 || e->data[end - 1] != '\0')
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is corrupt\n");

NULL termination seems like an unnecessary requirement, if secret length
is added as an output argument to the function.

> +
> +      *secret_ptr = e->data;
> +      return GRUB_ERR_NONE;
> +    }
> +    ptr += e->len;
> +    len -= e->len;
> +  }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not contain disk decryption password\n");
> +}
> +
> +static grub_err_t
> +grub_efi_secret_get (const char *arg __attribute__((unused)), grub_uint8_t **ptr)
> +{
> +  unsigned int i;
> +
> +  for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> +    {
> +      grub_efi_packed_guid_t *guid =
> +	&grub_efi_system_table->configuration_table[i].vendor_guid;
> +
> +      if (! grub_memcmp (guid, &secret_guid, sizeof (grub_efi_packed_guid_t))) {

Open brace should be on a separate line and indented 2 spaces.

Glenn

> +	struct efi_secret *s =
> +	  grub_efi_system_table->configuration_table[i].vendor_table;
> +
> +	return grub_efi_secret_find(s, ptr);
> +      }
> +    }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI configuration table");
> +}
> +
> +static struct grub_secret_entry secret = {
> +  .name = "efisecret",
> +  .get = grub_efi_secret_get,
> +  .put = grub_efi_secret_put,
> +};
> +
> +GRUB_MOD_INIT(efisecret)
> +{
> +  grub_cryptodisk_add_secret_provider (&secret);
> +}
> +
> +GRUB_MOD_FINI(efisecret)
> +{
> +  grub_cryptodisk_remove_secret_provider (&secret);
> +}
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index f1a52210c..f33a8f2ab 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -299,6 +299,21 @@
>      { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
>    }
>  
> +#define GRUB_EFI_SECRET_TABLE_GUID \
> +  { 0xadf956ad, 0xe98c, 0x484c, \
> +    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
> +  }
> +
> +#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \
> +  { 0x1e74f542, 0x71dd, 0x4d66, \
> +    { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \
> +  }
> +
> +#define GRUB_EFI_DISKPASSWD_GUID \
> +  { 0x736869e5, 0x84f0, 0x4973, \
> +    { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \
> +  }
> +
>  #define GRUB_EFI_ACPI_TABLE_GUID	\
>    { 0xeb9d2d30, 0x2d88, 0x11d3, \
>      { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \

Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support

[James Bottomley]

On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote:
> On Mon,  7 Feb 2022 10:29:43 -0500
> James Bottomley <jejb@linux.ibm.com> wrote:
> 
> > Make use of the new OS provided secrets API so that if the new '-s'
> > option is passed in we try to extract the secret from the API
> > rather than prompting for it.
> > 
> > The primary consumer of this is AMD SEV, which has been programmed
> > to provide an injectable secret to the encrypted virtual
> > machine.  OVMF provides the secret area and passes it into the EFI
> > Configuration Tables.  The grub EFI layer pulls the secret out and
> > primes the secrets API with it.  The upshot of all of this is that
> > a SEV protected VM can do an encrypted boot with a protected boot
> > secret.
> 
> I think I prefer the key protector framework proposed in the first
> patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It
> feels like a more generic mechanism (though admittedly very similar
> to yours) because its not tied to cryptodisk.

It's not really an either/or, though is it?  It looks to me like one
can trivially evolve into the other.

>  I'm not sure where we'd want to use the secrets/protectors framework
> outside of cryptodisk, but it seems like it could be useful. The
> advantage of this patch is that it allows for the clearing of key
> data from memory.

So if you evolve one into the other the put API would naturally remain
even if the tpm2 protector didn't want it.

> I don't think we should have both a secrets and a key protectors
> framework, as its seems they are aiming to accomplish basically the
> same thing.

Well, I don't think we would either.  Looking at the protectors API, it
would become an incremental patch to this.  Where I come from, this is
how we do API development: you merge an implementation and an API
that's as minimal as possible.  You try to make sure the API can be
extended if necessary, but you don't extend it yourself until another
implementation comes along that needs the extension.  This way you
don't need to bikeshed about future implementations because the perfect
is the enemy of the good.

> The name "secrets" seems a bit more generic than "protectors"
> because, as in this case, the secret is not protected. There is
> something I don't like about the word "secrets", but I don't have a
> better suggestion, so this might be what sticks. One thing going for
> "secrets" over "protectors", is that the cryptomount option "-s"
> works better than "-p" for protectors because that's already taken by
> the password option.
[...]

> +  else if (state[4].set)	/* secret module */
> > +    {
> > +      grub_err_t rc;
> > +
> > +      if (argc < 1)
> > +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must
> > be specified");
> > +#ifndef GRUB_UTIL
> > +      grub_dl_load (args[0]);
> > +#endif
> > +      se = grub_named_list_find (GRUB_AS_NAMED_LIST
> > (secret_providers), args[0]);
> > +      if (se == NULL)
> > +	return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret
> > provider is found");
> > +
> > +      rc = se->get (args[1], &cargs.key_data);
> > +      if (rc)
> > +	return rc;
> > +      cargs.key_len = grub_strlen((char *) cargs.key_data);
> 
> It seems better to me to send a pointer to cargs.key_len to se->get()
> because it already knows the length without having to do a strlen.
> And this will allow NULLs in the key data.

The original implementation was based on the grub password limitations
in that it had to be a zero terminated ASCII string.  While the zero
termination could now be relaxed, the ASCII requirement remains,
because the LUKS tools don't like passphrases with NULLS in them.  It
does simplify the code to pass the length pointer into the get API, so
I'll do that.

[...]
> > @@ -1385,7 +1437,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] <SOURCE|-u UUID|-a|-b|-
> > s MOD [ID]>"),
> 
> Seems like this should be:
>  "[-p password|-s MOD [ID]] <SOURCE|-u UUID|-a|-b"

Yes, that's effectively what the implementation turned out to be, I
just forgot to update the description string.

> diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > index c6524c9ea..60249f1fc 100644
> > --- a/include/grub/cryptodisk.h
> > +++ b/include/grub/cryptodisk.h
> > @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
> >  grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
> >  grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t
> > disk);
> >  
> > +struct grub_secret_entry {
> 
> s/grub_secret_entry/grub_secret_provider/

OK

> > +  /* as named list */
> > +  struct grub_secret_entry *next;
> > +  struct grub_secret_entry **prev;
> > +  const char *name;
> > +
> > +  /* additional entries */
> > +  grub_err_t (*get) (const char *arg, grub_uint8_t **secret);
> 
> I like having this first arg to pass some data to the secret
> provider. I'm guessing this is to accomodate a keyfiles secrets
> provider. It seems like it could also be used to differentiate
> between different elements using the same secrets provider.
> Hypotheticaly say one had a USB device with a bunch of key slots, a
> numerical value could be passed to choose which slot to use. In light
> of that, perhaps the arg should be a void *, to be more generic.

It's always going to be a pointer into the command line arguments, so
the type char * is the correct one for that.  It could become char
**arg and be passed &arg[1] so the provider could take multiple
optional arguments, but since the first implementation doesn't consume
this, I think that should be for future users to sort out.

> > +  grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t
> > **secret);
> 
> I don't really like the names get and put. Something more descriptive
> would be nice. I like "recover_key" or perhaps "recover_secret" for
> "get", and "dispose" or "release" for "put". I'm open to other names
> also.

get_secret; release_secret?

James

Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support

[Glenn Washburn]

Finally getting back to this...

On Thu, 17 Feb 2022 17:18:47 -0500
James Bottomley <jejb@linux.ibm.com> wrote:

> On Mon, 2022-02-14 at 16:18 -0600, Glenn Washburn wrote:
> > On Mon,  7 Feb 2022 10:29:43 -0500
> > James Bottomley <jejb@linux.ibm.com> wrote:
> > 
> > > Make use of the new OS provided secrets API so that if the new '-s'
> > > option is passed in we try to extract the secret from the API
> > > rather than prompting for it.
> > > 
> > > The primary consumer of this is AMD SEV, which has been programmed
> > > to provide an injectable secret to the encrypted virtual
> > > machine.  OVMF provides the secret area and passes it into the EFI
> > > Configuration Tables.  The grub EFI layer pulls the secret out and
> > > primes the secrets API with it.  The upshot of all of this is that
> > > a SEV protected VM can do an encrypted boot with a protected boot
> > > secret.
> > 
> > I think I prefer the key protector framework proposed in the first
> > patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It
> > feels like a more generic mechanism (though admittedly very similar
> > to yours) because its not tied to cryptodisk.
> 
> It's not really an either/or, though is it?  It looks to me like one
> can trivially evolve into the other.

Yep, this patch can easily become essentially what the key protector
framework is. I make the comparison because (1) its pending so it seems
likely that this would be modified soon, (2) I like that the key
protectors framework is in its own file as opposed to put in with
cryptodisk (though this patch is smaller so makes less sense as a
separate file), and (3) I find it aestetically unpleasing to add this
patch and then turn around and take it out and put it somewhere else.
But yes, it can be done.

> >  I'm not sure where we'd want to use the secrets/protectors framework
> > outside of cryptodisk, but it seems like it could be useful. The
> > advantage of this patch is that it allows for the clearing of key
> > data from memory.
> 
> So if you evolve one into the other the put API would naturally remain
> even if the tpm2 protector didn't want it.

Yep, we're on the same page here.

> > I don't think we should have both a secrets and a key protectors
> > framework, as its seems they are aiming to accomplish basically the
> > same thing.
> 
> Well, I don't think we would either.  Looking at the protectors API, it
> would become an incremental patch to this.  Where I come from, this is
> how we do API development: you merge an implementation and an API
> that's as minimal as possible.  You try to make sure the API can be
> extended if necessary, but you don't extend it yourself until another
> implementation comes along that needs the extension.  This way you
> don't need to bikeshed about future implementations because the perfect
> is the enemy of the good.

Yep, agreed. Both patches provide minimal APIs for the feature sets
they are introducing, as it should be. Aside from name changes, I'm not
criticizing your API (which to be clear I'm meaning the function
signatures). So I'm not seeing the above as relevant here.

I suspect that the core of your issue is that I was too general in my
comments above giving the impression that I'm rejecting this patch
without a clear path towards making it acceptable. To be more specific,
I liked that the key protectors patch was separated from cryptodisk
both in naming and in a separate file. I've since reconsidered the
separate file, which also doesn't make as much sense for this patch
because its small. I forgot to mention it before, but I was also
thinking that the function names could have a grub_secret_ prefix
instead of grub_cryptodisk_.

I think another issue, in which I've been on your side of the table of,
is determining which suggestions or dislikes are deal breakers. In the
abcense of clear communication on what's a deal breaker it can seem
like all are deal breakers. Me registering a complaint about something
doesn't necessarily mean I think it can't be accepted as is. However,
as a reviewer I feel it is my duty to list everything I have a problem
with. Ideally it should be very clear, but sometimes its hard to pin
point how a patch might be changed to be better. And if you want more
clarity (eg. on the specifics of a displike or how much of a deal
breaker something is) asking never hurts.

As for perfect being the enemy of the good, that's only if one forego's
the good in hand for an unattainable perfect. Good should just be an
iteration toward perfect. Perfect is the north star that provides
guidance on what is good. So I'm noting where its not perfect or at
least can be better (according to me). That doesn't mean if its not
perfect it can't be committed. But if we can make changes to make good
better now, then I think we should do that.

Also, I don't believe I'm bikeshedding about future implementations
here unless you consider the TPM unlock patch series the future. I'm
commenting on the relative difference in two patch series that are
looking to get included right now. So this isn't some hypothetical
situation.

Also, some of my comments may seem minor, trivial, or nit-picking. And
I'd agree with that for somethings, and I think that's okay as long as
we don't let the trivial be the enemy of the good. If we can get a
better patch series committed, I think thats good. 

> > The name "secrets" seems a bit more generic than "protectors"
> > because, as in this case, the secret is not protected. There is
> > something I don't like about the word "secrets", but I don't have a
> > better suggestion, so this might be what sticks. One thing going for
> > "secrets" over "protectors", is that the cryptomount option "-s"
> > works better than "-p" for protectors because that's already taken by
> > the password option.
> [...]
> 
> > +  else if (state[4].set)	/* secret module */
> > > +    {
> > > +      grub_err_t rc;
> > > +
> > > +      if (argc < 1)
> > > +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "secret module must
> > > be specified");
> > > +#ifndef GRUB_UTIL
> > > +      grub_dl_load (args[0]);
> > > +#endif
> > > +      se = grub_named_list_find (GRUB_AS_NAMED_LIST
> > > (secret_providers), args[0]);
> > > +      if (se == NULL)
> > > +	return grub_error (GRUB_ERR_INVALID_COMMAND, "No secret
> > > provider is found");

I didn't mention the first time, but I also like the idea of having an
inline function grub_secrets_find_provider which does the above 3 lines
(maybe 6). The one drawback I see is an extra branch to check the
functions return and return if error. Not a deal breaker.

> > > +
> > > +      rc = se->get (args[1], &cargs.key_data);
> > > +      if (rc)
> > > +	return rc;
> > > +      cargs.key_len = grub_strlen((char *) cargs.key_data);
> > 
> > It seems better to me to send a pointer to cargs.key_len to se->get()
> > because it already knows the length without having to do a strlen.
> > And this will allow NULLs in the key data.
> 
> The original implementation was based on the grub password limitations
> in that it had to be a zero terminated ASCII string.  While the zero
> termination could now be relaxed, the ASCII requirement remains,
> because the LUKS tools don't like passphrases with NULLS in them.  It
> does simplify the code to pass the length pointer into the get API, so
> I'll do that.
> 
> [...]
> > > @@ -1385,7 +1437,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] <SOURCE|-u UUID|-a|-b|-
> > > s MOD [ID]>"),
> > 
> > Seems like this should be:
> >  "[-p password|-s MOD [ID]] <SOURCE|-u UUID|-a|-b"
> 
> Yes, that's effectively what the implementation turned out to be, I
> just forgot to update the description string.
> 
> > diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> > > index c6524c9ea..60249f1fc 100644
> > > --- a/include/grub/cryptodisk.h
> > > +++ b/include/grub/cryptodisk.h
> > > @@ -183,4 +183,18 @@ grub_util_get_geli_uuid (const char *dev);
> > >  grub_cryptodisk_t grub_cryptodisk_get_by_uuid (const char *uuid);
> > >  grub_cryptodisk_t grub_cryptodisk_get_by_source_disk (grub_disk_t
> > > disk);
> > >  
> > > +struct grub_secret_entry {
> > 
> > s/grub_secret_entry/grub_secret_provider/
> 
> OK
> 
> > > +  /* as named list */
> > > +  struct grub_secret_entry *next;
> > > +  struct grub_secret_entry **prev;
> > > +  const char *name;
> > > +
> > > +  /* additional entries */
> > > +  grub_err_t (*get) (const char *arg, grub_uint8_t **secret);
> > 
> > I like having this first arg to pass some data to the secret
> > provider. I'm guessing this is to accomodate a keyfiles secrets
> > provider. It seems like it could also be used to differentiate
> > between different elements using the same secrets provider.
> > Hypotheticaly say one had a USB device with a bunch of key slots, a
> > numerical value could be passed to choose which slot to use. In light
> > of that, perhaps the arg should be a void *, to be more generic.
> 
> It's always going to be a pointer into the command line arguments, so
> the type char * is the correct one for that.  It could become char
> **arg and be passed &arg[1] so the provider could take multiple
> optional arguments, but since the first implementation doesn't consume
> this, I think that should be for future users to sort out.

I was saying this in the context of having a secret api that was less
tied to cryptodisk. We can change it down the road fairly easily if it
ever comes to that.

> > > +  grub_err_t (*put) (const char *arg, int have_it, grub_uint8_t
> > > **secret);
> > 
> > I don't really like the names get and put. Something more descriptive
> > would be nice. I like "recover_key" or perhaps "recover_secret" for
> > "get", and "dispose" or "release" for "put". I'm open to other names
> > also.
> 
> get_secret; release_secret?

This seems reasonable.

Glenn

Re: [PATCH v4 0/2] use confidential computing provisioned secrets for disk decryption

[Heinrich Schuchardt]

On 2/7/22 16:29, James Bottomley wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
> 
> v4: Update to new password passing API and fold in review comments
>      original patch 1 (which contained a password passing API) is
>      removed and patch 2 is updated and patch 3 largely unchanged.
> 
> v3: make password getter specify prompt requirement.  Update for TDX:
>      Make name more generic and expand size of secret area
> 
>      https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba
>      https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299
> 
>      Redo the cryptodisk secret handler to make it completely generic
>      and pluggable using a list of named secret providers.  Also allow
>      an optional additional argument for secret providers that may have
>      more than one secret.
> 
> v2: update geli.c to use conditional prompt and add callback for
>      variable message printing and secret destruction
> 
> To achieve encrypted disk images in the AMD SEV and other confidential
> computing encrypted virtual machines, we need to add the ability for
> grub to retrieve the disk passphrase from an OVMF provisioned
> configuration table.
> 
> https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c
> 
> (this now needs additional patches to update for the change in flow in v4)
> 
> The patches in this series modify grub to look for the disk passphrase
> in the secret configuration table and use it to decrypt any disks in
> the system if they are found.  This is so an encrypted image with a
> properly injected password will boot without any user intervention.
> 
> The three patches firstly modify the cryptodisk consumers to allow
> arbitrary password getters instead of the current console based one.
> The next patch adds a '-s module [id]' option to cryptodisk to allow
> it to use plugin provided passwords and the final one adds a sevsecret
> command to check for the secrets configuration table and provision the
> disk passphrase from it if an entry is found.  With all this in place,
> the sequence to boot an encrypted volume without user intervention is:
> 
> cryptomount -s efisecret -a
> source (crypto0)/boot/grub.cfg
> 
> Assuming there's a standard Linux root partition.
> 
> James

Is there a text document that defines the EFI secret table and its contents?

Best regards

Heinrich

> 
> ---
> 
> James Bottomley (2):
>    cryptodisk: add OS provided secret support
>    efi: Add API for retrieving the EFI secret for cryptodisk
> 
>   grub-core/Makefile.core.def    |   8 ++
>   grub-core/disk/cryptodisk.c    |  56 +++++++++++++-
>   grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>   include/grub/cryptodisk.h      |  14 ++++
>   include/grub/efi/api.h         |  15 ++++
>   5 files changed, 220 insertions(+), 2 deletions(-)
>   create mode 100644 grub-core/disk/efi/efisecret.c
> 

Re: [PATCH v4 0/2] use confidential computing provisioned secrets for disk decryption

[Dov Murik]

On 26/02/2022 10:04, Heinrich Schuchardt wrote:
> On 2/7/22 16:29, James Bottomley wrote:
>> From: James Bottomley <James.Bottomley@HansenPartnership.com>
>>
>> v4: Update to new password passing API and fold in review comments
>>      original patch 1 (which contained a password passing API) is
>>      removed and patch 2 is updated and patch 3 largely unchanged.
>>
>> v3: make password getter specify prompt requirement.  Update for TDX:
>>      Make name more generic and expand size of secret area
>>
>>     
>> https://github.com/tianocore/edk2/commit/96201ae7bf97c3a2c0ef386110bb93d25e9af1ba
>>
>>     
>> https://github.com/tianocore/edk2/commit/caf8b3872ae2ac961c9fdf4d1d2c5d072c207299
>>
>>
>>      Redo the cryptodisk secret handler to make it completely generic
>>      and pluggable using a list of named secret providers.  Also allow
>>      an optional additional argument for secret providers that may have
>>      more than one secret.
>>
>> v2: update geli.c to use conditional prompt and add callback for
>>      variable message printing and secret destruction
>>
>> To achieve encrypted disk images in the AMD SEV and other confidential
>> computing encrypted virtual machines, we need to add the ability for
>> grub to retrieve the disk passphrase from an OVMF provisioned
>> configuration table.
>>
>> https://github.com/tianocore/edk2/commit/01726b6d23d4c8a870dbd5b96c0b9e3caf38ef3c
>>
>>
>> (this now needs additional patches to update for the change in flow in
>> v4)
>>
>> The patches in this series modify grub to look for the disk passphrase
>> in the secret configuration table and use it to decrypt any disks in
>> the system if they are found.  This is so an encrypted image with a
>> properly injected password will boot without any user intervention.
>>
>> The three patches firstly modify the cryptodisk consumers to allow
>> arbitrary password getters instead of the current console based one.
>> The next patch adds a '-s module [id]' option to cryptodisk to allow
>> it to use plugin provided passwords and the final one adds a sevsecret
>> command to check for the secrets configuration table and provision the
>> disk passphrase from it if an entry is found.  With all this in place,
>> the sequence to boot an encrypted volume without user intervention is:
>>
>> cryptomount -s efisecret -a
>> source (crypto0)/boot/grub.cfg
>>
>> Assuming there's a standard Linux root partition.
>>
>> James
> 
> Is there a text document that defines the EFI secret table and its
> contents?
> 

Such documentation appears in the kernel driver we're proposing [1] to
allow userspace programs to read secrets from the same area (similarly
to how grub can read a secret from it).

In that patch [1], look for "Structure of the EFI secret area" in efi_secret.c.

Here's the relevant part:

+/*
+ * Structure of the EFI secret area
+ *
+ * Offset   Length
+ * (bytes)  (bytes)  Usage
+ * -------  -------  -----
+ *       0       16  Secret table header GUID (must be 1e74f542-71dd-4d66-963e-ef4287ff173b)
+ *      16        4  Length of bytes of the entire secret area
+ *
+ *      20       16  First secret entry's GUID
+ *      36        4  First secret entry's length in bytes (= 16 + 4 + x)
+ *      40        x  First secret entry's data
+ *
+ *    40+x       16  Second secret entry's GUID
+ *    56+x        4  Second secret entry's length in bytes (= 16 + 4 + y)
+ *    60+x        y  Second secret entry's data
+ *
+ * (... and so on for additional entries)
+ *
+ * The GUID of each secret entry designates the usage of the secret data.
+ */


Note that grub is looking for one entry from this table: an entry with
GUID 736870e5-84f0-4973-92ec-06879ce3da0b (GRUB_EFI_DISKPASSWD_GUID).


We'll also add similar documentation to kbs-rs [2] (Key Broker Service) which is
one of the options for the Guest Owner server that generates this secret area
(to be securely injected into the guest).


[1] https://lore.kernel.org/linux-coco/20220201124413.1093099-4-dovmurik@linux.ibm.com/#Z31drivers:virt:coco:efi_secret:efi_secret.c

[2] https://github.com/confidential-containers/kbs-rs


-Dov


> Best regards
> 
> Heinrich
> 
>>
>> ---
>>
>> James Bottomley (2):
>>    cryptodisk: add OS provided secret support
>>    efi: Add API for retrieving the EFI secret for cryptodisk
>>
>>   grub-core/Makefile.core.def    |   8 ++
>>   grub-core/disk/cryptodisk.c    |  56 +++++++++++++-
>>   grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>>   include/grub/cryptodisk.h      |  14 ++++
>>   include/grub/efi/api.h         |  15 ++++
>>   5 files changed, 220 insertions(+), 2 deletions(-)
>>   create mode 100644 grub-core/disk/efi/efisecret.c
>>
>