[2/5,v2] efi_loader: Implement EFI variable handling via OP-TEE

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 | 643 ++++++++++++++++++++++++++++++
 3 files changed, 656 insertions(+)
 create mode 100644 lib/efi_loader/efi_variable_tee.c

On 5/11/20 8:14 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 | 643 ++++++++++++++++++++++++++++++
>  3 files changed, 656 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..f8857bb81e72
> --- /dev/null
> +++ b/lib/efi_loader/efi_variable_tee.c
> @@ -0,0 +1,643 @@
> +// 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>
> +
> +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;
> +};
> +
> +/**
> + * get_connection() - Retrieve OP-TEE session for a specific UUID.
> + *
> + * @conn:   session buffer to fill
> + * Return:  status code
> + */
> +static int get_connection(struct mm_connection *conn)
> +{
> +	static const struct tee_optee_ta_uuid uuid = PTA_STMM_UUID;
> +	struct udevice *tee = NULL;
> +	struct tee_open_session_arg arg;
> +	int rc;
> +
> +	tee = tee_find_device(tee, NULL, NULL, NULL);
> +	if (!tee)
> +		return -ENODEV;
> +
> +	memset(&arg, 0, sizeof(arg));
> +	tee_optee_ta_uuid_to_octets(arg.uuid, &uuid);
> +	rc = tee_open_session(tee, &arg, 0, NULL);
> +	if (!rc) {
> +		conn->tee = tee;
> +		conn->session = arg.session;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
> + *
> + * @comm_buf:		locally allocted communcation buffer
> + * @dsize:		buffer size
> + * Return:		status code
> + */
> +static efi_status_t optee_mm_communicate(void *comm_buf, ulong dsize)
> +{
> +	ulong buf_size;
> +	efi_status_t ret;
> +	struct efi_mm_communicate_header *mm_hdr;
> +	struct mm_connection conn = { NULL, 0 };
> +	struct tee_invoke_arg arg;
> +	struct tee_param param[2];
> +	struct tee_shm *shm = NULL;
> +	int rc;
> +
> +	if (!comm_buf)
> +		return EFI_INVALID_PARAMETER;
> +
> +	mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> +	buf_size = mm_hdr->message_len + sizeof(efi_guid_t) + sizeof(size_t);
> +
> +	if (dsize != buf_size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	rc = get_connection(&conn);
> +	if (rc) {
> +		log_err("Unable to open OP-TEE session (err=%d)\n", rc);
> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	if (tee_shm_register(conn.tee, comm_buf, buf_size, 0, &shm)) {
> +		log_err("Unable to register shared memory\n");
> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.func = PTA_STMM_CMDID_COMMUNICATE;
> +	arg.session = conn.session;
> +
> +	memset(param, 0, sizeof(param));
> +	param[0].attr = TEE_PARAM_ATTR_TYPE_MEMREF_INOUT;
> +	param[0].u.memref.size = buf_size;
> +	param[0].u.memref.shm = shm;
> +	param[1].attr = TEE_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +
> +	rc = tee_invoke_func(conn.tee, &arg, 2, param);
> +	if (rc)
> +		return EFI_INVALID_PARAMETER;
> +	tee_shm_free(shm);
> +	tee_close_session(conn.tee, conn.session);
> +
> +	switch (param[1].u.value.a) {
> +	case ARM_SMC_MM_RET_SUCCESS:
> +		ret = EFI_SUCCESS;
> +		break;
> +
> +	case ARM_SMC_MM_RET_INVALID_PARAMS:
> +		ret = EFI_INVALID_PARAMETER;
> +		break;
> +
> +	case ARM_SMC_MM_RET_DENIED:
> +		ret = EFI_ACCESS_DENIED;
> +		break;
> +
> +	case ARM_SMC_MM_RET_NO_MEMORY:
> +		ret = EFI_OUT_OF_RESOURCES;
> +		break;
> +
> +	default:
> +		ret = EFI_ACCESS_DENIED;
> +	}
> +
> +	return ret;
> +}
> +
> +/**
> + * mm_communicate() - Adjust the cmonnucation buffer to StandAlonneMM and send
> + * it to OP-TEE
> + *
> + * @comm_buf:		locally allocted communcation buffer
> + * @dsize:		buffer size
> + * Return:		status code
> + */
> +static efi_status_t mm_communicate(u8 *comm_buf, efi_uintn_t dsize)
> +{
> +	efi_status_t ret;
> +	struct efi_mm_communicate_header *mm_hdr;
> +	struct smm_variable_communicate_header *var_hdr;
> +
> +	dsize += MM_COMMUNICATE_HEADER_SIZE + MM_VARIABLE_COMMUNICATE_SIZE;
> +	mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> +	var_hdr = (struct smm_variable_communicate_header *)mm_hdr->data;
> +
> +	ret = optee_mm_communicate(comm_buf, dsize);
> +	if (ret != EFI_SUCCESS) {
> +		log_err("%s failed!\n", __func__);
> +		return ret;
> +	}
> +
> +	return var_hdr->ret_status;
> +}
> +
> +/**
> + * setup_mm_hdr() -	Allocate a buffer for StandAloneMM and initialize the
> + *			header data.
> + *
> + * @dptr:		pointer address of the corresponding StandAloneMM
> + *			function
> + * @payload_size:	buffer size
> + * @func:		standAloneMM function number
> + * @ret:		EFI return code
> + * Return:		buffer or NULL
> + */
> +static u8 *setup_mm_hdr(void **dptr, efi_uintn_t payload_size,
> +			efi_uintn_t func, efi_status_t *ret)
> +{
> +	const efi_guid_t mm_var_guid = EFI_MM_VARIABLE_GUID;
> +	struct efi_mm_communicate_header *mm_hdr;
> +	struct smm_variable_communicate_header *var_hdr;
> +	u8 *comm_buf;
> +
> +	/* In the init function we initialize max_buffer_size with
> +	 * 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
> +	 */
> +	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 efi_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 smm_variable_communicate_header *)mm_hdr->data;
> +	var_hdr->function = func;
> +	if (dptr)
> +		*dptr = var_hdr->data;
> +	*ret = EFI_SUCCESS;
> +
> +	return comm_buf;
> +}
> +
> +/**
> + * get_max_payload() - Get variable payload size from StandAloneMM.
> + *
> + * @size:    size of the variable in storage
> + * Return:   status code
> + */
> +efi_status_t EFIAPI get_max_payload(efi_uintn_t *size)
> +{
> +	struct smm_variable_payload_size *var_payload = NULL;
> +	efi_uintn_t payload_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	if (!size)
> +		return EFI_INVALID_PARAMETER;
> +
> +	payload_size = sizeof(*var_payload);
> +	comm_buf = setup_mm_hdr((void **)&var_payload, payload_size,
> +				SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	*size = var_payload->size;
> +
> +out:
> +	free(comm_buf);
> +	return ret;
> +}
> +
> +/**
> + * efi_get_variable() - retrieve value of a UEFI variable
> + *
> + * This function implements the GetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @name:		name of the variable
> + * @guid:		vendor GUID
> + * @attr:		attributes of the variable
> + * @data_size:		size of the buffer to which the variable value is copied
> + * @data:		buffer to which the variable value is copied
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_get_variable(u16 *name, const efi_guid_t *guid,
> +				     u32 *attr, efi_uintn_t *data_size,
> +				     void *data)
> +{
> +	struct smm_variable_access *var_acc;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t name_size;
> +	efi_uintn_t tmp_dsize;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %p %p %p", name, guid, attr, data_size, data);
> +
> +	if (!name || !guid || !data_size)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Check payload size */
> +	name_size = u16_strsize(name);
> +	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Trim output buffer size */
> +	tmp_dsize = *data_size;
> +	if (name_size + tmp_dsize >
> +			max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> +		tmp_dsize = max_payload_size -
> +				MM_VARIABLE_ACCESS_HEADER_SIZE -
> +				name_size;
> +	}
> +
> +	/* Get communication buffer and initialize header */
> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				SMM_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 smm_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 that EFI_INVALID_PARAMETER should be returned if
there is no '\0' character in the first *variable_name_size words of
variable_name. I think we should add this test here instead of using
max(out_name_size, in_name_size) later in the code.

You are currently calling EFI_EXIT() in many places. Depending on the
level of code optimizations done by the compiler and the debug settings
this may need to unnecessary code size. I suggest to use a single exit
point in each of the functions, e.g.

	if (out_name_size > in_name_size) {
		ret = EFI_INVALID_PARAMETER;
		goto out;
	}
...
out:
	EFI_EXIT(ret);
}

Best regards

Heinrich

> +
> +	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);
> +	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> +				SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> +				&ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	/* Fill in contents */
> +	guidcpy(&var_getnext->guid, guid);
> +	var_getnext->name_size = out_name_size;
> +	memcpy(var_getnext->name, variable_name, in_name_size);
> +	if (out_name_size > in_name_size) {
> +		memset((u8 *)var_getnext->name + in_name_size, 0x0,
> +		       out_name_size - in_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 */
> +		*variable_name_size = var_getnext->name_size;
> +	}
> +	if (ret != EFI_SUCCESS)
> +		goto done;
> +
> +	guidcpy(guid, &var_getnext->guid);
> +	memcpy(variable_name, (u8 *)var_getnext->name,
> +	       var_getnext->name_size);
> +
> +done:
> +	free(comm_buf);
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_set_variable() - set value of a UEFI variable
> + *
> + * This function implements the SetVariable runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @name:		name of the variable
> + * @guid:		vendor GUID
> + * @attr:		attributes of the variable
> + * @data_size:		size of the buffer with the variable value
> + * @data:		buffer with the variable value
> + * Return:		status code
> + */
> +efi_status_t EFIAPI efi_set_variable(u16 *name, const efi_guid_t *guid,
> +				     u32 attr, efi_uintn_t data_size,
> +				     const void *data)
> +{
> +	struct smm_variable_access *var_acc;
> +	efi_uintn_t payload_size;
> +	efi_uintn_t name_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("\"%ls\" %pUl %x %zu %p", name, guid, attr, data_size, data);
> +
> +	if (!name || name[0] == 0 || !guid)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +	if (data_size > 0 && !data)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Check payload size */
> +	name_size = u16_strsize(name);
> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> +	if (payload_size > max_payload_size)
> +		return EFI_EXIT(EFI_INVALID_PARAMETER);
> +
> +	/* Get communication buffer and initialize header */
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	/* Fill in contents */
> +	guidcpy(&var_acc->guid, guid);
> +	var_acc->data_size = data_size;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attr;
> +	memcpy(var_acc->name, name, name_size);
> +	memcpy((u8 *)var_acc->name + name_size, data, data_size);
> +
> +	/* Communicate */
> +	ret = mm_communicate(comm_buf, payload_size);
> +	free(comm_buf);
> +
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_query_variable_info() - get information about EFI variables
> + *
> + * This function implements the QueryVariableInfo() runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @attributes:				bitmask to select variables to be
> + *					queried
> + * @maximum_variable_storage_size:	maximum size of storage area for the
> + *					selected variable types
> + * @remaining_variable_storage_size:	remaining size of storage are for the
> + *					selected variable types
> + * @maximum_variable_size:		maximum size of a variable of the
> + *					selected type
> + * Returns:				status code
> + */
> +efi_status_t EFIAPI __efi_runtime
> +efi_query_variable_info(u32 attributes, u64 *max_variable_storage_size,
> +			u64 *remain_variable_storage_size,
> +			u64 *max_variable_size)
> +{
> +	struct smm_variable_query_info *mm_query_info;
> +	efi_uintn_t payload_size;
> +	efi_status_t ret;
> +	u8 *comm_buf;
> +
> +	EFI_ENTRY("%x %p %p %p", attributes, max_variable_storage_size,
> +		  remain_variable_storage_size, max_variable_size);
> +
> +	payload_size = sizeof(*mm_query_info);
> +	comm_buf = setup_mm_hdr((void **)&mm_query_info, payload_size,
> +				SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
> +				&ret);
> +	if (!comm_buf)
> +		return EFI_EXIT(ret);
> +
> +	mm_query_info->attr = attributes;
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +	*max_variable_storage_size = mm_query_info->max_variable_storage;
> +	*remain_variable_storage_size =
> +			mm_query_info->remaining_variable_storage;
> +	*max_variable_size = mm_query_info->max_variable_size;
> +
> +out:
> +	free(comm_buf);
> +	return EFI_EXIT(ret);
> +}
> +
> +/**
> + * efi_get_variable_runtime() - runtime implementation of GetVariable()
> + *
> + * @variable_name:	name of the variable
> + * @guid:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer to which the variable value is copied
> + * @data:		buffer to which the variable value is copied
> + * Return:		status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_get_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> +			 u32 *attributes, efi_uintn_t *data_size, void *data)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_get_next_variable_name_runtime() - runtime implementation of
> + *					  GetNextVariable()
> + *
> + * @variable_name_size:	size of variable_name buffer in byte
> + * @variable_name:	name of uefi variable's name in u16
> + * @guid:		vendor's guid
> + * Return:              status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_get_next_variable_name_runtime(efi_uintn_t *variable_name_size,
> +				   u16 *variable_name, efi_guid_t *guid)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_query_variable_info() - get information about EFI variables
> + *
> + * This function implements the QueryVariableInfo() runtime service.
> + *
> + * See the Unified Extensible Firmware Interface (UEFI) specification for
> + * details.
> + *
> + * @attributes:				bitmask to select variables to be
> + *					queried
> + * @maximum_variable_storage_size:	maximum size of storage area for the
> + *					selected variable types
> + * @remaining_variable_storage_size:	remaining size of storage are for the
> + *					selected variable types
> + * @maximum_variable_size:		maximum size of a variable of the
> + *					selected type
> + * Return:				status code
> + */
> +efi_status_t EFIAPI __efi_runtime
> +efi_query_variable_info_runtime(u32 attributes, u64 *max_variable_storage_size,
> +				u64 *remain_variable_storage_size,
> +				u64 *max_variable_size)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_set_variable_runtime() - runtime implementation of SetVariable()
> + *
> + * @variable_name:	name of the variable
> + * @guid:		vendor GUID
> + * @attributes:		attributes of the variable
> + * @data_size:		size of the buffer with the variable value
> + * @data:		buffer with the variable value
> + * Return:		status code
> + */
> +static efi_status_t __efi_runtime EFIAPI
> +efi_set_variable_runtime(u16 *variable_name, const efi_guid_t *guid,
> +			 u32 attributes, efi_uintn_t data_size,
> +			 const void *data)
> +{
> +	return EFI_UNSUPPORTED;
> +}
> +
> +/**
> + * efi_variables_boot_exit_notify() - notify ExitBootServices() is called
> + */
> +void efi_variables_boot_exit_notify(void)
> +{
> +	u8 *comm_buf;
> +	efi_status_t ret;
> +
> +	comm_buf = setup_mm_hdr(NULL, 0,
> +				SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE, &ret);
> +	if (comm_buf)
> +		ret = mm_communicate(comm_buf, 0);
> +	else
> +		ret = EFI_NOT_FOUND;
> +
> +	if (ret != EFI_SUCCESS)
> +		log_err("Unable to notify StMM for ExitBootServices\n");
> +	free(comm_buf);
> +
> +	/* Update runtime service table */
> +	efi_runtime_services.query_variable_info =
> +			efi_query_variable_info_runtime;
> +	efi_runtime_services.get_variable = efi_get_variable_runtime;
> +	efi_runtime_services.get_next_variable_name =
> +			efi_get_next_variable_name_runtime;
> +	efi_runtime_services.set_variable = efi_set_variable_runtime;
> +	efi_update_table_header_crc32(&efi_runtime_services.hdr);
> +}
> +
> +/**
> + * efi_init_variables() - initialize variable services
> + *
> + * Return:	status code
> + */
> +efi_status_t efi_init_variables(void)
> +{
> +	efi_status_t ret;
> +
> +	ret = get_max_payload(&max_payload_size);
> +	if (ret != EFI_SUCCESS)
> +		return ret;
> +
> +	max_buffer_size = MM_COMMUNICATE_HEADER_SIZE +
> +			  MM_VARIABLE_COMMUNICATE_SIZE +
> +			  max_payload_size;
> +
> +	return EFI_SUCCESS;
> +}
>

On Wed, May 13, 2020 at 08:14:19AM +0200, Heinrich Schuchardt wrote:
> On 5/11/20 8:14 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
> > +

[...]

> > +	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 that EFI_INVALID_PARAMETER should be returned if
> there is no '\0' character in the first *variable_name_size words of
> variable_name. I think we should add this test here instead of using
> max(out_name_size, in_name_size) later in the code.

Ok I'll have a look 

> 
> You are currently calling EFI_EXIT() in many places. Depending on the
> level of code optimizations done by the compiler and the debug settings
> this may need to unnecessary code size. I suggest to use a single exit
> point in each of the functions, e.g.
> 
> 	if (out_name_size > in_name_size) {
> 		ret = EFI_INVALID_PARAMETER;
> 		goto out;
> 	}
> ...
> out:
> 	EFI_EXIT(ret);

Fair enough, most of U-Boot is coded that way anyway, might as well have a
common approach.

I'll post a v3 with the changes, so you can do your testing directly in that 


Regards
/Ilias

On 11.05.20 20:14, 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 | 643 ++++++++++++++++++++++++++++++
>  3 files changed, 656 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

Dear Ilias,

at least CONFIG_TEE=y is needed as dependency. Otherwise compiling fails.

If OP-TEE is not found:

=> bootefi hello
Unable to open OP-TEE session (err=-19)
mm_communicate failed!
Error: Cannot initialize UEFI sub-system, r = 3

I think it could be allowable to boot without variable support unless we
are in secure boot audit mode. So if CONFIG_EFI_SECURE_BOOT=n, maybe we
should be less strict. I guess it will end up in weighing user
friendliness against complexity. What are your ideas?

Best regards

Heinrich

On Fri, May 15, 2020 at 01:55:35PM +0200, Heinrich Schuchardt wrote:
> On 11.05.20 20:14, 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 | 643 ++++++++++++++++++++++++++++++
> >  3 files changed, 656 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
> 
> Dear Ilias,
> 
> at least CONFIG_TEE=y is needed as dependency. Otherwise compiling fails.

Ok I'll add that

> 
> If OP-TEE is not found:
> 
> => bootefi hello
> Unable to open OP-TEE session (err=-19)
> mm_communicate failed!
> Error: Cannot initialize UEFI sub-system, r = 3
> 
> I think it could be allowable to boot without variable support unless we
> are in secure boot audit mode. So if CONFIG_EFI_SECURE_BOOT=n, maybe we
> should be less strict. I guess it will end up in weighing user
> friendliness against complexity. What are your ideas?

I don't have any strong opinions on that tbh. But I think i'd choose the
stricter approach. If it's a UEFI boot/whatever, variables must be initialized
and accessible.

Regards
/Ilias
> 
> Best regards
> 
> Heinrich