[PATCH v2 2/2] lsefi: fixed memory leaks
From: Fedora Ninjas Signed-off-by: Renaud Métrich --- grub-core/commands/efi/lsefi.c | 4 1 file changed, 4 insertions(+) diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c index 7acba3b39..f0b9201f1 100644 --- a/grub-core/commands/efi/lsefi.c +++ b/grub-core/commands/efi/lsefi.c @@ -186,8 +186,12 @@ grub_cmd_lsefi (grub_command_t cmd __attribute__ ((unused)), (unsigned) protocols[j]->data4[7]); } + if (protocols) + grub_efi_free_pool (protocols); } + grub_free (handles); + return 0; } -- 2.34.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
[PATCH v2 1/2] efi: new 'connectefi' command
When efi.quickboot is enabled on VMWare (which is the default for
hardware release 16 and later), it may happen that not all EFI devices
are connected. Due to this, browsing the devices in make_devices() just
fails to find devices, in particular disks or partitions for a given
disk.
This typically happens when network booting, then trying to chainload to
local disk (this is used in deployment tools such as Red Hat Satellite),
which is done through using the following grub.cfg snippet:
8< 8< 8<
unset prefix
search --file --set=prefix /EFI/redhat/grubx64.efi
if [ -n "$prefix" ]; then
chainloader ($prefix)/EFI/redhat/grubx64/efi
...
8< 8< 8<
With efi.quickboot, none of the devices are connected, causing "search"
to fail. Sometimes devices are connected but not the partition of the
disk matching $prefix, causing partition to not be found by
"chainloader".
This patch introduces a new "connectefi" command which recursively
connects all EFI devices starting from PCI root devices.
Typical grub.cfg snippet would then be:
8< 8< 8<
unset prefix
connectefi
search --file --set=prefix /EFI/redhat/grubx64.efi
if [ -n "$prefix" ]; then
chainloader ($prefix)/EFI/redhat/grubx64/efi
...
8< 8< 8<
In the future, if really needed, the code may be enhanced to avoid the
recursion and only connect PCI devices block devices.
Signed-off-by: Renaud Métrich
---
grub-core/commands/efi/lsefi.c | 52 ++
grub-core/disk/efi/efidisk.c | 13 +
grub-core/kern/efi/efi.c | 13 +
include/grub/efi/disk.h| 2 ++
include/grub/efi/efi.h | 5
5 files changed, 85 insertions(+)
diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c
index d1ce99af4..7acba3b39 100644
--- a/grub-core/commands/efi/lsefi.c
+++ b/grub-core/commands/efi/lsefi.c
@@ -19,6 +19,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -26,6 +27,7 @@
#include
#include
#include
+#include
GRUB_MOD_LICENSE ("GPLv3+");
@@ -78,6 +80,54 @@ struct known_protocol
{ GRUB_EFI_USB_IO_PROTOCOL_GUID, "USB I/O" },
};
+static grub_err_t
+grub_cmd_connectefi (grub_command_t cmd __attribute__ ((unused)),
+ int argc __attribute__ ((unused)),
+ char **args __attribute__ ((unused)))
+{
+ grub_efi_handle_t *handles;
+ grub_efi_uintn_t num_handles;
+ unsigned i, j;
+ grub_efi_guid_t pci_root_guid = GRUB_EFI_PCI_ROOT_IO_GUID;
+
+ handles = grub_efi_locate_handle (GRUB_EFI_ALL_HANDLES,
+ NULL, NULL, &num_handles);
+
+ for (i = 0; i < num_handles; i++)
+{
+ grub_efi_handle_t handle = handles[i];
+ grub_efi_status_t status;
+ grub_efi_uintn_t num_protocols;
+ grub_efi_packed_guid_t **protocols;
+
+ status = efi_call_3
(grub_efi_system_table->boot_services->protocols_per_handle,
+ handle, &protocols, &num_protocols);
+ if (status != GRUB_EFI_SUCCESS)
+ {
+ grub_dprintf ("efi", "handle %p: unable to retrieve protocols\n",
handle);
+ continue;
+ }
+ for (j = 0; j < num_protocols; j++)
+ if (! grub_memcmp (protocols[j], &pci_root_guid, sizeof
(pci_root_guid)))
+ {
+ status = grub_efi_connect_controller(handle, NULL, NULL, 1);
+ grub_dprintf ("efi", "connected handle %p (PCI root) "
+ "recursively: result=%" PRIxGRUB_EFI_STATUS "\n",
+ handle, status);
+ break;
+ }
+
+ if (protocols)
+ grub_efi_free_pool (protocols);
+}
+
+ grub_free (handles);
+
+ grub_efidisk_reenumerate_disks ();
+
+ return GRUB_ERR_NONE;
+}
+
static grub_err_t
grub_cmd_lsefi (grub_command_t cmd __attribute__ ((unused)),
int argc __attribute__ ((unused)),
@@ -147,6 +197,8 @@ GRUB_MOD_INIT(lsefi)
{
cmd = grub_register_command ("lsefi", grub_cmd_lsefi,
NULL, "Display EFI handles.");
+ cmd = grub_register_command ("connectefi", grub_cmd_connectefi,
+ NULL, "Connect EFI handles.");
}
GRUB_MOD_FINI(lsefi)
diff --git a/grub-core/disk/efi/efidisk.c b/grub-core/disk/efi/efidisk.c
index f077b5f55..eb0de5625 100644
--- a/grub-core/disk/efi/efidisk.c
+++ b/grub-core/disk/efi/efidisk.c
@@ -396,6 +396,19 @@ enumerate_disks (void)
free_devices (devices);
}
+void
+grub_efidisk_reenumerate_disks (void)
+{
+ free_devices (fd_devices);
+ free_devices (hd_devices);
+ free_devices (cd_devices);
+ fd_devices = 0;
+ hd_devices = 0;
+ cd_devices = 0;
+
+ enumerate_disks ();
+}
+
static int
grub_efidisk_iterate (grub_disk_dev_iterate_hook_t hook, void *hook_data,
grub_disk_pull_t pull)
diff --git a/grub-co
[PATCH v2 2/2] lsefi: fixed memory leaks
Signed-off-by: Renaud Métrich --- grub-core/commands/efi/lsefi.c | 4 1 file changed, 4 insertions(+) diff --git a/grub-core/commands/efi/lsefi.c b/grub-core/commands/efi/lsefi.c index 7acba3b39..f0b9201f1 100644 --- a/grub-core/commands/efi/lsefi.c +++ b/grub-core/commands/efi/lsefi.c @@ -186,8 +186,12 @@ grub_cmd_lsefi (grub_command_t cmd __attribute__ ((unused)), (unsigned) protocols[j]->data4[7]); } + if (protocols) + grub_efi_free_pool (protocols); } + grub_free (handles); + return 0; } -- 2.34.1 ___ Grub-devel mailing list Grub-devel@gnu.org https://lists.gnu.org/mailman/listinfo/grub-devel
Re: [PATCH v4 1/2] cryptodisk: add OS provided secret support
On Mon, 7 Feb 2022 10:29:43 -0500
James Bottomley 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.
I think I prefer the key protector framework proposed in the first
patch of Hernan Gatta's Automatic TPM Disk Unlock lock series. It feels
like a more generic mechanism (though admittedly very similar to yours)
because its not tied to cryptodisk. I'm not sure where we'd want to use
the secrets/protectors framework outside of cryptodisk, but it seems
like it could be useful. The advantage of this patch is that it allows
for the clearing of key data from memory.
I don't think we should have both a secrets and a key protectors
framework, as its seems they are aiming to accomplish basically the
same thing.
The name "secrets" seems a bit more generic than "protectors" because,
as in this case, the secret is not protected. There is something I
don't like about the word "secrets", but I don't have a better
suggestion, so this might be what sticks. One thing going for "secrets"
over "protectors", is that the cryptomount option "-s" works better
than "-p" for protectors because that's already taken by the password
option.
>
> Signed-off-by: James Bottomley
>
> ---
>
> v2: add callback for after secret use
> v3: update to use named module mechanism for secret loading
> v4: update for new secret passing API
> ---
> grub-core/disk/cryptodisk.c | 56 +++--
> include/grub/cryptodisk.h | 14 ++
> 2 files changed, 68 insertions(+), 2 deletions(-)
>
> diff --git a/grub-core/disk/cryptodisk.c b/grub-core/disk/cryptodisk.c
> index 497097394..f9c86f958 100644
> --- a/grub-core/disk/cryptodisk.c
> +++ b/grub-core/disk/cryptodisk.c
> @@ -42,6 +42,7 @@ static const struct grub_arg_option options[] =
> {"all", 'a', 0, N_("Mount all."), 0, 0},
> {"boot", 'b', 0, N_("Mount all volumes with `boot' flag set."), 0, 0},
> {"password", 'p', 0, N_("Password to open volumes."), 0,
> ARG_TYPE_STRING},
> +{"secret", 's', 0, N_("Get secret passphrase from named module and
> optional identifier"), 0, 0},
> {0, 0, 0, 0, 0, 0}
>};
>
> @@ -984,6 +985,9 @@ grub_util_cryptodisk_get_uuid (grub_disk_t disk)
>
> #endif
>
> +/* variable to hold the list of secret providers */
> +static struct grub_secret_entry *secret_providers;
> +
> static void
> cryptodisk_close (grub_cryptodisk_t dev)
> {
> @@ -1064,6 +1068,18 @@ grub_cryptodisk_scan_device_real (const char *name,
>return dev;
> }
>
> +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_err_t
> @@ -1160,10 +1176,14 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
> {
>struct grub_arg_list *state = ctxt->state;
>struct grub_cryptomount_args cargs = {0};
> + struct grub_secret_entry *se = NULL;
>
> - 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");
>
> + if (state[3].set && state[4].set)
> +return grub_error (GRUB_ERR_BAD_ARGUMENT, "-p and -s are exclusive
> options");
> +
>if (grub_cryptodisk_list == NULL)
> return grub_error (GRUB_ERR_BAD_MODULE, "no cryptodisk modules loaded");
>
> @@ -1172,6 +1192,24 @@ grub_cmd_cryptomount (grub_extcmd_context_t ctxt, int
> argc, char **args)
>cargs.key_data = (grub_uint8_t *) state[3].arg;
>cargs.key_len = grub_strlen (state[3].arg);
> }
> + else if (state[4].set) /* secret module */
> +{
> + 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], &cargs.key_data);
> + if (rc)
> + return rc;
> + cargs.key_len = grub_strlen((char *) cargs.key_data);
It seems better to me to send a pointer to cargs.key_len to se->get()
because it alre
Re: [PATCH v4 2/2] efi: Add API for retrieving the EFI secret for cryptodisk
On Mon, 7 Feb 2022 10:29:44 -0500
James Bottomley 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
>
> ---
>
> 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 0..4cecebbdc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,129 @@
> +#include
> +#include
> +#include
> +#include
> +#include
> +#include
> +
> +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?
> + i
