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

[Dov Murik]

On 09/11/2021 15:53, 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
> v4: review fixes
> ---
>  grub-core/Makefile.core.def    |   8 +++
>  grub-core/disk/efi/efisecret.c | 125 +++++++++++++++++++++++++++++++++
>  include/grub/efi/api.h         |  15 ++++
>  3 files changed, 148 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..a8d1f39fc
> --- /dev/null
> +++ b/grub-core/disk/efi/efisecret.c
> @@ -0,0 +1,125 @@
> +#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;
> +  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);

Maybe clearer (not tested):

struct secret_entry *e = (struct secret_entry *)(*ptr - offsetof(struct 
secret_entry, data));



> +
> +  /* destroy the secret */
> +  grub_memset (e, 0, e->len);

This destroys the GUID field (good), the secret data (good), but
also the len field.  Without the correct length value it will be
impossible to scan the GUIDed secrets table after the erased entry.

If the luks passphrase is the only secret, then there's no
problem.  But if the OS wants to use other secrets from this
table, I suggest erasing only e->guid and e->data.


-Dov


> +  *ptr = NULL;
> +
> +  if (have_it)
> +    return GRUB_ERR_NONE;
> +
> +  return  grub_error (GRUB_ERR_ACCESS_DENIED, "EFI secret failed to unlock 
> any volumes");
> +}
> +

[snip]