diff mbox series

[v4,1/3] efi/libstub: Copy confidential computing secret area

Message ID 20211020061408.3447533-2-dovmurik@linux.ibm.com (mailing list archive)
State New, archived
Headers show
Series Allow guest access to EFI confidential computing secret area | expand

Commit Message

Dov Murik Oct. 20, 2021, 6:14 a.m. UTC
Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
Virtualization) allows a guest owner to inject secrets into the VMs
memory without the host/hypervisor being able to read them.

Firmware support for secret injection is available in OVMF, which
reserves a memory area for secret injection and includes a pointer to it
the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
However, OVMF doesn't force the guest OS to keep this memory area
reserved.

If EFI exposes such a table entry, efi/libstub will copy this area to a
reserved memory for future use inside the kernel.

A pointer to the new copy is kept in the EFI table under
LINUX_EFI_COCO_SECRET_AREA_GUID.

The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.

Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
---
 drivers/firmware/efi/Kconfig            | 12 +++++
 drivers/firmware/efi/libstub/Makefile   |  1 +
 drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
 drivers/firmware/efi/libstub/efi-stub.c |  2 +
 drivers/firmware/efi/libstub/efistub.h  |  6 +++
 drivers/firmware/efi/libstub/x86-stub.c |  2 +
 include/linux/efi.h                     |  6 +++
 7 files changed, 97 insertions(+)
 create mode 100644 drivers/firmware/efi/libstub/coco.c

Comments

Greg KH Oct. 20, 2021, 6:39 a.m. UTC | #1
On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
> Virtualization) allows a guest owner to inject secrets into the VMs
> memory without the host/hypervisor being able to read them.
> 
> Firmware support for secret injection is available in OVMF, which
> reserves a memory area for secret injection and includes a pointer to it
> the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
> However, OVMF doesn't force the guest OS to keep this memory area
> reserved.
> 
> If EFI exposes such a table entry, efi/libstub will copy this area to a
> reserved memory for future use inside the kernel.
> 
> A pointer to the new copy is kept in the EFI table under
> LINUX_EFI_COCO_SECRET_AREA_GUID.
> 
> The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
> 
> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> ---
>  drivers/firmware/efi/Kconfig            | 12 +++++
>  drivers/firmware/efi/libstub/Makefile   |  1 +
>  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
>  drivers/firmware/efi/libstub/efi-stub.c |  2 +
>  drivers/firmware/efi/libstub/efistub.h  |  6 +++
>  drivers/firmware/efi/libstub/x86-stub.c |  2 +
>  include/linux/efi.h                     |  6 +++
>  7 files changed, 97 insertions(+)
>  create mode 100644 drivers/firmware/efi/libstub/coco.c
> 
> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> index 2c3dac5ecb36..68d1c5e6a7b5 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
>  
>  	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
>  	  information.
> +
> +config EFI_COCO_SECRET
> +	bool "Copy and reserve EFI Confidential Computing secret area"
> +	depends on EFI
> +	default n

default is always "n", no need to list this.

> +	help
> +	  Copy memory reserved by EFI for Confidential Computing (coco)
> +	  injected secrets, if EFI exposes such a table entry.

Why would you want to "copy" secret memory?

This sounds really odd here, it sounds like you are opening up a
security hole.  Are you sure this is the correct text that everyone on
the "COCO" group agrees with?

> +
> +	  If you say Y here, the EFI stub copy the EFI secret area (if
> +	  available) and reserve it for use inside the kernel.  This will
> +	  allow the virt/coo/efi_secret module to access the secrets.

What is "virt/coo/efi_secret"?

> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index d0537573501e..fdada3fd5d9b 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>  lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
>  				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
>  
> +lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
>  lib-$(CONFIG_ARM)		+= arm32-stub.o
>  lib-$(CONFIG_ARM64)		+= arm64-stub.o
>  lib-$(CONFIG_X86)		+= x86-stub.o
> diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
> new file mode 100644
> index 000000000000..bf546b6a3f72
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/coco.c
> @@ -0,0 +1,68 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Confidential computing (coco) secret area handling
> + *
> + * Copyright (C) 2021 IBM Corporation
> + * Author: Dov Murik <dovmurik@linux.ibm.com>
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/sizes.h>
> +#include <asm/efi.h>
> +
> +#include "efistub.h"
> +
> +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
> +	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
> +
> +/**
> + * struct efi_coco_secret_table - EFI config table that points to the
> + * confidential computing secret area. The guid
> + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
> + * @base:	Physical address of the EFI secret area
> + * @size:	Size (in bytes) of the EFI secret area
> + */
> +struct efi_coco_secret_table {
> +	u64 base;
> +	u64 size;

__le64?  Or is this really in host endian format?

> +} __attribute((packed));
> +
> +/*
> + * Create a copy of EFI's confidential computing secret area (if available) so
> + * that the secrets are accessible in the kernel after ExitBootServices.
> + */
> +void efi_copy_coco_secret_area(void)
> +{
> +	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
> +	efi_status_t status;
> +	struct efi_coco_secret_table *secret_table;
> +	struct linux_efi_coco_secret_area *secret_area;
> +
> +	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
> +	if (!secret_table)
> +		return;
> +
> +	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
> +		return;
> +
> +	/* Allocate space for the secret area and copy it */
> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> +			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
> +
> +	if (status != EFI_SUCCESS) {
> +		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
> +		return;
> +	}
> +
> +	secret_area->size = secret_table->size;
> +	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);

Why the double cast?

And you can treat this value as a "raw" pointer directly?  No need to
map it at all?  What could go wrong...

> +
> +	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
> +	if (status != EFI_SUCCESS)
> +		goto err_free;
> +
> +	return;
> +
> +err_free:
> +	efi_bs_call(free_pool, secret_area);

This memory is never freed when shutting down the system?

thanks,

greg k-h
Ard Biesheuvel Oct. 20, 2021, 7:02 a.m. UTC | #2
On Wed, 20 Oct 2021 at 08:44, Greg KH <gregkh@linuxfoundation.org> wrote:
>
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> > Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
> > Virtualization) allows a guest owner to inject secrets into the VMs
> > memory without the host/hypervisor being able to read them.
> >
> > Firmware support for secret injection is available in OVMF, which
> > reserves a memory area for secret injection and includes a pointer to it
> > the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
> > However, OVMF doesn't force the guest OS to keep this memory area
> > reserved.
> >
> > If EFI exposes such a table entry, efi/libstub will copy this area to a
> > reserved memory for future use inside the kernel.
> >
> > A pointer to the new copy is kept in the EFI table under
> > LINUX_EFI_COCO_SECRET_AREA_GUID.
> >
> > The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
> >
> > Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
> > ---
> >  drivers/firmware/efi/Kconfig            | 12 +++++
> >  drivers/firmware/efi/libstub/Makefile   |  1 +
> >  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
> >  drivers/firmware/efi/libstub/efi-stub.c |  2 +
> >  drivers/firmware/efi/libstub/efistub.h  |  6 +++
> >  drivers/firmware/efi/libstub/x86-stub.c |  2 +
> >  include/linux/efi.h                     |  6 +++
> >  7 files changed, 97 insertions(+)
> >  create mode 100644 drivers/firmware/efi/libstub/coco.c
> >
> > diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
> > index 2c3dac5ecb36..68d1c5e6a7b5 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
> >
> >         See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
> >         information.
> > +
> > +config EFI_COCO_SECRET
> > +     bool "Copy and reserve EFI Confidential Computing secret area"
> > +     depends on EFI
> > +     default n
>
> default is always "n", no need to list this.
>
> > +     help
> > +       Copy memory reserved by EFI for Confidential Computing (coco)
> > +       injected secrets, if EFI exposes such a table entry.
>
> Why would you want to "copy" secret memory?
>
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone on
> the "COCO" group agrees with?
>
> > +
> > +       If you say Y here, the EFI stub copy the EFI secret area (if
> > +       available) and reserve it for use inside the kernel.  This will
> > +       allow the virt/coo/efi_secret module to access the secrets.
>
> What is "virt/coo/efi_secret"?
>
> > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> > index d0537573501e..fdada3fd5d9b 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
> >  lib-$(CONFIG_EFI_GENERIC_STUB)       += efi-stub.o fdt.o string.o \
> >                                  $(patsubst %.c,lib-%.o,$(efi-deps-y))
> >
> > +lib-$(CONFIG_EFI_COCO_SECRET)        += coco.o
> >  lib-$(CONFIG_ARM)            += arm32-stub.o
> >  lib-$(CONFIG_ARM64)          += arm64-stub.o
> >  lib-$(CONFIG_X86)            += x86-stub.o
> > diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
> > new file mode 100644
> > index 000000000000..bf546b6a3f72
> > --- /dev/null
> > +++ b/drivers/firmware/efi/libstub/coco.c
> > @@ -0,0 +1,68 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/*
> > + * Confidential computing (coco) secret area handling
> > + *
> > + * Copyright (C) 2021 IBM Corporation
> > + * Author: Dov Murik <dovmurik@linux.ibm.com>
> > + */
> > +
> > +#include <linux/efi.h>
> > +#include <linux/sizes.h>
> > +#include <asm/efi.h>
> > +
> > +#include "efistub.h"
> > +
> > +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
> > +     EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
> > +
> > +/**
> > + * struct efi_coco_secret_table - EFI config table that points to the
> > + * confidential computing secret area. The guid
> > + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
> > + * @base:    Physical address of the EFI secret area
> > + * @size:    Size (in bytes) of the EFI secret area
> > + */
> > +struct efi_coco_secret_table {
> > +     u64 base;
> > +     u64 size;
>
> __le64?  Or is this really in host endian format?
>

EFI hosts are always LE so either is fine.

> > +} __attribute((packed));
> > +
> > +/*
> > + * Create a copy of EFI's confidential computing secret area (if available) so
> > + * that the secrets are accessible in the kernel after ExitBootServices.
> > + */
> > +void efi_copy_coco_secret_area(void)
> > +{
> > +     efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
> > +     efi_status_t status;
> > +     struct efi_coco_secret_table *secret_table;
> > +     struct linux_efi_coco_secret_area *secret_area;
> > +
> > +     secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
> > +     if (!secret_table)
> > +             return;
> > +
> > +     if (secret_table->size == 0 || secret_table->size >= SZ_4G)
> > +             return;
> > +
> > +     /* Allocate space for the secret area and copy it */
> > +     status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
> > +                          sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
> > +
> > +     if (status != EFI_SUCCESS) {
> > +             efi_err("Unable to allocate memory for confidential computing secret area copy\n");
> > +             return;
> > +     }
> > +
> > +     secret_area->size = secret_table->size;
> > +     memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
>
> Why the double cast?
>

This is generally needed for compatibility with 32-bit hosts, where
casting a u64 to void* causes warnings, even though we know in that
case that only the lower 32 bits will contain anything (even with PAE
etc, as 32-bit UEFI only uses 32-bit addressable memory)

In this particular case, it probably makes little sense, as COCO is
not going to run on 32-bit hosts anyway (famous last words)

> And you can treat this value as a "raw" pointer directly?  No need to
> map it at all?  What could go wrong...
>

Yes. EFI boot services (as well as this code) are guaranteed to run
under a 1:1 mapping of system memory.

> > +
> > +     status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
> > +     if (status != EFI_SUCCESS)
> > +             goto err_free;
> > +
> > +     return;
> > +
> > +err_free:
> > +     efi_bs_call(free_pool, secret_area);
>
> This memory is never freed when shutting down the system?
>

All boot services memory is implicitly freed when the system calls
ExitBootServices() so this is fine.
Dov Murik Oct. 20, 2021, 8:02 a.m. UTC | #3
On 20/10/2021 9:39, Greg KH wrote:
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
>> Confidential computing (coco) hardware such as AMD SEV (Secure Encrypted
>> Virtualization) allows a guest owner to inject secrets into the VMs
>> memory without the host/hypervisor being able to read them.
>>
>> Firmware support for secret injection is available in OVMF, which
>> reserves a memory area for secret injection and includes a pointer to it
>> the in EFI config table entry LINUX_EFI_COCO_SECRET_TABLE_GUID.
>> However, OVMF doesn't force the guest OS to keep this memory area
>> reserved.
>>
>> If EFI exposes such a table entry, efi/libstub will copy this area to a
>> reserved memory for future use inside the kernel.
>>
>> A pointer to the new copy is kept in the EFI table under
>> LINUX_EFI_COCO_SECRET_AREA_GUID.
>>
>> The new functionality can be enabled with CONFIG_EFI_COCO_SECRET=y.
>>
>> Signed-off-by: Dov Murik <dovmurik@linux.ibm.com>
>> ---
>>  drivers/firmware/efi/Kconfig            | 12 +++++
>>  drivers/firmware/efi/libstub/Makefile   |  1 +
>>  drivers/firmware/efi/libstub/coco.c     | 68 +++++++++++++++++++++++++
>>  drivers/firmware/efi/libstub/efi-stub.c |  2 +
>>  drivers/firmware/efi/libstub/efistub.h  |  6 +++
>>  drivers/firmware/efi/libstub/x86-stub.c |  2 +
>>  include/linux/efi.h                     |  6 +++
>>  7 files changed, 97 insertions(+)
>>  create mode 100644 drivers/firmware/efi/libstub/coco.c
>>
>> diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
>> index 2c3dac5ecb36..68d1c5e6a7b5 100644
>> --- a/drivers/firmware/efi/Kconfig
>> +++ b/drivers/firmware/efi/Kconfig
>> @@ -284,3 +284,15 @@ config EFI_CUSTOM_SSDT_OVERLAYS
>>  
>>  	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
>>  	  information.
>> +
>> +config EFI_COCO_SECRET
>> +	bool "Copy and reserve EFI Confidential Computing secret area"
>> +	depends on EFI
>> +	default n
> 
> default is always "n", no need to list this.
> 

OK, I'll remove.


>> +	help
>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
>> +	  injected secrets, if EFI exposes such a table entry.
> 
> Why would you want to "copy" secret memory?
> 
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone on
> the "COCO" group agrees with?

I understand the security concern: we don't want several copies of the
secrets all around the guest's memory.

I'll try to see if I can just reserve the memory (instruct EFI to leave
it intact) at its current address instead of creating a copy.  I'm open
to suggestions/pointers.


linux-coco list is CC'd on this series; feedback is welcome.


> 
>> +
>> +	  If you say Y here, the EFI stub copy the EFI secret area (if
>> +	  available) and reserve it for use inside the kernel.  This will
>> +	  allow the virt/coo/efi_secret module to access the secrets.
> 
> What is "virt/coo/efi_secret"?
> 

Typo: that should be virt/coco/efi_secret (the module introduced in
patch 3).

-Dov

>> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
>> index d0537573501e..fdada3fd5d9b 100644
>> --- a/drivers/firmware/efi/libstub/Makefile
>> +++ b/drivers/firmware/efi/libstub/Makefile
>> @@ -66,6 +66,7 @@ $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
>>  lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
>>  				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
>>  
>> +lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
>>  lib-$(CONFIG_ARM)		+= arm32-stub.o
>>  lib-$(CONFIG_ARM64)		+= arm64-stub.o
>>  lib-$(CONFIG_X86)		+= x86-stub.o
>> diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
>> new file mode 100644
>> index 000000000000..bf546b6a3f72
>> --- /dev/null
>> +++ b/drivers/firmware/efi/libstub/coco.c
>> @@ -0,0 +1,68 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +/*
>> + * Confidential computing (coco) secret area handling
>> + *
>> + * Copyright (C) 2021 IBM Corporation
>> + * Author: Dov Murik <dovmurik@linux.ibm.com>
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <linux/sizes.h>
>> +#include <asm/efi.h>
>> +
>> +#include "efistub.h"
>> +
>> +#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
>> +	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
>> +
>> +/**
>> + * struct efi_coco_secret_table - EFI config table that points to the
>> + * confidential computing secret area. The guid
>> + * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
>> + * @base:	Physical address of the EFI secret area
>> + * @size:	Size (in bytes) of the EFI secret area
>> + */
>> +struct efi_coco_secret_table {
>> +	u64 base;
>> +	u64 size;
> 
> __le64?  Or is this really in host endian format?
> 
>> +} __attribute((packed));
>> +
>> +/*
>> + * Create a copy of EFI's confidential computing secret area (if available) so
>> + * that the secrets are accessible in the kernel after ExitBootServices.
>> + */
>> +void efi_copy_coco_secret_area(void)
>> +{
>> +	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
>> +	efi_status_t status;
>> +	struct efi_coco_secret_table *secret_table;
>> +	struct linux_efi_coco_secret_area *secret_area;
>> +
>> +	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
>> +	if (!secret_table)
>> +		return;
>> +
>> +	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
>> +		return;
>> +
>> +	/* Allocate space for the secret area and copy it */
>> +	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
>> +			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
>> +
>> +	if (status != EFI_SUCCESS) {
>> +		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
>> +		return;
>> +	}
>> +
>> +	secret_area->size = secret_table->size;
>> +	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
> 
> Why the double cast?
> 
> And you can treat this value as a "raw" pointer directly?  No need to
> map it at all?  What could go wrong...
> 
>> +
>> +	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
>> +	if (status != EFI_SUCCESS)
>> +		goto err_free;
>> +
>> +	return;
>> +
>> +err_free:
>> +	efi_bs_call(free_pool, secret_area);
> 
> This memory is never freed when shutting down the system?
> 
> thanks,
> 
> greg k-h
>
James Bottomley Oct. 20, 2021, noon UTC | #4
On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
[...]
> > +	help
> > +	  Copy memory reserved by EFI for Confidential Computing (coco)
> > +	  injected secrets, if EFI exposes such a table entry.
> 
> Why would you want to "copy" secret memory?
> 
> This sounds really odd here, it sounds like you are opening up a
> security hole.  Are you sure this is the correct text that everyone
> on the "COCO" group agrees with?

The way this works is that EFI covers the secret area with a boot time
handoff block, which means it gets destroyed as soon as
ExitBootServices is called as a security measure ... if you do nothing
the secret is shredded.  This means you need to make a copy of it
before that happens if there are secrets that need to live beyond the
EFI boot stub.

James
Greg KH Oct. 20, 2021, 12:11 p.m. UTC | #5
On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> > On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> [...]
> > > +	help
> > > +	  Copy memory reserved by EFI for Confidential Computing (coco)
> > > +	  injected secrets, if EFI exposes such a table entry.
> > 
> > Why would you want to "copy" secret memory?
> > 
> > This sounds really odd here, it sounds like you are opening up a
> > security hole.  Are you sure this is the correct text that everyone
> > on the "COCO" group agrees with?
> 
> The way this works is that EFI covers the secret area with a boot time
> handoff block, which means it gets destroyed as soon as
> ExitBootServices is called as a security measure ... if you do nothing
> the secret is shredded.  This means you need to make a copy of it
> before that happens if there are secrets that need to live beyond the
> EFI boot stub.

Ok, but "copy secrets" does sound really odd, so you all need a much
better description here, and hopefully somewhere else in Documentation/
to describe exactly what this new API is and is to be used for.

Otherwise I read this as "hey a backdoor to read the secrets I wasn't
supposed to be able to see!"

thanks,

greg k-h
Dov Murik Oct. 20, 2021, 12:52 p.m. UTC | #6
On 20/10/2021 15:11, Greg KH wrote:
> On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
>> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
>>> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
>> [...]
>>>> +	help
>>>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
>>>> +	  injected secrets, if EFI exposes such a table entry.
>>>
>>> Why would you want to "copy" secret memory?
>>>
>>> This sounds really odd here, it sounds like you are opening up a
>>> security hole.  Are you sure this is the correct text that everyone
>>> on the "COCO" group agrees with?
>>
>> The way this works is that EFI covers the secret area with a boot time
>> handoff block, which means it gets destroyed as soon as
>> ExitBootServices is called as a security measure ... if you do nothing
>> the secret is shredded.  This means you need to make a copy of it
>> before that happens if there are secrets that need to live beyond the
>> EFI boot stub.
> 
> Ok, but "copy secrets" does sound really odd, so you all need a much
> better description here, and hopefully somewhere else in Documentation/
> to describe exactly what this new API is and is to be used for.
> 


So something like:


config EFI_COCO_SECRET
	bool "Keep the EFI Confidential Computing secret area"
	depends on EFI
	help
	  Confidential Computing platforms (such as AMD SEV) allow for
	  secrets injection during guest VM launch.  The secrets are
	  placed in a designated EFI memory area.  EFI destorys
	  the confidential computing secret area when ExitBootServices
	  is called.

	  In order to use the secrets in the kernel, the secret area
	  must be copied to kernel-reserved memory (before it is erased).

	  If you say Y here, the EFI stub will copy the EFI secret area (if
	  available) and reserve it for use inside the kernel.  This will
	  allow the virt/coco/efi_secret module to access the secrets.



and some new file like Documentation/security/coco/efi_secret.rst which
describes this whole protocol (from secret injection at VM launch
into an EFI page, through efistub and efi in linux, to the efi_secret
module which exposes the secrets).


Is that what you're looking for?



> Otherwise I read this as "hey a backdoor to read the secrets I wasn't
> supposed to be able to see!"
> 

Note that both EFI and kernel (and userspace, for that matter) are inside
the trusted zone in terms of AMD SEV (host/hypervisor => untrusted,
guest VM => trusted).  So it's OK for the guest kernel to see these secrets.


-Dov

> thanks,
> 
> greg k-h
>
Greg KH Oct. 20, 2021, 1:59 p.m. UTC | #7
On Wed, Oct 20, 2021 at 03:52:49PM +0300, Dov Murik wrote:
> 
> 
> On 20/10/2021 15:11, Greg KH wrote:
> > On Wed, Oct 20, 2021 at 08:00:28AM -0400, James Bottomley wrote:
> >> On Wed, 2021-10-20 at 08:39 +0200, Greg KH wrote:
> >>> On Wed, Oct 20, 2021 at 06:14:06AM +0000, Dov Murik wrote:
> >> [...]
> >>>> +	help
> >>>> +	  Copy memory reserved by EFI for Confidential Computing (coco)
> >>>> +	  injected secrets, if EFI exposes such a table entry.
> >>>
> >>> Why would you want to "copy" secret memory?
> >>>
> >>> This sounds really odd here, it sounds like you are opening up a
> >>> security hole.  Are you sure this is the correct text that everyone
> >>> on the "COCO" group agrees with?
> >>
> >> The way this works is that EFI covers the secret area with a boot time
> >> handoff block, which means it gets destroyed as soon as
> >> ExitBootServices is called as a security measure ... if you do nothing
> >> the secret is shredded.  This means you need to make a copy of it
> >> before that happens if there are secrets that need to live beyond the
> >> EFI boot stub.
> > 
> > Ok, but "copy secrets" does sound really odd, so you all need a much
> > better description here, and hopefully somewhere else in Documentation/
> > to describe exactly what this new API is and is to be used for.
> > 
> 
> 
> So something like:
> 
> 
> config EFI_COCO_SECRET
> 	bool "Keep the EFI Confidential Computing secret area"
> 	depends on EFI
> 	help
> 	  Confidential Computing platforms (such as AMD SEV) allow for
> 	  secrets injection during guest VM launch.  The secrets are
> 	  placed in a designated EFI memory area.  EFI destorys
> 	  the confidential computing secret area when ExitBootServices
> 	  is called.

That last sentence does not make much sense to me, sorry.

> 	  In order to use the secrets in the kernel, the secret area
> 	  must be copied to kernel-reserved memory (before it is erased).
> 
> 	  If you say Y here, the EFI stub will copy the EFI secret area (if
> 	  available) and reserve it for use inside the kernel.  This will
> 	  allow the virt/coco/efi_secret module to access the secrets.

Really this is about getting that data out to userspace, right?  Should
you mention that here?

> and some new file like Documentation/security/coco/efi_secret.rst which
> describes this whole protocol (from secret injection at VM launch
> into an EFI page, through efistub and efi in linux, to the efi_secret
> module which exposes the secrets).

Yes, that would be good to have documented.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 2c3dac5ecb36..68d1c5e6a7b5 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -284,3 +284,15 @@  config EFI_CUSTOM_SSDT_OVERLAYS
 
 	  See Documentation/admin-guide/acpi/ssdt-overlays.rst for more
 	  information.
+
+config EFI_COCO_SECRET
+	bool "Copy and reserve EFI Confidential Computing secret area"
+	depends on EFI
+	default n
+	help
+	  Copy memory reserved by EFI for Confidential Computing (coco)
+	  injected secrets, if EFI exposes such a table entry.
+
+	  If you say Y here, the EFI stub copy the EFI secret area (if
+	  available) and reserve it for use inside the kernel.  This will
+	  allow the virt/coo/efi_secret module to access the secrets.
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index d0537573501e..fdada3fd5d9b 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -66,6 +66,7 @@  $(obj)/lib-%.o: $(srctree)/lib/%.c FORCE
 lib-$(CONFIG_EFI_GENERIC_STUB)	+= efi-stub.o fdt.o string.o \
 				   $(patsubst %.c,lib-%.o,$(efi-deps-y))
 
+lib-$(CONFIG_EFI_COCO_SECRET)	+= coco.o
 lib-$(CONFIG_ARM)		+= arm32-stub.o
 lib-$(CONFIG_ARM64)		+= arm64-stub.o
 lib-$(CONFIG_X86)		+= x86-stub.o
diff --git a/drivers/firmware/efi/libstub/coco.c b/drivers/firmware/efi/libstub/coco.c
new file mode 100644
index 000000000000..bf546b6a3f72
--- /dev/null
+++ b/drivers/firmware/efi/libstub/coco.c
@@ -0,0 +1,68 @@ 
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * Confidential computing (coco) secret area handling
+ *
+ * Copyright (C) 2021 IBM Corporation
+ * Author: Dov Murik <dovmurik@linux.ibm.com>
+ */
+
+#include <linux/efi.h>
+#include <linux/sizes.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+#define LINUX_EFI_COCO_SECRET_TABLE_GUID                                                           \
+	EFI_GUID(0xadf956ad, 0xe98c, 0x484c, 0xae, 0x11, 0xb5, 0x1c, 0x7d, 0x33, 0x64, 0x47)
+
+/**
+ * struct efi_coco_secret_table - EFI config table that points to the
+ * confidential computing secret area. The guid
+ * LINUX_EFI_COCO_SECRET_TABLE_GUID holds this table.
+ * @base:	Physical address of the EFI secret area
+ * @size:	Size (in bytes) of the EFI secret area
+ */
+struct efi_coco_secret_table {
+	u64 base;
+	u64 size;
+} __attribute((packed));
+
+/*
+ * Create a copy of EFI's confidential computing secret area (if available) so
+ * that the secrets are accessible in the kernel after ExitBootServices.
+ */
+void efi_copy_coco_secret_area(void)
+{
+	efi_guid_t linux_secret_area_guid = LINUX_EFI_COCO_SECRET_AREA_GUID;
+	efi_status_t status;
+	struct efi_coco_secret_table *secret_table;
+	struct linux_efi_coco_secret_area *secret_area;
+
+	secret_table = get_efi_config_table(LINUX_EFI_COCO_SECRET_TABLE_GUID);
+	if (!secret_table)
+		return;
+
+	if (secret_table->size == 0 || secret_table->size >= SZ_4G)
+		return;
+
+	/* Allocate space for the secret area and copy it */
+	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+			     sizeof(*secret_area) + secret_table->size, (void **)&secret_area);
+
+	if (status != EFI_SUCCESS) {
+		efi_err("Unable to allocate memory for confidential computing secret area copy\n");
+		return;
+	}
+
+	secret_area->size = secret_table->size;
+	memcpy(secret_area->area, (void *)(unsigned long)secret_table->base, secret_table->size);
+
+	status = efi_bs_call(install_configuration_table, &linux_secret_area_guid, secret_area);
+	if (status != EFI_SUCCESS)
+		goto err_free;
+
+	return;
+
+err_free:
+	efi_bs_call(free_pool, secret_area);
+}
diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c
index 26e69788f27a..18b3acd15c85 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -205,6 +205,8 @@  efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	/* Ask the firmware to clear memory on unclean shutdown */
 	efi_enable_reset_attack_mitigation();
 
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index cde0a2ef507d..a23771547790 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -858,4 +858,10 @@  efi_enable_reset_attack_mitigation(void) { }
 
 void efi_retrieve_tpm2_eventlog(void);
 
+#ifdef CONFIG_EFI_COCO_SECRET
+void efi_copy_coco_secret_area(void);
+#else
+static inline void efi_copy_coco_secret_area(void) { }
+#endif
+
 #endif
diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
index f14c4ff5839f..4ad85e1b6191 100644
--- a/drivers/firmware/efi/libstub/x86-stub.c
+++ b/drivers/firmware/efi/libstub/x86-stub.c
@@ -793,6 +793,8 @@  unsigned long efi_main(efi_handle_t handle,
 
 	efi_retrieve_tpm2_eventlog();
 
+	efi_copy_coco_secret_area();
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 6b5d36babfcc..9021dd521302 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -359,6 +359,7 @@  void efi_native_runtime_setup(void);
 #define LINUX_EFI_MEMRESERVE_TABLE_GUID		EFI_GUID(0x888eb0c6, 0x8ede, 0x4ff5,  0xa8, 0xf0, 0x9a, 0xee, 0x5c, 0xb9, 0x77, 0xc2)
 #define LINUX_EFI_INITRD_MEDIA_GUID		EFI_GUID(0x5568e427, 0x68fc, 0x4f3d,  0xac, 0x74, 0xca, 0x55, 0x52, 0x31, 0xcc, 0x68)
 #define LINUX_EFI_MOK_VARIABLE_TABLE_GUID	EFI_GUID(0xc451ed2b, 0x9694, 0x45d3,  0xba, 0xba, 0xed, 0x9f, 0x89, 0x88, 0xa3, 0x89)
+#define LINUX_EFI_COCO_SECRET_AREA_GUID		EFI_GUID(0x940ed1e9, 0xd3da, 0x408b,  0xb3, 0x07, 0xe3, 0x2d, 0x25, 0x4a, 0x65, 0x16)
 
 /* OEM GUIDs */
 #define DELLEMC_EFI_RCI2_TABLE_GUID		EFI_GUID(0x2d9f28a2, 0xa886, 0x456a,  0x97, 0xa8, 0xf1, 0x1e, 0xf2, 0x4f, 0xf4, 0x55)
@@ -1282,4 +1283,9 @@  static inline struct efi_mokvar_table_entry *efi_mokvar_entry_find(
 }
 #endif
 
+struct linux_efi_coco_secret_area {
+	u32	size;
+	u8	area[];
+};
+
 #endif /* _LINUX_EFI_H */