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

[Dr. David Alan Gilbert]

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

use offsetof ?

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

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

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