[RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Masahisa Kojima]

This RFC series introduces the op-tee based EFI Runtime Variable
Service.

The eMMC device is typically owned by the non-secure world(linux in
this case). There is an existing solution utilizing eMMC RPMB partition
for EFI Variables, it is implemented by interacting with
OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
and tee-supplicant. The last piece is the tee-based variable access
driver to interact with OP-TEE and StandaloneMM.

Masahisa Kojima (2):
  efi: expose efivar generic ops register function
  tee: Add op-tee helper functions for variable access

 drivers/firmware/efi/efi.c           |  12 +
 drivers/tee/optee/Kconfig            |  10 +
 drivers/tee/optee/Makefile           |   1 +
 drivers/tee/optee/mm_communication.h | 249 +++++++++++
 drivers/tee/optee/optee_private.h    |   5 +-
 drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
 drivers/tee/tee_core.c               |  23 ++
 include/linux/efi.h                  |   4 +
 include/linux/tee_drv.h              |  23 ++
 9 files changed, 924 insertions(+), 1 deletion(-)
 create mode 100644 drivers/tee/optee/mm_communication.h
 create mode 100644 drivers/tee/optee/optee_stmm_efi.c

-- 
2.30.2

[RFC PATCH 1/2] efi: expose efivar generic ops register function

[Masahisa Kojima]

This is a preparation for supporting efivar operations
provided by other than efi subsystem.
Both register and unregister functions are exposed
so that non-efi subsystem can revert the efi generic
operation.

Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/firmware/efi/efi.c | 12 ++++++++++++
 include/linux/efi.h        |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 52146f95d58e..4e576b62c170 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -227,6 +227,18 @@ static void generic_ops_unregister(void)
 	efivars_unregister(&generic_efivars);
 }
 
+void efivars_generic_ops_register(void)
+{
+	generic_ops_register();
+}
+EXPORT_SYMBOL_GPL(efivars_generic_ops_register);
+
+void efivars_generic_ops_unregister(void)
+{
+	generic_ops_unregister();
+}
+EXPORT_SYMBOL_GPL(efivars_generic_ops_unregister);
+
 #ifdef CONFIG_EFI_CUSTOM_SSDT_OVERLAYS
 #define EFIVAR_SSDT_NAME_MAX	16UL
 static char efivar_ssdt[EFIVAR_SSDT_NAME_MAX] __initdata;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 1a1adc8d3ba3..5e301c00e9b0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1332,4 +1332,7 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
 	return xen_efi_config_table_is_usable(guid, table);
 }
 
+void efivars_generic_ops_register(void);
+void efivars_generic_ops_unregister(void);
+
 #endif /* _LINUX_EFI_H */
-- 
2.30.2

[RFC PATCH 2/2] tee: Add op-tee helper functions for variable access

[Masahisa Kojima]

When the flash is not owned by the non-secure world, accessing the EFI
variables is straightforward and done via EFI Runtime Variable Services.
In this case, critical variables for system integrity and security
are normally stored in the dedicated secure storage and only accessible
from the secure world.

On the other hand, the small embedded devices don't have the special
dedicated secure storage. The eMMC device with an RPMB partition is
becoming more common, we can use an RPMB partition to store the
EFI Variables.

The eMMC device is typically owned by the non-secure world(linux in
this case). There is an existing solution utilizing eMMC RPMB partition
for EFI Variables, it is implemented by interacting with
OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
and tee-supplicant. The last piece is the tee-based variable access
driver to interact with OP-TEE and StandaloneMM.

So let's add the kernel functions needed.

This feature is implemented as a kernel module.
StMM PTA has TA_FLAG_DEVICE_ENUM_SUPP flag when registered to OP-TEE
so that this optee_stmm_efi module is probed after tee-supplicant starts,
since "SetVariable" EFI Runtime Variable Service requires to
interact with tee-supplicant.

Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/tee/optee/Kconfig            |  10 +
 drivers/tee/optee/Makefile           |   1 +
 drivers/tee/optee/mm_communication.h | 249 +++++++++++
 drivers/tee/optee/optee_private.h    |   5 +-
 drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
 drivers/tee/tee_core.c               |  23 ++
 include/linux/efi.h                  |   1 +
 include/linux/tee_drv.h              |  23 ++
 8 files changed, 909 insertions(+), 1 deletion(-)
 create mode 100644 drivers/tee/optee/mm_communication.h
 create mode 100644 drivers/tee/optee/optee_stmm_efi.c

diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
index f121c224e682..a0b699977e51 100644
--- a/drivers/tee/optee/Kconfig
+++ b/drivers/tee/optee/Kconfig
@@ -7,3 +7,13 @@ config OPTEE
 	help
 	  This implements the OP-TEE Trusted Execution Environment (TEE)
 	  driver.
+
+config EFI_STMM_OPTEE
+	tristate "OP-TEE based EFI runtime variable service driver"
+	depends on OPTEE
+	help
+	  This  driver provides support for OP-TEE based EFI runtime
+	  variable service driver.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called optee_stmm_efi.
diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
index a6eff388d300..9acb0b2de8fe 100644
--- a/drivers/tee/optee/Makefile
+++ b/drivers/tee/optee/Makefile
@@ -8,6 +8,7 @@ optee-objs += supp.o
 optee-objs += device.o
 optee-objs += smc_abi.o
 optee-objs += ffa_abi.o
+obj-$(CONFIG_EFI_STMM_OPTEE) += optee_stmm_efi.o
 
 # for tracing framework to find optee_trace.h
 CFLAGS_smc_abi.o := -I$(src)
diff --git a/drivers/tee/optee/mm_communication.h b/drivers/tee/optee/mm_communication.h
new file mode 100644
index 000000000000..23e6672991ef
--- /dev/null
+++ b/drivers/tee/optee/mm_communication.h
@@ -0,0 +1,249 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ *  Headers for EFI variable service via StandAloneMM, EDK2 application running
+ *  in OP-TEE
+ *
+ *  Copyright (c) 2017, Intel Corporation. All rights reserved.
+ *  Copyright (C) 2020 Linaro Ltd.
+ */
+
+#ifndef _MM_COMMUNICATION_H_
+#define _MM_COMMUNICATION_H_
+
+/*
+ * Interface to the pseudo Trusted Application (TA), which provides a
+ * communication channel with the Standalone MM (Management Mode)
+ * Secure Partition running at Secure-EL0
+ */
+
+#define PTA_STMM_CMDID_COMMUNICATE 0
+
+/* OP-TEE is using big endian GUIDs while UEFI uses little endian ones */
+#define PTA_STMM_UUID \
+	UUID_INIT(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)
+
+/* Defined in EDK2 MdePkg/Include/Protocol/MmCommunication.h */
+
+/**
+ * struct efi_mm_communicate_header - Header used for SMM variable communication
+
+ * @header_guid:  header use for disambiguation of content
+ * @message_len:  length of the message. Does not include the size of the
+ *                header
+ * @data:         payload of the message
+ *
+ * Defined in EDK2 as EFI_MM_COMMUNICATE_HEADER.
+ * To avoid confusion in interpreting frames, the communication buffer should
+ * always begin with efi_mm_communicate_header.
+ */
+struct efi_mm_communicate_header {
+	efi_guid_t header_guid;
+	size_t     message_len;
+	u8         data[];
+} __packed;
+
+#define MM_COMMUNICATE_HEADER_SIZE \
+	(sizeof(struct efi_mm_communicate_header))
+
+/* Defined in EDK2 ArmPkg/Include/IndustryStandard/ArmMmSvc.h */
+
+/* SPM return error codes */
+#define ARM_SVC_SPM_RET_SUCCESS               0
+#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
+#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
+#define ARM_SVC_SPM_RET_DENIED               -3
+#define ARM_SVC_SPM_RET_NO_MEMORY            -5
+
+/* Defined in EDK2 MdeModulePkg/Include/Guid/SmmVariableCommon.h */
+
+#define SMM_VARIABLE_FUNCTION_GET_VARIABLE  1
+/*
+ * The payload for this function is
+ * SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
+ */
+#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME  2
+/*
+ * The payload for this function is SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
+ */
+#define SMM_VARIABLE_FUNCTION_SET_VARIABLE  3
+/*
+ * The payload for this function is
+ * SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
+ */
+#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO  4
+/*
+ * It is a notify event, no extra payload for this function.
+ */
+#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT  5
+/*
+ * It is a notify event, no extra payload for this function.
+ */
+#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE  6
+/*
+ * The payload for this function is VARIABLE_INFO_ENTRY.
+ * The GUID in EFI_SMM_COMMUNICATE_HEADER is gEfiSmmVariableProtocolGuid.
+ */
+#define SMM_VARIABLE_FUNCTION_GET_STATISTICS  7
+/*
+ * The payload for this function is SMM_VARIABLE_COMMUNICATE_LOCK_VARIABLE
+ */
+#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE   8
+
+#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET  9
+
+#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET  10
+
+#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE  11
+/*
+ * The payload for this function is
+ * SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
+ */
+#define SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT 12
+
+#define SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE  13
+/*
+ * The payload for this function is
+ * SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO
+ */
+#define SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO  14
+
+/**
+ * struct smm_variable_communicate_header - Used for SMM variable communication
+
+ * @function:     function to call in Smm.
+ * @ret_status:   return status
+ * @data:         payload
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_HEADER.
+ */
+struct smm_variable_communicate_header {
+	size_t  function;
+	efi_status_t ret_status;
+	u8 data[];
+};
+
+#define MM_VARIABLE_COMMUNICATE_SIZE \
+	(sizeof(struct smm_variable_communicate_header))
+
+/**
+ * struct smm_variable_access - Used to communicate with StMM by
+ *                              SetVariable and GetVariable.
+
+ * @guid:         vendor GUID
+ * @data_size:    size of EFI variable data
+ * @name_size:    size of EFI name
+ * @attr:         attributes
+ * @name:         variable name
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
+ *
+ */
+struct smm_variable_access {
+	efi_guid_t  guid;
+	size_t data_size;
+	size_t name_size;
+	u32 attr;
+	u16 name[];
+};
+
+#define MM_VARIABLE_ACCESS_HEADER_SIZE \
+	(sizeof(struct smm_variable_access))
+/**
+ * struct smm_variable_payload_size - Used to get the max allowed
+ *                                    payload used in StMM.
+ *
+ * @size:  size to fill in
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE.
+ *
+ */
+struct smm_variable_payload_size {
+	size_t size;
+};
+
+/**
+ * struct smm_variable_getnext - Used to communicate with StMM for
+ *                               GetNextVariableName.
+ *
+ * @guid:       vendor GUID
+ * @name_size:  size of the name of the variable
+ * @name:       variable name
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
+ */
+struct smm_variable_getnext {
+	efi_guid_t  guid;
+	size_t name_size;
+	u16         name[];
+};
+
+#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
+	(sizeof(struct smm_variable_getnext))
+
+/**
+ * struct smm_variable_query_info - Used to communicate with StMM for
+ *                                  QueryVariableInfo.
+ *
+ * @max_variable_storage:        max available storage
+ * @remaining_variable_storage:  remaining available storage
+ * @max_variable_size:           max variable supported size
+ * @attr:                        attributes to query storage for
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
+ */
+struct smm_variable_query_info {
+	u64 max_variable_storage;
+	u64 remaining_variable_storage;
+	u64 max_variable_size;
+	u32 attr;
+};
+
+#define VAR_CHECK_VARIABLE_PROPERTY_REVISION 0x0001
+#define VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY BIT(0)
+/**
+ * struct var_check_property - Used to store variable properties in StMM
+ *
+ * @revision:   magic revision number for variable property checking
+ * @property:   properties mask for the variable used in StMM.
+ *              Currently RO flag is supported
+ * @attributes: variable attributes used in StMM checking when properties
+ *              for a variable are enabled
+ * @minsize:    minimum allowed size for variable payload checked against
+ *              smm_variable_access->datasize in StMM
+ * @maxsize:    maximum allowed size for variable payload checked against
+ *              smm_variable_access->datasize in StMM
+ *
+ * Defined in EDK2 as VAR_CHECK_VARIABLE_PROPERTY.
+ */
+struct var_check_property {
+	u16 revision;
+	u16 property;
+	u32 attributes;
+	size_t minsize;
+	size_t maxsize;
+};
+
+/**
+ * struct smm_variable_var_check_property - Used to communicate variable
+ *                                          properties with StMM
+ *
+ * @guid:       vendor GUID
+ * @name_size:  size of EFI name
+ * @property:   variable properties struct
+ * @name:       variable name
+ *
+ * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY.
+ */
+struct smm_variable_var_check_property {
+	efi_guid_t guid;
+	size_t name_size;
+	struct var_check_property property;
+	u16 name[];
+};
+
+#endif /* _MM_COMMUNICATION_H_ */
diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
index 04ae58892608..10af910e0dce 100644
--- a/drivers/tee/optee/optee_private.h
+++ b/drivers/tee/optee/optee_private.h
@@ -12,6 +12,7 @@
 #include <linux/tee_drv.h>
 #include <linux/types.h>
 #include "optee_msg.h"
+#include "optee_private.h"
 
 #define DRIVER_NAME "optee"
 
@@ -19,6 +20,7 @@
 
 /* Some Global Platform error codes used in this driver */
 #define TEEC_SUCCESS			0x00000000
+#define TEEC_ERROR_EXCESS_DATA		0xFFFF0004
 #define TEEC_ERROR_BAD_PARAMETERS	0xFFFF0006
 #define TEEC_ERROR_NOT_SUPPORTED	0xFFFF000A
 #define TEEC_ERROR_COMMUNICATION	0xFFFF000E
@@ -250,7 +252,6 @@ void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
 			       int (*shm_unregister)(struct tee_context *ctx,
 						     struct tee_shm *shm));
 
-
 void optee_remove_common(struct optee *optee);
 int optee_open(struct tee_context *ctx, bool cap_memref_null);
 void optee_release(struct tee_context *ctx);
@@ -322,4 +323,6 @@ void optee_smc_abi_unregister(void);
 int optee_ffa_abi_register(void);
 void optee_ffa_abi_unregister(void);
 
+int optee_efivar_ops_init(struct tee_context *ctx);
+
 #endif /*OPTEE_PRIVATE_H*/
diff --git a/drivers/tee/optee/optee_stmm_efi.c b/drivers/tee/optee/optee_stmm_efi.c
new file mode 100644
index 000000000000..6dcf1eb4b96c
--- /dev/null
+++ b/drivers/tee/optee/optee_stmm_efi.c
@@ -0,0 +1,598 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI variable service via OP-TEE
+ *
+ *  Copyright (C) 2022 Linaro
+ */
+
+#include <linux/efi.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/tee.h>
+#include <linux/tee_drv.h>
+#include <linux/ucs2_string.h>
+#include "mm_communication.h"
+#include "optee_private.h"
+
+static struct efivars optee_efivars;
+static struct efivar_operations optee_ops;
+
+static size_t max_buffer_size; /* comm + var + func + data */
+static size_t max_payload_size; /* func + data */
+
+struct mm_connection {
+	struct tee_context *ctx;
+	u32 session;
+};
+
+/* UUID of the stmm PTA */
+static const struct tee_client_device_id optee_stmm_efi_id_table[] = {
+	{PTA_STMM_UUID},
+	{}
+};
+
+static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+/**
+ * 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)
+{
+	struct tee_context *ctx = NULL;
+	struct tee_ioctl_open_session_arg arg;
+	int rc;
+
+	memset(&arg, 0, sizeof(arg));
+
+	/* Open context with OP-TEE driver */
+	ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
+	if (IS_ERR(ctx))
+		return -ENODEV;
+
+	export_uuid(arg.uuid, &optee_stmm_efi_id_table[0].uuid);
+	rc = tee_client_open_session(ctx, &arg, NULL);
+	if (!rc) {
+		conn->ctx = ctx;
+		conn->session = arg.session;
+	}
+
+	return rc;
+}
+
+/**
+ * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
+ *
+ * @comm_buf:		locally allocated communcation buffer
+ * @dsize:		buffer size
+ * Return:		status code
+ */
+static efi_status_t optee_mm_communicate(void *comm_buf, size_t dsize)
+{
+	size_t buf_size;
+	efi_status_t ret;
+	struct efi_mm_communicate_header *mm_hdr;
+	struct mm_connection conn = { NULL, 0 };
+	struct tee_ioctl_invoke_arg arg;
+	struct tee_param param[4];
+	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) {
+		pr_err("Unable to open OP-TEE session (err=%d)\n", rc);
+		return EFI_UNSUPPORTED;
+	}
+
+	shm = tee_shm_register_kernel_buf(conn.ctx, comm_buf, buf_size);
+	if (IS_ERR(shm)) {
+		pr_err("Unable to register shared memory\n");
+		tee_client_close_session(conn.ctx, conn.session);
+		return EFI_UNSUPPORTED;
+	}
+
+	memset(&arg, 0, sizeof(arg));
+	arg.func = PTA_STMM_CMDID_COMMUNICATE;
+	arg.session = conn.session;
+	arg.num_params = 4;
+
+	memset(param, 0, sizeof(param));
+	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
+	param[0].u.memref.size = buf_size;
+	param[0].u.memref.shm = shm;
+	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
+	param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
+	param[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
+
+	rc = tee_client_invoke_func(conn.ctx, &arg, param);
+	tee_shm_free(shm);
+	/* even if close session fails the session will be invalidaded */
+	tee_client_close_session(conn.ctx, conn.session);
+	if (rc)
+		return EFI_DEVICE_ERROR;
+	if (arg.ret == TEEC_ERROR_EXCESS_DATA)
+		pr_err("Variable payload too large\n");
+	if (arg.ret != TEEC_SUCCESS)
+		return EFI_DEVICE_ERROR;
+
+	switch (param[1].u.value.a) {
+	case ARM_SVC_SPM_RET_SUCCESS:
+		ret = EFI_SUCCESS;
+		break;
+
+	case ARM_SVC_SPM_RET_INVALID_PARAMS:
+		ret = EFI_INVALID_PARAMETER;
+		break;
+
+	case ARM_SVC_SPM_RET_DENIED:
+		ret = EFI_ACCESS_DENIED;
+		break;
+
+	case ARM_SVC_SPM_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 allocated communcation buffer
+ * @dsize:		buffer size
+ * Return:		status code
+ */
+static efi_status_t mm_communicate(u8 *comm_buf, size_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) {
+		pr_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, size_t payload_size, size_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 = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
+				   MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
+			   GFP_KERNEL);
+	if (!comm_buf) {
+		*ret = EFI_OUT_OF_RESOURCES;
+		return NULL;
+	}
+
+	mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
+	memcpy(&mm_hdr->header_guid, &mm_var_guid, sizeof(mm_hdr->header_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
+ */
+static efi_status_t get_max_payload(size_t *size)
+{
+	struct smm_variable_payload_size *var_payload = NULL;
+	size_t payload_size;
+	u8 *comm_buf = NULL;
+	efi_status_t ret;
+
+	if (!size) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	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)
+		goto out;
+
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	/* Make sure the buffer is big enough for storing variables */
+	if (var_payload->size < MM_VARIABLE_ACCESS_HEADER_SIZE + 0x20) {
+		ret = EFI_DEVICE_ERROR;
+		goto out;
+	}
+	*size = var_payload->size;
+	/*
+	 * There seems to be a bug in EDK2 miscalculating the boundaries and
+	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
+	 * it up here to ensure backwards compatibility with older versions
+	 * (cf. StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c.
+	 * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the
+	 * flexible array member).
+	 *
+	 * size is guaranteed to be > 2 due to checks on the beginning.
+	 */
+	*size -= 2;
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t get_property_int(u16 *name, size_t name_size,
+				     const efi_guid_t *vendor,
+				     struct var_check_property *var_property)
+{
+	struct smm_variable_var_check_property *smm_property;
+	size_t payload_size;
+	u8 *comm_buf = NULL;
+	efi_status_t ret;
+
+	memset(var_property, 0, sizeof(*var_property));
+	payload_size = sizeof(*smm_property) + name_size;
+	if (payload_size > max_payload_size) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	comm_buf = setup_mm_hdr(
+		(void **)&smm_property, payload_size,
+		SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
+	if (!comm_buf)
+		goto out;
+
+	memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
+	smm_property->name_size = name_size;
+	memcpy(smm_property->name, name, name_size);
+
+	ret = mm_communicate(comm_buf, payload_size);
+	/*
+	 * Currently only R/O property is supported in StMM.
+	 * Variables that are not set to R/O will not set the property in StMM
+	 * and the call will return EFI_NOT_FOUND. We are setting the
+	 * properties to 0x0 so checking against that is enough for the
+	 * EFI_NOT_FOUND case.
+	 */
+	if (ret == EFI_NOT_FOUND)
+		ret = EFI_SUCCESS;
+	if (ret != EFI_SUCCESS)
+		goto out;
+	memcpy(var_property, &smm_property->property, sizeof(*var_property));
+
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t optee_get_variable(u16 *name, efi_guid_t *vendor,
+				       u32 *attributes,
+				       unsigned long *data_size, void *data)
+{
+	struct var_check_property var_property;
+	struct smm_variable_access *var_acc;
+	size_t payload_size;
+	size_t name_size;
+	size_t tmp_dsize;
+	u8 *comm_buf = NULL;
+	efi_status_t ret;
+
+	if (!name || !vendor || !data_size) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
+	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	/* 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)
+		goto out;
+
+	/* Fill in contents */
+	memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
+	var_acc->data_size = tmp_dsize;
+	var_acc->name_size = name_size;
+	var_acc->attr = attributes ? *attributes : 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 out;
+
+	ret = get_property_int(name, name_size, vendor, &var_property);
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	if (attributes)
+		*attributes = var_acc->attr;
+
+	if (data)
+		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
+		       var_acc->data_size);
+	else
+		ret = EFI_INVALID_PARAMETER;
+
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t optee_get_next_variable(unsigned long *name_size,
+					    efi_char16_t *name,
+					    efi_guid_t *guid)
+{
+	struct smm_variable_getnext *var_getnext;
+	size_t payload_size;
+	size_t out_name_size;
+	size_t in_name_size;
+	u8 *comm_buf = NULL;
+	efi_status_t ret;
+
+	if (!name_size || !name || !guid) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	out_name_size = *name_size;
+	in_name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
+
+	if (out_name_size < in_name_size) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	if (in_name_size >
+	    max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	/* Trim output buffer size */
+	if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
+		out_name_size =
+			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
+
+	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
+	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
+				SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
+				&ret);
+	if (!comm_buf)
+		goto out;
+
+	/* Fill in contents */
+	memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
+	var_getnext->name_size = out_name_size;
+	memcpy(var_getnext->name, name, 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 */
+		*name_size = var_getnext->name_size;
+	}
+	if (ret != EFI_SUCCESS)
+		goto out;
+
+	memcpy(guid, &var_getnext->guid, sizeof(*guid));
+	memcpy(name, var_getnext->name, var_getnext->name_size);
+
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t optee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
+				       u32 attributes, unsigned long data_size,
+				       void *data)
+{
+	efi_status_t ret;
+	struct var_check_property var_property;
+	struct smm_variable_access *var_acc;
+	size_t payload_size;
+	size_t name_size;
+	u8 *comm_buf = NULL;
+
+	if (!name || name[0] == 0 || !vendor) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	if (data_size > 0 && !data) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	/* Check payload size */
+	name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
+	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
+	if (payload_size > max_payload_size) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+
+	/*
+	 * Allocate the buffer early, before switching to RW (if needed)
+	 * so we won't need to account for any failures in reading/setting
+	 * the properties, if the allocation fails
+	 */
+	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
+				SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
+	if (!comm_buf)
+		goto out;
+
+	/*
+	 * The API has the ability to override RO flags. If no RO check was
+	 * requested switch the variable to RW for the duration of this call
+	 */
+	ret = get_property_int(name, name_size, vendor, &var_property);
+	if (ret != EFI_SUCCESS) {
+		pr_err("Getting variable property failed\n");
+		goto out;
+	}
+
+	if (var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY) {
+		ret = EFI_WRITE_PROTECTED;
+		goto out;
+	}
+
+	/* Fill in contents */
+	memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
+	var_acc->data_size = data_size;
+	var_acc->name_size = name_size;
+	var_acc->attr = attributes;
+	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);
+	pr_info("Set Variable %s %d %lx\n", __FILE__, __LINE__, ret);
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static int optee_stmm_efi_probe(struct device *dev)
+{
+	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;
+
+	optee_ops.get_variable = optee_get_variable;
+	optee_ops.get_next_variable = optee_get_next_variable;
+	optee_ops.set_variable = optee_set_variable;
+	/* TODO: support nonblocking variant */
+	optee_ops.set_variable_nonblocking = NULL;
+	/* set NULL, always return EFI_SUCCESS by efi_query_variable_store() */
+	optee_ops.query_variable_store = NULL;
+
+	tee_register_efivar_ops(&optee_efivars, &optee_ops);
+
+	return 0;
+}
+
+static int optee_stmm_efi_remove(struct device *dev)
+{
+	tee_unregister_efivar_ops(&optee_efivars);
+
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(tee, optee_stmm_efi_id_table);
+
+static struct tee_client_driver optee_stmm_efi_driver = {
+	.id_table	= optee_stmm_efi_id_table,
+	.driver		= {
+		.name		= "optee-stmm-efi",
+		.bus		= &tee_bus_type,
+		.probe		= optee_stmm_efi_probe,
+		.remove		= optee_stmm_efi_remove,
+	},
+};
+
+static int __init optee_stmm_efi_mod_init(void)
+{
+	return driver_register(&optee_stmm_efi_driver.driver);
+}
+
+static void __exit optee_stmm_efi_mod_exit(void)
+{
+	driver_unregister(&optee_stmm_efi_driver.driver);
+}
+
+module_init(optee_stmm_efi_mod_init);
+module_exit(optee_stmm_efi_mod_exit);
+
+MODULE_LICENSE("GPL v2");
+MODULE_AUTHOR("Ilias Apalodimas <ilias.apalodimas@linaro.org>");
+MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>");
+MODULE_DESCRIPTION("OP-TEE based EFI runtime variable service driver");
diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
index 98da206cd761..ac46274844a3 100644
--- a/drivers/tee/tee_core.c
+++ b/drivers/tee/tee_core.c
@@ -7,6 +7,7 @@
 
 #include <linux/cdev.h>
 #include <linux/cred.h>
+#include <linux/efi.h>
 #include <linux/fs.h>
 #include <linux/idr.h>
 #include <linux/module.h>
@@ -1263,6 +1264,28 @@ static void __exit tee_exit(void)
 	tee_class = NULL;
 }
 
+void tee_register_efivar_ops(struct efivars *tee_efivars,
+			     struct efivar_operations *ops)
+{
+	/*
+	 * If the firmware EFI runtime services support SetVariable(),
+	 * tee-based EFI variable services are not used.
+	 */
+	if (!efivar_supports_writes()) {
+		pr_info("Use tee-based EFI runtime variable services\n");
+		efivars_generic_ops_unregister();
+		efivars_register(tee_efivars, ops);
+	}
+}
+EXPORT_SYMBOL_GPL(tee_register_efivar_ops);
+
+void tee_unregister_efivar_ops(struct efivars *tee_efivars)
+{
+	efivars_unregister(tee_efivars);
+	efivars_generic_ops_register();
+}
+EXPORT_SYMBOL_GPL(tee_unregister_efivar_ops);
+
 subsys_initcall(tee_init);
 module_exit(tee_exit);
 
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5e301c00e9b0..14d4aa83ce60 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -39,6 +39,7 @@
 #define EFI_WRITE_PROTECTED	( 8 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_OUT_OF_RESOURCES	( 9 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_NOT_FOUND		(14 | (1UL << (BITS_PER_LONG-1)))
+#define EFI_ACCESS_DENIED	(15 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_TIMEOUT		(18 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_ABORTED		(21 | (1UL << (BITS_PER_LONG-1)))
 #define EFI_SECURITY_VIOLATION	(26 | (1UL << (BITS_PER_LONG-1)))
diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
index 17eb1c5205d3..def4ea6212ee 100644
--- a/include/linux/tee_drv.h
+++ b/include/linux/tee_drv.h
@@ -7,6 +7,7 @@
 #define __TEE_DRV_H
 
 #include <linux/device.h>
+#include <linux/efi.h>
 #include <linux/idr.h>
 #include <linux/kref.h>
 #include <linux/list.h>
@@ -507,4 +508,26 @@ struct tee_context *teedev_open(struct tee_device *teedev);
  */
 void teedev_close_context(struct tee_context *ctx);
 
+/**
+ * tee_register_efivar_ops() - register the efivar ops
+ * @tee_efivars:	pointer to efivars structure
+ * @ops:		pointer to contain the efivar operation
+ *
+ * This function registers the tee-based efivar operation as an
+ * EFI Runtime Service.
+ *
+ */
+void tee_register_efivar_ops(struct efivars *tee_efivars,
+			     struct efivar_operations *ops);
+
+/**
+ * tee_unregister_efivar_ops() - unregister the efivar ops
+ * @tee_efivars:	pointer to efivars structure
+ *
+ * This function unregisters the tee-based efivar operation
+ * and reverts to the generic operation.
+ *
+ */
+void tee_unregister_efivar_ops(struct efivars *tee_efivars);
+
 #endif /*__TEE_DRV_H*/
-- 
2.30.2

Re: [RFC PATCH 2/2] tee: Add op-tee helper functions for variable access

[Jens Wiklander]

On Thu, Jan 26, 2023 at 10:21:19PM +0900, Masahisa Kojima wrote:
> When the flash is not owned by the non-secure world, accessing the EFI
> variables is straightforward and done via EFI Runtime Variable Services.
> In this case, critical variables for system integrity and security
> are normally stored in the dedicated secure storage and only accessible
> from the secure world.
> 
> On the other hand, the small embedded devices don't have the special
> dedicated secure storage. The eMMC device with an RPMB partition is
> becoming more common, we can use an RPMB partition to store the
> EFI Variables.
> 
> The eMMC device is typically owned by the non-secure world(linux in
> this case). There is an existing solution utilizing eMMC RPMB partition
> for EFI Variables, it is implemented by interacting with
> OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> and tee-supplicant. The last piece is the tee-based variable access
> driver to interact with OP-TEE and StandaloneMM.
> 
> So let's add the kernel functions needed.
> 
> This feature is implemented as a kernel module.
> StMM PTA has TA_FLAG_DEVICE_ENUM_SUPP flag when registered to OP-TEE
> so that this optee_stmm_efi module is probed after tee-supplicant starts,
> since "SetVariable" EFI Runtime Variable Service requires to
> interact with tee-supplicant.
> 
> Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  drivers/tee/optee/Kconfig            |  10 +
>  drivers/tee/optee/Makefile           |   1 +
>  drivers/tee/optee/mm_communication.h | 249 +++++++++++
>  drivers/tee/optee/optee_private.h    |   5 +-
>  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
>  drivers/tee/tee_core.c               |  23 ++
>  include/linux/efi.h                  |   1 +
>  include/linux/tee_drv.h              |  23 ++
>  8 files changed, 909 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/tee/optee/mm_communication.h
>  create mode 100644 drivers/tee/optee/optee_stmm_efi.c


Please split this patch into three patches, one for drivers/tee, one
for the driver/tee/optee, and one for include/linux/efi.h.

> 
> diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> index f121c224e682..a0b699977e51 100644
> --- a/drivers/tee/optee/Kconfig
> +++ b/drivers/tee/optee/Kconfig
> @@ -7,3 +7,13 @@ config OPTEE
>  	help
>  	  This implements the OP-TEE Trusted Execution Environment (TEE)
>  	  driver.
> +
> +config EFI_STMM_OPTEE

Should this be OPTEE_EFI_STMM?

> +	tristate "OP-TEE based EFI runtime variable service driver"
> +	depends on OPTEE
> +	help
> +	  This  driver provides support for OP-TEE based EFI runtime
> +	  variable service driver.
> +
> +	  To compile this driver as a module, choose M here: the module
> +	  will be called optee_stmm_efi.
> diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> index a6eff388d300..9acb0b2de8fe 100644
> --- a/drivers/tee/optee/Makefile
> +++ b/drivers/tee/optee/Makefile
> @@ -8,6 +8,7 @@ optee-objs += supp.o
>  optee-objs += device.o
>  optee-objs += smc_abi.o
>  optee-objs += ffa_abi.o
> +obj-$(CONFIG_EFI_STMM_OPTEE) += optee_stmm_efi.o
>  
>  # for tracing framework to find optee_trace.h
>  CFLAGS_smc_abi.o := -I$(src)
> diff --git a/drivers/tee/optee/mm_communication.h b/drivers/tee/optee/mm_communication.h
> new file mode 100644
> index 000000000000..23e6672991ef
> --- /dev/null
> +++ b/drivers/tee/optee/mm_communication.h
> @@ -0,0 +1,249 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> + *  in OP-TEE
> + *
> + *  Copyright (c) 2017, Intel Corporation. All rights reserved.
> + *  Copyright (C) 2020 Linaro Ltd.
> + */
> +
> +#ifndef _MM_COMMUNICATION_H_
> +#define _MM_COMMUNICATION_H_
> +
> +/*
> + * Interface to the pseudo Trusted Application (TA), which provides a
> + * communication channel with the Standalone MM (Management Mode)
> + * Secure Partition running at Secure-EL0
> + */
> +
> +#define PTA_STMM_CMDID_COMMUNICATE 0

PTA_STMM_CMD_COMMUNICATE

> +
> +/* OP-TEE is using big endian GUIDs while UEFI uses little endian ones */
> +#define PTA_STMM_UUID \
> +	UUID_INIT(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)
> +
> +/* Defined in EDK2 MdePkg/Include/Protocol/MmCommunication.h */
> +
> +/**
> + * struct efi_mm_communicate_header - Header used for SMM variable communication
> +
> + * @header_guid:  header use for disambiguation of content
> + * @message_len:  length of the message. Does not include the size of the
> + *                header
> + * @data:         payload of the message
> + *
> + * Defined in EDK2 as EFI_MM_COMMUNICATE_HEADER.
> + * To avoid confusion in interpreting frames, the communication buffer should
> + * always begin with efi_mm_communicate_header.
> + */
> +struct efi_mm_communicate_header {
> +	efi_guid_t header_guid;
> +	size_t     message_len;
> +	u8         data[];
> +} __packed;
> +
> +#define MM_COMMUNICATE_HEADER_SIZE \
> +	(sizeof(struct efi_mm_communicate_header))
> +
> +/* Defined in EDK2 ArmPkg/Include/IndustryStandard/ArmMmSvc.h */
> +
> +/* SPM return error codes */
> +#define ARM_SVC_SPM_RET_SUCCESS               0
> +#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
> +#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
> +#define ARM_SVC_SPM_RET_DENIED               -3
> +#define ARM_SVC_SPM_RET_NO_MEMORY            -5
> +
> +/* Defined in EDK2 MdeModulePkg/Include/Guid/SmmVariableCommon.h */
> +
> +#define SMM_VARIABLE_FUNCTION_GET_VARIABLE  1
> +/*
> + * The payload for this function is
> + * SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
> + */
> +#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME  2
> +/*
> + * The payload for this function is SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
> + */
> +#define SMM_VARIABLE_FUNCTION_SET_VARIABLE  3
> +/*
> + * The payload for this function is
> + * SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
> + */
> +#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO  4
> +/*
> + * It is a notify event, no extra payload for this function.
> + */
> +#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT  5
> +/*
> + * It is a notify event, no extra payload for this function.
> + */
> +#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE  6
> +/*
> + * The payload for this function is VARIABLE_INFO_ENTRY.
> + * The GUID in EFI_SMM_COMMUNICATE_HEADER is gEfiSmmVariableProtocolGuid.
> + */
> +#define SMM_VARIABLE_FUNCTION_GET_STATISTICS  7
> +/*
> + * The payload for this function is SMM_VARIABLE_COMMUNICATE_LOCK_VARIABLE
> + */
> +#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE   8
> +
> +#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET  9
> +
> +#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET  10
> +
> +#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE  11
> +/*
> + * The payload for this function is
> + * SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
> + */
> +#define SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT 12
> +
> +#define SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE  13
> +/*
> + * The payload for this function is
> + * SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO
> + */
> +#define SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO  14
> +
> +/**
> + * struct smm_variable_communicate_header - Used for SMM variable communication
> +
> + * @function:     function to call in Smm.
> + * @ret_status:   return status
> + * @data:         payload
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_HEADER.
> + */
> +struct smm_variable_communicate_header {
> +	size_t  function;
> +	efi_status_t ret_status;
> +	u8 data[];
> +};
> +
> +#define MM_VARIABLE_COMMUNICATE_SIZE \
> +	(sizeof(struct smm_variable_communicate_header))
> +
> +/**
> + * struct smm_variable_access - Used to communicate with StMM by
> + *                              SetVariable and GetVariable.
> +
> + * @guid:         vendor GUID
> + * @data_size:    size of EFI variable data
> + * @name_size:    size of EFI name
> + * @attr:         attributes
> + * @name:         variable name
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
> + *
> + */
> +struct smm_variable_access {
> +	efi_guid_t  guid;
> +	size_t data_size;
> +	size_t name_size;
> +	u32 attr;
> +	u16 name[];
> +};
> +
> +#define MM_VARIABLE_ACCESS_HEADER_SIZE \
> +	(sizeof(struct smm_variable_access))
> +/**
> + * struct smm_variable_payload_size - Used to get the max allowed
> + *                                    payload used in StMM.
> + *
> + * @size:  size to fill in
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE.
> + *
> + */
> +struct smm_variable_payload_size {
> +	size_t size;
> +};
> +
> +/**
> + * struct smm_variable_getnext - Used to communicate with StMM for
> + *                               GetNextVariableName.
> + *
> + * @guid:       vendor GUID
> + * @name_size:  size of the name of the variable
> + * @name:       variable name
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
> + */
> +struct smm_variable_getnext {
> +	efi_guid_t  guid;
> +	size_t name_size;
> +	u16         name[];
> +};
> +
> +#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
> +	(sizeof(struct smm_variable_getnext))
> +
> +/**
> + * struct smm_variable_query_info - Used to communicate with StMM for
> + *                                  QueryVariableInfo.
> + *
> + * @max_variable_storage:        max available storage
> + * @remaining_variable_storage:  remaining available storage
> + * @max_variable_size:           max variable supported size
> + * @attr:                        attributes to query storage for
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
> + */
> +struct smm_variable_query_info {
> +	u64 max_variable_storage;
> +	u64 remaining_variable_storage;
> +	u64 max_variable_size;
> +	u32 attr;
> +};
> +
> +#define VAR_CHECK_VARIABLE_PROPERTY_REVISION 0x0001
> +#define VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY BIT(0)
> +/**
> + * struct var_check_property - Used to store variable properties in StMM
> + *
> + * @revision:   magic revision number for variable property checking
> + * @property:   properties mask for the variable used in StMM.
> + *              Currently RO flag is supported
> + * @attributes: variable attributes used in StMM checking when properties
> + *              for a variable are enabled
> + * @minsize:    minimum allowed size for variable payload checked against
> + *              smm_variable_access->datasize in StMM
> + * @maxsize:    maximum allowed size for variable payload checked against
> + *              smm_variable_access->datasize in StMM
> + *
> + * Defined in EDK2 as VAR_CHECK_VARIABLE_PROPERTY.
> + */
> +struct var_check_property {
> +	u16 revision;
> +	u16 property;
> +	u32 attributes;
> +	size_t minsize;
> +	size_t maxsize;
> +};
> +
> +/**
> + * struct smm_variable_var_check_property - Used to communicate variable
> + *                                          properties with StMM
> + *
> + * @guid:       vendor GUID
> + * @name_size:  size of EFI name
> + * @property:   variable properties struct
> + * @name:       variable name
> + *
> + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY.
> + */
> +struct smm_variable_var_check_property {
> +	efi_guid_t guid;
> +	size_t name_size;
> +	struct var_check_property property;
> +	u16 name[];
> +};
> +
> +#endif /* _MM_COMMUNICATION_H_ */
> diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> index 04ae58892608..10af910e0dce 100644
> --- a/drivers/tee/optee/optee_private.h
> +++ b/drivers/tee/optee/optee_private.h
> @@ -12,6 +12,7 @@
>  #include <linux/tee_drv.h>
>  #include <linux/types.h>
>  #include "optee_msg.h"
> +#include "optee_private.h"

This is "optee_private.h", no need to include it.

>  
>  #define DRIVER_NAME "optee"
>  
> @@ -19,6 +20,7 @@
>  
>  /* Some Global Platform error codes used in this driver */
>  #define TEEC_SUCCESS			0x00000000
> +#define TEEC_ERROR_EXCESS_DATA		0xFFFF0004
>  #define TEEC_ERROR_BAD_PARAMETERS	0xFFFF0006
>  #define TEEC_ERROR_NOT_SUPPORTED	0xFFFF000A
>  #define TEEC_ERROR_COMMUNICATION	0xFFFF000E
> @@ -250,7 +252,6 @@ void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
>  			       int (*shm_unregister)(struct tee_context *ctx,
>  						     struct tee_shm *shm));
>  
> -
>  void optee_remove_common(struct optee *optee);
>  int optee_open(struct tee_context *ctx, bool cap_memref_null);
>  void optee_release(struct tee_context *ctx);
> @@ -322,4 +323,6 @@ void optee_smc_abi_unregister(void);
>  int optee_ffa_abi_register(void);
>  void optee_ffa_abi_unregister(void);
>  
> +int optee_efivar_ops_init(struct tee_context *ctx);

This isn't used or implemented as far as I can tell.

> +
>  #endif /*OPTEE_PRIVATE_H*/
> diff --git a/drivers/tee/optee/optee_stmm_efi.c b/drivers/tee/optee/optee_stmm_efi.c
> new file mode 100644
> index 000000000000..6dcf1eb4b96c
> --- /dev/null
> +++ b/drivers/tee/optee/optee_stmm_efi.c
> @@ -0,0 +1,598 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  EFI variable service via OP-TEE
> + *
> + *  Copyright (C) 2022 Linaro
> + */
> +
> +#include <linux/efi.h>
> +#include <linux/kernel.h>
> +#include <linux/slab.h>
> +#include <linux/tee.h>
> +#include <linux/tee_drv.h>
> +#include <linux/ucs2_string.h>
> +#include "mm_communication.h"
> +#include "optee_private.h"
> +
> +static struct efivars optee_efivars;
> +static struct efivar_operations optee_ops;

optee_efi_ops instead? To avoid being confused with the other optee_ops.

> +
> +static size_t max_buffer_size; /* comm + var + func + data */
> +static size_t max_payload_size; /* func + data */
> +
> +struct mm_connection {
> +	struct tee_context *ctx;
> +	u32 session;
> +};
> +
> +/* UUID of the stmm PTA */
> +static const struct tee_client_device_id optee_stmm_efi_id_table[] = {
> +	{PTA_STMM_UUID},
> +	{}
> +};
> +
> +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> +{
> +	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> +		return 1;
> +	else
> +		return 0;
> +}
> +
> +/**
> + * 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)
> +{
> +	struct tee_context *ctx = NULL;
> +	struct tee_ioctl_open_session_arg arg;
> +	int rc;
> +
> +	memset(&arg, 0, sizeof(arg));
> +
> +	/* Open context with OP-TEE driver */
> +	ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);

tee_client_close_context() is missing in the error paths.

> +	if (IS_ERR(ctx))
> +		return -ENODEV;
> +
> +	export_uuid(arg.uuid, &optee_stmm_efi_id_table[0].uuid);
> +	rc = tee_client_open_session(ctx, &arg, NULL);
> +	if (!rc) {
> +		conn->ctx = ctx;
> +		conn->session = arg.session;
> +	}
> +
> +	return rc;
> +}
> +
> +/**
> + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
> + *
> + * @comm_buf:		locally allocated communcation buffer
> + * @dsize:		buffer size
> + * Return:		status code
> + */
> +static efi_status_t optee_mm_communicate(void *comm_buf, size_t dsize)
> +{
> +	size_t buf_size;
> +	efi_status_t ret;
> +	struct efi_mm_communicate_header *mm_hdr;
> +	struct mm_connection conn = { NULL, 0 };
> +	struct tee_ioctl_invoke_arg arg;
> +	struct tee_param param[4];
> +	struct tee_shm *shm = NULL;
> +	int rc;
> +
> +	if (!comm_buf)
> +		return EFI_INVALID_PARAMETER;
> +
> +	mm_hdr = (struct efi_mm_communicate_header *)comm_buf;

No need to cast.

> +	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) {
> +		pr_err("Unable to open OP-TEE session (err=%d)\n", rc);

Perhaps better to print from get_connection() to be able to distinguish
between errors from tee_client_open_context() and
tee_client_open_session().

> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	shm = tee_shm_register_kernel_buf(conn.ctx, comm_buf, buf_size);
> +	if (IS_ERR(shm)) {
> +		pr_err("Unable to register shared memory\n");
> +		tee_client_close_session(conn.ctx, conn.session);
> +		return EFI_UNSUPPORTED;
> +	}
> +
> +	memset(&arg, 0, sizeof(arg));
> +	arg.func = PTA_STMM_CMDID_COMMUNICATE;
> +	arg.session = conn.session;
> +	arg.num_params = 4;
> +
> +	memset(param, 0, sizeof(param));
> +	param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
> +	param[0].u.memref.size = buf_size;
> +	param[0].u.memref.shm = shm;
> +	param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> +	param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> +	param[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> +
> +	rc = tee_client_invoke_func(conn.ctx, &arg, param);
> +	tee_shm_free(shm);
> +	/* even if close session fails the session will be invalidaded */
> +	tee_client_close_session(conn.ctx, conn.session);
> +	if (rc)
> +		return EFI_DEVICE_ERROR;
> +	if (arg.ret == TEEC_ERROR_EXCESS_DATA)
> +		pr_err("Variable payload too large\n");
> +	if (arg.ret != TEEC_SUCCESS)
> +		return EFI_DEVICE_ERROR;
> +
> +	switch (param[1].u.value.a) {
> +	case ARM_SVC_SPM_RET_SUCCESS:
> +		ret = EFI_SUCCESS;
> +		break;
> +
> +	case ARM_SVC_SPM_RET_INVALID_PARAMS:
> +		ret = EFI_INVALID_PARAMETER;
> +		break;
> +
> +	case ARM_SVC_SPM_RET_DENIED:
> +		ret = EFI_ACCESS_DENIED;
> +		break;
> +
> +	case ARM_SVC_SPM_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 allocated communcation buffer
> + * @dsize:		buffer size
> + * Return:		status code
> + */
> +static efi_status_t mm_communicate(u8 *comm_buf, size_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) {
> +		pr_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, size_t payload_size, size_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 = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
> +				   MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> +			   GFP_KERNEL);
> +	if (!comm_buf) {
> +		*ret = EFI_OUT_OF_RESOURCES;
> +		return NULL;
> +	}
> +
> +	mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> +	memcpy(&mm_hdr->header_guid, &mm_var_guid, sizeof(mm_hdr->header_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
> + */
> +static efi_status_t get_max_payload(size_t *size)
> +{
> +	struct smm_variable_payload_size *var_payload = NULL;
> +	size_t payload_size;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	if (!size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	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)
> +		goto out;
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	/* Make sure the buffer is big enough for storing variables */
> +	if (var_payload->size < MM_VARIABLE_ACCESS_HEADER_SIZE + 0x20) {
> +		ret = EFI_DEVICE_ERROR;
> +		goto out;
> +	}
> +	*size = var_payload->size;
> +	/*
> +	 * There seems to be a bug in EDK2 miscalculating the boundaries and
> +	 * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
> +	 * it up here to ensure backwards compatibility with older versions
> +	 * (cf. StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c.
> +	 * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the
> +	 * flexible array member).
> +	 *
> +	 * size is guaranteed to be > 2 due to checks on the beginning.
> +	 */
> +	*size -= 2;
> +out:
> +	kfree(comm_buf);
> +	return ret;
> +}
> +
> +static efi_status_t get_property_int(u16 *name, size_t name_size,
> +				     const efi_guid_t *vendor,
> +				     struct var_check_property *var_property)
> +{
> +	struct smm_variable_var_check_property *smm_property;
> +	size_t payload_size;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	memset(var_property, 0, sizeof(*var_property));
> +	payload_size = sizeof(*smm_property) + name_size;
> +	if (payload_size > max_payload_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	comm_buf = setup_mm_hdr(
> +		(void **)&smm_property, payload_size,
> +		SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> +	if (!comm_buf)
> +		goto out;
> +
> +	memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
> +	smm_property->name_size = name_size;
> +	memcpy(smm_property->name, name, name_size);
> +
> +	ret = mm_communicate(comm_buf, payload_size);
> +	/*
> +	 * Currently only R/O property is supported in StMM.
> +	 * Variables that are not set to R/O will not set the property in StMM
> +	 * and the call will return EFI_NOT_FOUND. We are setting the
> +	 * properties to 0x0 so checking against that is enough for the
> +	 * EFI_NOT_FOUND case.
> +	 */
> +	if (ret == EFI_NOT_FOUND)
> +		ret = EFI_SUCCESS;
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +	memcpy(var_property, &smm_property->property, sizeof(*var_property));
> +
> +out:
> +	kfree(comm_buf);
> +	return ret;
> +}
> +
> +static efi_status_t optee_get_variable(u16 *name, efi_guid_t *vendor,
> +				       u32 *attributes,
> +				       unsigned long *data_size, void *data)
> +{
> +	struct var_check_property var_property;
> +	struct smm_variable_access *var_acc;
> +	size_t payload_size;
> +	size_t name_size;
> +	size_t tmp_dsize;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	if (!name || !vendor || !data_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> +	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	/* 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)
> +		goto out;
> +
> +	/* Fill in contents */
> +	memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> +	var_acc->data_size = tmp_dsize;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attributes ? *attributes : 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 out;
> +
> +	ret = get_property_int(name, name_size, vendor, &var_property);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	if (attributes)
> +		*attributes = var_acc->attr;
> +
> +	if (data)
> +		memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> +		       var_acc->data_size);
> +	else
> +		ret = EFI_INVALID_PARAMETER;
> +
> +out:
> +	kfree(comm_buf);
> +	return ret;
> +}
> +
> +static efi_status_t optee_get_next_variable(unsigned long *name_size,
> +					    efi_char16_t *name,
> +					    efi_guid_t *guid)
> +{
> +	struct smm_variable_getnext *var_getnext;
> +	size_t payload_size;
> +	size_t out_name_size;
> +	size_t in_name_size;
> +	u8 *comm_buf = NULL;
> +	efi_status_t ret;
> +
> +	if (!name_size || !name || !guid) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	out_name_size = *name_size;
> +	in_name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> +
> +	if (out_name_size < in_name_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	if (in_name_size >
> +	    max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	/* Trim output buffer size */
> +	if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> +		out_name_size =
> +			max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
> +
> +	payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
> +	comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> +				SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> +				&ret);
> +	if (!comm_buf)
> +		goto out;
> +
> +	/* Fill in contents */
> +	memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> +	var_getnext->name_size = out_name_size;
> +	memcpy(var_getnext->name, name, 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 */
> +		*name_size = var_getnext->name_size;
> +	}
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	memcpy(guid, &var_getnext->guid, sizeof(*guid));
> +	memcpy(name, var_getnext->name, var_getnext->name_size);
> +
> +out:
> +	kfree(comm_buf);
> +	return ret;
> +}
> +
> +static efi_status_t optee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> +				       u32 attributes, unsigned long data_size,
> +				       void *data)
> +{
> +	efi_status_t ret;
> +	struct var_check_property var_property;
> +	struct smm_variable_access *var_acc;
> +	size_t payload_size;
> +	size_t name_size;
> +	u8 *comm_buf = NULL;
> +
> +	if (!name || name[0] == 0 || !vendor) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	if (data_size > 0 && !data) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +	/* Check payload size */
> +	name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> +	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> +	if (payload_size > max_payload_size) {
> +		ret = EFI_INVALID_PARAMETER;
> +		goto out;
> +	}
> +
> +	/*
> +	 * Allocate the buffer early, before switching to RW (if needed)
> +	 * so we won't need to account for any failures in reading/setting
> +	 * the properties, if the allocation fails
> +	 */
> +	comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> +				SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> +	if (!comm_buf)
> +		goto out;
> +
> +	/*
> +	 * The API has the ability to override RO flags. If no RO check was
> +	 * requested switch the variable to RW for the duration of this call
> +	 */
> +	ret = get_property_int(name, name_size, vendor, &var_property);
> +	if (ret != EFI_SUCCESS) {
> +		pr_err("Getting variable property failed\n");
> +		goto out;
> +	}
> +
> +	if (var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY) {
> +		ret = EFI_WRITE_PROTECTED;
> +		goto out;
> +	}
> +
> +	/* Fill in contents */
> +	memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> +	var_acc->data_size = data_size;
> +	var_acc->name_size = name_size;
> +	var_acc->attr = attributes;
> +	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);
> +	pr_info("Set Variable %s %d %lx\n", __FILE__, __LINE__, ret);
> +out:
> +	kfree(comm_buf);
> +	return ret;
> +}
> +
> +static int optee_stmm_efi_probe(struct device *dev)
> +{
> +	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;
> +
> +	optee_ops.get_variable = optee_get_variable;
> +	optee_ops.get_next_variable = optee_get_next_variable;
> +	optee_ops.set_variable = optee_set_variable;
> +	/* TODO: support nonblocking variant */
> +	optee_ops.set_variable_nonblocking = NULL;
> +	/* set NULL, always return EFI_SUCCESS by efi_query_variable_store() */
> +	optee_ops.query_variable_store = NULL;

Why aren't you using static initialization instead?

Thanks,
Jens

> +
> +	tee_register_efivar_ops(&optee_efivars, &optee_ops);
> +
> +	return 0;
> +}
> +
> +static int optee_stmm_efi_remove(struct device *dev)
> +{
> +	tee_unregister_efivar_ops(&optee_efivars);
> +
> +	return 0;
> +}
> +
> +MODULE_DEVICE_TABLE(tee, optee_stmm_efi_id_table);
> +
> +static struct tee_client_driver optee_stmm_efi_driver = {
> +	.id_table	= optee_stmm_efi_id_table,
> +	.driver		= {
> +		.name		= "optee-stmm-efi",
> +		.bus		= &tee_bus_type,
> +		.probe		= optee_stmm_efi_probe,
> +		.remove		= optee_stmm_efi_remove,
> +	},
> +};
> +
> +static int __init optee_stmm_efi_mod_init(void)
> +{
> +	return driver_register(&optee_stmm_efi_driver.driver);
> +}
> +
> +static void __exit optee_stmm_efi_mod_exit(void)
> +{
> +	driver_unregister(&optee_stmm_efi_driver.driver);
> +}
> +
> +module_init(optee_stmm_efi_mod_init);
> +module_exit(optee_stmm_efi_mod_exit);
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_AUTHOR("Ilias Apalodimas <ilias.apalodimas@linaro.org>");
> +MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>");
> +MODULE_DESCRIPTION("OP-TEE based EFI runtime variable service driver");
> diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> index 98da206cd761..ac46274844a3 100644
> --- a/drivers/tee/tee_core.c
> +++ b/drivers/tee/tee_core.c
> @@ -7,6 +7,7 @@
>  
>  #include <linux/cdev.h>
>  #include <linux/cred.h>
> +#include <linux/efi.h>
>  #include <linux/fs.h>
>  #include <linux/idr.h>
>  #include <linux/module.h>
> @@ -1263,6 +1264,28 @@ static void __exit tee_exit(void)
>  	tee_class = NULL;
>  }
>  
> +void tee_register_efivar_ops(struct efivars *tee_efivars,
> +			     struct efivar_operations *ops)
> +{
> +	/*
> +	 * If the firmware EFI runtime services support SetVariable(),
> +	 * tee-based EFI variable services are not used.
> +	 */
> +	if (!efivar_supports_writes()) {
> +		pr_info("Use tee-based EFI runtime variable services\n");
> +		efivars_generic_ops_unregister();
> +		efivars_register(tee_efivars, ops);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(tee_register_efivar_ops);
> +
> +void tee_unregister_efivar_ops(struct efivars *tee_efivars)
> +{
> +	efivars_unregister(tee_efivars);
> +	efivars_generic_ops_register();
> +}
> +EXPORT_SYMBOL_GPL(tee_unregister_efivar_ops);
> +
>  subsys_initcall(tee_init);
>  module_exit(tee_exit);
>  
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 5e301c00e9b0..14d4aa83ce60 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -39,6 +39,7 @@
>  #define EFI_WRITE_PROTECTED	( 8 | (1UL << (BITS_PER_LONG-1)))
>  #define EFI_OUT_OF_RESOURCES	( 9 | (1UL << (BITS_PER_LONG-1)))
>  #define EFI_NOT_FOUND		(14 | (1UL << (BITS_PER_LONG-1)))
> +#define EFI_ACCESS_DENIED	(15 | (1UL << (BITS_PER_LONG-1)))
>  #define EFI_TIMEOUT		(18 | (1UL << (BITS_PER_LONG-1)))
>  #define EFI_ABORTED		(21 | (1UL << (BITS_PER_LONG-1)))
>  #define EFI_SECURITY_VIOLATION	(26 | (1UL << (BITS_PER_LONG-1)))
> diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> index 17eb1c5205d3..def4ea6212ee 100644
> --- a/include/linux/tee_drv.h
> +++ b/include/linux/tee_drv.h
> @@ -7,6 +7,7 @@
>  #define __TEE_DRV_H
>  
>  #include <linux/device.h>
> +#include <linux/efi.h>
>  #include <linux/idr.h>
>  #include <linux/kref.h>
>  #include <linux/list.h>
> @@ -507,4 +508,26 @@ struct tee_context *teedev_open(struct tee_device *teedev);
>   */
>  void teedev_close_context(struct tee_context *ctx);
>  
> +/**
> + * tee_register_efivar_ops() - register the efivar ops
> + * @tee_efivars:	pointer to efivars structure
> + * @ops:		pointer to contain the efivar operation
> + *
> + * This function registers the tee-based efivar operation as an
> + * EFI Runtime Service.
> + *
> + */
> +void tee_register_efivar_ops(struct efivars *tee_efivars,
> +			     struct efivar_operations *ops);
> +
> +/**
> + * tee_unregister_efivar_ops() - unregister the efivar ops
> + * @tee_efivars:	pointer to efivars structure
> + *
> + * This function unregisters the tee-based efivar operation
> + * and reverts to the generic operation.
> + *
> + */
> +void tee_unregister_efivar_ops(struct efivars *tee_efivars);
> +
>  #endif /*__TEE_DRV_H*/
> -- 
> 2.30.2
> 

Re: [RFC PATCH 2/2] tee: Add op-tee helper functions for variable access

[Masahisa Kojima]

() Hi Jens,

On Fri, 3 Feb 2023 at 18:30, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Thu, Jan 26, 2023 at 10:21:19PM +0900, Masahisa Kojima wrote:
> > When the flash is not owned by the non-secure world, accessing the EFI
> > variables is straightforward and done via EFI Runtime Variable Services.
> > In this case, critical variables for system integrity and security
> > are normally stored in the dedicated secure storage and only accessible
> > from the secure world.
> >
> > On the other hand, the small embedded devices don't have the special
> > dedicated secure storage. The eMMC device with an RPMB partition is
> > becoming more common, we can use an RPMB partition to store the
> > EFI Variables.
> >
> > The eMMC device is typically owned by the non-secure world(linux in
> > this case). There is an existing solution utilizing eMMC RPMB partition
> > for EFI Variables, it is implemented by interacting with
> > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > and tee-supplicant. The last piece is the tee-based variable access
> > driver to interact with OP-TEE and StandaloneMM.
> >
> > So let's add the kernel functions needed.
> >
> > This feature is implemented as a kernel module.
> > StMM PTA has TA_FLAG_DEVICE_ENUM_SUPP flag when registered to OP-TEE
> > so that this optee_stmm_efi module is probed after tee-supplicant starts,
> > since "SetVariable" EFI Runtime Variable Service requires to
> > interact with tee-supplicant.
> >
> > Co-developed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  drivers/tee/optee/Kconfig            |  10 +
> >  drivers/tee/optee/Makefile           |   1 +
> >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> >  drivers/tee/optee/optee_private.h    |   5 +-
> >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> >  drivers/tee/tee_core.c               |  23 ++
> >  include/linux/efi.h                  |   1 +
> >  include/linux/tee_drv.h              |  23 ++
> >  8 files changed, 909 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/tee/optee/mm_communication.h
> >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
>
>
> Please split this patch into three patches, one for drivers/tee, one
> for the driver/tee/optee, and one for include/linux/efi.h.

OK.

>
> >
> > diff --git a/drivers/tee/optee/Kconfig b/drivers/tee/optee/Kconfig
> > index f121c224e682..a0b699977e51 100644
> > --- a/drivers/tee/optee/Kconfig
> > +++ b/drivers/tee/optee/Kconfig
> > @@ -7,3 +7,13 @@ config OPTEE
> >       help
> >         This implements the OP-TEE Trusted Execution Environment (TEE)
> >         driver.
> > +
> > +config EFI_STMM_OPTEE
>
> Should this be OPTEE_EFI_STMM?

OK.

>
> > +     tristate "OP-TEE based EFI runtime variable service driver"
> > +     depends on OPTEE
> > +     help
> > +       This  driver provides support for OP-TEE based EFI runtime
> > +       variable service driver.
> > +
> > +       To compile this driver as a module, choose M here: the module
> > +       will be called optee_stmm_efi.
> > diff --git a/drivers/tee/optee/Makefile b/drivers/tee/optee/Makefile
> > index a6eff388d300..9acb0b2de8fe 100644
> > --- a/drivers/tee/optee/Makefile
> > +++ b/drivers/tee/optee/Makefile
> > @@ -8,6 +8,7 @@ optee-objs += supp.o
> >  optee-objs += device.o
> >  optee-objs += smc_abi.o
> >  optee-objs += ffa_abi.o
> > +obj-$(CONFIG_EFI_STMM_OPTEE) += optee_stmm_efi.o
> >
> >  # for tracing framework to find optee_trace.h
> >  CFLAGS_smc_abi.o := -I$(src)
> > diff --git a/drivers/tee/optee/mm_communication.h b/drivers/tee/optee/mm_communication.h
> > new file mode 100644
> > index 000000000000..23e6672991ef
> > --- /dev/null
> > +++ b/drivers/tee/optee/mm_communication.h
> > @@ -0,0 +1,249 @@
> > +/* SPDX-License-Identifier: GPL-2.0+ */
> > +/*
> > + *  Headers for EFI variable service via StandAloneMM, EDK2 application running
> > + *  in OP-TEE
> > + *
> > + *  Copyright (c) 2017, Intel Corporation. All rights reserved.
> > + *  Copyright (C) 2020 Linaro Ltd.
> > + */
> > +
> > +#ifndef _MM_COMMUNICATION_H_
> > +#define _MM_COMMUNICATION_H_
> > +
> > +/*
> > + * Interface to the pseudo Trusted Application (TA), which provides a
> > + * communication channel with the Standalone MM (Management Mode)
> > + * Secure Partition running at Secure-EL0
> > + */
> > +
> > +#define PTA_STMM_CMDID_COMMUNICATE 0
>
> PTA_STMM_CMD_COMMUNICATE

OK.

>
> > +
> > +/* OP-TEE is using big endian GUIDs while UEFI uses little endian ones */
> > +#define PTA_STMM_UUID \
> > +     UUID_INIT(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)
> > +
> > +/* Defined in EDK2 MdePkg/Include/Protocol/MmCommunication.h */
> > +
> > +/**
> > + * struct efi_mm_communicate_header - Header used for SMM variable communication
> > +
> > + * @header_guid:  header use for disambiguation of content
> > + * @message_len:  length of the message. Does not include the size of the
> > + *                header
> > + * @data:         payload of the message
> > + *
> > + * Defined in EDK2 as EFI_MM_COMMUNICATE_HEADER.
> > + * To avoid confusion in interpreting frames, the communication buffer should
> > + * always begin with efi_mm_communicate_header.
> > + */
> > +struct efi_mm_communicate_header {
> > +     efi_guid_t header_guid;
> > +     size_t     message_len;
> > +     u8         data[];
> > +} __packed;
> > +
> > +#define MM_COMMUNICATE_HEADER_SIZE \
> > +     (sizeof(struct efi_mm_communicate_header))
> > +
> > +/* Defined in EDK2 ArmPkg/Include/IndustryStandard/ArmMmSvc.h */
> > +
> > +/* SPM return error codes */
> > +#define ARM_SVC_SPM_RET_SUCCESS               0
> > +#define ARM_SVC_SPM_RET_NOT_SUPPORTED        -1
> > +#define ARM_SVC_SPM_RET_INVALID_PARAMS       -2
> > +#define ARM_SVC_SPM_RET_DENIED               -3
> > +#define ARM_SVC_SPM_RET_NO_MEMORY            -5
> > +
> > +/* Defined in EDK2 MdeModulePkg/Include/Guid/SmmVariableCommon.h */
> > +
> > +#define SMM_VARIABLE_FUNCTION_GET_VARIABLE  1
> > +/*
> > + * The payload for this function is
> > + * SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME  2
> > +/*
> > + * The payload for this function is SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_SET_VARIABLE  3
> > +/*
> > + * The payload for this function is
> > + * SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO  4
> > +/*
> > + * It is a notify event, no extra payload for this function.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_READY_TO_BOOT  5
> > +/*
> > + * It is a notify event, no extra payload for this function.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_EXIT_BOOT_SERVICE  6
> > +/*
> > + * The payload for this function is VARIABLE_INFO_ENTRY.
> > + * The GUID in EFI_SMM_COMMUNICATE_HEADER is gEfiSmmVariableProtocolGuid.
> > + */
> > +#define SMM_VARIABLE_FUNCTION_GET_STATISTICS  7
> > +/*
> > + * The payload for this function is SMM_VARIABLE_COMMUNICATE_LOCK_VARIABLE
> > + */
> > +#define SMM_VARIABLE_FUNCTION_LOCK_VARIABLE   8
> > +
> > +#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_SET  9
> > +
> > +#define SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET  10
> > +
> > +#define SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE  11
> > +/*
> > + * The payload for this function is
> > + * SMM_VARIABLE_COMMUNICATE_RUNTIME_VARIABLE_CACHE_CONTEXT
> > + */
> > +#define SMM_VARIABLE_FUNCTION_INIT_RUNTIME_VARIABLE_CACHE_CONTEXT 12
> > +
> > +#define SMM_VARIABLE_FUNCTION_SYNC_RUNTIME_CACHE  13
> > +/*
> > + * The payload for this function is
> > + * SMM_VARIABLE_COMMUNICATE_GET_RUNTIME_CACHE_INFO
> > + */
> > +#define SMM_VARIABLE_FUNCTION_GET_RUNTIME_CACHE_INFO  14
> > +
> > +/**
> > + * struct smm_variable_communicate_header - Used for SMM variable communication
> > +
> > + * @function:     function to call in Smm.
> > + * @ret_status:   return status
> > + * @data:         payload
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_HEADER.
> > + */
> > +struct smm_variable_communicate_header {
> > +     size_t  function;
> > +     efi_status_t ret_status;
> > +     u8 data[];
> > +};
> > +
> > +#define MM_VARIABLE_COMMUNICATE_SIZE \
> > +     (sizeof(struct smm_variable_communicate_header))
> > +
> > +/**
> > + * struct smm_variable_access - Used to communicate with StMM by
> > + *                              SetVariable and GetVariable.
> > +
> > + * @guid:         vendor GUID
> > + * @data_size:    size of EFI variable data
> > + * @name_size:    size of EFI name
> > + * @attr:         attributes
> > + * @name:         variable name
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_ACCESS_VARIABLE.
> > + *
> > + */
> > +struct smm_variable_access {
> > +     efi_guid_t  guid;
> > +     size_t data_size;
> > +     size_t name_size;
> > +     u32 attr;
> > +     u16 name[];
> > +};
> > +
> > +#define MM_VARIABLE_ACCESS_HEADER_SIZE \
> > +     (sizeof(struct smm_variable_access))
> > +/**
> > + * struct smm_variable_payload_size - Used to get the max allowed
> > + *                                    payload used in StMM.
> > + *
> > + * @size:  size to fill in
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_PAYLOAD_SIZE.
> > + *
> > + */
> > +struct smm_variable_payload_size {
> > +     size_t size;
> > +};
> > +
> > +/**
> > + * struct smm_variable_getnext - Used to communicate with StMM for
> > + *                               GetNextVariableName.
> > + *
> > + * @guid:       vendor GUID
> > + * @name_size:  size of the name of the variable
> > + * @name:       variable name
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_GET_NEXT_VARIABLE_NAME.
> > + */
> > +struct smm_variable_getnext {
> > +     efi_guid_t  guid;
> > +     size_t name_size;
> > +     u16         name[];
> > +};
> > +
> > +#define MM_VARIABLE_GET_NEXT_HEADER_SIZE \
> > +     (sizeof(struct smm_variable_getnext))
> > +
> > +/**
> > + * struct smm_variable_query_info - Used to communicate with StMM for
> > + *                                  QueryVariableInfo.
> > + *
> > + * @max_variable_storage:        max available storage
> > + * @remaining_variable_storage:  remaining available storage
> > + * @max_variable_size:           max variable supported size
> > + * @attr:                        attributes to query storage for
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_QUERY_VARIABLE_INFO.
> > + */
> > +struct smm_variable_query_info {
> > +     u64 max_variable_storage;
> > +     u64 remaining_variable_storage;
> > +     u64 max_variable_size;
> > +     u32 attr;
> > +};
> > +
> > +#define VAR_CHECK_VARIABLE_PROPERTY_REVISION 0x0001
> > +#define VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY BIT(0)
> > +/**
> > + * struct var_check_property - Used to store variable properties in StMM
> > + *
> > + * @revision:   magic revision number for variable property checking
> > + * @property:   properties mask for the variable used in StMM.
> > + *              Currently RO flag is supported
> > + * @attributes: variable attributes used in StMM checking when properties
> > + *              for a variable are enabled
> > + * @minsize:    minimum allowed size for variable payload checked against
> > + *              smm_variable_access->datasize in StMM
> > + * @maxsize:    maximum allowed size for variable payload checked against
> > + *              smm_variable_access->datasize in StMM
> > + *
> > + * Defined in EDK2 as VAR_CHECK_VARIABLE_PROPERTY.
> > + */
> > +struct var_check_property {
> > +     u16 revision;
> > +     u16 property;
> > +     u32 attributes;
> > +     size_t minsize;
> > +     size_t maxsize;
> > +};
> > +
> > +/**
> > + * struct smm_variable_var_check_property - Used to communicate variable
> > + *                                          properties with StMM
> > + *
> > + * @guid:       vendor GUID
> > + * @name_size:  size of EFI name
> > + * @property:   variable properties struct
> > + * @name:       variable name
> > + *
> > + * Defined in EDK2 as SMM_VARIABLE_COMMUNICATE_VAR_CHECK_VARIABLE_PROPERTY.
> > + */
> > +struct smm_variable_var_check_property {
> > +     efi_guid_t guid;
> > +     size_t name_size;
> > +     struct var_check_property property;
> > +     u16 name[];
> > +};
> > +
> > +#endif /* _MM_COMMUNICATION_H_ */
> > diff --git a/drivers/tee/optee/optee_private.h b/drivers/tee/optee/optee_private.h
> > index 04ae58892608..10af910e0dce 100644
> > --- a/drivers/tee/optee/optee_private.h
> > +++ b/drivers/tee/optee/optee_private.h
> > @@ -12,6 +12,7 @@
> >  #include <linux/tee_drv.h>
> >  #include <linux/types.h>
> >  #include "optee_msg.h"
> > +#include "optee_private.h"
>
> This is "optee_private.h", no need to include it.

I added this line by mistake, remove this line.

>
> >
> >  #define DRIVER_NAME "optee"
> >
> > @@ -19,6 +20,7 @@
> >
> >  /* Some Global Platform error codes used in this driver */
> >  #define TEEC_SUCCESS                 0x00000000
> > +#define TEEC_ERROR_EXCESS_DATA               0xFFFF0004
> >  #define TEEC_ERROR_BAD_PARAMETERS    0xFFFF0006
> >  #define TEEC_ERROR_NOT_SUPPORTED     0xFFFF000A
> >  #define TEEC_ERROR_COMMUNICATION     0xFFFF000E
> > @@ -250,7 +252,6 @@ void optee_pool_op_free_helper(struct tee_shm_pool *pool, struct tee_shm *shm,
> >                              int (*shm_unregister)(struct tee_context *ctx,
> >                                                    struct tee_shm *shm));
> >
> > -
> >  void optee_remove_common(struct optee *optee);
> >  int optee_open(struct tee_context *ctx, bool cap_memref_null);
> >  void optee_release(struct tee_context *ctx);
> > @@ -322,4 +323,6 @@ void optee_smc_abi_unregister(void);
> >  int optee_ffa_abi_register(void);
> >  void optee_ffa_abi_unregister(void);
> >
> > +int optee_efivar_ops_init(struct tee_context *ctx);
>
> This isn't used or implemented as far as I can tell.

This is the previous version of code, I will remove it.

>
> > +
> >  #endif /*OPTEE_PRIVATE_H*/
> > diff --git a/drivers/tee/optee/optee_stmm_efi.c b/drivers/tee/optee/optee_stmm_efi.c
> > new file mode 100644
> > index 000000000000..6dcf1eb4b96c
> > --- /dev/null
> > +++ b/drivers/tee/optee/optee_stmm_efi.c
> > @@ -0,0 +1,598 @@
> > +// SPDX-License-Identifier: GPL-2.0+
> > +/*
> > + *  EFI variable service via OP-TEE
> > + *
> > + *  Copyright (C) 2022 Linaro
> > + */
> > +
> > +#include <linux/efi.h>
> > +#include <linux/kernel.h>
> > +#include <linux/slab.h>
> > +#include <linux/tee.h>
> > +#include <linux/tee_drv.h>
> > +#include <linux/ucs2_string.h>
> > +#include "mm_communication.h"
> > +#include "optee_private.h"
> > +
> > +static struct efivars optee_efivars;
> > +static struct efivar_operations optee_ops;
>
> optee_efi_ops instead? To avoid being confused with the other optee_ops.
>
> > +
> > +static size_t max_buffer_size; /* comm + var + func + data */
> > +static size_t max_payload_size; /* func + data */
> > +
> > +struct mm_connection {
> > +     struct tee_context *ctx;
> > +     u32 session;
> > +};
> > +
> > +/* UUID of the stmm PTA */
> > +static const struct tee_client_device_id optee_stmm_efi_id_table[] = {
> > +     {PTA_STMM_UUID},
> > +     {}
> > +};
> > +
> > +static int optee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
> > +{
> > +     if (ver->impl_id == TEE_IMPL_ID_OPTEE)
> > +             return 1;
> > +     else
> > +             return 0;
> > +}
> > +
> > +/**
> > + * 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)
> > +{
> > +     struct tee_context *ctx = NULL;
> > +     struct tee_ioctl_open_session_arg arg;
> > +     int rc;
> > +
> > +     memset(&arg, 0, sizeof(arg));
> > +
> > +     /* Open context with OP-TEE driver */
> > +     ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
>
> tee_client_close_context() is missing in the error paths.

Yes, I will add tee_client_close_context in the error paths.

>
> > +     if (IS_ERR(ctx))
> > +             return -ENODEV;
> > +
> > +     export_uuid(arg.uuid, &optee_stmm_efi_id_table[0].uuid);
> > +     rc = tee_client_open_session(ctx, &arg, NULL);
> > +     if (!rc) {
> > +             conn->ctx = ctx;
> > +             conn->session = arg.session;
> > +     }
> > +
> > +     return rc;
> > +}
> > +
> > +/**
> > + * optee_mm_communicate() - Pass a buffer to StandaloneMM running in OP-TEE
> > + *
> > + * @comm_buf:                locally allocated communcation buffer
> > + * @dsize:           buffer size
> > + * Return:           status code
> > + */
> > +static efi_status_t optee_mm_communicate(void *comm_buf, size_t dsize)
> > +{
> > +     size_t buf_size;
> > +     efi_status_t ret;
> > +     struct efi_mm_communicate_header *mm_hdr;
> > +     struct mm_connection conn = { NULL, 0 };
> > +     struct tee_ioctl_invoke_arg arg;
> > +     struct tee_param param[4];
> > +     struct tee_shm *shm = NULL;
> > +     int rc;
> > +
> > +     if (!comm_buf)
> > +             return EFI_INVALID_PARAMETER;
> > +
> > +     mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
>
> No need to cast.

OK.

>
> > +     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) {
> > +             pr_err("Unable to open OP-TEE session (err=%d)\n", rc);
>
> Perhaps better to print from get_connection() to be able to distinguish
> between errors from tee_client_open_context() and
> tee_client_open_session().

OK, I will update.

>
> > +             return EFI_UNSUPPORTED;
> > +     }
> > +
> > +     shm = tee_shm_register_kernel_buf(conn.ctx, comm_buf, buf_size);
> > +     if (IS_ERR(shm)) {
> > +             pr_err("Unable to register shared memory\n");
> > +             tee_client_close_session(conn.ctx, conn.session);
> > +             return EFI_UNSUPPORTED;
> > +     }
> > +
> > +     memset(&arg, 0, sizeof(arg));
> > +     arg.func = PTA_STMM_CMDID_COMMUNICATE;
> > +     arg.session = conn.session;
> > +     arg.num_params = 4;
> > +
> > +     memset(param, 0, sizeof(param));
> > +     param[0].attr = TEE_IOCTL_PARAM_ATTR_TYPE_MEMREF_INOUT;
> > +     param[0].u.memref.size = buf_size;
> > +     param[0].u.memref.shm = shm;
> > +     param[1].attr = TEE_IOCTL_PARAM_ATTR_TYPE_VALUE_OUTPUT;
> > +     param[2].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> > +     param[3].attr = TEE_IOCTL_PARAM_ATTR_TYPE_NONE;
> > +
> > +     rc = tee_client_invoke_func(conn.ctx, &arg, param);
> > +     tee_shm_free(shm);
> > +     /* even if close session fails the session will be invalidaded */
> > +     tee_client_close_session(conn.ctx, conn.session);
> > +     if (rc)
> > +             return EFI_DEVICE_ERROR;
> > +     if (arg.ret == TEEC_ERROR_EXCESS_DATA)
> > +             pr_err("Variable payload too large\n");
> > +     if (arg.ret != TEEC_SUCCESS)
> > +             return EFI_DEVICE_ERROR;
> > +
> > +     switch (param[1].u.value.a) {
> > +     case ARM_SVC_SPM_RET_SUCCESS:
> > +             ret = EFI_SUCCESS;
> > +             break;
> > +
> > +     case ARM_SVC_SPM_RET_INVALID_PARAMS:
> > +             ret = EFI_INVALID_PARAMETER;
> > +             break;
> > +
> > +     case ARM_SVC_SPM_RET_DENIED:
> > +             ret = EFI_ACCESS_DENIED;
> > +             break;
> > +
> > +     case ARM_SVC_SPM_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 allocated communcation buffer
> > + * @dsize:           buffer size
> > + * Return:           status code
> > + */
> > +static efi_status_t mm_communicate(u8 *comm_buf, size_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) {
> > +             pr_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, size_t payload_size, size_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 = kzalloc(MM_COMMUNICATE_HEADER_SIZE +
> > +                                MM_VARIABLE_COMMUNICATE_SIZE + payload_size,
> > +                        GFP_KERNEL);
> > +     if (!comm_buf) {
> > +             *ret = EFI_OUT_OF_RESOURCES;
> > +             return NULL;
> > +     }
> > +
> > +     mm_hdr = (struct efi_mm_communicate_header *)comm_buf;
> > +     memcpy(&mm_hdr->header_guid, &mm_var_guid, sizeof(mm_hdr->header_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
> > + */
> > +static efi_status_t get_max_payload(size_t *size)
> > +{
> > +     struct smm_variable_payload_size *var_payload = NULL;
> > +     size_t payload_size;
> > +     u8 *comm_buf = NULL;
> > +     efi_status_t ret;
> > +
> > +     if (!size) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     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)
> > +             goto out;
> > +
> > +     ret = mm_communicate(comm_buf, payload_size);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     /* Make sure the buffer is big enough for storing variables */
> > +     if (var_payload->size < MM_VARIABLE_ACCESS_HEADER_SIZE + 0x20) {
> > +             ret = EFI_DEVICE_ERROR;
> > +             goto out;
> > +     }
> > +     *size = var_payload->size;
> > +     /*
> > +      * There seems to be a bug in EDK2 miscalculating the boundaries and
> > +      * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
> > +      * it up here to ensure backwards compatibility with older versions
> > +      * (cf. StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c.
> > +      * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the
> > +      * flexible array member).
> > +      *
> > +      * size is guaranteed to be > 2 due to checks on the beginning.
> > +      */
> > +     *size -= 2;
> > +out:
> > +     kfree(comm_buf);
> > +     return ret;
> > +}
> > +
> > +static efi_status_t get_property_int(u16 *name, size_t name_size,
> > +                                  const efi_guid_t *vendor,
> > +                                  struct var_check_property *var_property)
> > +{
> > +     struct smm_variable_var_check_property *smm_property;
> > +     size_t payload_size;
> > +     u8 *comm_buf = NULL;
> > +     efi_status_t ret;
> > +
> > +     memset(var_property, 0, sizeof(*var_property));
> > +     payload_size = sizeof(*smm_property) + name_size;
> > +     if (payload_size > max_payload_size) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +     comm_buf = setup_mm_hdr(
> > +             (void **)&smm_property, payload_size,
> > +             SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> > +     if (!comm_buf)
> > +             goto out;
> > +
> > +     memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
> > +     smm_property->name_size = name_size;
> > +     memcpy(smm_property->name, name, name_size);
> > +
> > +     ret = mm_communicate(comm_buf, payload_size);
> > +     /*
> > +      * Currently only R/O property is supported in StMM.
> > +      * Variables that are not set to R/O will not set the property in StMM
> > +      * and the call will return EFI_NOT_FOUND. We are setting the
> > +      * properties to 0x0 so checking against that is enough for the
> > +      * EFI_NOT_FOUND case.
> > +      */
> > +     if (ret == EFI_NOT_FOUND)
> > +             ret = EFI_SUCCESS;
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +     memcpy(var_property, &smm_property->property, sizeof(*var_property));
> > +
> > +out:
> > +     kfree(comm_buf);
> > +     return ret;
> > +}
> > +
> > +static efi_status_t optee_get_variable(u16 *name, efi_guid_t *vendor,
> > +                                    u32 *attributes,
> > +                                    unsigned long *data_size, void *data)
> > +{
> > +     struct var_check_property var_property;
> > +     struct smm_variable_access *var_acc;
> > +     size_t payload_size;
> > +     size_t name_size;
> > +     size_t tmp_dsize;
> > +     u8 *comm_buf = NULL;
> > +     efi_status_t ret;
> > +
> > +     if (!name || !vendor || !data_size) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > +     if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     /* 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)
> > +             goto out;
> > +
> > +     /* Fill in contents */
> > +     memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> > +     var_acc->data_size = tmp_dsize;
> > +     var_acc->name_size = name_size;
> > +     var_acc->attr = attributes ? *attributes : 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 out;
> > +
> > +     ret = get_property_int(name, name_size, vendor, &var_property);
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     if (attributes)
> > +             *attributes = var_acc->attr;
> > +
> > +     if (data)
> > +             memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> > +                    var_acc->data_size);
> > +     else
> > +             ret = EFI_INVALID_PARAMETER;
> > +
> > +out:
> > +     kfree(comm_buf);
> > +     return ret;
> > +}
> > +
> > +static efi_status_t optee_get_next_variable(unsigned long *name_size,
> > +                                         efi_char16_t *name,
> > +                                         efi_guid_t *guid)
> > +{
> > +     struct smm_variable_getnext *var_getnext;
> > +     size_t payload_size;
> > +     size_t out_name_size;
> > +     size_t in_name_size;
> > +     u8 *comm_buf = NULL;
> > +     efi_status_t ret;
> > +
> > +     if (!name_size || !name || !guid) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     out_name_size = *name_size;
> > +     in_name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > +
> > +     if (out_name_size < in_name_size) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     if (in_name_size >
> > +         max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     /* Trim output buffer size */
> > +     if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> > +             out_name_size =
> > +                     max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
> > +
> > +     payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
> > +     comm_buf = setup_mm_hdr((void **)&var_getnext, payload_size,
> > +                             SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> > +                             &ret);
> > +     if (!comm_buf)
> > +             goto out;
> > +
> > +     /* Fill in contents */
> > +     memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> > +     var_getnext->name_size = out_name_size;
> > +     memcpy(var_getnext->name, name, 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 */
> > +             *name_size = var_getnext->name_size;
> > +     }
> > +     if (ret != EFI_SUCCESS)
> > +             goto out;
> > +
> > +     memcpy(guid, &var_getnext->guid, sizeof(*guid));
> > +     memcpy(name, var_getnext->name, var_getnext->name_size);
> > +
> > +out:
> > +     kfree(comm_buf);
> > +     return ret;
> > +}
> > +
> > +static efi_status_t optee_set_variable(efi_char16_t *name, efi_guid_t *vendor,
> > +                                    u32 attributes, unsigned long data_size,
> > +                                    void *data)
> > +{
> > +     efi_status_t ret;
> > +     struct var_check_property var_property;
> > +     struct smm_variable_access *var_acc;
> > +     size_t payload_size;
> > +     size_t name_size;
> > +     u8 *comm_buf = NULL;
> > +
> > +     if (!name || name[0] == 0 || !vendor) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +     if (data_size > 0 && !data) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +     /* Check payload size */
> > +     name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > +     payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> > +     if (payload_size > max_payload_size) {
> > +             ret = EFI_INVALID_PARAMETER;
> > +             goto out;
> > +     }
> > +
> > +     /*
> > +      * Allocate the buffer early, before switching to RW (if needed)
> > +      * so we won't need to account for any failures in reading/setting
> > +      * the properties, if the allocation fails
> > +      */
> > +     comm_buf = setup_mm_hdr((void **)&var_acc, payload_size,
> > +                             SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> > +     if (!comm_buf)
> > +             goto out;
> > +
> > +     /*
> > +      * The API has the ability to override RO flags. If no RO check was
> > +      * requested switch the variable to RW for the duration of this call
> > +      */
> > +     ret = get_property_int(name, name_size, vendor, &var_property);
> > +     if (ret != EFI_SUCCESS) {
> > +             pr_err("Getting variable property failed\n");
> > +             goto out;
> > +     }
> > +
> > +     if (var_property.property & VAR_CHECK_VARIABLE_PROPERTY_READ_ONLY) {
> > +             ret = EFI_WRITE_PROTECTED;
> > +             goto out;
> > +     }
> > +
> > +     /* Fill in contents */
> > +     memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> > +     var_acc->data_size = data_size;
> > +     var_acc->name_size = name_size;
> > +     var_acc->attr = attributes;
> > +     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);
> > +     pr_info("Set Variable %s %d %lx\n", __FILE__, __LINE__, ret);
> > +out:
> > +     kfree(comm_buf);
> > +     return ret;
> > +}
> > +
> > +static int optee_stmm_efi_probe(struct device *dev)
> > +{
> > +     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;
> > +
> > +     optee_ops.get_variable = optee_get_variable;
> > +     optee_ops.get_next_variable = optee_get_next_variable;
> > +     optee_ops.set_variable = optee_set_variable;
> > +     /* TODO: support nonblocking variant */
> > +     optee_ops.set_variable_nonblocking = NULL;
> > +     /* set NULL, always return EFI_SUCCESS by efi_query_variable_store() */
> > +     optee_ops.query_variable_store = NULL;
>
> Why aren't you using static initialization instead?

On second thought, I think we can implement QueryVariableInfo(),
so it is better to set StMM based QueryVariableInfo() rather than set
efi_query_variable_store().

Thank you for your review.

Regards,
Masahisa Kojima



>
> Thanks,
> Jens
>
> > +
> > +     tee_register_efivar_ops(&optee_efivars, &optee_ops);
> > +
> > +     return 0;
> > +}
> > +
> > +static int optee_stmm_efi_remove(struct device *dev)
> > +{
> > +     tee_unregister_efivar_ops(&optee_efivars);
> > +
> > +     return 0;
> > +}
> > +
> > +MODULE_DEVICE_TABLE(tee, optee_stmm_efi_id_table);
> > +
> > +static struct tee_client_driver optee_stmm_efi_driver = {
> > +     .id_table       = optee_stmm_efi_id_table,
> > +     .driver         = {
> > +             .name           = "optee-stmm-efi",
> > +             .bus            = &tee_bus_type,
> > +             .probe          = optee_stmm_efi_probe,
> > +             .remove         = optee_stmm_efi_remove,
> > +     },
> > +};
> > +
> > +static int __init optee_stmm_efi_mod_init(void)
> > +{
> > +     return driver_register(&optee_stmm_efi_driver.driver);
> > +}
> > +
> > +static void __exit optee_stmm_efi_mod_exit(void)
> > +{
> > +     driver_unregister(&optee_stmm_efi_driver.driver);
> > +}
> > +
> > +module_init(optee_stmm_efi_mod_init);
> > +module_exit(optee_stmm_efi_mod_exit);
> > +
> > +MODULE_LICENSE("GPL v2");
> > +MODULE_AUTHOR("Ilias Apalodimas <ilias.apalodimas@linaro.org>");
> > +MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>");
> > +MODULE_DESCRIPTION("OP-TEE based EFI runtime variable service driver");
> > diff --git a/drivers/tee/tee_core.c b/drivers/tee/tee_core.c
> > index 98da206cd761..ac46274844a3 100644
> > --- a/drivers/tee/tee_core.c
> > +++ b/drivers/tee/tee_core.c
> > @@ -7,6 +7,7 @@
> >
> >  #include <linux/cdev.h>
> >  #include <linux/cred.h>
> > +#include <linux/efi.h>
> >  #include <linux/fs.h>
> >  #include <linux/idr.h>
> >  #include <linux/module.h>
> > @@ -1263,6 +1264,28 @@ static void __exit tee_exit(void)
> >       tee_class = NULL;
> >  }
> >
> > +void tee_register_efivar_ops(struct efivars *tee_efivars,
> > +                          struct efivar_operations *ops)
> > +{
> > +     /*
> > +      * If the firmware EFI runtime services support SetVariable(),
> > +      * tee-based EFI variable services are not used.
> > +      */
> > +     if (!efivar_supports_writes()) {
> > +             pr_info("Use tee-based EFI runtime variable services\n");
> > +             efivars_generic_ops_unregister();
> > +             efivars_register(tee_efivars, ops);
> > +     }
> > +}
> > +EXPORT_SYMBOL_GPL(tee_register_efivar_ops);
> > +
> > +void tee_unregister_efivar_ops(struct efivars *tee_efivars)
> > +{
> > +     efivars_unregister(tee_efivars);
> > +     efivars_generic_ops_register();
> > +}
> > +EXPORT_SYMBOL_GPL(tee_unregister_efivar_ops);
> > +
> >  subsys_initcall(tee_init);
> >  module_exit(tee_exit);
> >
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 5e301c00e9b0..14d4aa83ce60 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -39,6 +39,7 @@
> >  #define EFI_WRITE_PROTECTED  ( 8 | (1UL << (BITS_PER_LONG-1)))
> >  #define EFI_OUT_OF_RESOURCES ( 9 | (1UL << (BITS_PER_LONG-1)))
> >  #define EFI_NOT_FOUND                (14 | (1UL << (BITS_PER_LONG-1)))
> > +#define EFI_ACCESS_DENIED    (15 | (1UL << (BITS_PER_LONG-1)))
> >  #define EFI_TIMEOUT          (18 | (1UL << (BITS_PER_LONG-1)))
> >  #define EFI_ABORTED          (21 | (1UL << (BITS_PER_LONG-1)))
> >  #define EFI_SECURITY_VIOLATION       (26 | (1UL << (BITS_PER_LONG-1)))
> > diff --git a/include/linux/tee_drv.h b/include/linux/tee_drv.h
> > index 17eb1c5205d3..def4ea6212ee 100644
> > --- a/include/linux/tee_drv.h
> > +++ b/include/linux/tee_drv.h
> > @@ -7,6 +7,7 @@
> >  #define __TEE_DRV_H
> >
> >  #include <linux/device.h>
> > +#include <linux/efi.h>
> >  #include <linux/idr.h>
> >  #include <linux/kref.h>
> >  #include <linux/list.h>
> > @@ -507,4 +508,26 @@ struct tee_context *teedev_open(struct tee_device *teedev);
> >   */
> >  void teedev_close_context(struct tee_context *ctx);
> >
> > +/**
> > + * tee_register_efivar_ops() - register the efivar ops
> > + * @tee_efivars:     pointer to efivars structure
> > + * @ops:             pointer to contain the efivar operation
> > + *
> > + * This function registers the tee-based efivar operation as an
> > + * EFI Runtime Service.
> > + *
> > + */
> > +void tee_register_efivar_ops(struct efivars *tee_efivars,
> > +                          struct efivar_operations *ops);
> > +
> > +/**
> > + * tee_unregister_efivar_ops() - unregister the efivar ops
> > + * @tee_efivars:     pointer to efivars structure
> > + *
> > + * This function unregisters the tee-based efivar operation
> > + * and reverts to the generic operation.
> > + *
> > + */
> > +void tee_unregister_efivar_ops(struct efivars *tee_efivars);
> > +
> >  #endif /*__TEE_DRV_H*/
> > --
> > 2.30.2
> >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Sumit Garg]

Hi Masahisa,

On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> This RFC series introduces the op-tee based EFI Runtime Variable
> Service.
>
> The eMMC device is typically owned by the non-secure world(linux in
> this case). There is an existing solution utilizing eMMC RPMB partition
> for EFI Variables, it is implemented by interacting with
> OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> and tee-supplicant. The last piece is the tee-based variable access
> driver to interact with OP-TEE and StandaloneMM.
>

After an overall look at the APIs, following are some initial comments:
- Is there any reason to have the edk2 specific StandaloneMM stack in
Linux to communicate with OP-TEE pseudo TA?
- I think the OP-TEE pseudo TA should be able to expose a rather
generic invoke commands such as:
     TEE_EFI_GET_VARIABLE
     TEE_EFI_GET_NEXT_VARIABLE
     TEE_EFI_SET_VARIABLE
  So it should no longer be tied to StMM stack and other TEE
implementations can re-use the abstracted interface to communicate
with its corresponding secure storage TA.

-Sumit

> Masahisa Kojima (2):
>   efi: expose efivar generic ops register function
>   tee: Add op-tee helper functions for variable access
>
>  drivers/firmware/efi/efi.c           |  12 +
>  drivers/tee/optee/Kconfig            |  10 +
>  drivers/tee/optee/Makefile           |   1 +
>  drivers/tee/optee/mm_communication.h | 249 +++++++++++
>  drivers/tee/optee/optee_private.h    |   5 +-
>  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
>  drivers/tee/tee_core.c               |  23 ++
>  include/linux/efi.h                  |   4 +
>  include/linux/tee_drv.h              |  23 ++
>  9 files changed, 924 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/tee/optee/mm_communication.h
>  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
>
> --
> 2.30.2
>

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Ilias Apalodimas]

On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> Hi Masahisa,
>
> On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This RFC series introduces the op-tee based EFI Runtime Variable
> > Service.
> >
> > The eMMC device is typically owned by the non-secure world(linux in
> > this case). There is an existing solution utilizing eMMC RPMB partition
> > for EFI Variables, it is implemented by interacting with
> > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > and tee-supplicant. The last piece is the tee-based variable access
> > driver to interact with OP-TEE and StandaloneMM.
> >
>
> After an overall look at the APIs, following are some initial comments:
> - Is there any reason to have the edk2 specific StandaloneMM stack in
> Linux to communicate with OP-TEE pseudo TA?

The problem is StMM is not a pseudo-TA and pretending to be one
is not easy.  It looks like one, but due to the internal ABI, it's compiled
and launched differently within OP-TEE.  We could still add this but...

> - I think the OP-TEE pseudo TA should be able to expose a rather
> generic invoke commands such as:
>      TEE_EFI_GET_VARIABLE
>      TEE_EFI_GET_NEXT_VARIABLE
>      TEE_EFI_SET_VARIABLE
>   So it should no longer be tied to StMM stack and other TEE
> implementations can re-use the abstracted interface to communicate
> with its corresponding secure storage TA.

I talked about this with Jens, but there's an assumption here that every
TEE from that point onward will implement this.  But without a standard
describing this, I don't see how we can convince others.  The current code
puts the responsibility back to the tee/core subsystem, so any vendor can
provide his own callbacks

Regards
/Ilias
>
> -Sumit
>
> > Masahisa Kojima (2):
> >   efi: expose efivar generic ops register function
> >   tee: Add op-tee helper functions for variable access
> >
> >  drivers/firmware/efi/efi.c           |  12 +
> >  drivers/tee/optee/Kconfig            |  10 +
> >  drivers/tee/optee/Makefile           |   1 +
> >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> >  drivers/tee/optee/optee_private.h    |   5 +-
> >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> >  drivers/tee/tee_core.c               |  23 ++
> >  include/linux/efi.h                  |   4 +
> >  include/linux/tee_drv.h              |  23 ++
> >  9 files changed, 924 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/tee/optee/mm_communication.h
> >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> >
> > --
> > 2.30.2
> >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Jens Wiklander]

Hi Sumit,

On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> Hi Masahisa,
> 
> On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
> >
> > This RFC series introduces the op-tee based EFI Runtime Variable
> > Service.
> >
> > The eMMC device is typically owned by the non-secure world(linux in
> > this case). There is an existing solution utilizing eMMC RPMB partition
> > for EFI Variables, it is implemented by interacting with
> > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > and tee-supplicant. The last piece is the tee-based variable access
> > driver to interact with OP-TEE and StandaloneMM.
> >
> 
> After an overall look at the APIs, following are some initial comments:
> - Is there any reason to have the edk2 specific StandaloneMM stack in
> Linux to communicate with OP-TEE pseudo TA?
> - I think the OP-TEE pseudo TA should be able to expose a rather
> generic invoke commands such as:
>      TEE_EFI_GET_VARIABLE
>      TEE_EFI_GET_NEXT_VARIABLE
>      TEE_EFI_SET_VARIABLE
>   So it should no longer be tied to StMM stack and other TEE
> implementations can re-use the abstracted interface to communicate
> with its corresponding secure storage TA.

In the current setup we have the following layers in the kernel:
1. efivar_operations
2. MM
3. PTA_STMM
4. OP-TEE MSG

and in the secure world:
S1. internal to StMM
S2. MM interface to StMM
S3. PTA_STMM
S4. OP-TEE MSG

If I understand you correctly you'd like to see this instead:
Kernel:
1. efivar_operations
2. PTA_EFIVAR
4. OP-TEE MSG

Since we still have the MM interface with StMM we'd have this in the secure
world:
S1. internal to StMM
S2. MM interface to StMM
S3. PTA_EFIVAR
S4. OP-TEE MSG

At S3 we'd have to convert between EFIVAR and MM messages. The
difference is that we're moving the EFIVAR <-> MM conversion from the
non-secure world into the secure world. We're still using OP-TEE
specific communication at the fourth layer. So we're only moving problem
around, I'd rather avoid growing the OP-TEE part in the secure world.

Cheers,
Jens

> 
> -Sumit
> 
> > Masahisa Kojima (2):
> >   efi: expose efivar generic ops register function
> >   tee: Add op-tee helper functions for variable access
> >
> >  drivers/firmware/efi/efi.c           |  12 +
> >  drivers/tee/optee/Kconfig            |  10 +
> >  drivers/tee/optee/Makefile           |   1 +
> >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> >  drivers/tee/optee/optee_private.h    |   5 +-
> >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> >  drivers/tee/tee_core.c               |  23 ++
> >  include/linux/efi.h                  |   4 +
> >  include/linux/tee_drv.h              |  23 ++
> >  9 files changed, 924 insertions(+), 1 deletion(-)
> >  create mode 100644 drivers/tee/optee/mm_communication.h
> >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> >
> > --
> > 2.30.2
> >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Sumit Garg]

Hi Jens,

On Fri, 3 Feb 2023 at 13:59, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> Hi Sumit,
>
> On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> > Hi Masahisa,
> >
> > On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> > >
> > > This RFC series introduces the op-tee based EFI Runtime Variable
> > > Service.
> > >
> > > The eMMC device is typically owned by the non-secure world(linux in
> > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > for EFI Variables, it is implemented by interacting with
> > > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > > and tee-supplicant. The last piece is the tee-based variable access
> > > driver to interact with OP-TEE and StandaloneMM.
> > >
> >
> > After an overall look at the APIs, following are some initial comments:
> > - Is there any reason to have the edk2 specific StandaloneMM stack in
> > Linux to communicate with OP-TEE pseudo TA?
> > - I think the OP-TEE pseudo TA should be able to expose a rather
> > generic invoke commands such as:
> >      TEE_EFI_GET_VARIABLE
> >      TEE_EFI_GET_NEXT_VARIABLE
> >      TEE_EFI_SET_VARIABLE
> >   So it should no longer be tied to StMM stack and other TEE
> > implementations can re-use the abstracted interface to communicate
> > with its corresponding secure storage TA.
>
> In the current setup we have the following layers in the kernel:
> 1. efivar_operations
> 2. MM
> 3. PTA_STMM
> 4. OP-TEE MSG
>
> and in the secure world:
> S1. internal to StMM
> S2. MM interface to StMM
> S3. PTA_STMM
> S4. OP-TEE MSG
>
> If I understand you correctly you'd like to see this instead:
> Kernel:
> 1. efivar_operations
> 2. PTA_EFIVAR
> 4. OP-TEE MSG
>
> Since we still have the MM interface with StMM we'd have this in the secure
> world:
> S1. internal to StMM
> S2. MM interface to StMM
> S3. PTA_EFIVAR
> S4. OP-TEE MSG
>
> At S3 we'd have to convert between EFIVAR and MM messages. The
> difference is that we're moving the EFIVAR <-> MM conversion from the
> non-secure world into the secure world. We're still using OP-TEE
> specific communication at the fourth layer. So we're only moving problem
> around, I'd rather avoid growing the OP-TEE part in the secure world.
>

If you look carefully, we are essentially defining an ABI towards the
secure world. The approach in this patch-set adds the MM interface as
a redundant ABI layer which makes it complex to maintain. Now think
about if every TEE implementation would propose such a complex ABI. It
looks like a maintenance nightmare to me.

The concerns you are highlighting about OP-TEE size, I think those are
implementation details which can be simplified later but once you have
defined an ABI then you are stuck with its maintainability.

-Sumit

> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Masahisa Kojima (2):
> > >   efi: expose efivar generic ops register function
> > >   tee: Add op-tee helper functions for variable access
> > >
> > >  drivers/firmware/efi/efi.c           |  12 +
> > >  drivers/tee/optee/Kconfig            |  10 +
> > >  drivers/tee/optee/Makefile           |   1 +
> > >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> > >  drivers/tee/optee/optee_private.h    |   5 +-
> > >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> > >  drivers/tee/tee_core.c               |  23 ++
> > >  include/linux/efi.h                  |   4 +
> > >  include/linux/tee_drv.h              |  23 ++
> > >  9 files changed, 924 insertions(+), 1 deletion(-)
> > >  create mode 100644 drivers/tee/optee/mm_communication.h
> > >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> > >
> > > --
> > > 2.30.2
> > >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Jens Wiklander]

On Fri, Feb 03, 2023 at 03:03:34PM +0530, Sumit Garg wrote:
> Hi Jens,
> 
> On Fri, 3 Feb 2023 at 13:59, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > Hi Sumit,
> >
> > On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> > > Hi Masahisa,
> > >
> > > On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> > > <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > This RFC series introduces the op-tee based EFI Runtime Variable
> > > > Service.
> > > >
> > > > The eMMC device is typically owned by the non-secure world(linux in
> > > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > > for EFI Variables, it is implemented by interacting with
> > > > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > > > and tee-supplicant. The last piece is the tee-based variable access
> > > > driver to interact with OP-TEE and StandaloneMM.
> > > >
> > >
> > > After an overall look at the APIs, following are some initial comments:
> > > - Is there any reason to have the edk2 specific StandaloneMM stack in
> > > Linux to communicate with OP-TEE pseudo TA?
> > > - I think the OP-TEE pseudo TA should be able to expose a rather
> > > generic invoke commands such as:
> > >      TEE_EFI_GET_VARIABLE
> > >      TEE_EFI_GET_NEXT_VARIABLE
> > >      TEE_EFI_SET_VARIABLE
> > >   So it should no longer be tied to StMM stack and other TEE
> > > implementations can re-use the abstracted interface to communicate
> > > with its corresponding secure storage TA.
> >
> > In the current setup we have the following layers in the kernel:
> > 1. efivar_operations
> > 2. MM
> > 3. PTA_STMM
> > 4. OP-TEE MSG
> >
> > and in the secure world:
> > S1. internal to StMM
> > S2. MM interface to StMM
> > S3. PTA_STMM
> > S4. OP-TEE MSG
> >
> > If I understand you correctly you'd like to see this instead:
> > Kernel:
> > 1. efivar_operations
> > 2. PTA_EFIVAR
> > 4. OP-TEE MSG
> >
> > Since we still have the MM interface with StMM we'd have this in the secure
> > world:
> > S1. internal to StMM
> > S2. MM interface to StMM
> > S3. PTA_EFIVAR
> > S4. OP-TEE MSG
> >
> > At S3 we'd have to convert between EFIVAR and MM messages. The
> > difference is that we're moving the EFIVAR <-> MM conversion from the
> > non-secure world into the secure world. We're still using OP-TEE
> > specific communication at the fourth layer. So we're only moving problem
> > around, I'd rather avoid growing the OP-TEE part in the secure world.
> >
> 
> If you look carefully, we are essentially defining an ABI towards the
> secure world. The approach in this patch-set adds the MM interface as
> a redundant ABI layer which makes it complex to maintain. Now think
> about if every TEE implementation would propose such a complex ABI. It
> looks like a maintenance nightmare to me.
> 
> The concerns you are highlighting about OP-TEE size, I think those are
> implementation details which can be simplified later but once you have
> defined an ABI then you are stuck with its maintainability.

You have a point, but keep in mind that it's StMM that matters here.
StMM uses the MM protocol. It was originially using raw SMCs as a
conduit, but with the need for OP-TEE accessing RPMB that's not usable.
So instead we use OP-TEE MSG as a conduit. Seen from that perspective
we're only resuing something established instead of inventing something
new.

Cheers,
Jens

> 
> -Sumit
> 
> > Cheers,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > > Masahisa Kojima (2):
> > > >   efi: expose efivar generic ops register function
> > > >   tee: Add op-tee helper functions for variable access
> > > >
> > > >  drivers/firmware/efi/efi.c           |  12 +
> > > >  drivers/tee/optee/Kconfig            |  10 +
> > > >  drivers/tee/optee/Makefile           |   1 +
> > > >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> > > >  drivers/tee/optee/optee_private.h    |   5 +-
> > > >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> > > >  drivers/tee/tee_core.c               |  23 ++
> > > >  include/linux/efi.h                  |   4 +
> > > >  include/linux/tee_drv.h              |  23 ++
> > > >  9 files changed, 924 insertions(+), 1 deletion(-)
> > > >  create mode 100644 drivers/tee/optee/mm_communication.h
> > > >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> > > >
> > > > --
> > > > 2.30.2
> > > >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Sumit Garg]

On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
>
> On Fri, Feb 03, 2023 at 03:03:34PM +0530, Sumit Garg wrote:
> > Hi Jens,
> >
> > On Fri, 3 Feb 2023 at 13:59, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> > > Hi Sumit,
> > >
> > > On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> > > > Hi Masahisa,
> > > >
> > > > On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> > > > <masahisa.kojima@linaro.org> wrote:
> > > > >
> > > > > This RFC series introduces the op-tee based EFI Runtime Variable
> > > > > Service.
> > > > >
> > > > > The eMMC device is typically owned by the non-secure world(linux in
> > > > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > > > for EFI Variables, it is implemented by interacting with
> > > > > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > > > > and tee-supplicant. The last piece is the tee-based variable access
> > > > > driver to interact with OP-TEE and StandaloneMM.
> > > > >
> > > >
> > > > After an overall look at the APIs, following are some initial comments:
> > > > - Is there any reason to have the edk2 specific StandaloneMM stack in
> > > > Linux to communicate with OP-TEE pseudo TA?
> > > > - I think the OP-TEE pseudo TA should be able to expose a rather
> > > > generic invoke commands such as:
> > > >      TEE_EFI_GET_VARIABLE
> > > >      TEE_EFI_GET_NEXT_VARIABLE
> > > >      TEE_EFI_SET_VARIABLE
> > > >   So it should no longer be tied to StMM stack and other TEE
> > > > implementations can re-use the abstracted interface to communicate
> > > > with its corresponding secure storage TA.
> > >
> > > In the current setup we have the following layers in the kernel:
> > > 1. efivar_operations
> > > 2. MM
> > > 3. PTA_STMM
> > > 4. OP-TEE MSG
> > >
> > > and in the secure world:
> > > S1. internal to StMM
> > > S2. MM interface to StMM
> > > S3. PTA_STMM
> > > S4. OP-TEE MSG
> > >
> > > If I understand you correctly you'd like to see this instead:
> > > Kernel:
> > > 1. efivar_operations
> > > 2. PTA_EFIVAR
> > > 4. OP-TEE MSG
> > >
> > > Since we still have the MM interface with StMM we'd have this in the secure
> > > world:
> > > S1. internal to StMM
> > > S2. MM interface to StMM
> > > S3. PTA_EFIVAR
> > > S4. OP-TEE MSG
> > >
> > > At S3 we'd have to convert between EFIVAR and MM messages. The
> > > difference is that we're moving the EFIVAR <-> MM conversion from the
> > > non-secure world into the secure world. We're still using OP-TEE
> > > specific communication at the fourth layer. So we're only moving problem
> > > around, I'd rather avoid growing the OP-TEE part in the secure world.
> > >
> >
> > If you look carefully, we are essentially defining an ABI towards the
> > secure world. The approach in this patch-set adds the MM interface as
> > a redundant ABI layer which makes it complex to maintain. Now think
> > about if every TEE implementation would propose such a complex ABI. It
> > looks like a maintenance nightmare to me.
> >
> > The concerns you are highlighting about OP-TEE size, I think those are
> > implementation details which can be simplified later but once you have
> > defined an ABI then you are stuck with its maintainability.
>
> You have a point, but keep in mind that it's StMM that matters here.
> StMM uses the MM protocol. It was originially using raw SMCs as a
> conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> we're only resuing something established instead of inventing something
> new.

Aren't we already adding PTA_STMM?

Isn't the StMM specific to Arm as you already mentioned it was
designed to specifically use raw SMCs? So if in future AMD TEE wants
to implement EFI services, can we suggest they reuse the MM interface?

I am not sure why we need to redirect EFI variables via MM interface
communication buffers rather than directly using the TEE shared memory
approach.

Ard,

Since you have better insights into how EFI runtime services have to
be implemented, can you share your opinion here? It may be something I
am missing here.

-Sumit

>
> Cheers,
> Jens
>
> >
> > -Sumit
> >
> > > Cheers,
> > > Jens
> > >
> > > >
> > > > -Sumit
> > > >
> > > > > Masahisa Kojima (2):
> > > > >   efi: expose efivar generic ops register function
> > > > >   tee: Add op-tee helper functions for variable access
> > > > >
> > > > >  drivers/firmware/efi/efi.c           |  12 +
> > > > >  drivers/tee/optee/Kconfig            |  10 +
> > > > >  drivers/tee/optee/Makefile           |   1 +
> > > > >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> > > > >  drivers/tee/optee/optee_private.h    |   5 +-
> > > > >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> > > > >  drivers/tee/tee_core.c               |  23 ++
> > > > >  include/linux/efi.h                  |   4 +
> > > > >  include/linux/tee_drv.h              |  23 ++
> > > > >  9 files changed, 924 insertions(+), 1 deletion(-)
> > > > >  create mode 100644 drivers/tee/optee/mm_communication.h
> > > > >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Jens Wiklander]

On Mon, Feb 6, 2023 at 7:44 AM Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > On Fri, Feb 03, 2023 at 03:03:34PM +0530, Sumit Garg wrote:
> > > Hi Jens,
> > >
> > > On Fri, 3 Feb 2023 at 13:59, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > > > Hi Sumit,
> > > >
> > > > On Thu, Feb 02, 2023 at 05:35:49PM +0530, Sumit Garg wrote:
> > > > > Hi Masahisa,
> > > > >
> > > > > On Thu, 26 Jan 2023 at 18:52, Masahisa Kojima
> > > > > <masahisa.kojima@linaro.org> wrote:
> > > > > >
> > > > > > This RFC series introduces the op-tee based EFI Runtime Variable
> > > > > > Service.
> > > > > >
> > > > > > The eMMC device is typically owned by the non-secure world(linux in
> > > > > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > > > > for EFI Variables, it is implemented by interacting with
> > > > > > OP-TEE, StandaloneMM(as EFI Variable Service Pseudo TA), eMMC driver
> > > > > > and tee-supplicant. The last piece is the tee-based variable access
> > > > > > driver to interact with OP-TEE and StandaloneMM.
> > > > > >
> > > > >
> > > > > After an overall look at the APIs, following are some initial comments:
> > > > > - Is there any reason to have the edk2 specific StandaloneMM stack in
> > > > > Linux to communicate with OP-TEE pseudo TA?
> > > > > - I think the OP-TEE pseudo TA should be able to expose a rather
> > > > > generic invoke commands such as:
> > > > >      TEE_EFI_GET_VARIABLE
> > > > >      TEE_EFI_GET_NEXT_VARIABLE
> > > > >      TEE_EFI_SET_VARIABLE
> > > > >   So it should no longer be tied to StMM stack and other TEE
> > > > > implementations can re-use the abstracted interface to communicate
> > > > > with its corresponding secure storage TA.
> > > >
> > > > In the current setup we have the following layers in the kernel:
> > > > 1. efivar_operations
> > > > 2. MM
> > > > 3. PTA_STMM
> > > > 4. OP-TEE MSG
> > > >
> > > > and in the secure world:
> > > > S1. internal to StMM
> > > > S2. MM interface to StMM
> > > > S3. PTA_STMM
> > > > S4. OP-TEE MSG
> > > >
> > > > If I understand you correctly you'd like to see this instead:
> > > > Kernel:
> > > > 1. efivar_operations
> > > > 2. PTA_EFIVAR
> > > > 4. OP-TEE MSG
> > > >
> > > > Since we still have the MM interface with StMM we'd have this in the secure
> > > > world:
> > > > S1. internal to StMM
> > > > S2. MM interface to StMM
> > > > S3. PTA_EFIVAR
> > > > S4. OP-TEE MSG
> > > >
> > > > At S3 we'd have to convert between EFIVAR and MM messages. The
> > > > difference is that we're moving the EFIVAR <-> MM conversion from the
> > > > non-secure world into the secure world. We're still using OP-TEE
> > > > specific communication at the fourth layer. So we're only moving problem
> > > > around, I'd rather avoid growing the OP-TEE part in the secure world.
> > > >
> > >
> > > If you look carefully, we are essentially defining an ABI towards the
> > > secure world. The approach in this patch-set adds the MM interface as
> > > a redundant ABI layer which makes it complex to maintain. Now think
> > > about if every TEE implementation would propose such a complex ABI. It
> > > looks like a maintenance nightmare to me.
> > >
> > > The concerns you are highlighting about OP-TEE size, I think those are
> > > implementation details which can be simplified later but once you have
> > > defined an ABI then you are stuck with its maintainability.
> >
> > You have a point, but keep in mind that it's StMM that matters here.
> > StMM uses the MM protocol. It was originially using raw SMCs as a
> > conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> > So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> > we're only resuing something established instead of inventing something
> > new.
>
> Aren't we already adding PTA_STMM?

Yes, something is need to recieve those messages and forward the MM
stuff to secure user space.

>
> Isn't the StMM specific to Arm as you already mentioned it was
> designed to specifically use raw SMCs? So if in future AMD TEE wants
> to implement EFI services, can we suggest they reuse the MM interface?

I wouldn't suggest anything until I understood that problem better.

>
> I am not sure why we need to redirect EFI variables via MM interface
> communication buffers rather than directly using the TEE shared memory
> approach.

I allways assumed that was done in order to keep the changes in StMM
at a mininum compared to non-TEE configurations.

Cheers,
Jens

>
> Ard,
>
> Since you have better insights into how EFI runtime services have to
> be implemented, can you share your opinion here? It may be something I
> am missing here.
>
> -Sumit
>
> >
> > Cheers,
> > Jens
> >
> > >
> > > -Sumit
> > >
> > > > Cheers,
> > > > Jens
> > > >
> > > > >
> > > > > -Sumit
> > > > >
> > > > > > Masahisa Kojima (2):
> > > > > >   efi: expose efivar generic ops register function
> > > > > >   tee: Add op-tee helper functions for variable access
> > > > > >
> > > > > >  drivers/firmware/efi/efi.c           |  12 +
> > > > > >  drivers/tee/optee/Kconfig            |  10 +
> > > > > >  drivers/tee/optee/Makefile           |   1 +
> > > > > >  drivers/tee/optee/mm_communication.h | 249 +++++++++++
> > > > > >  drivers/tee/optee/optee_private.h    |   5 +-
> > > > > >  drivers/tee/optee/optee_stmm_efi.c   | 598 +++++++++++++++++++++++++++
> > > > > >  drivers/tee/tee_core.c               |  23 ++
> > > > > >  include/linux/efi.h                  |   4 +
> > > > > >  include/linux/tee_drv.h              |  23 ++
> > > > > >  9 files changed, 924 insertions(+), 1 deletion(-)
> > > > > >  create mode 100644 drivers/tee/optee/mm_communication.h
> > > > > >  create mode 100644 drivers/tee/optee/optee_stmm_efi.c
> > > > > >
> > > > > > --
> > > > > > 2.30.2
> > > > > >

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Ard Biesheuvel]

On Mon, 6 Feb 2023 at 07:44, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
..
> > StMM uses the MM protocol. It was originially using raw SMCs as a
> > conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> > So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> > we're only resuing something established instead of inventing something
> > new.
>
> Aren't we already adding PTA_STMM?
>
> Isn't the StMM specific to Arm as you already mentioned it was
> designed to specifically use raw SMCs? So if in future AMD TEE wants
> to implement EFI services, can we suggest they reuse the MM interface?
>
> I am not sure why we need to redirect EFI variables via MM interface
> communication buffers rather than directly using the TEE shared memory
> approach.
>
> Ard,
>
> Since you have better insights into how EFI runtime services have to
> be implemented, can you share your opinion here? It may be something I
> am missing here.
>

Hello Sumit,

I'm not sure I understand what you are asking me here. Allow me to
reiterate, apologies if I am stating the obvious:

The EFI spec describes how the OS should expose the EFI runtime
services, but this is difficult to implement when access to the
underlying storage requires arbitration between accesses by the OS
itself and accesses made by the firmware.

On systems where this issue is absent, the EFI runtime service
implementation for the variable services are very thin wrappers around
calls into standalone MM, which are not standardized, but are also not
ARM specific (standalone MM is being used on other architectures as
well, and 'classic' SMM uses the same protocol but dispatches the call
into the secure/SMM world in a different way)

On systems where arbitration is needed, the standalone MM payload
needs to call back up into the OS to request access to the flash
storage. OP-TEE is a suitable vehicle for this, as it already does the
same thing for other reasons, and is already set up to dispatch SMC
calls that are taken to S-EL1.

All of his is uncharted territory as far as the EFI spec is concerned,
as it occurs inside the StMM pseudo-API call, which itself normally
occurs inside the EFI runtime service call. Given that we cannot use
the latter (as the firmware does not provide a working get/setvariable
at OS runtime [0]), we need to provide some plumbing to call the StMM
pseudo-API directly, and expose it as an alternative efivars
implementation. (Note that this should mean that other implementations
of the StMM pseudo-API that do not require this arbitration may
potentially be accessed in the same way, although I don't see a reason
to use it like that.)

If I am understanding you correctly, your question is whether OP-TEE
should expose the StMM pseudo-API in the usual way as well, and make
the OS rely on the usual discovery mechanisms etc to bind to it?

If that is indeed your question: what purpose would that serve,
exactly? In principle, the OS piece that implements efivars on top of
the StMM pseudo-API should not be specific to any TEE implementation
or API, and I think the fact that OP-TEE is the provider in this case
is an implementation detail.

If you feel that OP-TEE should not expose this interface directly to
begin with, and rely on some marshalling layer to expose the StMM
pseudo-API on top of its ordinary exposed API, that is really an
OP-TEE internal matter (which I think is what you discussed with Jens
up in the thread?) Since StMM calls are defined in terms of SMC
instructions + arguments, this would require more code inside OP-TEE
to translate access to an API that it already exposes directly as
well, so I'm not sure what the gain would be.

The thing to remember is that, even though the wrappers are very thin,
it is fundamentally the StMM API that is being exposed, not the EFI
runtime services API, and so whether or not a use case may materialize
that wants to expose a different API via efivars is out of scope here,
even if they are roughly shaped like get/setvariable.



-- 
Ard.


[0] It is permitted for implementations of, e.g., Get/SetTime or
ResetSystem to use Get/SetVariable internally, and this is quite
clearly broken if the EFI variable services cannot be used. However,
firmware implementations would presumably avoid that situation.

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Sumit Garg]

Thanks Ard for the detailed background information.

On Mon, 6 Feb 2023 at 14:52, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Mon, 6 Feb 2023 at 07:44, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > >
> ..
> > > StMM uses the MM protocol. It was originially using raw SMCs as a
> > > conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> > > So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> > > we're only resuing something established instead of inventing something
> > > new.
> >
> > Aren't we already adding PTA_STMM?
> >
> > Isn't the StMM specific to Arm as you already mentioned it was
> > designed to specifically use raw SMCs? So if in future AMD TEE wants
> > to implement EFI services, can we suggest they reuse the MM interface?
> >
> > I am not sure why we need to redirect EFI variables via MM interface
> > communication buffers rather than directly using the TEE shared memory
> > approach.
> >
> > Ard,
> >
> > Since you have better insights into how EFI runtime services have to
> > be implemented, can you share your opinion here? It may be something I
> > am missing here.
> >
>
> Hello Sumit,
>
> I'm not sure I understand what you are asking me here. Allow me to
> reiterate, apologies if I am stating the obvious:
>
> The EFI spec describes how the OS should expose the EFI runtime
> services, but this is difficult to implement when access to the
> underlying storage requires arbitration between accesses by the OS
> itself and accesses made by the firmware.

Agree.

>
> On systems where this issue is absent, the EFI runtime service
> implementation for the variable services are very thin wrappers around
> calls into standalone MM, which are not standardized, but are also not
> ARM specific (standalone MM is being used on other architectures as
> well, and 'classic' SMM uses the same protocol but dispatches the call
> into the secure/SMM world in a different way)
>

Thanks for the clarification. So wouldn't it be better to have the
standalone MM API reside here: drivers/firmware/efi/ and it should be
exposed instead of efivars ops? As you mention below that there is
nothing OP-TEE specific in there.

> On systems where arbitration is needed, the standalone MM payload
> needs to call back up into the OS to request access to the flash
> storage. OP-TEE is a suitable vehicle for this, as it already does the
> same thing for other reasons, and is already set up to dispatch SMC
> calls that are taken to S-EL1.

Agree.

>
> All of his is uncharted territory as far as the EFI spec is concerned,
> as it occurs inside the StMM pseudo-API call, which itself normally
> occurs inside the EFI runtime service call. Given that we cannot use
> the latter (as the firmware does not provide a working get/setvariable
> at OS runtime [0]), we need to provide some plumbing to call the StMM
> pseudo-API directly, and expose it as an alternative efivars
> implementation. (Note that this should mean that other implementations
> of the StMM pseudo-API that do not require this arbitration may
> potentially be accessed in the same way, although I don't see a reason
> to use it like that.)
>
> If I am understanding you correctly, your question is whether OP-TEE
> should expose the StMM pseudo-API in the usual way as well, and make
> the OS rely on the usual discovery mechanisms etc to bind to it?

No, I am trying to understand and generalize how an EFI runtime
service ABI would look like among Linux kernel and a TEE. As you may
be aware there are multiple TEE implementations and OP-TEE is one of
them. So we should try to have a generic TEE client driver [1] rather
than every other TEE implementation coming up with its own driver.

>
> If that is indeed your question: what purpose would that serve,
> exactly?

> In principle, the OS piece that implements efivars on top of
> the StMM pseudo-API should not be specific to any TEE implementation
> or API, and I think the fact that OP-TEE is the provider in this case
> is an implementation detail.

Yeah as I said above we should abstract the StMM pieces out of an
OP-TEE driver and then the driver can be a generic TEE client driver
which is just providing the underline vehicle (invoke commands and
StMM buffer passing) as you described above.

>
> If you feel that OP-TEE should not expose this interface directly to
> begin with, and rely on some marshalling layer to expose the StMM
> pseudo-API on top of its ordinary exposed API, that is really an
> OP-TEE internal matter (which I think is what you discussed with Jens
> up in the thread?) Since StMM calls are defined in terms of SMC
> instructions + arguments, this would require more code inside OP-TEE
> to translate access to an API that it already exposes directly as
> well, so I'm not sure what the gain would be.

No I am not against OP-TEE exposing StMM stuff but rather the StMM
stuff (buffer allocation etc.) being bundled into OP-TEE client
driver.

>
> The thing to remember is that, even though the wrappers are very thin,
> it is fundamentally the StMM API that is being exposed, not the EFI
> runtime services API, and so whether or not a use case may materialize
> that wants to expose a different API via efivars is out of scope here,
> even if they are roughly shaped like get/setvariable.
>

Okay I get your point. If we want the StMM API to be exposed then I
think the EFI subsystem is the suitable place for that.

[1] Although there can be minor differences allowed based on TEE
implementation ID. You can consider it analogous to how we use
multiple DT compatibles for a generic platform driver.

-Sumit

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Masahisa Kojima]

On Mon, 6 Feb 2023 at 20:12, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Thanks Ard for the detailed background information.
>
> On Mon, 6 Feb 2023 at 14:52, Ard Biesheuvel <ardb@kernel.org> wrote:
> >
> > On Mon, 6 Feb 2023 at 07:44, Sumit Garg <sumit.garg@linaro.org> wrote:
> > >
> > > On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> > > >
> > ..
> > > > StMM uses the MM protocol. It was originially using raw SMCs as a
> > > > conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> > > > So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> > > > we're only resuing something established instead of inventing something
> > > > new.
> > >
> > > Aren't we already adding PTA_STMM?
> > >
> > > Isn't the StMM specific to Arm as you already mentioned it was
> > > designed to specifically use raw SMCs? So if in future AMD TEE wants
> > > to implement EFI services, can we suggest they reuse the MM interface?
> > >
> > > I am not sure why we need to redirect EFI variables via MM interface
> > > communication buffers rather than directly using the TEE shared memory
> > > approach.
> > >
> > > Ard,
> > >
> > > Since you have better insights into how EFI runtime services have to
> > > be implemented, can you share your opinion here? It may be something I
> > > am missing here.
> > >
> >
> > Hello Sumit,
> >
> > I'm not sure I understand what you are asking me here. Allow me to
> > reiterate, apologies if I am stating the obvious:
> >
> > The EFI spec describes how the OS should expose the EFI runtime
> > services, but this is difficult to implement when access to the
> > underlying storage requires arbitration between accesses by the OS
> > itself and accesses made by the firmware.
>
> Agree.
>
> >
> > On systems where this issue is absent, the EFI runtime service
> > implementation for the variable services are very thin wrappers around
> > calls into standalone MM, which are not standardized, but are also not
> > ARM specific (standalone MM is being used on other architectures as
> > well, and 'classic' SMM uses the same protocol but dispatches the call
> > into the secure/SMM world in a different way)
> >
>
> Thanks for the clarification. So wouldn't it be better to have the
> standalone MM API reside here: drivers/firmware/efi/ and it should be
> exposed instead of efivars ops? As you mention below that there is
> nothing OP-TEE specific in there.
>
> > On systems where arbitration is needed, the standalone MM payload
> > needs to call back up into the OS to request access to the flash
> > storage. OP-TEE is a suitable vehicle for this, as it already does the
> > same thing for other reasons, and is already set up to dispatch SMC
> > calls that are taken to S-EL1.
>
> Agree.
>
> >
> > All of his is uncharted territory as far as the EFI spec is concerned,
> > as it occurs inside the StMM pseudo-API call, which itself normally
> > occurs inside the EFI runtime service call. Given that we cannot use
> > the latter (as the firmware does not provide a working get/setvariable
> > at OS runtime [0]), we need to provide some plumbing to call the StMM
> > pseudo-API directly, and expose it as an alternative efivars
> > implementation. (Note that this should mean that other implementations
> > of the StMM pseudo-API that do not require this arbitration may
> > potentially be accessed in the same way, although I don't see a reason
> > to use it like that.)
> >
> > If I am understanding you correctly, your question is whether OP-TEE
> > should expose the StMM pseudo-API in the usual way as well, and make
> > the OS rely on the usual discovery mechanisms etc to bind to it?
>
> No, I am trying to understand and generalize how an EFI runtime
> service ABI would look like among Linux kernel and a TEE. As you may
> be aware there are multiple TEE implementations and OP-TEE is one of
> them. So we should try to have a generic TEE client driver [1] rather
> than every other TEE implementation coming up with its own driver.
>
> >
> > If that is indeed your question: what purpose would that serve,
> > exactly?
>
> > In principle, the OS piece that implements efivars on top of
> > the StMM pseudo-API should not be specific to any TEE implementation
> > or API, and I think the fact that OP-TEE is the provider in this case
> > is an implementation detail.
>
> Yeah as I said above we should abstract the StMM pieces out of an
> OP-TEE driver and then the driver can be a generic TEE client driver
> which is just providing the underline vehicle (invoke commands and
> StMM buffer passing) as you described above.
>
> >
> > If you feel that OP-TEE should not expose this interface directly to
> > begin with, and rely on some marshalling layer to expose the StMM
> > pseudo-API on top of its ordinary exposed API, that is really an
> > OP-TEE internal matter (which I think is what you discussed with Jens
> > up in the thread?) Since StMM calls are defined in terms of SMC
> > instructions + arguments, this would require more code inside OP-TEE
> > to translate access to an API that it already exposes directly as
> > well, so I'm not sure what the gain would be.
>
> No I am not against OP-TEE exposing StMM stuff but rather the StMM
> stuff (buffer allocation etc.) being bundled into OP-TEE client
> driver.
>
> >
> > The thing to remember is that, even though the wrappers are very thin,
> > it is fundamentally the StMM API that is being exposed, not the EFI
> > runtime services API, and so whether or not a use case may materialize
> > that wants to expose a different API via efivars is out of scope here,
> > even if they are roughly shaped like get/setvariable.
> >
>
> Okay I get your point. If we want the StMM API to be exposed then I
> think the EFI subsystem is the suitable place for that.

Thank you for your comments.
In the next version, I move the StMM code under drivers/firmware/efi/stmm,
then 'optee' prefix is changed to 'tee' because StMM code does not contain
OP-TEE specific code.

Regards,
Masahisa Kojima

>
> [1] Although there can be minor differences allowed based on TEE
> implementation ID. You can consider it analogous to how we use
> multiple DT compatibles for a generic platform driver.
>
> -Sumit

Re: [RFC PATCH 0/2] introduce op-tee based EFI Runtime Variable Service

[Ilias Apalodimas]

On Mon, Feb 06, 2023 at 12:14:12PM +0530, Sumit Garg wrote:
> On Fri, 3 Feb 2023 at 16:25, Jens Wiklander <jens.wiklander@linaro.org> wrote:
> >
> > > > > with its corresponding secure storage TA.
> > > >
> > > > In the current setup we have the following layers in the kernel:
> > > > 1. efivar_operations
> > > > 2. MM
> > > > 3. PTA_STMM
> > > > 4. OP-TEE MSG
> > > >
> > > > and in the secure world:
> > > > S1. internal to StMM
> > > > S2. MM interface to StMM
> > > > S3. PTA_STMM
> > > > S4. OP-TEE MSG
> > > >
> > > > If I understand you correctly you'd like to see this instead:
> > > > Kernel:
> > > > 1. efivar_operations
> > > > 2. PTA_EFIVAR
> > > > 4. OP-TEE MSG
> > > >
> > > > Since we still have the MM interface with StMM we'd have this in the secure
> > > > world:
> > > > S1. internal to StMM
> > > > S2. MM interface to StMM
> > > > S3. PTA_EFIVAR
> > > > S4. OP-TEE MSG
> > > >
> > > > At S3 we'd have to convert between EFIVAR and MM messages. The
> > > > difference is that we're moving the EFIVAR <-> MM conversion from the
> > > > non-secure world into the secure world. We're still using OP-TEE
> > > > specific communication at the fourth layer. So we're only moving problem
> > > > around, I'd rather avoid growing the OP-TEE part in the secure world.
> > > >
> > >
> > > If you look carefully, we are essentially defining an ABI towards the
> > > secure world. The approach in this patch-set adds the MM interface as
> > > a redundant ABI layer which makes it complex to maintain. Now think
> > > about if every TEE implementation would propose such a complex ABI. It
> > > looks like a maintenance nightmare to me.
> > >
> > > The concerns you are highlighting about OP-TEE size, I think those are
> > > implementation details which can be simplified later but once you have
> > > defined an ABI then you are stuck with its maintainability.
> >
> > You have a point, but keep in mind that it's StMM that matters here.
> > StMM uses the MM protocol. It was originially using raw SMCs as a
> > conduit, but with the need for OP-TEE accessing RPMB that's not usable.
> > So instead we use OP-TEE MSG as a conduit. Seen from that perspective
> > we're only resuing something established instead of inventing something
> > new.
> 
> Aren't we already adding PTA_STMM?

There's a sequence diagram that might help here [0].
The StMM PTA,  is responsible for wrapping the buffer it received from the
NS-world into an MM buffer.

> 
> Isn't the StMM specific to Arm as you already mentioned it was
> designed to specifically use raw SMCs? So if in future AMD TEE wants
> to implement EFI services, can we suggest they reuse the MM interface?

The MM interface is not exposed as an ABI to the non-secure world.  From
a Linux point of view, it's still a normal SMC invoke command towards OP-TEE.
What's 'special' and part of the ABI, is that the driver prepares the buffer
in a way StMM understands.  Then it gets handed over to OP-TEE, which
encapsulates it in an MM buffer and sends it to StMM.

As Jens already said, you asking to move the 'special stmm buffer' creation 
into OP-TEE instead of having the linux driver responsible for it.  That way we
can define an API for other TEEs, which will make the linux driver simpler.
We would ofc need to define some kind of versioning or service ID in that
API, so every TEE knows what it's supposed to call afterwards (iow an
internal TEE identifier in case we end up with multiple backends handling
EFI variables even in the same TEE). 

The proposal definitely makes sense,  but we are adding complexity and
knowledge of EFI to the secure world.  Someone still has to prepare a
buffer the way StMM understands it.

> 
> I am not sure why we need to redirect EFI variables via MM interface
> communication buffers rather than directly using the TEE shared memory
> approach.
> 
> Ard,
> 
> Since you have better insights into how EFI runtime services have to
> be implemented, can you share your opinion here? It may be something I
> am missing here.
> 
> -Sumit
> 

[0] https://apalos.github.io/Protected%20UEFI%20variables%20with%20U-Boot.html#Protected%20UEFI%20variables%20with%20U-Boot

Cheers
/Ilias