[PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[James Bottomley]

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 encrypted virtual
machine, we need to add the ability for grub to retrieve the disk
passphrase from the SEV launch secret.  To do this, we've modified
OVMF to set aside an area for the injected secret and pass up a
configuration table for it:

https://edk2.groups.io/g/devel/topic/78198617#67339

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' option to cryptodisk to allow it to use a
saved password 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:

sevsecret
cryptomount -s
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 AMD SEV injected secret for cryptodisk

 grub-core/Makefile.core.def    |   8 ++
 grub-core/disk/cryptodisk.c    |  61 ++++++++++++++-
 grub-core/disk/efi/sevsecret.c | 132 +++++++++++++++++++++++++++++++++
 grub-core/disk/geli.c          |  12 +--
 grub-core/disk/luks.c          |  12 +--
 grub-core/disk/luks2.c         |  12 +--
 include/grub/cryptodisk.h      |  11 ++-
 include/grub/efi/api.h         |  15 ++++
 8 files changed, 243 insertions(+), 20 deletions(-)
 create mode 100644 grub-core/disk/efi/sevsecret.c

-- 
2.26.2

[PATCH v2 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
---
 grub-core/disk/cryptodisk.c |  2 +-
 grub-core/disk/geli.c       | 12 +++++++-----
 grub-core/disk/luks.c       | 12 +++++++-----
 grub-core/disk/luks2.c      | 12 +++++++-----
 include/grub/cryptodisk.h   |  6 +++++-
 5 files changed, 27 insertions(+), 17 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index a3d672f68..682f5a55d 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -997,7 +997,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 e9d23299a..3fece3f4a 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 == grub_password_get)
+    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 59702067a..165f4a6bd 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 == grub_password_get)
+	  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 31d7166fc..984182aa9 100644
--- a/grub-core/disk/luks2.c
+++ b/grub-core/disk/luks2.c
@@ -531,7 +531,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
 
 static grub_err_t
 luks2_recover_key (grub_disk_t disk,
-		   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];
@@ -573,10 +574,11 @@ luks2_recover_key (grub_disk_t disk,
   /* Get the passphrase from the user. */
   if (disk->partition)
     part = grub_partition_get_name (disk->partition);
-  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
-		disk->partition ? "," : "", part ? : "",
-		crypt->uuid);
-  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
+  if (password_get == grub_password_get)
+    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
+		  disk->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/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
index e1b21e785..45dae5483 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -101,6 +101,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;
@@ -108,7 +111,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

[PATCH v2 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
---
 grub-core/disk/cryptodisk.c | 61 ++++++++++++++++++++++++++++++++++---
 include/grub/cryptodisk.h   |  5 +++
 2 files changed, 62 insertions(+), 4 deletions(-)

diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
index 682f5a55d..508b25450 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 OS provisioned secret and mount all volumes encrypted with that secret"), 0, 0},
     {0, 0, 0, 0, 0, 0}
   };
 
@@ -967,6 +968,12 @@ 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 passed in secret area. */
+static char *os_secret_area;
+static grub_secret_cb *os_secret_cb;
+static void *os_secret_arg;
 
 static void
 cryptodisk_close (grub_cryptodisk_t dev)
@@ -977,6 +984,17 @@ cryptodisk_close (grub_cryptodisk_t dev)
   grub_free (dev);
 }
 
+static int
+os_password_get(char buf[], unsigned len)
+{
+  /* 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)
 {
@@ -996,8 +1014,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);
@@ -1013,6 +1040,16 @@ grub_cryptodisk_scan_device_real (const char *name, grub_disk_t source)
   return GRUB_ERR_NONE;
 }
 
+grub_err_t
+grub_cryptodisk_set_secret (char *secret, grub_secret_cb *cb, void *arg)
+{
+  os_secret_area = secret;
+  os_secret_cb = cb;
+  os_secret_arg = arg;
+
+  return GRUB_ERR_NONE;
+}
+
 #ifdef GRUB_UTIL
 #include <grub/util/misc.h>
 grub_err_t
@@ -1089,7 +1126,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;
@@ -1107,6 +1144,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;
 
@@ -1117,11 +1155,25 @@ 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)
+    {
+      /* do we have a secret? */
+      if (os_secret_area == NULL)
+	return grub_error (GRUB_ERR_INVALID_COMMAND, "No OS secret is provisioned");
+
+      os_passwd = os_secret_area;
+      search_uuid = NULL;
+      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
+      os_passwd = NULL;
+
+      return os_secret_cb (have_it, os_secret_arg);
+    }
   else
     {
       grub_err_t err;
@@ -1132,6 +1184,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);
@@ -1299,7 +1352,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"),
 			      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 45dae5483..8236b5933 100644
--- a/include/grub/cryptodisk.h
+++ b/include/grub/cryptodisk.h
@@ -163,4 +163,9 @@ 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);
 
+typedef grub_err_t (grub_secret_cb)(int have_it, void *arg);
+
+grub_err_t grub_cryptodisk_set_secret(char *secret, grub_secret_cb *cb,
+				      void *arg);
+
 #endif
-- 
2.26.2

[PATCH v2 3/3] efi: Add API for retrieving the AMD SEV injected secret for cryptodisk

[James Bottomley]

This module is designed to provide a sevsecret command which
interrogates the EFI configuration table to find the location of the
sev secret injection and tries to register the secret with the
cryptodisk.

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

|GRUB_EFI_SEVSECRET_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
---
 grub-core/Makefile.core.def    |   8 ++
 grub-core/disk/efi/sevsecret.c | 132 +++++++++++++++++++++++++++++++++
 include/grub/efi/api.h         |  15 ++++
 3 files changed, 155 insertions(+)
 create mode 100644 grub-core/disk/efi/sevsecret.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index b5f47fc41..76ae5d8fc 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -784,6 +784,14 @@ module = {
   enable = efi;
 };
 
+module = {
+  name = sevsecret;
+
+  common = disk/efi/sevsecret.c;
+
+  enable = efi;
+};
+
 module = {
   name = lsefimmap;
 
diff --git a/grub-core/disk/efi/sevsecret.c b/grub-core/disk/efi/sevsecret.c
new file mode 100644
index 000000000..da59dd1ad
--- /dev/null
+++ b/grub-core/disk/efi/sevsecret.c
@@ -0,0 +1,132 @@
+#include <grub/err.h>
+#include <grub/command.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 sevsecret_guid = GRUB_EFI_SEVSECRET_TABLE_GUID;
+static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SEVSECRET_TABLE_HEADER_GUID;
+static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
+
+/*
+ * EFI places the secret in the lower 4GB, so it uses a UINT32
+ * for the pointer which we have to transform to the correct type
+ */
+struct sev_secret {
+  grub_uint32_t base;
+  grub_uint32_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_sevsecret_cb (int have_it, void *arg)
+{
+  struct secret_entry *e = arg;
+
+  /* destroy the secret */
+  grub_memset (e, 0, e->len);
+
+  if (have_it)
+    return GRUB_ERR_NONE;
+
+  return  grub_error (GRUB_ERR_ACCESS_DENIED, "SEV password failed to unlock any volumes");
+}
+
+static grub_err_t
+grub_efi_sevsecret_find (struct sev_secret *s)
+{
+  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, "SEV 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, "SEV secret area does not start with correct guid\n");
+  if (h->len < sizeof (*h))
+    return grub_error (GRUB_ERR_BAD_ARGUMENT, "SEV 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, "SEV 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 (e->data[end - 1] != '\0')
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "SEV secret area disk encryption password is not zero terminated\n");
+
+      return grub_cryptodisk_set_secret (e->data, grub_efi_sevsecret_cb, e);
+    }
+    ptr += e->len;
+    len -= e->len;
+  }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "SEV secret aread does not contain disk decryption password\n");
+}
+
+static grub_err_t
+grub_efi_sevsecret_init (void)
+{
+  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, &sevsecret_guid, sizeof (grub_efi_packed_guid_t))) {
+	struct sev_secret *s =
+	  grub_efi_system_table->configuration_table[i].vendor_table;
+
+	return grub_efi_sevsecret_find(s);
+      }
+    }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "No SEV secret found in the EFI configuration table");
+}
+
+static grub_err_t
+grub_cmd_sevsecret (grub_command_t cmd __attribute__ ((unused)),
+		    int argc __attribute__ ((unused)),
+		    char **args __attribute__ ((unused)))
+{
+  return grub_efi_sevsecret_init();
+}
+
+static grub_command_t cmd;
+
+GRUB_MOD_INIT(sevsecret)
+{
+  cmd = grub_register_command ("sevsecret", grub_cmd_sevsecret, 0,
+			       N_("Register a SEV secret with cryptomount if one exists"));
+}
+
+GRUB_MOD_FINI(sevsecret)
+{
+  grub_unregister_command (cmd);
+}
diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
index 1dcaa12f5..80eb880f8 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_SEVSECRET_TABLE_GUID \
+  { 0xadf956ad, 0xe98c, 0x484c, \
+    { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
+  }
+
+#define GRUB_EFI_SEVSECRET_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: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[Glenn Washburn]

On Fri, 13 Nov 2020 14:25:07 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> 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 encrypted virtual
> machine, we need to add the ability for grub to retrieve the disk
> passphrase from the SEV launch secret.  To do this, we've modified
> OVMF to set aside an area for the injected secret and pass up a
> configuration table for it:
> 
> https://edk2.groups.io/g/devel/topic/78198617#67339
> 
> 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.

I like this idea in general.

> The next patch adds a '-s' option to cryptodisk to allow it to use a
> saved password 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.

I'm not in favor of this approach.  This feels like a special case of
providing a key file to cryptomount.  We have working (and I believe
merge worthy) patches for adding key file support. Unfortunately, due
to the current position in the grub development cycle, they have not
been merged.  As a side note, it might be interesting to re-work the
key file patch series to use the arbitrary password getter mechanism
you've created.

What I would prefer, because it feels more generic, is to have the
sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
which outputs the secret data when read (or NULL string if some error
in finding the secret).  Then to cryptomount all devices that accept
the sev secret do:

  cryptomount -a -k (proc)/sevsecret

In this case you could re-use most of the code in
grub_efi_sevsecret_find and creating the procfs entry would be trivial
(see bottom of cryptodisk.c for an example on how to do this).

One potential issue could be getting error messages from
grub_efi_sevsecret_find back to the user and a solution could be to
replace the grub_error with grub_dprintf("sev", ...) statements and set
debug=sev unconditionally.  In most cases no output would be
generated, but some debug log messages should be generated on error.

Also, if this series does end up adding an option to cryptomount, a
documentation patch should be added.  I think we should start
documenting procfs paths as well.

Also, out of curiosity, is it possible that there are multiple
GRUB_EFI_DISKPASSWD_GUID entries defined?  You only get the first one,
but I'm wondering if the spec allows for more.

> With all this in place, the sequence to boot
> an encrypted volume without user intervention is:
> 
> sevsecret
> cryptomount -s
> source (crypto0)/boot/grub.cfg
> 
> Assuming there's a standard Linux root partition.
> 
> James

Glenn

Re: [PATCH v2 1/3] cryptodisk: make the password getter and additional argument to recover_key

[Glenn Washburn]

On Fri, 13 Nov 2020 14:25:08 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> 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
> ---
>  grub-core/disk/cryptodisk.c |  2 +-
>  grub-core/disk/geli.c       | 12 +++++++-----
>  grub-core/disk/luks.c       | 12 +++++++-----
>  grub-core/disk/luks2.c      | 12 +++++++-----
>  include/grub/cryptodisk.h   |  6 +++++-
>  5 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index a3d672f68..682f5a55d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -997,7 +997,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 e9d23299a..3fece3f4a 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 == grub_password_get)
> +    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 59702067a..165f4a6bd 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 == grub_password_get)
> +	  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "),
> source->name,
> +			source->partition ? "," : "", tmp ? : "",
> +			dev->uuid);

I don't really like this added if statement.  It feels hackish.  I also
don't have an unambiguously better alternative.

What feels like an imperfect alternative is to define a function
grub_password_prompt(char buf[], unsigned buf_size, const char
*prompt), which calls grub_password_get after displaying the string
prompt.  Typedef grub_passwd_cb to grub_password_prompt's signature.
Then some grub_passwd_cb implementations can decide when not to use the
given prompt.  Perhaps grub_password_prompt should have snprintf's
signature to be more complete and combine the grub_printf_ and
grub_password_get.

Any other ideas?

Glenn

Re: [PATCH v2 2/3] cryptodisk: add OS provided secret support

[Glenn Washburn]

On Fri, 13 Nov 2020 14:25:09 -0800
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.
> 
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
> 
> ---
> 
> v2: add callback for after secret use
> ---
>  grub-core/disk/cryptodisk.c | 61
> ++++++++++++++++++++++++++++++++++--- include/grub/cryptodisk.h   |
> 5 +++ 2 files changed, 62 insertions(+), 4 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 682f5a55d..508b25450 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 OS provisioned secret and mount all
> volumes encrypted with that secret"), 0, 0}, {0, 0, 0, 0, 0, 0}
>    };
>  
> @@ -967,6 +968,12 @@ 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 passed in secret area. */
> +static char *os_secret_area;
> +static grub_secret_cb *os_secret_cb;
> +static void *os_secret_arg;
>  
>  static void
>  cryptodisk_close (grub_cryptodisk_t dev)
> @@ -977,6 +984,17 @@ cryptodisk_close (grub_cryptodisk_t dev)
>    grub_free (dev);
>  }
>  
> +static int
> +os_password_get(char buf[], unsigned len)
> +{
> +  /* 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;

Shouldn't this return 0 for success?

> +}
> +
>  static grub_err_t
>  grub_cryptodisk_scan_device_real (const char *name, grub_disk_t
> source) {
> @@ -996,8 +1014,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);
> +

I think a cleaner approach might be to leave this essentially as it was,
just change grub_password_get to a global variable (maybe
grub_password_get_cb).  Then in grub_cmd_cryptomount set
grub_password_get_cb to grub_password_get by default, and to
os_password_get in the -s option handling.

>      if (err)
>      {
>        cryptodisk_close (dev);
> @@ -1013,6 +1040,16 @@ grub_cryptodisk_scan_device_real (const char
> *name, grub_disk_t source) return GRUB_ERR_NONE;
>  }
>  
> +grub_err_t
> +grub_cryptodisk_set_secret (char *secret, grub_secret_cb *cb, void
> *arg) +{
> +  os_secret_area = secret;

This looks like its asking to introduce memory leak bugs if something
else decides to use this. Its not in this patch series because secret
points to global memory, but for anyone else using this generic API it
might not be. Perhaps grub_cryptodisk_set_secret should take ownership
of the memory, and thus free os_secret_area if its non-NULL here.

> +  os_secret_cb = cb;
> +  os_secret_arg = arg;
> +
> +  return GRUB_ERR_NONE;
> +}
> +
>  #ifdef GRUB_UTIL
>  #include <grub/util/misc.h>
>  grub_err_t
> @@ -1089,7 +1126,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;
> @@ -1107,6 +1144,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;
>  
> @@ -1117,11 +1155,25 @@ 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)
> +    {
> +      /* do we have a secret? */
> +      if (os_secret_area == NULL)
> +	return grub_error (GRUB_ERR_INVALID_COMMAND, "No OS secret
> is provisioned"); +
> +      os_passwd = os_secret_area;
> +      search_uuid = NULL;
> +      grub_device_iterate (&grub_cryptodisk_scan_device, NULL);
> +      os_passwd = NULL;
> +
> +      return os_secret_cb (have_it, os_secret_arg);
> +    }
>    else
>      {
>        grub_err_t err;
> @@ -1132,6 +1184,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);
> @@ -1299,7 +1352,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"),
>  			      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 45dae5483..8236b5933 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -163,4 +163,9 @@ 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); 
> +typedef grub_err_t (grub_secret_cb)(int have_it, void *arg);
> +
> +grub_err_t grub_cryptodisk_set_secret(char *secret, grub_secret_cb
> *cb,
> +				      void *arg);
> +
>  #endif

Re: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[James Bottomley]

On Fri, 2020-11-13 at 19:50 -0600, Glenn Washburn wrote:
> On Fri, 13 Nov 2020 14:25:07 -0800
> James Bottomley <jejb@linux.ibm.com> wrote:
> 
> > 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 encrypted virtual
> > machine, we need to add the ability for grub to retrieve the disk
> > passphrase from the SEV launch secret.  To do this, we've modified
> > OVMF to set aside an area for the injected secret and pass up a
> > configuration table for it:
> > 
> > https://edk2.groups.io/g/devel/topic/78198617#67339
> > 
> > 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.
> 
> I like this idea in general.
> 
> > The next patch adds a '-s' option to cryptodisk to allow it to use
> > a
> > saved password 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.
> 
> I'm not in favor of this approach.  This feels like a special case of
> providing a key file to cryptomount.  We have working (and I believe
> merge worthy) patches for adding key file support. Unfortunately, due
> to the current position in the grub development cycle, they have not
> been merged.  As a side note, it might be interesting to re-work the
> key file patch series to use the arbitrary password getter mechanism
> you've created.
> 
> What I would prefer, because it feels more generic, is to have the
> sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
> which outputs the secret data when read (or NULL string if some error
> in finding the secret).  Then to cryptomount all devices that accept
> the sev secret do:
> 
>   cryptomount -a -k (proc)/sevsecret
> 
> In this case you could re-use most of the code in
> grub_efi_sevsecret_find and creating the procfs entry would be
> trivial (see bottom of cryptodisk.c for an example on how to do
> this).

A file interface feels slightly wrong for this.  What we need is a
use/release envelope ... effectively a way of enforcing the lifetime on
the secret use.  This allows a wider variety of threat models than the
simply file model because the latter assumes an infinite lifetime (even
though that can be hacked around using temporary filesystems).  From a
generality point of view it feels better to implement a file interface
as a special case of a use/release model because it simply has an empty
release function.

If you follow this model, a file use case would have a special
filesecret command and the use would go like:

filesecret <file>
cryptomount -s

> One potential issue could be getting error messages from
> grub_efi_sevsecret_find back to the user and a solution could be to
> replace the grub_error with grub_dprintf("sev", ...) statements and
> set debug=sev unconditionally.  In most cases no output would be
> generated, but some debug log messages should be generated on error.

Right, all this currently goes in the release function.

> Also, if this series does end up adding an option to cryptomount, a
> documentation patch should be added.  I think we should start
> documenting procfs paths as well.
> 
> Also, out of curiosity, is it possible that there are multiple
> GRUB_EFI_DISKPASSWD_GUID entries defined?  You only get the first
> one, but I'm wondering if the spec allows for more.

Well, I'm currently defining the spec by proposing patches.  I envisage
the secret area would contain multiple secrets, some of which may be
consumed before grub but a few may be consumed after grub by the OS.  I
also suspect if the cryptomount fails, it might be advisable to destroy
the entire secrets area rather than just the grub passphrase, because
failure to decrypt the /boot partition would indicate potential tamper
attempts.

Given the use case is an encrypted boot system on an untrusted cloud, I
can't see any reason for having multiple grub passwords ... the image
should only have a single /boot filesystem. 

James

Re: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[Glenn Washburn]

On Fri, 13 Nov 2020 18:48:30 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> On Fri, 2020-11-13 at 19:50 -0600, Glenn Washburn wrote:
> > On Fri, 13 Nov 2020 14:25:07 -0800
> > James Bottomley <jejb@linux.ibm.com> wrote:
> > 
> > > 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 encrypted virtual
> > > machine, we need to add the ability for grub to retrieve the disk
> > > passphrase from the SEV launch secret.  To do this, we've modified
> > > OVMF to set aside an area for the injected secret and pass up a
> > > configuration table for it:
> > > 
> > > https://edk2.groups.io/g/devel/topic/78198617#67339
> > > 
> > > 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.
> > 
> > I like this idea in general.
> > 
> > > The next patch adds a '-s' option to cryptodisk to allow it to use
> > > a
> > > saved password 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.
> > 
> > I'm not in favor of this approach.  This feels like a special case
> > of providing a key file to cryptomount.  We have working (and I
> > believe merge worthy) patches for adding key file support.
> > Unfortunately, due to the current position in the grub development
> > cycle, they have not been merged.  As a side note, it might be
> > interesting to re-work the key file patch series to use the
> > arbitrary password getter mechanism you've created.
> > 
> > What I would prefer, because it feels more generic, is to have the
> > sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
> > which outputs the secret data when read (or NULL string if some
> > error in finding the secret).  Then to cryptomount all devices that
> > accept the sev secret do:
> > 
> >   cryptomount -a -k (proc)/sevsecret
> > 
> > In this case you could re-use most of the code in
> > grub_efi_sevsecret_find and creating the procfs entry would be
> > trivial (see bottom of cryptodisk.c for an example on how to do
> > this).
> 
> A file interface feels slightly wrong for this.  What we need is a
> use/release envelope ... effectively a way of enforcing the lifetime
> on the secret use.  This allows a wider variety of threat models than
> the simply file model because the latter assumes an infinite lifetime
> (even though that can be hacked around using temporary filesystems).

Yep, that's exactly what I'm proposing. I don't think the file model as
it exists as a concept in *NIX has any such assumption about data
lifetimes. Sockets, pipes, and character devices are accessed as files
in *NIX, and none of those have that assumption.  I think it makes a
lot of sense to expose the data through a file-like interface that is
read once.  After a read its wiped, which feels like a socket or pipe.

> From a generality point of view it feels better to implement a file
> interface as a special case of a use/release model because it simply
> has an empty release function.
> 
> If you follow this model, a file use case would have a special
> filesecret command and the use would go like:
> 
> filesecret <file>
> cryptomount -s

This feels like a clumsy way of passing data to the cryptodisk mounting
system.  Your approach might be akin to the kernel keyring in linux,
where you have a program to manage keys and other programs that can
access the kernel key ring.  This is done so that user space doesn't
have direct access to keys because its untrusted.  I don't think we
have that concern here.

If we were to go your route, I think there should be one secret
management command that manages a database of secrets.  Perhaps
something like:

# Actually don't need this, just have the module init for
# sevsecret do this.
# keys --load sev --name sevkey
cryptomount -a -k key:sevkey

Here the secrets command could take a list of arguments to send to a
registered loader (in this case named sev) and associate the loaded key
with a name (in this case sevkey).  The -k parameter to cryptomount
would take an argument that can be "key:" followed by the name of the
associated key and use that as key material.  For the file case,
something like:

cryptomount -a -k file:(dev1)/path/to/key

or

keys --load file --name mykey1 (dev1)/path/to/key
cryptomount -a -k key:mykey1

I know next to nothing about TPM, but my understanding is that it can
have key material.  Perhaps there could there be something like:

cryptomount -a -k tpm:key1

Glenn

Re: [PATCH v2 1/3] cryptodisk: make the password getter and additional argument to recover_key

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 8246 bytes --]

On Fri, Nov 13, 2020 at 02:25:08PM -0800, James Bottomley wrote:
> 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>

I actually like this approach of having a callback to retrieve
passwords. Note though that this conflicts with the in-flight support
for detached headers and key files [1]. It's been a while since the last
revision has been posted, but this can mostly be attributed to the fact
that GRUB has been in a code freeze for some months in order to get
v2.06 out of the door.

In hindsight I'd have preferred your approach to the one in the
mentioned patch series. I'm not quite sure about a proper way forward
though -- depending on which series lands first, the other one'll have
to adjust code to make both play nicely with each other. Anyway, I'm
adding Denis to Cc.

Patrick

[1]: https://lists.gnu.org/archive/html/grub-devel/2020-08/msg00028.html

> ---
> 
> v2: add conditional prompting to geli.c
> ---
>  grub-core/disk/cryptodisk.c |  2 +-
>  grub-core/disk/geli.c       | 12 +++++++-----
>  grub-core/disk/luks.c       | 12 +++++++-----
>  grub-core/disk/luks2.c      | 12 +++++++-----
>  include/grub/cryptodisk.h   |  6 +++++-
>  5 files changed, 27 insertions(+), 17 deletions(-)
> 
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index a3d672f68..682f5a55d 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -997,7 +997,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 e9d23299a..3fece3f4a 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 == grub_password_get)
> +    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), source->name,
> +		  source->partition ? "," : "", tmp ? : "",
> +		  dev->uuid);

Ideally we'd be moving this print into `grub_password_get`, but it's not
that easy. First, it gets called by others, so an unconditional print
would change output for those. Second, the interface would get a bit
weird if we just pass the message to any `grub_passwd_cb`.

One thing that does sound generic enough though would be to pass
something like a "subject" to the callback, where subject is the thing
which we want to retrieve the secret for. If it's set, the callback will
print above message, otherwise it'll stay silent. This'd also allow
other callbacks to print nice messages if they require user input.
Furthermore, it would nicely deduplicate those messages across all
cryptodisk backends.

>    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 59702067a..165f4a6bd 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 == grub_password_get)
> +	  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 31d7166fc..984182aa9 100644
> --- a/grub-core/disk/luks2.c
> +++ b/grub-core/disk/luks2.c
> @@ -531,7 +531,8 @@ luks2_decrypt_key (grub_uint8_t *out_key,
>  
>  static grub_err_t
>  luks2_recover_key (grub_disk_t disk,
> -		   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];
> @@ -573,10 +574,11 @@ luks2_recover_key (grub_disk_t disk,
>    /* Get the passphrase from the user. */
>    if (disk->partition)
>      part = grub_partition_get_name (disk->partition);
> -  grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
> -		disk->partition ? "," : "", part ? : "",
> -		crypt->uuid);
> -  if (!grub_password_get (passphrase, MAX_PASSPHRASE))
> +  if (password_get == grub_password_get)
> +    grub_printf_ (N_("Enter passphrase for %s%s%s (%s): "), disk->name,
> +		  disk->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/include/grub/cryptodisk.h b/include/grub/cryptodisk.h
> index e1b21e785..45dae5483 100644
> --- a/include/grub/cryptodisk.h
> +++ b/include/grub/cryptodisk.h
> @@ -101,6 +101,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);
> +

Considering we have the keyfiles patch series in flight, we might want
to try make this function work for both. While `grub_password_get`
wouldn't require any additional input, the keyfile callback would need
to know where the file is located. So we might want to add an optional
payload here which is specific to the function.

Also, I'd maybe rename this to something like `grub_credentials_cb` or
`grub_secret_cb` to make it generic.

Patrick

>  struct grub_cryptodisk_dev
>  {
>    struct grub_cryptodisk_dev *next;
> @@ -108,7 +111,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
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 0/3] Add ability to use SEV provisioned secrets for disk decryption

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3997 bytes --]

On Fri, Nov 13, 2020 at 07:50:38PM -0600, Glenn Washburn wrote:
> On Fri, 13 Nov 2020 14:25:07 -0800
> James Bottomley <jejb@linux.ibm.com> wrote:
> 
> > 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 encrypted virtual
> > machine, we need to add the ability for grub to retrieve the disk
> > passphrase from the SEV launch secret.  To do this, we've modified
> > OVMF to set aside an area for the injected secret and pass up a
> > configuration table for it:
> > 
> > https://edk2.groups.io/g/devel/topic/78198617#67339
> > 
> > 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.
> 
> I like this idea in general.
> 
> > The next patch adds a '-s' option to cryptodisk to allow it to use a
> > saved password 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.
> 
> I'm not in favor of this approach.  This feels like a special case of
> providing a key file to cryptomount.  We have working (and I believe
> merge worthy) patches for adding key file support. Unfortunately, due
> to the current position in the grub development cycle, they have not
> been merged.  As a side note, it might be interesting to re-work the
> key file patch series to use the arbitrary password getter mechanism
> you've created.

Ah, should've read all messages first. I'm now repeating kind of the
same thing as a reply to patch 1/3.

> What I would prefer, because it feels more generic, is to have the
> sevsecret module create a procfs entry (perhaps (proc)/sevsecret),
> which outputs the secret data when read (or NULL string if some error
> in finding the secret).  Then to cryptomount all devices that accept
> the sev secret do:
> 
>   cryptomount -a -k (proc)/sevsecret

Interesting approach. I think I tend to agree, but I'm not too sure
about potential security implications. Anyway, I still like the password
callback function per-se as it nicely deduplicates existing code
already. So regardless of where we end up here, I think that would be
nice to have anyway.

Patrick

> In this case you could re-use most of the code in
> grub_efi_sevsecret_find and creating the procfs entry would be trivial
> (see bottom of cryptodisk.c for an example on how to do this).
> 
> One potential issue could be getting error messages from
> grub_efi_sevsecret_find back to the user and a solution could be to
> replace the grub_error with grub_dprintf("sev", ...) statements and set
> debug=sev unconditionally.  In most cases no output would be
> generated, but some debug log messages should be generated on error.
> 
> Also, if this series does end up adding an option to cryptomount, a
> documentation patch should be added.  I think we should start
> documenting procfs paths as well.
> 
> Also, out of curiosity, is it possible that there are multiple
> GRUB_EFI_DISKPASSWD_GUID entries defined?  You only get the first one,
> but I'm wondering if the spec allows for more.
> 
> > With all this in place, the sequence to boot
> > an encrypted volume without user intervention is:
> > 
> > sevsecret
> > cryptomount -s
> > source (crypto0)/boot/grub.cfg
> > 
> > Assuming there's a standard Linux root partition.
> > 
> > James
> 
> Glenn
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]