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

[Dov Murik <dovmurik@linux.ibm.com>]

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
>