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

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

[James Bottomley]

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     | 129 +++++++++++++++++++++++++++++
 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, 277 insertions(+), 20 deletions(-)
 create mode 100644 grub-core/disk/efi/efisecret.c

-- 
2.26.2

[PATCH 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 b62835acc..c51c2edb8 100644
--- a/grub-core/disk/cryptodisk.c
+++ b/grub-core/disk/cryptodisk.c
@@ -1011,7 +1011,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 7460d7b58..7597d8576 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

[PATCH 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 c51c2edb8..c5eb45de4 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}
   };
 
@@ -981,6 +982,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)
@@ -991,6 +996,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)
 {
@@ -1010,8 +1030,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);
@@ -1027,6 +1056,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
@@ -1103,7 +1144,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;
@@ -1121,6 +1162,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;
 
@@ -1131,11 +1173,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;
@@ -1146,6 +1214,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);
@@ -1313,7 +1382,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

[PATCH 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
---
 grub-core/Makefile.core.def    |   8 ++
 grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
 include/grub/efi/api.h         |  15 ++++
 3 files changed, 152 insertions(+)
 create mode 100644 grub-core/disk/efi/efisecret.c

diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
index 68b9e9f68..0b7e365cd 100644
--- a/grub-core/Makefile.core.def
+++ b/grub-core/Makefile.core.def
@@ -785,6 +785,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..318af0784
--- /dev/null
+++ b/grub-core/disk/efi/efisecret.c
@@ -0,0 +1,129 @@
+#include <grub/err.h>
+#include <grub/misc.h>
+#include <grub/cryptodisk.h>
+#include <grub/efi/efi.h>
+#include <grub/efi/api.h>
+#include <grub/dl.h>
+
+GRUB_MOD_LICENSE ("GPLv3+");
+
+static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
+static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID;
+static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
+
+/*
+ * 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 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 (e->data[end - 1] != '\0')
+	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is not zero terminated\n");
+
+      *secret_ptr = e->data;
+      return GRUB_ERR_NONE;
+    }
+    ptr += e->len;
+    len -= e->len;
+  }
+  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret aread 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 39733585b..53a47f658 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: [PATCH v3 1/3] cryptodisk: make the password getter and additional argument to recover_key

[Dmitry]

Hi,

Please see inline

чт, 31 дек. 2020 г. в 20:39, James Bottomley <jejb@linux.ibm.com>:
>
> 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 b62835acc..c51c2edb8 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1011,7 +1011,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 7460d7b58..7597d8576 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)

Do you have any thoughts for the future if we want to add luks header and
master key passing to this function?

I'm using my own branch where I added this in a trivial way:
static grub_err_t
luks2_recover_key (grub_disk_t source,
           grub_cryptodisk_t crypt,
           grub_file_t hdr_file, grub_file_t key_file, grub_file_t mkey_file)

https://gitlab.com/reagentoo/grub/-/blob/cryptopatch_tiny_v2/grub-core/disk/luks2.c#L571-573

But I'm at a loss to think of how this can be done in combination with
a 'grub_passwd_cb*'.

Best regards,
Dmitry

>  {
>    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);

password_get?

>  };
>  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

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

[Glenn Washburn]

James,

I like the improvements here. However, I've been thinking more about
this and other improvements that deal with passing parameters to
modules used by cryptomount. I have some ideas that I've not had the
time to fully investigate or code up proof of concepts. One idea is
that we shouldn't be changing the function declaration of recover key,
that is to say adding new parameters. Instead we should be adding the
parameters to grub_cryptodisk_t and setting them in
grub_cryptodisk_scan_device_real. This would satisfy needs of this
patch series as well as the key file, detached header, sending password
as cryptomount arg, and master key features without cluttering the
function signature.

So, I don't think this is the right approach.

Glenn


On Thu, 31 Dec 2020 09:36:16 -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
> 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 b62835acc..c51c2edb8 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -1011,7 +1011,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 7460d7b58..7597d8576 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;
>  

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

[Glenn Washburn]

On Thu, 31 Dec 2020 21:42:55 +0300
Dmitry <reagentoo@gmail.com> wrote:

> Hi,
> 
> Please see inline
> 
> чт, 31 дек. 2020 г. в 20:39, James Bottomley <jejb@linux.ibm.com>:
> >
> > 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 b62835acc..c51c2edb8 100644
> > --- a/grub-core/disk/cryptodisk.c
> > +++ b/grub-core/disk/cryptodisk.c
> > @@ -1011,7 +1011,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 7460d7b58..7597d8576 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)
> 
> Do you have any thoughts for the future if we want to add luks header
> and master key passing to this function?
> 
> I'm using my own branch where I added this in a trivial way:
> static grub_err_t
> luks2_recover_key (grub_disk_t source,
>            grub_cryptodisk_t crypt,
>            grub_file_t hdr_file, grub_file_t key_file, grub_file_t
> mkey_file)
> 
> https://gitlab.com/reagentoo/grub/-/blob/cryptopatch_tiny_v2/grub-core/disk/luks2.c#L571-573
> 
> But I'm at a loss to think of how this can be done in combination with
> a 'grub_passwd_cb*'.

I don't think that this adding parameters to luks2_recovery_key is the
best/right approach (yet). We should/need not be modifying the function
signature. See my other response for more details that cover this
question. 

Glenn

> 
> >  {
> >    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);
> 
> password_get?
> 
> >  };
> >  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
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

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

[Dov Murik]

Hello James,

On 31/12/2020 19:36, 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
> ---
>   grub-core/Makefile.core.def    |   8 ++
>   grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
>   include/grub/efi/api.h         |  15 ++++
>   3 files changed, 152 insertions(+)
>   create mode 100644 grub-core/disk/efi/efisecret.c
> 
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 68b9e9f68..0b7e365cd 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -785,6 +785,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..318af0784
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include <grub/err.h>
> +#include <grub/misc.h>
> +#include <grub/cryptodisk.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid = GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +/*
> + * 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
> + */

This comment is no longer accurate now that efi_secret has 64-bit 
address and length fields. Just remove it?

> +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);

In grub-core/lib/libgcrypt/src/g10lib.h there's a wipememory macro which 
should circumvent optimizers which might remove the grub_memset call:


/* To avoid that a compiler optimizes certain memset calls away, these
    macros may be used instead. */
#define wipememory2(_ptr,_set,_len) do { \
               volatile char *_vptr=(volatile char *)(_ptr); \
               size_t _vlen=(_len); \
               while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
                   } while(0)
#define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)



Maybe the same thing should be used here or copied here? This is an 
attempt to wipe the disk decryption password (= part the EFI secret 
area) against future leakage.

(Note that I have no evidence that any optimizer currently removes the 
grub_memset call.)


> +  *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 cast to unsigned long can be removed (s->base is a grub_uint64_t).


> +
> +  /* 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 (e->data[end - 1] != '\0')
> +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk encryption password is not zero terminated\n");

I think there's a pathological edge case if e->len exactly equals 
sizeof(struct secret_entry) -- that is, it indicates a GUID struct with 
a zero-length data field.  In such a case, 'end' will be 0, the 
zero-termination check below might succeed because e->data[-1] == '\0' 
(last byte of length field in little-endian machine will probably be 
zero).  As a result secret_ptr will point to garbage.

A possible solution is to modify the above condition to:

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

I assume that a 0-length data field might be OK for other GUIDs but it 
is not valid for the disk password GUID.


> +
> +      *secret_ptr = e->data;
> +      return GRUB_ERR_NONE;
> +    }
> +    ptr += e->len;
> +    len -= e->len;
> +  }
> +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret aread does not contain disk decryption password\n");

typo: s/aread/area/


> +}
> +
> +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 39733585b..53a47f658 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 } \
> 


-Dov

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

[James Bottomley]

On Thu, 2020-12-31 at 21:42 +0300, Dmitry wrote:
[...]
> > --- 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)
> 
> Do you have any thoughts for the future if we want to add luks header
> and master key passing to this function?

I really don't think you want to add luks header, because that takes
what is a generic interface and makes it luks specific.  You could add
some sort of opaque context instead, which the caller doesn't
understand, but the callee does, but I don't currently know how you
plan to use the header, so I have no idea if this would work or not.

> I'm using my own branch where I added this in a trivial way:
> static grub_err_t
> luks2_recover_key (grub_disk_t source,
>            grub_cryptodisk_t crypt,
>            grub_file_t hdr_file, grub_file_t key_file, grub_file_t
> mkey_file)
> 
> https://gitlab.com/reagentoo/grub/-/blob/cryptopatch_tiny_v2/grub-core/disk/luks2.c#L571-573
> 
> But I'm at a loss to think of how this can be done in combination
> with a 'grub_passwd_cb*'.

Well, we're both adding arguments to the function, so you just would
combine the additions, I think.

James

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

[James Bottomley]

On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> James,
> 
> I like the improvements here. However, I've been thinking more about
> this and other improvements that deal with passing parameters to
> modules used by cryptomount. I have some ideas that I've not had the
> time to fully investigate or code up proof of concepts. One idea is
> that we shouldn't be changing the function declaration of recover
> key, that is to say adding new parameters. Instead we should be
> adding the parameters to grub_cryptodisk_t and setting them in
> grub_cryptodisk_scan_device_real. This would satisfy needs of this
> patch series as well as the key file, detached header, sending
> password as cryptomount arg, and master key features without
> cluttering the function signature.

Keeping large amounts of shared state between caller and callee can be
a debugger's nightmare.  In this case the only consumer of the password
callback is the recover function, so it seems appropriate it should be
an argument to that function.

> So, I don't think this is the right approach.

The thing this patch demonstrates is that altering the function
signatures is fairly easy, so it would be a simple patch to alter them
again if the password callback were decided to be an essential
component of the cryptodisk device ... but it should really driven the
need which isn't apparent yet.

James

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

[Glenn Washburn]

On Mon, 04 Jan 2021 22:48:49 -0800
James Bottomley <jejb@linux.ibm.com> wrote:

> On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> > James,
> > 
> > I like the improvements here. However, I've been thinking more about
> > this and other improvements that deal with passing parameters to
> > modules used by cryptomount. I have some ideas that I've not had the
> > time to fully investigate or code up proof of concepts. One idea is
> > that we shouldn't be changing the function declaration of recover
> > key, that is to say adding new parameters. Instead we should be
> > adding the parameters to grub_cryptodisk_t and setting them in
> > grub_cryptodisk_scan_device_real. This would satisfy needs of this
> > patch series as well as the key file, detached header, sending
> > password as cryptomount arg, and master key features without
> > cluttering the function signature.
> 
> Keeping large amounts of shared state between caller and callee can be
> a debugger's nightmare.  In this case the only consumer of the
> password callback is the recover function, so it seems appropriate it
> should be an argument to that function.

Please see my recently submitted RFC patch for a more concrete idea of
exactly what I'm suggesting. I don't see the concern about shared state
as being applicable in this case, even though your point is valid in
other situations.

> > So, I don't think this is the right approach.
> 
> The thing this patch demonstrates is that altering the function
> signatures is fairly easy, so it would be a simple patch to alter them
> again if the password callback were decided to be an essential
> component of the cryptodisk device ... but it should really driven the
> need which isn't apparent yet.

By easy, I take you to mean there aren't a lot of places needing to be
modified because of the change in signature, and in that sense its
easy, which is good. But its also unnecessary. I'm not sure if you've
looked at the struct grub_cryptodisk yet, but its already pretty
cluttered. So I can see why you might not want to add more. However, I
think my solution (again see RFC patch) is the cleaner, more scalable
solution.

Glenn

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

[James Bottomley]

On Tue, 2021-01-05 at 14:03 -0600, Glenn Washburn wrote:
> On Mon, 04 Jan 2021 22:48:49 -0800
> James Bottomley <jejb@linux.ibm.com> wrote:
> 
> > On Sat, 2021-01-02 at 19:45 -0600, Glenn Washburn wrote:
> > > James,
> > > 
> > > I like the improvements here. However, I've been thinking more
> > > about this and other improvements that deal with passing
> > > parameters to modules used by cryptomount. I have some ideas that
> > > I've not had the time to fully investigate or code up proof of
> > > concepts. One idea is that we shouldn't be changing the function
> > > declaration of recover key, that is to say adding new parameters.
> > > Instead we should be adding the parameters to grub_cryptodisk_t
> > > and setting them in grub_cryptodisk_scan_device_real. This would
> > > satisfy needs of this patch series as well as the key file,
> > > detached header, sending password as cryptomount arg, and master
> > > key features without cluttering the function signature.
> > 
> > Keeping large amounts of shared state between caller and callee can
> > be a debugger's nightmare.  In this case the only consumer of the
> > password callback is the recover function, so it seems appropriate
> > it should be an argument to that function.
> 
> Please see my recently submitted RFC patch for a more concrete idea
> of exactly what I'm suggesting. I don't see the concern about shared
> state as being applicable in this case, even though your point is
> valid in other situations.

Well, I've seen the patch, but it doesn't seem to address the root of
the problem of where the password data might be used: if the password
data is used by more than one function then it should likely be part of
the shared data; if it's only used by a single function it makes more
sense as an argument.  I think you need to flesh your RFC out further
to make that determination.

On what is passed, we do have a question about whether it's data or a
functional callback.  I do tend to prefer callbacks in situations like
this because they solve the lifetime issues (secrets should have well
defined lifetimes to make sure there's a limited window for leaking
them).  A simple data pointer doesn't necessarily do this.

So I think the important question is functional callback or data and
where it's passed is simply a more minor detail the solution to which
will become apparent in the use cases and which it's not hugely
important to get right in the first instance.

James

> > > So, I don't think this is the right approach.
> > 
> > The thing this patch demonstrates is that altering the function
> > signatures is fairly easy, so it would be a simple patch to alter
> > them again if the password callback were decided to be an essential
> > component of the cryptodisk device ... but it should really driven
> > the need which isn't apparent yet.
> 
> By easy, I take you to mean there aren't a lot of places needing to
> be modified because of the change in signature, and in that sense its
> easy, which is good. But its also unnecessary. I'm not sure if you've
> looked at the struct grub_cryptodisk yet, but its already pretty
> cluttered. So I can see why you might not want to add more. However,
> I think my solution (again see RFC patch) is the cleaner, more
> scalable solution.

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

[James Bottomley]

On Sun, 2021-01-03 at 12:46 +0200, Dov Murik wrote:
> Hello James,
> 
> On 31/12/2020 19:36, James Bottomley wrote:
[...]
> > diff --git a/grub-core/disk/efi/efisecret.c b/grub-
> > core/disk/efi/efisecret.c
> > new file mode 100644
> > index 000000000..318af0784
> > --- /dev/null
> > +++ b/grub-core/disk/efi/efisecret.c
> > @@ -0,0 +1,129 @@
> > +#include <grub/err.h>
> > +#include <grub/misc.h>
> > +#include <grub/cryptodisk.h>
> > +#include <grub/efi/efi.h>
> > +#include <grub/efi/api.h>
> > +#include <grub/dl.h>
> > +
> > +GRUB_MOD_LICENSE ("GPLv3+");
> > +
> > +static grub_efi_packed_guid_t secret_guid =
> > GRUB_EFI_SECRET_TABLE_GUID;
> > +static grub_efi_packed_guid_t tableheader_guid =
> > GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> > +static grub_efi_packed_guid_t diskpasswd_guid =
> > GRUB_EFI_DISKPASSWD_GUID;
> > +
> > +/*
> > + * 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
> > + */
> 
> This comment is no longer accurate now that efi_secret has 64-bit 
> address and length fields. Just remove it?

Yes, an oversight, I'll update the comment.

> > +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);
> 
> In grub-core/lib/libgcrypt/src/g10lib.h there's a wipememory macro
> which should circumvent optimizers which might remove the grub_memset
> call:
> 
> 
> /* To avoid that a compiler optimizes certain memset calls away,
> these
>     macros may be used instead. */
> #define wipememory2(_ptr,_set,_len) do { \
>                volatile char *_vptr=(volatile char *)(_ptr); \
>                size_t _vlen=(_len); \
>                while(_vlen) { *_vptr=(_set); _vptr++; _vlen--; } \
>                    } while(0)
> #define wipememory(_ptr,_len) wipememory2(_ptr,0,_len)
> 
> 
> 
> Maybe the same thing should be used here or copied here? This is an 
> attempt to wipe the disk decryption password (= part the EFI secret 
> area) against future leakage.
> 
> (Note that I have no evidence that any optimizer currently removes
> the grub_memset call.)

Actually, this gets into the whole systems annoyance about who should
see the secret and how long should it live.  The truth is this secret
has to be used twice: once for grub to find the kernel and initrd and
once for the initrd to mount root.  The fact that there's no current
mechanism in place for grub to pass the secret to the initrd is a bit
of a systems failure.  I was thinking the config table could do that,
in which case this code should really be eliminated.  Likely the best
thing to do would be to have an OVMF exitbootservices notifier which
wipes everything.

> > +  *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 cast to unsigned long can be removed (s->base is a
> grub_uint64_t).

The style of casting is best practice for converting integers to
pointers ... it's actually what the C manual recommends so it's best to
keep it for the compiler to be aware this is exactly what is intended. 
Just as an illustration think what happens on a 32 bit machine:
grub_uint64_t is too big and we need the unsigned long cast to truncate
it.

> > +
> > +  /* 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 (e->data[end - 1] != '\0')
> > +	return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk
> > encryption password is not zero terminated\n");
> 
> I think there's a pathological edge case if e->len exactly equals 
> sizeof(struct secret_entry) -- that is, it indicates a GUID struct
> with a zero-length data field.  In such a case, 'end' will be 0, the 
> zero-termination check below might succeed because e->data[-1] ==
> '\0' (last byte of length field in little-endian machine will
> probably be zero).  As a result secret_ptr will point to garbage.
> 
> A possible solution is to modify the above condition to:
> 
>    if (end <= 0 || e->data[end - 1] != '\0')
>      return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk 
> encryption password is not zero terminated\n");
> 
> I assume that a 0-length data field might be OK for other GUIDs but
> it is not valid for the disk password GUID.

Sure, although I think end < 2 just in case a zero length string causes
other problems.

> > +
> > +      *secret_ptr = e->data;
> > +      return GRUB_ERR_NONE;
> > +    }
> > +    ptr += e->len;
> > +    len -= e->len;
> > +  }
> > +  return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret aread does
> > not contain disk decryption password\n");
> 
> typo: s/aread/area/

fixed.

James