[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for crypto
From: |
Glenn Washburn |
Subject: |
Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk |
Date: |
Mon, 14 Feb 2022 16:20:54 -0600 |
On Mon, 7 Feb 2022 10:29:44 -0500
James Bottomley <jejb@linux.ibm.com> wrote:
> This module is designed to provide an efisecret provider which
> interrogates the EFI configuration table to find the location of the
> confidential computing secret and tries to register the secret with
> the cryptodisk.
>
> The secret is stored in a boot allocated area, usually a page in size.
> The layout of the secret injection area is a header
>
> |GRUB_EFI_SECRET_TABLE_HEADER_GUID|len|
>
> with entries of the form
>
> |guid|len|data|
>
> the guid corresponding to the disk encryption passphrase is
> GRUB_EFI_DISKPASSWD_GUID and data must be a zero terminated string.
Is the NULL termination requirement something specified in some
specification? Otherwise, its not clear to me why it must be so.
> To get a high entropy string that doesn't need large numbers of
> iterations, use a base64 encoding of 33 bytes of random data.
I won't reiterate the comments by David Gilbert, which I also agree
with.
>
> Signed-off-by: James Bottomley <jejb@linux.ibm.com>
>
> ---
>
> v2: use callback to print failure message and destroy secret
> v3: change to generic naming to use for TDX and SEV and use new mechanism
> v4: review fixes
> ---
> grub-core/Makefile.core.def | 8 ++
> grub-core/disk/efi/efisecret.c | 129 +++++++++++++++++++++++++++++++++
> include/grub/efi/api.h | 15 ++++
> 3 files changed, 152 insertions(+)
> create mode 100644 grub-core/disk/efi/efisecret.c
>
> diff --git a/grub-core/Makefile.core.def b/grub-core/Makefile.core.def
> index 8022e1c0a..6293ddaa5 100644
> --- a/grub-core/Makefile.core.def
> +++ b/grub-core/Makefile.core.def
> @@ -788,6 +788,14 @@ module = {
> enable = efi;
> };
>
> +module = {
> + name = efisecret;
> +
> + common = disk/efi/efisecret.c;
> +
> + enable = efi;
> +};
> +
> module = {
> name = lsefimmap;
>
> diff --git a/grub-core/disk/efi/efisecret.c b/grub-core/disk/efi/efisecret.c
> new file mode 100644
> index 000000000..4cecebbdc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include <grub/err.h>
> +#include <grub/misc.h>
> +#include <grub/cryptodisk.h>
> +#include <grub/efi/efi.h>
> +#include <grub/efi/api.h>
> +#include <grub/dl.h>
> +
> +GRUB_MOD_LICENSE ("GPLv3+");
> +
> +static grub_efi_packed_guid_t secret_guid = GRUB_EFI_SECRET_TABLE_GUID;
> +static grub_efi_packed_guid_t tableheader_guid =
> GRUB_EFI_SECRET_TABLE_HEADER_GUID;
> +static grub_efi_packed_guid_t diskpasswd_guid = GRUB_EFI_DISKPASSWD_GUID;
> +
> +struct efi_secret {
I find this name confusing. Perhaps a better name would be
"efi_secret_table_location"?
> + grub_uint64_t base;
> + grub_uint64_t size;
> +};
> +
> +struct secret_header {
> + grub_efi_packed_guid_t guid;
> + grub_uint32_t len;
> +};
> +
> +struct secret_entry {
> + grub_efi_packed_guid_t guid;
> + grub_uint32_t len;
> + grub_uint8_t data[0];
> +};
> +
> +static grub_err_t
> +grub_efi_secret_put (const char *arg __attribute__((unused)), int have_it,
> + grub_uint8_t **ptr)
> +{
> + struct secret_entry *e = (struct secret_entry *)(*ptr - (long)&((struct
> secret_entry *)0)->data);
> + int len = e->len;
> +
> + /* destroy the secret */
> + grub_memset (e, 0, len);
> + /* put back the length to make sure the table is still traversable */
> + e->len = len;
> +
> + *ptr = NULL;
> +
> + if (have_it)
> + return GRUB_ERR_NONE;
> +
> + return grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock
> any volumes");
I'm not seeing why the "have_it" argument and returning an error here is
useful. Its seems a bit out of place. Wouldn't it be better to do this
in the caller?
> +}
> +
> +static grub_err_t
> +grub_efi_secret_find (struct efi_secret *s, grub_uint8_t **secret_ptr)
> +{
> + int len;
> + struct secret_header *h;
> + struct secret_entry *e;
> + unsigned char *ptr = (unsigned char *)(unsigned long)s->base;
> +
> + /* the area must be big enough for a guid and a u32 length */
> + if (s->size < sizeof (*h))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too
> small");
> +
> + h = (struct secret_header *)ptr;
> + if (grub_memcmp(&h->guid, &tableheader_guid, sizeof (h->guid)))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not
> start with correct guid\n");
> + if (h->len < sizeof (*h))
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is too
> small\n");
> +
> + len = h->len - sizeof (*h);
> + ptr += sizeof (*h);
> +
> + while (len >= (int)sizeof (*e)) {
> + e = (struct secret_entry *)ptr;
> + if (e->len < sizeof(*e) || e->len > (unsigned int)len)
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area is
> corrupt\n");
> +
> + if (! grub_memcmp (&e->guid, &diskpasswd_guid, sizeof (e->guid))) {
> + int end = e->len - sizeof(*e);
> +
> + /*
> + * the passphrase must be a zero terminated string because the
> + * password routines call grub_strlen () to find its size
> + */
I'm confused, what password routines are being referenced here?
> + if (end < 2 || e->data[end - 1] != '\0')
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area disk
> encryption password is corrupt\n");
NULL termination seems like an unnecessary requirement, if secret length
is added as an output argument to the function.
> +
> + *secret_ptr = e->data;
> + return GRUB_ERR_NONE;
> + }
> + ptr += e->len;
> + len -= e->len;
> + }
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "EFI secret area does not
> contain disk decryption password\n");
> +}
> +
> +static grub_err_t
> +grub_efi_secret_get (const char *arg __attribute__((unused)), grub_uint8_t
> **ptr)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < grub_efi_system_table->num_table_entries; i++)
> + {
> + grub_efi_packed_guid_t *guid =
> + &grub_efi_system_table->configuration_table[i].vendor_guid;
> +
> + if (! grub_memcmp (guid, &secret_guid, sizeof
> (grub_efi_packed_guid_t))) {
Open brace should be on a separate line and indented 2 spaces.
Glenn
> + struct efi_secret *s =
> + grub_efi_system_table->configuration_table[i].vendor_table;
> +
> + return grub_efi_secret_find(s, ptr);
> + }
> + }
> + return grub_error (GRUB_ERR_BAD_ARGUMENT, "No secret found in the EFI
> configuration table");
> +}
> +
> +static struct grub_secret_entry secret = {
> + .name = "efisecret",
> + .get = grub_efi_secret_get,
> + .put = grub_efi_secret_put,
> +};
> +
> +GRUB_MOD_INIT(efisecret)
> +{
> + grub_cryptodisk_add_secret_provider (&secret);
> +}
> +
> +GRUB_MOD_FINI(efisecret)
> +{
> + grub_cryptodisk_remove_secret_provider (&secret);
> +}
> diff --git a/include/grub/efi/api.h b/include/grub/efi/api.h
> index f1a52210c..f33a8f2ab 100644
> --- a/include/grub/efi/api.h
> +++ b/include/grub/efi/api.h
> @@ -299,6 +299,21 @@
> { 0x9a, 0x16, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \
> }
>
> +#define GRUB_EFI_SECRET_TABLE_GUID \
> + { 0xadf956ad, 0xe98c, 0x484c, \
> + { 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47} \
> + }
> +
> +#define GRUB_EFI_SECRET_TABLE_HEADER_GUID \
> + { 0x1e74f542, 0x71dd, 0x4d66, \
> + { 0x96, 0x3e, 0xef, 0x42, 0x87, 0xff, 0x17, 0x3b } \
> + }
> +
> +#define GRUB_EFI_DISKPASSWD_GUID \
> + { 0x736869e5, 0x84f0, 0x4973, \
> + { 0x92, 0xec, 0x06, 0x87, 0x9c, 0xe3, 0xda, 0x0b } \
> + }
> +
> #define GRUB_EFI_ACPI_TABLE_GUID \
> { 0xeb9d2d30, 0x2d88, 0x11d3, \
> { 0x9a, 0x16, 0x0, 0x90, 0x27, 0x3f, 0xc1, 0x4d } \