[PATCH 3/6] efi_loader: Implement EFI variable handling via OP-TEE

[Ilias Apalodimas]

Hi Heinrich,

On Sat, May 09, 2020 at 11:14:51AM +0200, Heinrich Schuchardt wrote:
> On 5/6/20 9:12 PM, Ilias Apalodimas wrote:
> > In OP-TEE we can run EDK2's StandAloneMM on a secure partition.
> > StandAloneMM is responsible for the UEFI variable support. In
> > combination with OP-TEE and it's U-Boot supplicant, variables are
> > authenticated/validated in secure world and stored on an RPMB partition.
> >
> > So let's add a new config option in U-Boot implementing the necessary
> > calls to OP-TEE for the variable management.
> >
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>
> > Signed-off-by: Pipat Methavanitpong <pipat1010 at gmail.com>
> > Signed-off-by: Sughosh Ganu <sughosh.ganu at linaro.org>
> > ---
> >  lib/efi_loader/Kconfig            |   9 +
> >  lib/efi_loader/Makefile           |   4 +
> >  lib/efi_loader/efi_variable_tee.c | 645 ++++++++++++++++++++++++++++++
> >  3 files changed, 658 insertions(+)
> >  create mode 100644 lib/efi_loader/efi_variable_tee.c
> >
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 1cfa24ffcf72..e385cd0b9dae 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -164,4 +164,13 @@ config EFI_SECURE_BOOT
> >  	  it is signed with a trusted key. To do that, you need to install,
> >  	  at least, PK, KEK and db.
> >
> > +config EFI_MM_COMM_TEE
> > +	bool "UEFI variables storage service via OP-TEE"
> > +	depends on SUPPORT_EMMC_RPMB
> > +	default n
> > +	help
> > +	  If OP-TEE is present and running StandAloneMM dispatch all UEFI variable
> > +	  related operations to that. The application will verify, authenticate and
> > +	  store the variables on an RPMB
> > +
> >  endif
> > diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
> > index eff3c25ec301..dba652121eab 100644
> > --- a/lib/efi_loader/Makefile
> > +++ b/lib/efi_loader/Makefile
> > @@ -34,7 +34,11 @@ obj-y += efi_root_node.o
> >  obj-y += efi_runtime.o
> >  obj-y += efi_setup.o
> >  obj-$(CONFIG_EFI_UNICODE_COLLATION_PROTOCOL2) += efi_unicode_collation.o
> > +ifeq ($(CONFIG_EFI_MM_COMM_TEE),y)
> > +obj-y += efi_variable_tee.o
> > +else
> >  obj-y += efi_variable.o
> > +endif
> >  obj-y += efi_watchdog.o
> >  obj-$(CONFIG_LCD) += efi_gop.o
> >  obj-$(CONFIG_DM_VIDEO) += efi_gop.o
> > diff --git a/lib/efi_loader/efi_variable_tee.c b/lib/efi_loader/efi_variable_tee.c
> > new file mode 100644
> > index 000000000000..d4e206e22073
> > --- /dev/null
> > +++ b/lib/efi_loader/efi_variable_tee.c
> > @@ -0,0 +1,645 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI variable service via OP-TEE
> > + *
> > + *  Copyright (C) 2019 Linaro Ltd. <sughosh.ganu at linaro.org>
> > + *  Copyright (C) 2019 Linaro Ltd. <ilias.apalodimas at linaro.org>
> > + */
> > +
> > +#include <common.h>
> > +#include <efi.h>
> > +#include <efi_api.h>
> > +#include <efi_loader.h>
> > +#include <tee.h>
> > +#include <malloc.h>
> > +#include <mm_communication.h>
> > +#include <mm_variable.h>
> > +
> > +#define PTA_STMM_CMDID_COMMUNICATE 0
> 
> This seems to be a part of the OP-TEE communication interface to SMM and
> should be in an include with a comment describing the meaning of the
> constant.

Ok

> 
> > +#define PTA_STMM_UUID { 0xed32d533, 0x99e6, 0x4209, {\
> > +			0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7 } }
> > +
> > +#define EFI_MM_VARIABLE_GUID \
> > +	EFI_GUID(0xed32d533, 0x99e6, 0x4209, \
> > +		 0x9c, 0xc0, 0x2d, 0x72, 0xcd, 0xd9, 0x98, 0xa7)
> 
> These two GUIDs are just the same by value. Looking at EFI_GUID() and
> tee_optee_ta_uuid_to_octets() it seems that TEE is using big endian
> GUIDs while UEFI uses low endian ones. I think this deserves a comment
> in your code.
> 
> I would prefer to see the GUIDs in the includes. Anyway you copied from
> MdeModulePkg/Include/Protocol/SmmVariable.h

Fair enough. The reasopn we had two discrete header files was because we
followed the EDK2 example. Your recoomendation makes sense though, we'll move
everything to a single header file with sufficient commenting and add these
values as well.

> 
> > +
> > +static efi_uintn_t max_buffer_size;	/* comm + var + func + data */
> > +static efi_uintn_t max_payload_size;	/* func + data */
> > +
> > +struct mm_connection {
> > +	struct udevice *tee;
> > +	u32 session;
> > +};
> > +
> > +int get_connection(struct mm_connection *conn)
> 
> This function is only used locally. Please, mark it as static.
> 
> Please, comment your functions according to
> https://www.kernel.org/doc/html/latest/doc-guide/kernel-doc.html#function-documentation
> 

Will do 

> > +{
> > + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE

[...]

> > + *
> > + * @comm_buf:		Locally allocted communcation buffer
> 
> %s/allocted/allocated/
> 
> > + * @dsize:		Buffer size
> > + * Return:		status code
> 
> Please, be consistent in the capitalization of the argument descriptions.
> 

Ok 

> > + */
> 
> %s/cmonnucation/communication/
> 
> > + * it to OP-TEE
> > + *
> > + * @comm_buf:		Locally allocted communcation buffer
> 
> %s/allocted/allocated/
> 
> You can use spell-checking in vim with 'set spell'.
> 

Ok 

> > +	/* On the init function we initialize max_buffer_size with

[...]

> 
> %s/On the init/In the init/
> 
> > +	 * get_max_payload(). So skip the test if max_buffer_size is initialized
> > +	 * StandAloneMM will perform similar checks and drop the buffer if it's
> > +	 * too long
> 
> %s/too long/too long./
> 

Ok

> > +	 */
> > +	if (max_buffer_size && max_buffer_size <
> > +			(MM_COMMUNICATE_HEADER_SIZE +
> > +			 MM_VARIABLE_COMMUNICATE_SIZE +
> > +			 payload_size)) {
> > +		*ret = EFI_INVALID_PARAMETER;
> > +		return NULL;
> > +	}
> > +
> > +	comm_buf = calloc(1, MM_COMMUNICATE_HEADER_SIZE +
> > +			  MM_VARIABLE_COMMUNICATE_SIZE +
> > +			  payload_size);
> > +	if (!comm_buf) {
> > +		*ret = EFI_OUT_OF_RESOURCES;
> > +		return NULL;
> > +	}
> > +
> > +	mm_hdr = (struct mm_communicate_header *)comm_buf;
> > +	guidcpy(&mm_hdr->header_guid, &mm_var_guid);
> > +	mm_hdr->message_len = MM_VARIABLE_COMMUNICATE_SIZE + payload_size;
> > +
> > +	var_hdr = (struct mm_variable_communicate_header *)mm_hdr->data;
> > +	var_hdr->function = func;
> > +	if (dptr)
> > +		*dptr = var_hdr->data;
> > +	*ret = EFI_SUCCESS;
> > +
> > +	return comm_buf;
> > +}
> > +
> > +/**
> > + * Get variable payload size from StandAloneMM
> 
> get_max_payload() - get variable ....
> 
> > + *
> > + * @size:    Size of the variable in storage
> 

[...]

> %s/comm/communication/
> 

Ok

> > +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> > +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> > +				MM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> > +	if (!comm_buf)
> > +		return EFI_EXIT(ret);
> > +
> > +	/* Fill in contents */
> > +	guidcpy(&var_acc->guid, guid);
> > +	var_acc->data_size = tmp_dsize;
> > +	var_acc->name_size = name_size;
> > +	var_acc->attr = attr ? *attr : 0;
> > +	memcpy(var_acc->name, name, name_size);
> > +
> > +	/* Communicate */
> > +	ret = mm_communicate(comm_buf, payload_size);
> > +	if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> > +		/* Update with reported data size for trimmed case */
> > +		*data_size = var_acc->data_size;
> > +	}
> > +	if (ret != EFI_SUCCESS)
> > +		goto done;
> > +
> > +	if (attr)
> > +		*attr = var_acc->attr;
> > +	if (data)
> > +		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> > +		       var_acc->data_size);
> > +	else
> > +		ret = EFI_INVALID_PARAMETER;
> > +
> > +done:
> > +	free(comm_buf);
> > +	return EFI_EXIT(ret);
> > +}
> > +
> > +/**
> > + * efi_get_next_variable_name() - enumerate the current variable names
> > + *
> > + * @variable_name_size:	size of variable_name buffer in byte
> > + * @variable_name:	name of uefi variable's name in u16
> > + * @guid:		vendor's guid
> > + *
> > + * This function implements the GetNextVariableName service.
> > + *
> > + * See the Unified Extensible Firmware Interface (UEFI) specification for
> > + * details.
> > + *
> > + * Return: status code
> > + */
> > +efi_status_t EFIAPI efi_get_next_variable_name(efi_uintn_t *variable_name_size,
> > +					       u16 *variable_name,
> > +					       efi_guid_t *guid)
> > +{
> > +	struct mm_variable_getnext *var_getnext;
> > +	efi_uintn_t payload_size;
> > +	efi_uintn_t tmp_dsize;
> > +	efi_uintn_t name_size;
> > +	efi_status_t ret;
> > +	efi_uintn_t out_name_size;
> > +	efi_uintn_t in_name_size;
> > +	u8 *comm_buf;
> > +
> > +	EFI_ENTRY("%p \"%ls\" %pUl", variable_name_size, variable_name, guid);
> > +
> > +	if (!variable_name_size || !variable_name || !guid)
> > +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +
> > +	out_name_size = *variable_name_size;
> > +	in_name_size = u16_strsize(variable_name);
> 
> The UEFI spec requires: "The size must be large enough to fit input
> string supplied in VariableName buffer."
> 
> Further it is required to return EFI_INVALID_PARAMETER if the
> "Null-terminator is not found in the first VariableNameSize bytes of the
> input VariableName buffer."
> 
> Please, investigate if SMM takes care of the check or we should do it.
> 

I think it already does, but I'll double check and make sure

> > +
> > +	name_size = u16_strsize(variable_name);
> > +	if (name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> > +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> > +
> > +	/* Trim output buffer size */
> > +	tmp_dsize = *variable_name_size;
> > +	if (name_size + tmp_dsize >
> > +			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> > +		tmp_dsize = max_payload_size -
> > +				MM_VARIABLE_GET_NEXT_HEADER_SIZE -
> > +				name_size;
> > +	}
> > +
> > +	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE +
> > +			max(out_name_size, in_name_size);
> 
> Only out_name_size is relevant here.
> 

Ths was copied from edk2, I haven't really looked into further details on why
they had that check. I'll have a look 

> > +	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> > +				MM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> > +				&ret);
> > +	if (!comm_buf)
> > +		return EFI_EXIT(ret);
> > +
> > +	*max_variable_size = mm_query_info->max_variable_size;

[...]

> > +
> > +out:
> > +	free(comm_buf);
> > +	return EFI_EXIT(ret);
> > +}
> 
> The code below for runtime seems to be duplicated from efi_variable.c.
> 

Yes is it. 

> Let's keep it like this for now. We can look for a better solution once
> we investigate a runtime solution.

As we discussed on IRC, i *think* having runtime variables is doable.
The OP-TEE supplicant uses ioctls to communicate with the kernel and access the
RPMB once linux is up and running. So the kernel will take care of any
concurrent accesses.
The only problem I can think of now, is what happens to the variable *writes*
until the supplicant is up. Your patches shadowing variables into a different
memory location might be one solution to that problem.
We might not even need those, because of the internal of StMM. EDK2 expects byte
addressable memory for the variable access. Since the RPMB cant be memory
mapped, my EDK2 driver creates a memory copy of the variables and syncs the
variables on read/write with the hardware. As long as the linux EFI stub only
reads variables we might be fine using the code as -is.

In any case I agree, let's merge this as is, since accessing runtime variables
will return EFI_UNSUPPORTED and won't crash the system and I'll look into
runtime variables details afterwards.

> Overall the code looks good and clean (except for my nitpicky comments).

Thanks! I'll fix up the comments and send a v2.

> 
> Thanks a lot for all the effort you invested here.
> 


Regards
/Ilias