[RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption

[RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption

[James Bottomley]

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

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

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
source (crypto0)/boot/grub.cfg

Assuming there's a standard Linux root partition.

James

---

James Bottomley (3):
  cryptodisk: make the password getter and additional argument to
    recover_key
  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        |  77 +++++++++++++++++-
 grub-core/disk/efi/efisecret.c     | 125 +++++++++++++++++++++++++++++
 grub-core/disk/geli.c              |  12 +--
 grub-core/disk/luks.c              |  12 +--
 grub-core/disk/luks2.c             |  12 +--
 grub-core/lib/crypto.c             |   4 +
 grub-core/osdep/unix/password.c    |   4 +
 grub-core/osdep/windows/password.c |   4 +
 include/grub/cryptodisk.h          |  20 ++++-
 include/grub/efi/api.h             |  15 ++++
 11 files changed, 273 insertions(+), 20 deletions(-)
 create mode 100644 grub-core/disk/efi/efisecret.c

-- 
2.26.2

[RESEND v3 1/3] cryptodisk: make the password getter and additional argument to recover_key

[James Bottomley]

For AMD SEV environments, the grub boot password has to be retrieved
from a given memory location rather than prompted for.  This means
that the standard password getter needs to be replaced with one that
gets the passphrase from the SEV area and uses that instead.  Adding
the password getter as a passed in argument to recover_key() makes
this possible.

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

---

v2: add conditional prompting to geli.c
v3: make getter specify prompt requirement
---
 grub-core/disk/cryptodisk.c        |  2 +-
 grub-core/disk/geli.c              | 12 +++++++-----
 grub-core/disk/luks.c              | 12 +++++++-----
 grub-core/disk/luks2.c             | 12 +++++++-----
 grub-core/lib/crypto.c             |  4 ++++
 grub-core/osdep/unix/password.c    |  4 ++++
 grub-core/osdep/windows/password.c |  4 ++++
 include/grub/cryptodisk.h          |  6 +++++-
 8 files changed, 39 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 90f82b2d3..b52a3cfd6 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1015,7 +1015,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, grub_password_get);
     if (err)
     {
       cryptodisk_close (dev);
diff --git a/grub-core/disk/geli.c b/grub-core/disk/geli.c
index 2f34a35e6..3d826104d 100644
--- a/grub-core/disk/geli.c
+++ b/grub-core/disk/geli.c
@@ -398,7 +398,8 @@ 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_passwd_cb *password_get)
 {
   grub_size_t keysize;
   grub_uint8_t digest[GRUB_CRYPTO_MAX_MDLEN];
@@ -438,11 +439,12 @@ recover_key (grub_disk_t source, grub_cryptodisk_t dev)
   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);
+  if (password_get (NULL, 0))
+    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))
+  if (!password_get (passphrase, MAX_PASSPHRASE))
     return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
 
   /* Calculate the PBKDF2 of the user supplied passphrase.  */
diff --git a/grub-core/disk/luks.c b/grub-core/disk/luks.c
index 13103ea6a..13eee2a18 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_passwd_cb *password_get)
 {
   struct grub_luks_phdr header;
   grub_size_t keysize;
@@ -187,11 +188,12 @@ luks_recover_key (grub_disk_t source,
   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);
+  if (password_get (NULL, 0))
+	  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))
+  if (!password_get (passphrase, MAX_PASSPHRASE))
     {
       grub_free (split_key);
       return grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
diff --git a/grub-core/disk/luks2.c b/grub-core/disk/luks2.c
index 371a53b83..7625c1768 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_passwd_cb *password_get)
 {
   grub_uint8_t candidate_key[GRUB_CRYPTODISK_MAX_KEYLEN];
   char passphrase[MAX_PASSPHRASE], cipher[32];
@@ -584,10 +585,11 @@ luks2_recover_key (grub_disk_t source,
   /* 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))
+  if (password_get (NULL, 0))
+    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
+		  source->partition ? "," : "", part ? : "",
+		  crypt->uuid);
+  if (!password_get (passphrase, MAX_PASSPHRASE))
     {
       ret = grub_error (GRUB_ERR_BAD_ARGUMENT, "Passphrase not supplied");
       goto err;
diff --git a/grub-core/lib/crypto.c b/grub-core/lib/crypto.c
index ca334d5a4..34272a7ad 100644
--- a/grub-core/lib/crypto.c
+++ b/grub-core/lib/crypto.c
@@ -456,6 +456,10 @@ grub_password_get (char buf[], unsigned buf_size)
   unsigned cur_len = 0;
   int key;
 
+  if (!buf)
+    /* want prompt */
+    return 1;
+
   while (1)
     {
       key = grub_getkey (); 
diff --git a/grub-core/osdep/unix/password.c b/grub-core/osdep/unix/password.c
index 9996b244b..365ac4bad 100644
--- a/grub-core/osdep/unix/password.c
+++ b/grub-core/osdep/unix/password.c
@@ -34,6 +34,10 @@ grub_password_get (char buf[], unsigned buf_size)
   int tty_changed = 0;
   char *ptr;
 
+  if (!buf)
+    /* want prompt */
+    return 1;
+
   grub_refresh ();
 
   /* Disable echoing. Based on glibc.  */
diff --git a/grub-core/osdep/windows/password.c b/grub-core/osdep/windows/password.c
index 1d3af0c2c..2a6615611 100644
--- a/grub-core/osdep/windows/password.c
+++ b/grub-core/osdep/windows/password.c
@@ -33,6 +33,10 @@ grub_password_get (char buf[], unsigned buf_size)
   DWORD mode = 0;
   char *ptr;
 
+  if (!buf)
+    /* want prompt */
+    return 1;
+
   grub_refresh ();
   
   GetConsoleMode (hStdin, &mode);
diff --git a/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index dcf17fbb3..737487bb4 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -112,6 +112,9 @@ struct grub_cryptodisk
 };
 typedef struct grub_cryptodisk *grub_cryptodisk_t;
 
+/* must match prototype for grub_password_get */
+typedef int (grub_passwd_cb)(char buf[], unsigned buf_size);
+
 struct grub_cryptodisk_dev
 {
   struct grub_cryptodisk_dev *next;
@@ -119,7 +122,8 @@ 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_passwd_cb *get_password);
 };
 typedef struct grub_cryptodisk_dev *grub_cryptodisk_dev_t;
 
-- 
2.26.2

[RESEND v3 2/3] 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
---
 grub-core/disk/cryptodisk.c | 77 +++++++++++++++++++++++++++++++++++--
 include/grub/cryptodisk.h   | 14 +++++++
 2 files changed, 87 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index b52a3cfd6..1fe1357e3 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},
+    {"secret", 's', 0, N_("Get secret passphrase from named module and optional identifier"), 0, 0},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -985,6 +986,10 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
 
 static int check_boot, have_it;
 static char *search_uuid;
+static char *os_passwd;
+
+/* variable to hold the list of secret providers */
+static struct grub_secret_entry *secret_providers;
 
 static void
 cryptodisk_close (grub_cryptodisk_t dev)
@@ -995,6 +1000,21 @@ cryptodisk_close (grub_cryptodisk_t dev)
   grub_free (dev);
 }
 
+static int
+os_password_get(char buf[], unsigned len)
+{
+  if (!buf)
+    /* we're not interactive so no prompt */
+    return 0;
+
+  /* os_passwd should be null terminated, so just copy everything */
+  grub_strncpy(buf, os_passwd, len);
+  /* and add a terminator just in case */
+  buf[len - 1] = 0;
+
+  return 1;
+}
+
 static grub_err_t
 grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
 {
@@ -1014,8 +1034,17 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
       return grub_errno;
     if (!dev)
       continue;
-    
-    err = cr->recover_key (source, dev, grub_password_get);
+
+    if (os_passwd)
+      {
+	err = cr->recover_key (source, dev, os_password_get);
+	if (err)
+	  /* if the key doesn't work ignore the access denied error */
+	  grub_error_pop();
+      }
+    else
+      err = cr->recover_key (source, dev, grub_password_get);
+
     if (err)
     {
       cryptodisk_close (dev);
@@ -1031,6 +1060,18 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
   return GRUB_ERR_NONE;
 }
 
+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
@@ -1107,7 +1148,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 {
   struct grub_arg_list *state = ctxt->state;
 
-  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");
 
   have_it = 0;
@@ -1125,6 +1166,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
 
       check_boot = state[2].set;
       search_uuid = args[0];
+      os_passwd = NULL;
       grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
       search_uuid = NULL;
 
@@ -1135,11 +1177,37 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
   else if (state[1].set || (argc == 0 && state[2].set))
     {
       search_uuid = NULL;
+      os_passwd = NULL;
       check_boot = state[2].set;
       grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
       search_uuid = NULL;
       return GRUB_ERR_NONE;
     }
+  else if (state[3].set)
+    {
+      struct grub_secret_entry *se;
+      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], &os_passwd);
+      if (rc)
+	return rc;
+
+      search_uuid = NULL;
+      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+      rc = se->put (args[1], have_it, &os_passwd);
+      os_passwd = NULL;
+
+      return rc;
+    }
   else
     {
       grub_err_t err;
@@ -1150,6 +1218,7 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int argc, char **args)
       grub_size_t len;
 
       search_uuid = NULL;
+      os_passwd = NULL;
       check_boot = state[2].set;
       diskname = args[0];
       len = grub_strlen (diskname);
@@ -1317,7 +1386,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_("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 737487bb4..8ae5aed97 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -174,4 +174,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, char **secret);
+  grub_err_t (*put)(const char *arg, int have_it, char **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.26.2

[RESEND v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk

[James Bottomley]

This module is designed to provide an efisecret command 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 | 125 +++++++++++++++++++++++++++++++++
 include/grub/efi/api.h         |  15 ++++
 3 files changed, 148 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..a8d1f39fc
--- /dev/null
+++ b/grub-core/disk/efi/efisecret.c
@@ -0,0 +1,125 @@
+#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;
+  char data[0];
+};
+
+static grub_err_t
+grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
+		     char **ptr)
+{
+  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data);
+
+  /* destroy the secret */
+  grub_memset (e, 0, e->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, char **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)), char **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.26.2

Re: [RESEND v3 3/3] efi: Add API for retrieving the EFI secret for cryptodisk

[Dov Murik]

On 09/11/2021 15:53, James Bottomley wrote:
> This module is designed to provide an efisecret command 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 | 125 +++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h         |  15 ++++
>  3 files changed, 148 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..a8d1f39fc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,125 @@
> +#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;
> +  char data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> +		     char **ptr)
> +{
> +  struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct secret_entry *)0)->data);

Maybe clearer (not tested):

struct secret_entry *e = (struct secret_entry *)(*ptr - offsetof(struct secret_entry, data));



> +
> +  /* destroy the secret */
> +  grub_memset (e, 0, e->len);

This destroys the GUID field (good), the secret data (good), but
also the len field.  Without the correct length value it will be
impossible to scan the GUIDed secrets table after the erased entry.

If the luks passphrase is the only secret, then there's no
problem.  But if the OS wants to use other secrets from this
table, I suggest erasing only e->guid and e->data.


-Dov


> +  *ptr = NULL;
> +
> +  if (have_it)
> +    return GRUB_ERR_NONE;
> +
> +  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock any volumes");
> +}
> +

[snip]

Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption

[Daniel Kiper]

Hey,

Adding Denis, Patrick and Glenn...

James, please add them to the loop next time.

On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote:
> From: James Bottomley <James.Bottomley@HansenPartnership.com>
>
> 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
>
> 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
> source (crypto0)/boot/grub.cfg
>
> Assuming there's a standard Linux root partition.

Thank you for posting this patch series. Unfortunately it conflicts with
[1] patches. And I want to take [1] first because it is an important
improvement for GRUB's crypto infrastructure. Additionally, as Glenn
said in [1], this crypto infra change should simplify your code too.

I have just finished reviewing Glenn's patches and waiting for v4.
I hope we will be able to merge it soon. If you could take a look at
the [1] and check if it does not make any troubles for you it would
be perfect.

I will drop you a line when Glenn's patches are in the tree and you can
rebase your patch set on top of it.

Daniel

[1] https://lists.gnu.org/archive/html/grub-devel/2021-10/msg00107.html

Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption

[James Bottomley]

On Thu, 2021-11-18 at 15:49 +0100, Daniel Kiper wrote:
> Hey,
> 
> Adding Denis, Patrick and Glenn...
> 
> James, please add them to the loop next time.

Sure ... is there some way of telling who should be cc'd (I'm not a fan
of the kernel get_maintainer.pl but it gives you a list you can trim)?

> 
> On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote:
> > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > 
> > 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
> > 
> > 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
> > source (crypto0)/boot/grub.cfg
> > 
> > Assuming there's a standard Linux root partition.
> 
> Thank you for posting this patch series. Unfortunately it conflicts
> with [1] patches. And I want to take [1] first because it is an
> important improvement for GRUB's crypto infrastructure. Additionally,
> as Glenn said in [1], this crypto infra change should simplify your
> code too.
> 
> I have just finished reviewing Glenn's patches and waiting for v4.
> I hope we will be able to merge it soon. If you could take a look at
> the [1] and check if it does not make any troubles for you it would
> be perfect.
> 
> I will drop you a line when Glenn's patches are in the tree and you
> can rebase your patch set on top of it.

Yes, the rebase looks trivial.  I'll do it and repost as soon as the
patches are upstream.

Regards,

James

Re: [RESEND v3 0/3] use confidential computing provisioned secrets for disk decryption

[Daniel Kiper]

On Thu, Nov 18, 2021 at 12:15:55PM -0500, James Bottomley wrote:
> On Thu, 2021-11-18 at 15:49 +0100, Daniel Kiper wrote:
> > Hey,
> >
> > Adding Denis, Patrick and Glenn...
> >
> > James, please add them to the loop next time.
>
> Sure ... is there some way of telling who should be cc'd (I'm not a fan
> of the kernel get_maintainer.pl but it gives you a list you can trim)?

You can take a look at the MAINTAINERS files. Sadly it only lists core
maintainers who should be CC-ed. If you CC them they will tell you if you
should add anybody else. I know it is not perfect but... I hope we will
improve this at some point.

> > On Tue, Nov 09, 2021 at 08:53:53AM -0500, James Bottomley wrote:
> > > From: James Bottomley <James.Bottomley@HansenPartnership.com>
> > >
> > > 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
> > >
> > > 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
> > > source (crypto0)/boot/grub.cfg
> > >
> > > Assuming there's a standard Linux root partition.
> >
> > Thank you for posting this patch series. Unfortunately it conflicts
> > with [1] patches. And I want to take [1] first because it is an
> > important improvement for GRUB's crypto infrastructure. Additionally,
> > as Glenn said in [1], this crypto infra change should simplify your
> > code too.
> >
> > I have just finished reviewing Glenn's patches and waiting for v4.
> > I hope we will be able to merge it soon. If you could take a look at
> > the [1] and check if it does not make any troubles for you it would
> > be perfect.
> >
> > I will drop you a line when Glenn's patches are in the tree and you
> > can rebase your patch set on top of it.
>
> Yes, the rebase looks trivial.  I'll do it and repost as soon as the
> patches are upstream.

Cool! Thanks!

Daniel