[PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Masahisa Kojima]

This series introduces the 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.

Changelog:
v7 -> v8
Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
- fix typos
- refactor error handling, direct return if applicable
- use devm_add_action_or_reset() for closing of tee context/session
- remove obvious comment

v6 -> v7
Patch #1-#4 are not updated.
Patch #5 is added into this series, original patch is here:
https://lore.kernel.org/all/20230609094532.562934-1-ilias.apalodimas@linaro.org/

There are two issues in the v6 series and v7 series addresses those.

1) efivar ops is not restored when the tee-supplicant daemon terminates.
 -> As the following patch says, user must remove the device before
    terminating tee-supplicant daemon.
    https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/

2) cause panic when someone remounts the efivarfs as RW even if
SetVariable is not supported
 -> The fifth patch addresses this issue.
   "[PATCH v7 5/5] efivarfs: force RO when remounting if SetVariable is
    not supported"

v5 -> v6
- new patch #4 is added in this series, #1-#3 patches are unchanged.
  automatically update super block flag when the efivarops support
  SetVariable runtime service, so that user does not need to manually
  remount the efivarfs as RW.

v4 -> v5
- rebase to efi-next based on v6.4-rc1
- set generic_ops.query_variable_info, it works as expected as follows.
$ df -h /sys/firmware/efi/efivars/
Filesystem      Size  Used Avail Use% Mounted on
efivarfs         16K  1.3K   15K   8% /sys/firmware/efi/efivars

v3 -> v4:
- replace the reference from EDK2 to PI Specification
- remove EDK2 source code reference comments
- prepare nonblocking variant of set_variable, it just returns
  EFI_UNSUPPORTED
- remove redundant buffer size check
- argument name change in mm_communicate
- function interface changes in setup_mm_hdr to remove (void **) cast

v2 -> v3:
- add CONFIG_EFI dependency to TEE_STMM_EFI
- add missing return code check for tee_client_invoke_func()
- directly call efivars_register/unregister from tee_stmm_efi.c

rfc v1 -> v2:
- split patch into three patches, one for drivers/tee,
  one for include/linux/efi.h, and one for the driver/firmware/efi/stmm
- context/session management into probe() and remove() same as other tee
client driver
- StMM variable driver is moved from driver/tee/optee to driver/firmware/efi
- use "tee" prefix instead of "optee" in driver/firmware/efi/stmm/tee_stmm_efi.c,
  this file does not contain op-tee specific code, abstracted by tee layer and
  StMM variable driver will work on other tee implementation.
- PTA_STMM_CMD_COMMUNICATE -> PTA_STMM_CMD_COMMUNICATE
- implement query_variable_store() but currently not used
- no use of TEEC_SUCCESS, it is defined in driver/tee/optee/optee_private.h.
  Other tee client drivers use 0 instead of using TEEC_SUCCESS
- remove TEEC_ERROR_EXCESS_DATA status, it is referred just to output
error message



Ilias Apalodimas (1):
  efivarfs: force RO when remounting if SetVariable is not supported

Masahisa Kojima (4):
  efi: expose efivar generic ops register function
  efi: Add EFI_ACCESS_DENIED status code
  efi: Add tee-based EFI variable driver
  efivarfs: automatically update super block flag

 drivers/firmware/efi/Kconfig                 |  15 +
 drivers/firmware/efi/Makefile                |   1 +
 drivers/firmware/efi/efi.c                   |  18 +
 drivers/firmware/efi/stmm/mm_communication.h | 236 +++++++
 drivers/firmware/efi/stmm/tee_stmm_efi.c     | 612 +++++++++++++++++++
 drivers/firmware/efi/vars.c                  |   8 +
 fs/efivarfs/super.c                          |  45 ++
 include/linux/efi.h                          |  12 +
 8 files changed, 947 insertions(+)
 create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
 create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c


base-commit: f6e6e95ce16205025b7b8680a66c30a0c4ec2270
-- 
2.30.2

[PATCH v8 1/5] 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.

Acked-by: Sumit Garg <sumit.garg@linaro.org>
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 1599f1176842..53ae25bbb6ac 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -231,6 +231,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 e9004358f7bd..faea18b42c08 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1364,4 +1364,7 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
 
 umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
 
+void efivars_generic_ops_register(void);
+void efivars_generic_ops_unregister(void);
+
 #endif /* _LINUX_EFI_H */
-- 
2.30.2

[PATCH v8 2/5] efi: Add EFI_ACCESS_DENIED status code

[Masahisa Kojima]

This commit adds the EFI_ACCESS_DENIED status code.

Acked-by: Sumit Garg <sumit.garg@linaro.org>
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>
---
 include/linux/efi.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index faea18b42c08..603bba2d6437 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)))
-- 
2.30.2

[PATCH v8 3/5] efi: Add tee-based EFI variable driver

[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
TEE(OP-TEE in this case), 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 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 tee_stmm_efi module is probed after tee-supplicant starts,
since "SetVariable" EFI Runtime Variable Service requires to
interact with tee-supplicant.

Acked-by: Sumit Garg <sumit.garg@linaro.org>
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/Kconfig                 |  15 +
 drivers/firmware/efi/Makefile                |   1 +
 drivers/firmware/efi/stmm/mm_communication.h | 236 +++++++
 drivers/firmware/efi/stmm/tee_stmm_efi.c     | 612 +++++++++++++++++++
 4 files changed, 864 insertions(+)
 create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
 create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c

diff --git a/drivers/firmware/efi/Kconfig b/drivers/firmware/efi/Kconfig
index 231f1c70d1db..ea73be9a4b81 100644
--- a/drivers/firmware/efi/Kconfig
+++ b/drivers/firmware/efi/Kconfig
@@ -301,3 +301,18 @@ config UEFI_CPER_X86
 	bool
 	depends on UEFI_CPER && X86
 	default y
+
+config TEE_STMM_EFI
+	tristate "TEE-based EFI runtime variable service driver"
+	depends on EFI && OPTEE && !EFI_VARS_PSTORE
+	help
+	  Select this config option if TEE is compiled to include StandAloneMM
+	  as a separate secure partition. It has the ability to check and store
+	  EFI variables on an RPMB or any other non-volatile medium used by
+	  StandAloneMM.
+
+	  Enabling this will change the EFI runtime services from the firmware
+	  provided functions to TEE calls.
+
+	  To compile this driver as a module, choose M here: the module
+	  will be called tee_stmm_efi.
diff --git a/drivers/firmware/efi/Makefile b/drivers/firmware/efi/Makefile
index e489fefd23da..a2d0009560d0 100644
--- a/drivers/firmware/efi/Makefile
+++ b/drivers/firmware/efi/Makefile
@@ -42,3 +42,4 @@ obj-$(CONFIG_EFI_EARLYCON)		+= earlycon.o
 obj-$(CONFIG_UEFI_CPER_ARM)		+= cper-arm.o
 obj-$(CONFIG_UEFI_CPER_X86)		+= cper-x86.o
 obj-$(CONFIG_UNACCEPTED_MEMORY)		+= unaccepted_memory.o
+obj-$(CONFIG_TEE_STMM_EFI)		+= stmm/tee_stmm_efi.o
diff --git a/drivers/firmware/efi/stmm/mm_communication.h b/drivers/firmware/efi/stmm/mm_communication.h
new file mode 100644
index 000000000000..52a1f32cd1eb
--- /dev/null
+++ b/drivers/firmware/efi/stmm/mm_communication.h
@@ -0,0 +1,236 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ *  Headers for EFI variable service via StandAloneMM, EDK2 application running
+ *  in OP-TEE. Most of the structs and defines resemble the EDK2 naming.
+ *
+ *  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_CMD_COMMUNICATE 0
+
+/*
+ * Defined in OP-TEE, this UUID is used to identify the pseudo-TA.
+ * 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)
+
+/**
+ * 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 the PI spec 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))
+
+/* 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
+
+#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
+ */
+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
+ *
+ */
+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
+ *
+ */
+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
+ *
+ */
+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
+ *
+ */
+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
+ *
+ */
+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
+ *
+ */
+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/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
new file mode 100644
index 000000000000..e03475966dc1
--- /dev/null
+++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
@@ -0,0 +1,612 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  EFI variable service via 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"
+
+static struct efivars tee_efivars;
+static struct efivar_operations tee_efivar_ops;
+
+static size_t max_buffer_size; /* comm + var + func + data */
+static size_t max_payload_size; /* func + data */
+
+struct tee_stmm_efi_private {
+	struct tee_context *ctx;
+	u32 session;
+	struct device *dev;
+};
+
+static struct tee_stmm_efi_private pvt_data;
+
+/* UUID of the stmm PTA */
+static const struct tee_client_device_id tee_stmm_efi_id_table[] = {
+	{PTA_STMM_UUID},
+	{}
+};
+
+static int tee_ctx_match(struct tee_ioctl_version_data *ver, const void *data)
+{
+	/* currently only OP-TEE is supported as a communication path */
+	if (ver->impl_id == TEE_IMPL_ID_OPTEE)
+		return 1;
+	else
+		return 0;
+}
+
+/**
+ * tee_mm_communicate() - Pass a buffer to StandaloneMM running in TEE
+ *
+ * @comm_buf:		locally allocated communication buffer
+ * @dsize:		buffer size
+ * Return:		status code
+ */
+static efi_status_t tee_mm_communicate(void *comm_buf, size_t dsize)
+{
+	size_t buf_size;
+	struct efi_mm_communicate_header *mm_hdr;
+	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;
+
+	shm = tee_shm_register_kernel_buf(pvt_data.ctx, comm_buf, buf_size);
+	if (IS_ERR(shm)) {
+		dev_err(pvt_data.dev, "Unable to register shared memory\n");
+		return EFI_UNSUPPORTED;
+	}
+
+	memset(&arg, 0, sizeof(arg));
+	arg.func = PTA_STMM_CMD_COMMUNICATE;
+	arg.session = pvt_data.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(pvt_data.ctx, &arg, param);
+	tee_shm_free(shm);
+
+	if (rc < 0 || arg.ret != 0) {
+		dev_err(pvt_data.dev,
+			"PTA_STMM_CMD_COMMUNICATE invoke error: 0x%x\n", arg.ret);
+		return EFI_DEVICE_ERROR;
+	}
+
+	switch (param[1].u.value.a) {
+	case ARM_SVC_SPM_RET_SUCCESS:
+		return EFI_SUCCESS;
+
+	case ARM_SVC_SPM_RET_INVALID_PARAMS:
+		return EFI_INVALID_PARAMETER;
+
+	case ARM_SVC_SPM_RET_DENIED:
+		return EFI_ACCESS_DENIED;
+
+	case ARM_SVC_SPM_RET_NO_MEMORY:
+		return EFI_OUT_OF_RESOURCES;
+
+	default:
+		return EFI_ACCESS_DENIED;
+	}
+}
+
+/**
+ * mm_communicate() - Adjust the communication buffer to StandAlonneMM and send
+ * it to TEE
+ *
+ * @comm_buf:		locally allocated communication buffer, buffer should
+ *			be enough big to have some headers and payload
+ * @payload_size:	payload size
+ * Return:		status code
+ */
+static efi_status_t mm_communicate(u8 *comm_buf, size_t payload_size)
+{
+	size_t dsize;
+	efi_status_t ret;
+	struct efi_mm_communicate_header *mm_hdr;
+	struct smm_variable_communicate_header *var_hdr;
+
+	dsize = payload_size + 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 = tee_mm_communicate(comm_buf, dsize);
+	if (ret != EFI_SUCCESS) {
+		dev_err(pvt_data.dev, "%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 to store allocated buffer
+ * @payload_size:	payload size
+ * @func:		standAloneMM function number
+ * @ret:		EFI return code
+ * Return:		pointer to corresponding StandAloneMM function buffer or NULL
+ */
+static void *setup_mm_hdr(u8 **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 = comm_buf;
+	*ret = EFI_SUCCESS;
+
+	return var_hdr->data;
+}
+
+/**
+ * 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)
+		return EFI_INVALID_PARAMETER;
+
+	payload_size = sizeof(*var_payload);
+	var_payload = setup_mm_hdr(&comm_buf, payload_size,
+				   SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
+				   &ret);
+	if (!var_payload)
+		return EFI_OUT_OF_RESOURCES;
+
+	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)
+		return EFI_INVALID_PARAMETER;
+
+	smm_property = setup_mm_hdr(
+		&comm_buf, payload_size,
+		SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
+	if (!smm_property)
+		return EFI_OUT_OF_RESOURCES;
+
+	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 tee_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)
+		return EFI_INVALID_PARAMETER;
+
+	name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
+	if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE)
+		return EFI_INVALID_PARAMETER;
+
+	/* Trim output buffer size */
+	tmp_dsize = *data_size;
+	if (name_size + tmp_dsize >
+	    max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
+		tmp_dsize = max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE -
+			    name_size;
+	}
+
+	payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
+	var_acc = setup_mm_hdr(&comm_buf, payload_size,
+			       SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
+	if (!var_acc)
+		return EFI_OUT_OF_RESOURCES;
+
+	/* 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);
+
+	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) {
+		ret = EFI_INVALID_PARAMETER;
+		goto out;
+	}
+	memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
+	       var_acc->data_size);
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t tee_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)
+		return EFI_INVALID_PARAMETER;
+
+	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)
+		return EFI_INVALID_PARAMETER;
+
+	if (in_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
+		return EFI_INVALID_PARAMETER;
+
+	/* 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;
+	var_getnext = setup_mm_hdr(&comm_buf, payload_size,
+				   SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
+				   &ret);
+	if (!var_getnext)
+		return EFI_OUT_OF_RESOURCES;
+
+	/* 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);
+
+	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 tee_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)
+		return EFI_INVALID_PARAMETER;
+
+	if (data_size > 0 && !data)
+		return EFI_INVALID_PARAMETER;
+
+	/* 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)
+		return EFI_INVALID_PARAMETER;
+
+	/*
+	 * 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
+	 */
+	var_acc = setup_mm_hdr(&comm_buf, payload_size,
+			       SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
+	if (!var_acc)
+		return EFI_OUT_OF_RESOURCES;
+
+	/*
+	 * 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) {
+		dev_err(pvt_data.dev, "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);
+
+	ret = mm_communicate(comm_buf, payload_size);
+	dev_dbg(pvt_data.dev, "Set Variable %s %d %lx\n", __FILE__, __LINE__, ret);
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static efi_status_t tee_set_variable_nonblocking(efi_char16_t *name,
+						 efi_guid_t *vendor,
+						 u32 attributes,
+						 unsigned long data_size,
+						 void *data)
+{
+	return EFI_UNSUPPORTED;
+}
+
+static efi_status_t tee_query_variable_info(u32 attributes,
+					    u64 *max_variable_storage_size,
+					    u64 *remain_variable_storage_size,
+					    u64 *max_variable_size)
+{
+	struct smm_variable_query_info *mm_query_info;
+	size_t payload_size;
+	efi_status_t ret;
+	u8 *comm_buf;
+
+	payload_size = sizeof(*mm_query_info);
+	mm_query_info = setup_mm_hdr(&comm_buf, payload_size,
+				SMM_VARIABLE_FUNCTION_QUERY_VARIABLE_INFO,
+				&ret);
+	if (!mm_query_info)
+		return EFI_OUT_OF_RESOURCES;
+
+	mm_query_info->attr = attributes;
+	ret = mm_communicate(comm_buf, payload_size);
+	if (ret != EFI_SUCCESS)
+		goto out;
+	*max_variable_storage_size = mm_query_info->max_variable_storage;
+	*remain_variable_storage_size =
+		mm_query_info->remaining_variable_storage;
+	*max_variable_size = mm_query_info->max_variable_size;
+
+out:
+	kfree(comm_buf);
+	return ret;
+}
+
+static void tee_stmm_efi_close_context(void *data)
+{
+	tee_client_close_context(pvt_data.ctx);
+}
+
+static void tee_stmm_efi_close_session(void *data)
+{
+	tee_client_close_session(pvt_data.ctx, pvt_data.session);
+}
+
+static int tee_stmm_efi_probe(struct device *dev)
+{
+	struct tee_ioctl_open_session_arg sess_arg;
+	efi_status_t ret;
+	int rc;
+
+	/* Open context with TEE driver */
+	pvt_data.ctx = tee_client_open_context(NULL, tee_ctx_match, NULL, NULL);
+	if (IS_ERR(pvt_data.ctx))
+		return -ENODEV;
+
+	rc = devm_add_action_or_reset(dev, tee_stmm_efi_close_context, NULL);
+	if (rc)
+		return rc;
+
+	/* Open session with StMM PTA */
+	memset(&sess_arg, 0, sizeof(sess_arg));
+	export_uuid(sess_arg.uuid, &tee_stmm_efi_id_table[0].uuid);
+	rc = tee_client_open_session(pvt_data.ctx, &sess_arg, NULL);
+	if ((rc < 0) || (sess_arg.ret != 0)) {
+		dev_err(dev, "tee_client_open_session failed, err: %x\n",
+			sess_arg.ret);
+		return -EINVAL;
+	}
+	pvt_data.session = sess_arg.session;
+	pvt_data.dev = dev;
+	rc = devm_add_action_or_reset(dev, tee_stmm_efi_close_session, NULL);
+	if (rc)
+		return rc;
+
+	ret = get_max_payload(&max_payload_size);
+	if (ret != EFI_SUCCESS)
+		return -EIO;
+
+	max_buffer_size = MM_COMMUNICATE_HEADER_SIZE +
+			  MM_VARIABLE_COMMUNICATE_SIZE +
+			  max_payload_size;
+
+	tee_efivar_ops.get_variable = tee_get_variable;
+	tee_efivar_ops.get_next_variable = tee_get_next_variable;
+	tee_efivar_ops.set_variable = tee_set_variable;
+	tee_efivar_ops.set_variable_nonblocking = tee_set_variable_nonblocking;
+	tee_efivar_ops.query_variable_store = efi_query_variable_store;
+	tee_efivar_ops.query_variable_info = tee_query_variable_info;
+
+	efivars_generic_ops_unregister();
+	pr_info("Use tee-based EFI runtime variable services\n");
+	efivars_register(&tee_efivars, &tee_efivar_ops);
+
+	return 0;
+}
+
+static int tee_stmm_efi_remove(struct device *dev)
+{
+	efivars_unregister(&tee_efivars);
+	efivars_generic_ops_register();
+
+	return 0;
+}
+
+MODULE_DEVICE_TABLE(tee, tee_stmm_efi_id_table);
+
+static struct tee_client_driver tee_stmm_efi_driver = {
+	.id_table	= tee_stmm_efi_id_table,
+	.driver		= {
+		.name		= "tee-stmm-efi",
+		.bus		= &tee_bus_type,
+		.probe		= tee_stmm_efi_probe,
+		.remove		= tee_stmm_efi_remove,
+	},
+};
+
+static int __init tee_stmm_efi_mod_init(void)
+{
+	return driver_register(&tee_stmm_efi_driver.driver);
+}
+
+static void __exit tee_stmm_efi_mod_exit(void)
+{
+	driver_unregister(&tee_stmm_efi_driver.driver);
+}
+
+module_init(tee_stmm_efi_mod_init);
+module_exit(tee_stmm_efi_mod_exit);
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Ilias Apalodimas <ilias.apalodimas@linaro.org>");
+MODULE_AUTHOR("Masahisa Kojima <masahisa.kojima@linaro.org>");
+MODULE_DESCRIPTION("TEE based EFI runtime variable service driver");
-- 
2.30.2

Re: [PATCH v8 3/5] efi: Add tee-based EFI variable driver

[Jonathan Cameron]

On Mon,  7 Aug 2023 11:53:40 +0900
Masahisa Kojima <masahisa.kojima@linaro.org> 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
> TEE(OP-TEE in this case), 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 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 tee_stmm_efi module is probed after tee-supplicant starts,
> since "SetVariable" EFI Runtime Variable Service requires to
> interact with tee-supplicant.
> 
> Acked-by: Sumit Garg <sumit.garg@linaro.org>
> 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>

I'm going to point out some stuff in here about the use of globals
etc which wouldn't be acceptable in many subsystems.  However, it's
up to the relevant maintainers on whether they want that stuff cleaned
up or not.

Other than that, this looks fine to me, but I'm reluctant to give an RB
with those globals in place.

Jonathan

> diff --git a/drivers/firmware/efi/stmm/tee_stmm_efi.c b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> new file mode 100644
> index 000000000000..e03475966dc1
> --- /dev/null
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -0,0 +1,612 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  EFI variable service via 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"
> +
> +static struct efivars tee_efivars;
> +static struct efivar_operations tee_efivar_ops;

Hmm. Globals.  Never a good thing to see in a driver, but from a quick
look it seems the various efi callbacks take no useful parameters
that would let us do the usual embedded structure and container_of
tricks.  So whilst I'd like to see that fixed, it's not my subsystem
and it would be a non trivial amount of work.

> +
> +static size_t max_buffer_size; /* comm + var + func + data */
> +static size_t max_payload_size; /* func + data */
> +
> +struct tee_stmm_efi_private {
> +	struct tee_context *ctx;
> +	u32 session;
> +	struct device *dev;
> +};
> +
> +static struct tee_stmm_efi_private pvt_data;

...
> +
> +static int tee_stmm_efi_probe(struct device *dev)
> +{
> +	struct tee_ioctl_open_session_arg sess_arg;
> +	efi_status_t ret;
> +	int rc;
> +
> +	/* Open context with TEE driver */

My natural aversion to comments as things that bit rot applies here.
Fairly obvious this opens the context from the function name, so not
sure the comment adds anything.

> +	pvt_data.ctx = tee_client_open_context(NULL, tee_ctx_match, NULL, NULL);
> +	if (IS_ERR(pvt_data.ctx))
> +		return -ENODEV;

[PATCH v8 4/5] efivarfs: automatically update super block flag

[Masahisa Kojima]

efivar operation is updated when the tee_stmm_efi module is probed.
tee_stmm_efi module supports SetVariable runtime service,
but user needs to manually remount the efivarfs as RW to enable
the write access if the previous efivar operation does not support
SerVariable and efivarfs is mounted as read-only.

This commit notifies the update of efivar operation to
efivarfs subsystem, then drops SB_RDONLY flag if the efivar
operation supports SetVariable.

Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
---
 drivers/firmware/efi/efi.c  |  6 ++++++
 drivers/firmware/efi/vars.c |  8 ++++++++
 fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
 include/linux/efi.h         |  8 ++++++++
 4 files changed, 55 insertions(+)

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 53ae25bbb6ac..d2eec5ed8e5e 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -32,6 +32,7 @@
 #include <linux/ucs2_string.h>
 #include <linux/memblock.h>
 #include <linux/security.h>
+#include <linux/notifier.h>
 
 #include <asm/early_ioremap.h>
 
@@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
 	.is_visible = efi_attr_is_visible,
 };
 
+struct blocking_notifier_head efivar_ops_nh;
+EXPORT_SYMBOL_GPL(efivar_ops_nh);
+
 static struct efivars generic_efivars;
 static struct efivar_operations generic_ops;
 
@@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
 		platform_device_register_simple("efivars", 0, NULL, 0);
 	}
 
+	BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
+
 	error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
 	if (error) {
 		pr_err("efi: Sysfs attribute export failed with error %d.\n",
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index e9dc7116daf1..f654e6f6af87 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
 		     const struct efivar_operations *ops)
 {
 	int rv;
+	int event;
 
 	if (down_interruptible(&efivars_lock))
 		return -EINTR;
@@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
 
 	__efivars = efivars;
 
+	if (efivar_supports_writes())
+		event = EFIVAR_OPS_RDWR;
+	else
+		event = EFIVAR_OPS_RDONLY;
+
+	blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
+
 	pr_info("Registered efivars operations\n");
 	rv = 0;
 out:
diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index e028fafa04f3..0f6e4d223aea 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -14,11 +14,36 @@
 #include <linux/slab.h>
 #include <linux/magic.h>
 #include <linux/statfs.h>
+#include <linux/notifier.h>
 
 #include "internal.h"
 
 LIST_HEAD(efivarfs_list);
 
+struct efivarfs_info {
+	struct super_block *sb;
+	struct notifier_block nb;
+};
+
+static struct efivarfs_info info;
+
+static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
+				 void *data)
+{
+	switch (event) {
+	case EFIVAR_OPS_RDONLY:
+		info.sb->s_flags |= SB_RDONLY;
+		break;
+	case EFIVAR_OPS_RDWR:
+		info.sb->s_flags &= ~SB_RDONLY;
+		break;
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
 static void efivarfs_evict_inode(struct inode *inode)
 {
 	clear_inode(inode);
@@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
 	if (!root)
 		return -ENOMEM;
 
+	info.sb = sb;
+	info.nb.notifier_call = efivarfs_ops_notifier;
+	err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
+	if (err)
+		return err;
+
 	INIT_LIST_HEAD(&efivarfs_list);
 
 	err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
@@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
 
 static void efivarfs_kill_sb(struct super_block *sb)
 {
+	blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
+	info.sb = NULL;
 	kill_litter_super(sb);
 
 	if (!efivar_is_available())
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 603bba2d6437..17cd628b5c42 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
 
 umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
 
+/*
+ * efivar ops event type
+ */
+#define EFIVAR_OPS_RDONLY 0
+#define EFIVAR_OPS_RDWR 1
+
+extern struct blocking_notifier_head efivar_ops_nh;
+
 void efivars_generic_ops_register(void);
 void efivars_generic_ops_unregister(void);
 
-- 
2.30.2

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Ilias Apalodimas]

Kojima-san
Apologies for the late reply, I just found some to test this.

On Sun, 6 Aug 2023 at 19:55, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> efivar operation is updated when the tee_stmm_efi module is probed.
> tee_stmm_efi module supports SetVariable runtime service,
> but user needs to manually remount the efivarfs as RW to enable
> the write access if the previous efivar operation does not support
> SerVariable and efivarfs is mounted as read-only.
>
> This commit notifies the update of efivar operation to
> efivarfs subsystem, then drops SB_RDONLY flag if the efivar
> operation supports SetVariable.

The RO->RW transition works fine and I did manage to test basic stuff
like setting up efibootmgr options.  IIUC the RW->RO should be covered
by this patchset [0] ?
Wouldn't it be better to detect that the tee device of the supplicant
closes and use that to switch the permissions?  I get why we need that
for the TPM, the entire subsystem needs to send TPM commands *before*
the supplicant dies.   But this is not needed for the EFI variables
case, we could just remount the FS as RO the moment the supplicant
dies.

[0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/

Regards
/Ilias
>
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
>  drivers/firmware/efi/efi.c  |  6 ++++++
>  drivers/firmware/efi/vars.c |  8 ++++++++
>  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
>  include/linux/efi.h         |  8 ++++++++
>  4 files changed, 55 insertions(+)
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 53ae25bbb6ac..d2eec5ed8e5e 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -32,6 +32,7 @@
>  #include <linux/ucs2_string.h>
>  #include <linux/memblock.h>
>  #include <linux/security.h>
> +#include <linux/notifier.h>
>
>  #include <asm/early_ioremap.h>
>
> @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
>         .is_visible = efi_attr_is_visible,
>  };
>
> +struct blocking_notifier_head efivar_ops_nh;
> +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> +
>  static struct efivars generic_efivars;
>  static struct efivar_operations generic_ops;
>
> @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
>                 platform_device_register_simple("efivars", 0, NULL, 0);
>         }
>
> +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> +
>         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
>         if (error) {
>                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index e9dc7116daf1..f654e6f6af87 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
>                      const struct efivar_operations *ops)
>  {
>         int rv;
> +       int event;
>
>         if (down_interruptible(&efivars_lock))
>                 return -EINTR;
> @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
>
>         __efivars = efivars;
>
> +       if (efivar_supports_writes())
> +               event = EFIVAR_OPS_RDWR;
> +       else
> +               event = EFIVAR_OPS_RDONLY;
> +
> +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> +
>         pr_info("Registered efivars operations\n");
>         rv = 0;
>  out:
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index e028fafa04f3..0f6e4d223aea 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -14,11 +14,36 @@
>  #include <linux/slab.h>
>  #include <linux/magic.h>
>  #include <linux/statfs.h>
> +#include <linux/notifier.h>
>
>  #include "internal.h"
>
>  LIST_HEAD(efivarfs_list);
>
> +struct efivarfs_info {
> +       struct super_block *sb;
> +       struct notifier_block nb;
> +};
> +
> +static struct efivarfs_info info;
> +
> +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> +                                void *data)
> +{
> +       switch (event) {
> +       case EFIVAR_OPS_RDONLY:
> +               info.sb->s_flags |= SB_RDONLY;
> +               break;
> +       case EFIVAR_OPS_RDWR:
> +               info.sb->s_flags &= ~SB_RDONLY;
> +               break;
> +       default:
> +               return NOTIFY_DONE;
> +       }
> +
> +       return NOTIFY_OK;
> +}
> +
>  static void efivarfs_evict_inode(struct inode *inode)
>  {
>         clear_inode(inode);
> @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
>         if (!root)
>                 return -ENOMEM;
>
> +       info.sb = sb;
> +       info.nb.notifier_call = efivarfs_ops_notifier;
> +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> +       if (err)
> +               return err;
> +
>         INIT_LIST_HEAD(&efivarfs_list);
>
>         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
>
>  static void efivarfs_kill_sb(struct super_block *sb)
>  {
> +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> +       info.sb = NULL;
>         kill_litter_super(sb);
>
>         if (!efivar_is_available())
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 603bba2d6437..17cd628b5c42 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
>
>  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
>
> +/*
> + * efivar ops event type
> + */
> +#define EFIVAR_OPS_RDONLY 0
> +#define EFIVAR_OPS_RDWR 1
> +
> +extern struct blocking_notifier_head efivar_ops_nh;
> +
>  void efivars_generic_ops_register(void);
>  void efivars_generic_ops_unregister(void);
>
> --
> 2.30.2
>

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Masahisa Kojima]

Hi Ilias,


On Thu, 12 Oct 2023 at 02:00, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Kojima-san
> Apologies for the late reply, I just found some to test this.
>
> On Sun, 6 Aug 2023 at 19:55, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >
> > efivar operation is updated when the tee_stmm_efi module is probed.
> > tee_stmm_efi module supports SetVariable runtime service,
> > but user needs to manually remount the efivarfs as RW to enable
> > the write access if the previous efivar operation does not support
> > SerVariable and efivarfs is mounted as read-only.
> >
> > This commit notifies the update of efivar operation to
> > efivarfs subsystem, then drops SB_RDONLY flag if the efivar
> > operation supports SetVariable.
>
> The RO->RW transition works fine and I did manage to test basic stuff
> like setting up efibootmgr options.  IIUC the RW->RO should be covered
> by this patchset [0] ?

Yes.

> Wouldn't it be better to detect that the tee device of the supplicant
> closes and use that to switch the permissions?  I get why we need that
> for the TPM, the entire subsystem needs to send TPM commands *before*
> the supplicant dies.   But this is not needed for the EFI variables
> case, we could just remount the FS as RO the moment the supplicant
> dies.

OK, I will add the patch to restore the efivars generic ops when
tee-supplicant stops.

Thanks,
Masahisa Kojima

>
> [0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
>
> Regards
> /Ilias
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  drivers/firmware/efi/efi.c  |  6 ++++++
> >  drivers/firmware/efi/vars.c |  8 ++++++++
> >  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/efi.h         |  8 ++++++++
> >  4 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 53ae25bbb6ac..d2eec5ed8e5e 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/ucs2_string.h>
> >  #include <linux/memblock.h>
> >  #include <linux/security.h>
> > +#include <linux/notifier.h>
> >
> >  #include <asm/early_ioremap.h>
> >
> > @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
> >         .is_visible = efi_attr_is_visible,
> >  };
> >
> > +struct blocking_notifier_head efivar_ops_nh;
> > +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> > +
> >  static struct efivars generic_efivars;
> >  static struct efivar_operations generic_ops;
> >
> > @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
> >                 platform_device_register_simple("efivars", 0, NULL, 0);
> >         }
> >
> > +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> > +
> >         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
> >         if (error) {
> >                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index e9dc7116daf1..f654e6f6af87 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
> >                      const struct efivar_operations *ops)
> >  {
> >         int rv;
> > +       int event;
> >
> >         if (down_interruptible(&efivars_lock))
> >                 return -EINTR;
> > @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
> >
> >         __efivars = efivars;
> >
> > +       if (efivar_supports_writes())
> > +               event = EFIVAR_OPS_RDWR;
> > +       else
> > +               event = EFIVAR_OPS_RDONLY;
> > +
> > +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> > +
> >         pr_info("Registered efivars operations\n");
> >         rv = 0;
> >  out:
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index e028fafa04f3..0f6e4d223aea 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -14,11 +14,36 @@
> >  #include <linux/slab.h>
> >  #include <linux/magic.h>
> >  #include <linux/statfs.h>
> > +#include <linux/notifier.h>
> >
> >  #include "internal.h"
> >
> >  LIST_HEAD(efivarfs_list);
> >
> > +struct efivarfs_info {
> > +       struct super_block *sb;
> > +       struct notifier_block nb;
> > +};
> > +
> > +static struct efivarfs_info info;
> > +
> > +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> > +                                void *data)
> > +{
> > +       switch (event) {
> > +       case EFIVAR_OPS_RDONLY:
> > +               info.sb->s_flags |= SB_RDONLY;
> > +               break;
> > +       case EFIVAR_OPS_RDWR:
> > +               info.sb->s_flags &= ~SB_RDONLY;
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> >  static void efivarfs_evict_inode(struct inode *inode)
> >  {
> >         clear_inode(inode);
> > @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> >         if (!root)
> >                 return -ENOMEM;
> >
> > +       info.sb = sb;
> > +       info.nb.notifier_call = efivarfs_ops_notifier;
> > +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> > +       if (err)
> > +               return err;
> > +
> >         INIT_LIST_HEAD(&efivarfs_list);
> >
> >         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> > @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
> >
> >  static void efivarfs_kill_sb(struct super_block *sb)
> >  {
> > +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> > +       info.sb = NULL;
> >         kill_litter_super(sb);
> >
> >         if (!efivar_is_available())
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 603bba2d6437..17cd628b5c42 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
> >
> >  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
> >
> > +/*
> > + * efivar ops event type
> > + */
> > +#define EFIVAR_OPS_RDONLY 0
> > +#define EFIVAR_OPS_RDWR 1
> > +
> > +extern struct blocking_notifier_head efivar_ops_nh;
> > +
> >  void efivars_generic_ops_register(void);
> >  void efivars_generic_ops_unregister(void);
> >
> > --
> > 2.30.2
> >

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Sumit Garg]

Hi Ilias,

On Wed, 11 Oct 2023 at 22:30, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Kojima-san
> Apologies for the late reply, I just found some to test this.
>
> On Sun, 6 Aug 2023 at 19:55, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >
> > efivar operation is updated when the tee_stmm_efi module is probed.
> > tee_stmm_efi module supports SetVariable runtime service,
> > but user needs to manually remount the efivarfs as RW to enable
> > the write access if the previous efivar operation does not support
> > SerVariable and efivarfs is mounted as read-only.
> >
> > This commit notifies the update of efivar operation to
> > efivarfs subsystem, then drops SB_RDONLY flag if the efivar
> > operation supports SetVariable.
>
> The RO->RW transition works fine and I did manage to test basic stuff
> like setting up efibootmgr options.  IIUC the RW->RO should be covered
> by this patchset [0] ?

Yeah.

> Wouldn't it be better to detect that the tee device of the supplicant
> closes and use that to switch the permissions?  I get why we need that
> for the TPM, the entire subsystem needs to send TPM commands *before*
> the supplicant dies.   But this is not needed for the EFI variables
> case, we could just remount the FS as RO the moment the supplicant
> dies.

As we discussed offline, we should have a unified approach to notify
kernel TEE client drivers. So the approach implemented as part of [0]
should address the needs for fTPM as well as EFI.

-Sumit

>
> [0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
>
> Regards
> /Ilias
> >
> > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > ---
> >  drivers/firmware/efi/efi.c  |  6 ++++++
> >  drivers/firmware/efi/vars.c |  8 ++++++++
> >  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
> >  include/linux/efi.h         |  8 ++++++++
> >  4 files changed, 55 insertions(+)
> >
> > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > index 53ae25bbb6ac..d2eec5ed8e5e 100644
> > --- a/drivers/firmware/efi/efi.c
> > +++ b/drivers/firmware/efi/efi.c
> > @@ -32,6 +32,7 @@
> >  #include <linux/ucs2_string.h>
> >  #include <linux/memblock.h>
> >  #include <linux/security.h>
> > +#include <linux/notifier.h>
> >
> >  #include <asm/early_ioremap.h>
> >
> > @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
> >         .is_visible = efi_attr_is_visible,
> >  };
> >
> > +struct blocking_notifier_head efivar_ops_nh;
> > +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> > +
> >  static struct efivars generic_efivars;
> >  static struct efivar_operations generic_ops;
> >
> > @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
> >                 platform_device_register_simple("efivars", 0, NULL, 0);
> >         }
> >
> > +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> > +
> >         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
> >         if (error) {
> >                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > index e9dc7116daf1..f654e6f6af87 100644
> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
> >                      const struct efivar_operations *ops)
> >  {
> >         int rv;
> > +       int event;
> >
> >         if (down_interruptible(&efivars_lock))
> >                 return -EINTR;
> > @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
> >
> >         __efivars = efivars;
> >
> > +       if (efivar_supports_writes())
> > +               event = EFIVAR_OPS_RDWR;
> > +       else
> > +               event = EFIVAR_OPS_RDONLY;
> > +
> > +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> > +
> >         pr_info("Registered efivars operations\n");
> >         rv = 0;
> >  out:
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index e028fafa04f3..0f6e4d223aea 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -14,11 +14,36 @@
> >  #include <linux/slab.h>
> >  #include <linux/magic.h>
> >  #include <linux/statfs.h>
> > +#include <linux/notifier.h>
> >
> >  #include "internal.h"
> >
> >  LIST_HEAD(efivarfs_list);
> >
> > +struct efivarfs_info {
> > +       struct super_block *sb;
> > +       struct notifier_block nb;
> > +};
> > +
> > +static struct efivarfs_info info;
> > +
> > +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> > +                                void *data)
> > +{
> > +       switch (event) {
> > +       case EFIVAR_OPS_RDONLY:
> > +               info.sb->s_flags |= SB_RDONLY;
> > +               break;
> > +       case EFIVAR_OPS_RDWR:
> > +               info.sb->s_flags &= ~SB_RDONLY;
> > +               break;
> > +       default:
> > +               return NOTIFY_DONE;
> > +       }
> > +
> > +       return NOTIFY_OK;
> > +}
> > +
> >  static void efivarfs_evict_inode(struct inode *inode)
> >  {
> >         clear_inode(inode);
> > @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> >         if (!root)
> >                 return -ENOMEM;
> >
> > +       info.sb = sb;
> > +       info.nb.notifier_call = efivarfs_ops_notifier;
> > +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> > +       if (err)
> > +               return err;
> > +
> >         INIT_LIST_HEAD(&efivarfs_list);
> >
> >         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> > @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
> >
> >  static void efivarfs_kill_sb(struct super_block *sb)
> >  {
> > +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> > +       info.sb = NULL;
> >         kill_litter_super(sb);
> >
> >         if (!efivar_is_available())
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 603bba2d6437..17cd628b5c42 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
> >
> >  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
> >
> > +/*
> > + * efivar ops event type
> > + */
> > +#define EFIVAR_OPS_RDONLY 0
> > +#define EFIVAR_OPS_RDWR 1
> > +
> > +extern struct blocking_notifier_head efivar_ops_nh;
> > +
> >  void efivars_generic_ops_register(void);
> >  void efivars_generic_ops_unregister(void);
> >
> > --
> > 2.30.2
> >

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Ilias Apalodimas]

Hi Sumit

On Fri, 13 Oct 2023 at 09:53, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> Hi Ilias,
>
> On Wed, 11 Oct 2023 at 22:30, Ilias Apalodimas
> <ilias.apalodimas@linaro.org> wrote:
> >
> > Kojima-san
> > Apologies for the late reply, I just found some to test this.
> >
> > On Sun, 6 Aug 2023 at 19:55, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > >
> > > efivar operation is updated when the tee_stmm_efi module is probed.
> > > tee_stmm_efi module supports SetVariable runtime service,
> > > but user needs to manually remount the efivarfs as RW to enable
> > > the write access if the previous efivar operation does not support
> > > SerVariable and efivarfs is mounted as read-only.
> > >
> > > This commit notifies the update of efivar operation to
> > > efivarfs subsystem, then drops SB_RDONLY flag if the efivar
> > > operation supports SetVariable.
> >
> > The RO->RW transition works fine and I did manage to test basic stuff
> > like setting up efibootmgr options.  IIUC the RW->RO should be covered
> > by this patchset [0] ?
>
> Yeah.
>
> > Wouldn't it be better to detect that the tee device of the supplicant
> > closes and use that to switch the permissions?  I get why we need that
> > for the TPM, the entire subsystem needs to send TPM commands *before*
> > the supplicant dies.   But this is not needed for the EFI variables
> > case, we could just remount the FS as RO the moment the supplicant
> > dies.
>
> As we discussed offline, we should have a unified approach to notify
> kernel TEE client drivers. So the approach implemented as part of [0]
> should address the needs for fTPM as well as EFI.

Ideally yes, we should have a unified approach.  But this is a bit
different IMHO. In the majority of the cases, the supplicant goes
away,  we lose access to storage and that's the only thing we care
about.  Only the TPM subsystem is 'special' because it has to perform
a shutdown of the device as well.  On top of that, I think we should
try to avoid the kernel depending on userspace apps as much as
possible.  I think it's best if we support both of these and add
documentation on why this is happening. Would it be hard to have a
combination of both of your patches?

Thanks
/Ilias

>
> -Sumit
>
> >
> > [0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
> >
> > Regards
> > /Ilias
> > >
> > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > ---
> > >  drivers/firmware/efi/efi.c  |  6 ++++++
> > >  drivers/firmware/efi/vars.c |  8 ++++++++
> > >  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
> > >  include/linux/efi.h         |  8 ++++++++
> > >  4 files changed, 55 insertions(+)
> > >
> > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > index 53ae25bbb6ac..d2eec5ed8e5e 100644
> > > --- a/drivers/firmware/efi/efi.c
> > > +++ b/drivers/firmware/efi/efi.c
> > > @@ -32,6 +32,7 @@
> > >  #include <linux/ucs2_string.h>
> > >  #include <linux/memblock.h>
> > >  #include <linux/security.h>
> > > +#include <linux/notifier.h>
> > >
> > >  #include <asm/early_ioremap.h>
> > >
> > > @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
> > >         .is_visible = efi_attr_is_visible,
> > >  };
> > >
> > > +struct blocking_notifier_head efivar_ops_nh;
> > > +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> > > +
> > >  static struct efivars generic_efivars;
> > >  static struct efivar_operations generic_ops;
> > >
> > > @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
> > >                 platform_device_register_simple("efivars", 0, NULL, 0);
> > >         }
> > >
> > > +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> > > +
> > >         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
> > >         if (error) {
> > >                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> > > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > > index e9dc7116daf1..f654e6f6af87 100644
> > > --- a/drivers/firmware/efi/vars.c
> > > +++ b/drivers/firmware/efi/vars.c
> > > @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
> > >                      const struct efivar_operations *ops)
> > >  {
> > >         int rv;
> > > +       int event;
> > >
> > >         if (down_interruptible(&efivars_lock))
> > >                 return -EINTR;
> > > @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
> > >
> > >         __efivars = efivars;
> > >
> > > +       if (efivar_supports_writes())
> > > +               event = EFIVAR_OPS_RDWR;
> > > +       else
> > > +               event = EFIVAR_OPS_RDONLY;
> > > +
> > > +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> > > +
> > >         pr_info("Registered efivars operations\n");
> > >         rv = 0;
> > >  out:
> > > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > > index e028fafa04f3..0f6e4d223aea 100644
> > > --- a/fs/efivarfs/super.c
> > > +++ b/fs/efivarfs/super.c
> > > @@ -14,11 +14,36 @@
> > >  #include <linux/slab.h>
> > >  #include <linux/magic.h>
> > >  #include <linux/statfs.h>
> > > +#include <linux/notifier.h>
> > >
> > >  #include "internal.h"
> > >
> > >  LIST_HEAD(efivarfs_list);
> > >
> > > +struct efivarfs_info {
> > > +       struct super_block *sb;
> > > +       struct notifier_block nb;
> > > +};
> > > +
> > > +static struct efivarfs_info info;
> > > +
> > > +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> > > +                                void *data)
> > > +{
> > > +       switch (event) {
> > > +       case EFIVAR_OPS_RDONLY:
> > > +               info.sb->s_flags |= SB_RDONLY;
> > > +               break;
> > > +       case EFIVAR_OPS_RDWR:
> > > +               info.sb->s_flags &= ~SB_RDONLY;
> > > +               break;
> > > +       default:
> > > +               return NOTIFY_DONE;
> > > +       }
> > > +
> > > +       return NOTIFY_OK;
> > > +}
> > > +
> > >  static void efivarfs_evict_inode(struct inode *inode)
> > >  {
> > >         clear_inode(inode);
> > > @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> > >         if (!root)
> > >                 return -ENOMEM;
> > >
> > > +       info.sb = sb;
> > > +       info.nb.notifier_call = efivarfs_ops_notifier;
> > > +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> > > +       if (err)
> > > +               return err;
> > > +
> > >         INIT_LIST_HEAD(&efivarfs_list);
> > >
> > >         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> > > @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
> > >
> > >  static void efivarfs_kill_sb(struct super_block *sb)
> > >  {
> > > +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> > > +       info.sb = NULL;
> > >         kill_litter_super(sb);
> > >
> > >         if (!efivar_is_available())
> > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > index 603bba2d6437..17cd628b5c42 100644
> > > --- a/include/linux/efi.h
> > > +++ b/include/linux/efi.h
> > > @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
> > >
> > >  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
> > >
> > > +/*
> > > + * efivar ops event type
> > > + */
> > > +#define EFIVAR_OPS_RDONLY 0
> > > +#define EFIVAR_OPS_RDWR 1
> > > +
> > > +extern struct blocking_notifier_head efivar_ops_nh;
> > > +
> > >  void efivars_generic_ops_register(void);
> > >  void efivars_generic_ops_unregister(void);
> > >
> > > --
> > > 2.30.2
> > >

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Sumit Garg]

On Fri, 13 Oct 2023 at 20:56, Ilias Apalodimas
<ilias.apalodimas@linaro.org> wrote:
>
> Hi Sumit
>
> On Fri, 13 Oct 2023 at 09:53, Sumit Garg <sumit.garg@linaro.org> wrote:
> >
> > Hi Ilias,
> >
> > On Wed, 11 Oct 2023 at 22:30, Ilias Apalodimas
> > <ilias.apalodimas@linaro.org> wrote:
> > >
> > > Kojima-san
> > > Apologies for the late reply, I just found some to test this.
> > >
> > > On Sun, 6 Aug 2023 at 19:55, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > > >
> > > > efivar operation is updated when the tee_stmm_efi module is probed.
> > > > tee_stmm_efi module supports SetVariable runtime service,
> > > > but user needs to manually remount the efivarfs as RW to enable
> > > > the write access if the previous efivar operation does not support
> > > > SerVariable and efivarfs is mounted as read-only.
> > > >
> > > > This commit notifies the update of efivar operation to
> > > > efivarfs subsystem, then drops SB_RDONLY flag if the efivar
> > > > operation supports SetVariable.
> > >
> > > The RO->RW transition works fine and I did manage to test basic stuff
> > > like setting up efibootmgr options.  IIUC the RW->RO should be covered
> > > by this patchset [0] ?
> >
> > Yeah.
> >
> > > Wouldn't it be better to detect that the tee device of the supplicant
> > > closes and use that to switch the permissions?  I get why we need that
> > > for the TPM, the entire subsystem needs to send TPM commands *before*
> > > the supplicant dies.   But this is not needed for the EFI variables
> > > case, we could just remount the FS as RO the moment the supplicant
> > > dies.
> >
> > As we discussed offline, we should have a unified approach to notify
> > kernel TEE client drivers. So the approach implemented as part of [0]
> > should address the needs for fTPM as well as EFI.
>
> Ideally yes, we should have a unified approach.  But this is a bit
> different IMHO. In the majority of the cases, the supplicant goes
> away,  we lose access to storage and that's the only thing we care
> about.

What do you think about in-flight secure storage operations while the
tee-supplicant goes away? They can fail abruptly if the tee-supplicant
goes away without waiting for them to complete. IMO, the graceful
handling should be that the tee-supplicant should be alive when the
dependent devices are being closed.

For an EFI variable store case as well isn't that a possible scenario?

> Only the TPM subsystem is 'special' because it has to perform
> a shutdown of the device as well.  On top of that, I think we should
> try to avoid the kernel depending on userspace apps as much as
> possible.

There is only tee-supplicant user-space dependency which has to be
handled carefully. I am very much up for RPMB fastpath via kernel and
get rid of this dependency as well.

>  I think it's best if we support both of these and add
> documentation on why this is happening. Would it be hard to have a
> combination of both of your patches?
>

It won't be hard but there can be corner cases as mentioned above.

-Sumit

> Thanks
> /Ilias
>
> >
> > -Sumit
> >
> > >
> > > [0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
> > >
> > > Regards
> > > /Ilias
> > > >
> > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > ---
> > > >  drivers/firmware/efi/efi.c  |  6 ++++++
> > > >  drivers/firmware/efi/vars.c |  8 ++++++++
> > > >  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
> > > >  include/linux/efi.h         |  8 ++++++++
> > > >  4 files changed, 55 insertions(+)
> > > >
> > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > index 53ae25bbb6ac..d2eec5ed8e5e 100644
> > > > --- a/drivers/firmware/efi/efi.c
> > > > +++ b/drivers/firmware/efi/efi.c
> > > > @@ -32,6 +32,7 @@
> > > >  #include <linux/ucs2_string.h>
> > > >  #include <linux/memblock.h>
> > > >  #include <linux/security.h>
> > > > +#include <linux/notifier.h>
> > > >
> > > >  #include <asm/early_ioremap.h>
> > > >
> > > > @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
> > > >         .is_visible = efi_attr_is_visible,
> > > >  };
> > > >
> > > > +struct blocking_notifier_head efivar_ops_nh;
> > > > +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> > > > +
> > > >  static struct efivars generic_efivars;
> > > >  static struct efivar_operations generic_ops;
> > > >
> > > > @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
> > > >                 platform_device_register_simple("efivars", 0, NULL, 0);
> > > >         }
> > > >
> > > > +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> > > > +
> > > >         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
> > > >         if (error) {
> > > >                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> > > > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > > > index e9dc7116daf1..f654e6f6af87 100644
> > > > --- a/drivers/firmware/efi/vars.c
> > > > +++ b/drivers/firmware/efi/vars.c
> > > > @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
> > > >                      const struct efivar_operations *ops)
> > > >  {
> > > >         int rv;
> > > > +       int event;
> > > >
> > > >         if (down_interruptible(&efivars_lock))
> > > >                 return -EINTR;
> > > > @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
> > > >
> > > >         __efivars = efivars;
> > > >
> > > > +       if (efivar_supports_writes())
> > > > +               event = EFIVAR_OPS_RDWR;
> > > > +       else
> > > > +               event = EFIVAR_OPS_RDONLY;
> > > > +
> > > > +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> > > > +
> > > >         pr_info("Registered efivars operations\n");
> > > >         rv = 0;
> > > >  out:
> > > > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > > > index e028fafa04f3..0f6e4d223aea 100644
> > > > --- a/fs/efivarfs/super.c
> > > > +++ b/fs/efivarfs/super.c
> > > > @@ -14,11 +14,36 @@
> > > >  #include <linux/slab.h>
> > > >  #include <linux/magic.h>
> > > >  #include <linux/statfs.h>
> > > > +#include <linux/notifier.h>
> > > >
> > > >  #include "internal.h"
> > > >
> > > >  LIST_HEAD(efivarfs_list);
> > > >
> > > > +struct efivarfs_info {
> > > > +       struct super_block *sb;
> > > > +       struct notifier_block nb;
> > > > +};
> > > > +
> > > > +static struct efivarfs_info info;
> > > > +
> > > > +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> > > > +                                void *data)
> > > > +{
> > > > +       switch (event) {
> > > > +       case EFIVAR_OPS_RDONLY:
> > > > +               info.sb->s_flags |= SB_RDONLY;
> > > > +               break;
> > > > +       case EFIVAR_OPS_RDWR:
> > > > +               info.sb->s_flags &= ~SB_RDONLY;
> > > > +               break;
> > > > +       default:
> > > > +               return NOTIFY_DONE;
> > > > +       }
> > > > +
> > > > +       return NOTIFY_OK;
> > > > +}
> > > > +
> > > >  static void efivarfs_evict_inode(struct inode *inode)
> > > >  {
> > > >         clear_inode(inode);
> > > > @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> > > >         if (!root)
> > > >                 return -ENOMEM;
> > > >
> > > > +       info.sb = sb;
> > > > +       info.nb.notifier_call = efivarfs_ops_notifier;
> > > > +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> > > > +       if (err)
> > > > +               return err;
> > > > +
> > > >         INIT_LIST_HEAD(&efivarfs_list);
> > > >
> > > >         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> > > > @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
> > > >
> > > >  static void efivarfs_kill_sb(struct super_block *sb)
> > > >  {
> > > > +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> > > > +       info.sb = NULL;
> > > >         kill_litter_super(sb);
> > > >
> > > >         if (!efivar_is_available())
> > > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > > index 603bba2d6437..17cd628b5c42 100644
> > > > --- a/include/linux/efi.h
> > > > +++ b/include/linux/efi.h
> > > > @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
> > > >
> > > >  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
> > > >
> > > > +/*
> > > > + * efivar ops event type
> > > > + */
> > > > +#define EFIVAR_OPS_RDONLY 0
> > > > +#define EFIVAR_OPS_RDWR 1
> > > > +
> > > > +extern struct blocking_notifier_head efivar_ops_nh;
> > > > +
> > > >  void efivars_generic_ops_register(void);
> > > >  void efivars_generic_ops_unregister(void);
> > > >
> > > > --
> > > > 2.30.2
> > > >

Re: [PATCH v8 4/5] efivarfs: automatically update super block flag

[Ilias Apalodimas]

Hi Sumit,

[...]

> > >
> > > > Wouldn't it be better to detect that the tee device of the supplicant
> > > > closes and use that to switch the permissions?  I get why we need that
> > > > for the TPM, the entire subsystem needs to send TPM commands *before*
> > > > the supplicant dies.   But this is not needed for the EFI variables
> > > > case, we could just remount the FS as RO the moment the supplicant
> > > > dies.
> > >
> > > As we discussed offline, we should have a unified approach to notify
> > > kernel TEE client drivers. So the approach implemented as part of [0]
> > > should address the needs for fTPM as well as EFI.
> >
> > Ideally yes, we should have a unified approach.  But this is a bit
> > different IMHO. In the majority of the cases, the supplicant goes
> > away,  we lose access to storage and that's the only thing we care
> > about.
>
> What do you think about in-flight secure storage operations while the
> tee-supplicant goes away? They can fail abruptly if the tee-supplicant
> goes away without waiting for them to complete. IMO, the graceful
> handling should be that the tee-supplicant should be alive when the
> dependent devices are being closed.
>
> For an EFI variable store case as well isn't that a possible scenario?

I think it is, but that would involve issuing a write and then killing
the supplicant immediately.
But that would lead to a weird state as well with the graceful exit.
IOW we don't block until the operation is finished, if you want to
shoot yourself in the foot you can start a write and then unbind. That
would lead to a partial EFI variable being written as well no?

>
> > Only the TPM subsystem is 'special' because it has to perform
> > a shutdown of the device as well.  On top of that, I think we should
> > try to avoid the kernel depending on userspace apps as much as
> > possible.
>
> There is only tee-supplicant user-space dependency which has to be
> handled carefully. I am very much up for RPMB fastpath via kernel and
> get rid of this dependency as well.

Yes, that is without a doubt our best solution.  Jens is already
working on that, but the supplicant has an entire ecosystem built
around it.  There might be cases where using the in-kernel replacement
might not be possible.

>
> >  I think it's best if we support both of these and add
> > documentation on why this is happening. Would it be hard to have a
> > combination of both of your patches?
> >
>
> It won't be hard but there can be corner cases as mentioned above.

TBH I am fine with merging it as-is. Both cases aren't ideal so it's
mostly a matter of documenting them properly.  I can update the docs
once we decide what to pull and mention the caveats in detail.
I am mostly interested to see what Jan is thinking since he pointed
out the behavior initially

Thanks
/Ilias
>
> -Sumit
>
> > Thanks
> > /Ilias
> >
> > >
> > > -Sumit
> > >
> > > >
> > > > [0] https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
> > > >
> > > > Regards
> > > > /Ilias
> > > > >
> > > > > Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> > > > > ---
> > > > >  drivers/firmware/efi/efi.c  |  6 ++++++
> > > > >  drivers/firmware/efi/vars.c |  8 ++++++++
> > > > >  fs/efivarfs/super.c         | 33 +++++++++++++++++++++++++++++++++
> > > > >  include/linux/efi.h         |  8 ++++++++
> > > > >  4 files changed, 55 insertions(+)
> > > > >
> > > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> > > > > index 53ae25bbb6ac..d2eec5ed8e5e 100644
> > > > > --- a/drivers/firmware/efi/efi.c
> > > > > +++ b/drivers/firmware/efi/efi.c
> > > > > @@ -32,6 +32,7 @@
> > > > >  #include <linux/ucs2_string.h>
> > > > >  #include <linux/memblock.h>
> > > > >  #include <linux/security.h>
> > > > > +#include <linux/notifier.h>
> > > > >
> > > > >  #include <asm/early_ioremap.h>
> > > > >
> > > > > @@ -187,6 +188,9 @@ static const struct attribute_group efi_subsys_attr_group = {
> > > > >         .is_visible = efi_attr_is_visible,
> > > > >  };
> > > > >
> > > > > +struct blocking_notifier_head efivar_ops_nh;
> > > > > +EXPORT_SYMBOL_GPL(efivar_ops_nh);
> > > > > +
> > > > >  static struct efivars generic_efivars;
> > > > >  static struct efivar_operations generic_ops;
> > > > >
> > > > > @@ -427,6 +431,8 @@ static int __init efisubsys_init(void)
> > > > >                 platform_device_register_simple("efivars", 0, NULL, 0);
> > > > >         }
> > > > >
> > > > > +       BLOCKING_INIT_NOTIFIER_HEAD(&efivar_ops_nh);
> > > > > +
> > > > >         error = sysfs_create_group(efi_kobj, &efi_subsys_attr_group);
> > > > >         if (error) {
> > > > >                 pr_err("efi: Sysfs attribute export failed with error %d.\n",
> > > > > diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> > > > > index e9dc7116daf1..f654e6f6af87 100644
> > > > > --- a/drivers/firmware/efi/vars.c
> > > > > +++ b/drivers/firmware/efi/vars.c
> > > > > @@ -63,6 +63,7 @@ int efivars_register(struct efivars *efivars,
> > > > >                      const struct efivar_operations *ops)
> > > > >  {
> > > > >         int rv;
> > > > > +       int event;
> > > > >
> > > > >         if (down_interruptible(&efivars_lock))
> > > > >                 return -EINTR;
> > > > > @@ -77,6 +78,13 @@ int efivars_register(struct efivars *efivars,
> > > > >
> > > > >         __efivars = efivars;
> > > > >
> > > > > +       if (efivar_supports_writes())
> > > > > +               event = EFIVAR_OPS_RDWR;
> > > > > +       else
> > > > > +               event = EFIVAR_OPS_RDONLY;
> > > > > +
> > > > > +       blocking_notifier_call_chain(&efivar_ops_nh, event, NULL);
> > > > > +
> > > > >         pr_info("Registered efivars operations\n");
> > > > >         rv = 0;
> > > > >  out:
> > > > > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > > > > index e028fafa04f3..0f6e4d223aea 100644
> > > > > --- a/fs/efivarfs/super.c
> > > > > +++ b/fs/efivarfs/super.c
> > > > > @@ -14,11 +14,36 @@
> > > > >  #include <linux/slab.h>
> > > > >  #include <linux/magic.h>
> > > > >  #include <linux/statfs.h>
> > > > > +#include <linux/notifier.h>
> > > > >
> > > > >  #include "internal.h"
> > > > >
> > > > >  LIST_HEAD(efivarfs_list);
> > > > >
> > > > > +struct efivarfs_info {
> > > > > +       struct super_block *sb;
> > > > > +       struct notifier_block nb;
> > > > > +};
> > > > > +
> > > > > +static struct efivarfs_info info;
> > > > > +
> > > > > +static int efivarfs_ops_notifier(struct notifier_block *nb, unsigned long event,
> > > > > +                                void *data)
> > > > > +{
> > > > > +       switch (event) {
> > > > > +       case EFIVAR_OPS_RDONLY:
> > > > > +               info.sb->s_flags |= SB_RDONLY;
> > > > > +               break;
> > > > > +       case EFIVAR_OPS_RDWR:
> > > > > +               info.sb->s_flags &= ~SB_RDONLY;
> > > > > +               break;
> > > > > +       default:
> > > > > +               return NOTIFY_DONE;
> > > > > +       }
> > > > > +
> > > > > +       return NOTIFY_OK;
> > > > > +}
> > > > > +
> > > > >  static void efivarfs_evict_inode(struct inode *inode)
> > > > >  {
> > > > >         clear_inode(inode);
> > > > > @@ -255,6 +280,12 @@ static int efivarfs_fill_super(struct super_block *sb, struct fs_context *fc)
> > > > >         if (!root)
> > > > >                 return -ENOMEM;
> > > > >
> > > > > +       info.sb = sb;
> > > > > +       info.nb.notifier_call = efivarfs_ops_notifier;
> > > > > +       err = blocking_notifier_chain_register(&efivar_ops_nh, &info.nb);
> > > > > +       if (err)
> > > > > +               return err;
> > > > > +
> > > > >         INIT_LIST_HEAD(&efivarfs_list);
> > > > >
> > > > >         err = efivar_init(efivarfs_callback, (void *)sb, true, &efivarfs_list);
> > > > > @@ -281,6 +312,8 @@ static int efivarfs_init_fs_context(struct fs_context *fc)
> > > > >
> > > > >  static void efivarfs_kill_sb(struct super_block *sb)
> > > > >  {
> > > > > +       blocking_notifier_chain_unregister(&efivar_ops_nh, &info.nb);
> > > > > +       info.sb = NULL;
> > > > >         kill_litter_super(sb);
> > > > >
> > > > >         if (!efivar_is_available())
> > > > > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > > > > index 603bba2d6437..17cd628b5c42 100644
> > > > > --- a/include/linux/efi.h
> > > > > +++ b/include/linux/efi.h
> > > > > @@ -1365,6 +1365,14 @@ bool efi_config_table_is_usable(const efi_guid_t *guid, unsigned long table)
> > > > >
> > > > >  umode_t efi_attr_is_visible(struct kobject *kobj, struct attribute *attr, int n);
> > > > >
> > > > > +/*
> > > > > + * efivar ops event type
> > > > > + */
> > > > > +#define EFIVAR_OPS_RDONLY 0
> > > > > +#define EFIVAR_OPS_RDWR 1
> > > > > +
> > > > > +extern struct blocking_notifier_head efivar_ops_nh;
> > > > > +
> > > > >  void efivars_generic_ops_register(void);
> > > > >  void efivars_generic_ops_unregister(void);
> > > > >
> > > > > --
> > > > > 2.30.2
> > > > >

[PATCH v8 5/5] efivarfs: force RO when remounting if SetVariable is not supported

[Masahisa Kojima]

From: Ilias Apalodimas <ilias.apalodimas@linaro.org>

If SetVariable at runtime is not supported by the firmware we never assign
a callback for that function. At the same time mount the efivarfs as
RO so no one can call that.  However, we never check the permission flags
when someone remounts the filesystem as RW. As a result this leads to a
crash looking like this:

$ mount -o remount,rw /sys/firmware/efi/efivars
$ efi-updatevar -f PK.auth PK

[  303.279166] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[  303.280482] Mem abort info:
[  303.280854]   ESR = 0x0000000086000004
[  303.281338]   EC = 0x21: IABT (current EL), IL = 32 bits
[  303.282016]   SET = 0, FnV = 0
[  303.282414]   EA = 0, S1PTW = 0
[  303.282821]   FSC = 0x04: level 0 translation fault
[  303.283771] user pgtable: 4k pages, 48-bit VAs, pgdp=000000004258c000
[  303.284913] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[  303.286076] Internal error: Oops: 0000000086000004 [#1] PREEMPT SMP
[  303.286936] Modules linked in: qrtr tpm_tis tpm_tis_core crct10dif_ce arm_smccc_trng rng_core drm fuse ip_tables x_tables ipv6
[  303.288586] CPU: 1 PID: 755 Comm: efi-updatevar Not tainted 6.3.0-rc1-00108-gc7d0c4695c68 #1
[  303.289748] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.04-00627-g88336918701d 04/01/2023
[  303.291150] pstate: 60400005 (nZCv daif +PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[  303.292123] pc : 0x0
[  303.292443] lr : efivar_set_variable_locked+0x74/0xec
[  303.293156] sp : ffff800008673c10
[  303.293619] x29: ffff800008673c10 x28: ffff0000037e8000 x27: 0000000000000000
[  303.294592] x26: 0000000000000800 x25: ffff000002467400 x24: 0000000000000027
[  303.295572] x23: ffffd49ea9832000 x22: ffff0000020c9800 x21: ffff000002467000
[  303.296566] x20: 0000000000000001 x19: 00000000000007fc x18: 0000000000000000
[  303.297531] x17: 0000000000000000 x16: 0000000000000000 x15: 0000aaaac807ab54
[  303.298495] x14: ed37489f673633c0 x13: 71c45c606de13f80 x12: 47464259e219acf4
[  303.299453] x11: ffff000002af7b01 x10: 0000000000000003 x9 : 0000000000000002
[  303.300431] x8 : 0000000000000010 x7 : ffffd49ea8973230 x6 : 0000000000a85201
[  303.301412] x5 : 0000000000000000 x4 : ffff0000020c9800 x3 : 00000000000007fc
[  303.302370] x2 : 0000000000000027 x1 : ffff000002467400 x0 : ffff000002467000
[  303.303341] Call trace:
[  303.303679]  0x0
[  303.303938]  efivar_entry_set_get_size+0x98/0x16c
[  303.304585]  efivarfs_file_write+0xd0/0x1a4
[  303.305148]  vfs_write+0xc4/0x2e4
[  303.305601]  ksys_write+0x70/0x104
[  303.306073]  __arm64_sys_write+0x1c/0x28
[  303.306622]  invoke_syscall+0x48/0x114
[  303.307156]  el0_svc_common.constprop.0+0x44/0xec
[  303.307803]  do_el0_svc+0x38/0x98
[  303.308268]  el0_svc+0x2c/0x84
[  303.308702]  el0t_64_sync_handler+0xf4/0x120
[  303.309293]  el0t_64_sync+0x190/0x194
[  303.309794] Code: ???????? ???????? ???????? ???????? (????????)
[  303.310612] ---[ end trace 0000000000000000 ]---

Fix this by adding a .reconfigure() function to the fs operations which
we can use to check the requested flags and deny anything that's not RO
if the firmware doesn't implement SetVariable at runtime.

Fixes: f88814cc2578 ("efi/efivars: Expose RT service availability via efivars abstraction")
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 fs/efivarfs/super.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index 0f6e4d223aea..942e748a4e03 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -15,6 +15,7 @@
 #include <linux/magic.h>
 #include <linux/statfs.h>
 #include <linux/notifier.h>
+#include <linux/printk.h>
 
 #include "internal.h"
 
@@ -300,8 +301,19 @@ static int efivarfs_get_tree(struct fs_context *fc)
 	return get_tree_single(fc, efivarfs_fill_super);
 }
 
+static int efivarfs_reconfigure(struct fs_context *fc)
+{
+	if (!efivar_supports_writes() && !(fc->sb_flags & SB_RDONLY)) {
+		pr_err("Firmware does not support SetVariableRT. Can not remount with rw\n");
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static const struct fs_context_operations efivarfs_context_ops = {
 	.get_tree	= efivarfs_get_tree,
+	.reconfigure	= efivarfs_reconfigure,
 };
 
 static int efivarfs_init_fs_context(struct fs_context *fc)
-- 
2.30.2

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Ilias Apalodimas]

Hi Jan,

On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>
> This series introduces the 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.
>
> Changelog:
> v7 -> v8
> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
> - fix typos
> - refactor error handling, direct return if applicable
> - use devm_add_action_or_reset() for closing of tee context/session
> - remove obvious comment

Any chance you can run this and see if it solves your issues?

Thanks
/Ilias
>
> v6 -> v7
> Patch #1-#4 are not updated.
> Patch #5 is added into this series, original patch is here:
> https://lore.kernel.org/all/20230609094532.562934-1-ilias.apalodimas@linaro.org/
>
> There are two issues in the v6 series and v7 series addresses those.
>
> 1) efivar ops is not restored when the tee-supplicant daemon terminates.
>  -> As the following patch says, user must remove the device before
>     terminating tee-supplicant daemon.
>     https://lore.kernel.org/all/20230728134832.326467-1-sumit.garg@linaro.org/
>
> 2) cause panic when someone remounts the efivarfs as RW even if
> SetVariable is not supported
>  -> The fifth patch addresses this issue.
>    "[PATCH v7 5/5] efivarfs: force RO when remounting if SetVariable is
>     not supported"
>
> v5 -> v6
> - new patch #4 is added in this series, #1-#3 patches are unchanged.
>   automatically update super block flag when the efivarops support
>   SetVariable runtime service, so that user does not need to manually
>   remount the efivarfs as RW.
>
> v4 -> v5
> - rebase to efi-next based on v6.4-rc1
> - set generic_ops.query_variable_info, it works as expected as follows.
> $ df -h /sys/firmware/efi/efivars/
> Filesystem      Size  Used Avail Use% Mounted on
> efivarfs         16K  1.3K   15K   8% /sys/firmware/efi/efivars
>
> v3 -> v4:
> - replace the reference from EDK2 to PI Specification
> - remove EDK2 source code reference comments
> - prepare nonblocking variant of set_variable, it just returns
>   EFI_UNSUPPORTED
> - remove redundant buffer size check
> - argument name change in mm_communicate
> - function interface changes in setup_mm_hdr to remove (void **) cast
>
> v2 -> v3:
> - add CONFIG_EFI dependency to TEE_STMM_EFI
> - add missing return code check for tee_client_invoke_func()
> - directly call efivars_register/unregister from tee_stmm_efi.c
>
> rfc v1 -> v2:
> - split patch into three patches, one for drivers/tee,
>   one for include/linux/efi.h, and one for the driver/firmware/efi/stmm
> - context/session management into probe() and remove() same as other tee
> client driver
> - StMM variable driver is moved from driver/tee/optee to driver/firmware/efi
> - use "tee" prefix instead of "optee" in driver/firmware/efi/stmm/tee_stmm_efi.c,
>   this file does not contain op-tee specific code, abstracted by tee layer and
>   StMM variable driver will work on other tee implementation.
> - PTA_STMM_CMD_COMMUNICATE -> PTA_STMM_CMD_COMMUNICATE
> - implement query_variable_store() but currently not used
> - no use of TEEC_SUCCESS, it is defined in driver/tee/optee/optee_private.h.
>   Other tee client drivers use 0 instead of using TEEC_SUCCESS
> - remove TEEC_ERROR_EXCESS_DATA status, it is referred just to output
> error message
>
>
>
> Ilias Apalodimas (1):
>   efivarfs: force RO when remounting if SetVariable is not supported
>
> Masahisa Kojima (4):
>   efi: expose efivar generic ops register function
>   efi: Add EFI_ACCESS_DENIED status code
>   efi: Add tee-based EFI variable driver
>   efivarfs: automatically update super block flag
>
>  drivers/firmware/efi/Kconfig                 |  15 +
>  drivers/firmware/efi/Makefile                |   1 +
>  drivers/firmware/efi/efi.c                   |  18 +
>  drivers/firmware/efi/stmm/mm_communication.h | 236 +++++++
>  drivers/firmware/efi/stmm/tee_stmm_efi.c     | 612 +++++++++++++++++++
>  drivers/firmware/efi/vars.c                  |   8 +
>  fs/efivarfs/super.c                          |  45 ++
>  include/linux/efi.h                          |  12 +
>  8 files changed, 947 insertions(+)
>  create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
>  create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c
>
>
> base-commit: f6e6e95ce16205025b7b8680a66c30a0c4ec2270
> --
> 2.30.2
>

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Jan Kiszka]

On 14.08.23 11:24, Ilias Apalodimas wrote:
> Hi Jan,
> 
> On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>
>> This series introduces the 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.
>>
>> Changelog:
>> v7 -> v8
>> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
>> - fix typos
>> - refactor error handling, direct return if applicable
>> - use devm_add_action_or_reset() for closing of tee context/session
>> - remove obvious comment
> 
> Any chance you can run this and see if it solves your issues?
> 

I also need [1], and I still need a cleanup script before terminating
the tee-supplicant, right? And if need some service in the initrd
already, I still need to start the supplicant there and transfer its
ownership to systemd later on? These patches here only make life easier
if the supplicant is started by systemd, after efivarfs has been
mounted, correct?

Jan

[1] https://lkml.org/lkml/2023/7/28/853

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Ilias Apalodimas]

On Tue, 15 Aug 2023 at 05:41, Masahisa Kojima
<masahisa.kojima@linaro.org> wrote:
>
> Hi Jan,
>
> 2023年8月15日(火) 2:23 Jan Kiszka <jan.kiszka@siemens.com>:
>>
>> On 14.08.23 11:24, Ilias Apalodimas wrote:
>> > Hi Jan,
>> >
>> > On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>> >>
>> >> This series introduces the 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.
>> >>
>> >> Changelog:
>> >> v7 -> v8
>> >> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
>> >> - fix typos
>> >> - refactor error handling, direct return if applicable
>> >> - use devm_add_action_or_reset() for closing of tee context/session
>> >> - remove obvious comment
>> >
>> > Any chance you can run this and see if it solves your issues?
>> >
>>
>> I also need [1], and I still need a cleanup script before terminating
>> the tee-supplicant, right?
>
>
> Yes, we need patch[1] and a cleanup script.
> Sorry, I should note in the cover letter.
>
>> And if need some service in the initrd
>> already, I still need to start the supplicant there and transfer its
>> ownership to systemd later on?
>
> Yes.
>
>> These patches here only make life easier
>> if the supplicant is started by systemd, after efivarfs has been
>> mounted, correct?

Not systemd specifically.  Any tool that can signal
<dev>/driver/unbind would work.  Sumit is just reusing the default
unbind notification mechanism

Thanks
/Ilias

>
> Yes.
>
> Thanks,
> Masahisa Kojima
>
>>
>>
>> Jan
>>
>> [1] https://lkml.org/lkml/2023/7/28/853
>>
>> --
>> Siemens AG, Technology
>> Linux Expert Center
>>

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Jan Kiszka]

On 16.08.23 13:58, Ilias Apalodimas wrote:
> On Tue, 15 Aug 2023 at 05:41, Masahisa Kojima
> <masahisa.kojima@linaro.org> wrote:
>>
>> Hi Jan,
>>
>> 2023年8月15日(火) 2:23 Jan Kiszka <jan.kiszka@siemens.com>:
>>>
>>> On 14.08.23 11:24, Ilias Apalodimas wrote:
>>>> Hi Jan,
>>>>
>>>> On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
>>>>>
>>>>> This series introduces the 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.
>>>>>
>>>>> Changelog:
>>>>> v7 -> v8
>>>>> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
>>>>> - fix typos
>>>>> - refactor error handling, direct return if applicable
>>>>> - use devm_add_action_or_reset() for closing of tee context/session
>>>>> - remove obvious comment
>>>>
>>>> Any chance you can run this and see if it solves your issues?
>>>>
>>>
>>> I also need [1], and I still need a cleanup script before terminating
>>> the tee-supplicant, right?
>>
>>
>> Yes, we need patch[1] and a cleanup script.
>> Sorry, I should note in the cover letter.
>>
>>> And if need some service in the initrd
>>> already, I still need to start the supplicant there and transfer its
>>> ownership to systemd later on?
>>
>> Yes.
>>
>>> These patches here only make life easier
>>> if the supplicant is started by systemd, after efivarfs has been
>>> mounted, correct?
> 
> Not systemd specifically.  Any tool that can signal
> <dev>/driver/unbind would work.  Sumit is just reusing the default
> unbind notification mechanism
> 

I was referring to the boot ordering topic, not the shutdown issue.

The latter has now a nicer way to trigger the device shutdown prior to
killing tee-supplicant, but you still need to do that explicitly, no?

Jan

-- 
Siemens AG, Technology
Linux Expert Center

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Sumit Garg]

On Wed, 16 Aug 2023 at 19:37, Jan Kiszka <jan.kiszka@siemens.com> wrote:
>
> On 16.08.23 13:58, Ilias Apalodimas wrote:
> > On Tue, 15 Aug 2023 at 05:41, Masahisa Kojima
> > <masahisa.kojima@linaro.org> wrote:
> >>
> >> Hi Jan,
> >>
> >> 2023年8月15日(火) 2:23 Jan Kiszka <jan.kiszka@siemens.com>:
> >>>
> >>> On 14.08.23 11:24, Ilias Apalodimas wrote:
> >>>> Hi Jan,
> >>>>
> >>>> On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> >>>>>
> >>>>> This series introduces the 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.
> >>>>>
> >>>>> Changelog:
> >>>>> v7 -> v8
> >>>>> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
> >>>>> - fix typos
> >>>>> - refactor error handling, direct return if applicable
> >>>>> - use devm_add_action_or_reset() for closing of tee context/session
> >>>>> - remove obvious comment
> >>>>
> >>>> Any chance you can run this and see if it solves your issues?
> >>>>
> >>>
> >>> I also need [1], and I still need a cleanup script before terminating
> >>> the tee-supplicant, right?
> >>
> >>
> >> Yes, we need patch[1] and a cleanup script.
> >> Sorry, I should note in the cover letter.
> >>
> >>> And if need some service in the initrd
> >>> already, I still need to start the supplicant there and transfer its
> >>> ownership to systemd later on?
> >>
> >> Yes.
> >>
> >>> These patches here only make life easier
> >>> if the supplicant is started by systemd, after efivarfs has been
> >>> mounted, correct?
> >
> > Not systemd specifically.  Any tool that can signal
> > <dev>/driver/unbind would work.  Sumit is just reusing the default
> > unbind notification mechanism
> >
>
> I was referring to the boot ordering topic, not the shutdown issue.
>
> The latter has now a nicer way to trigger the device shutdown prior to
> killing tee-supplicant, but you still need to do that explicitly, no?
>

Yeah it has to be done explicitly in user-space. As you have already
seen, my first try (v1 patch) to do it in kernel space failed. The
reason being that when those devices are being removed, the
tee-supplicant has to be alive to handle RPC calls. The kernel only
gets notified once "/dev/teepriv0" fd is closed and by that time
tee-supplicant is already dead.

-Sumit

> Jan
>
> --
> Siemens AG, Technology
> Linux Expert Center
>

Re: [PATCH v8 0/5] introduce tee-based EFI Runtime Variable Service

[Ilias Apalodimas]

On Thu, 17 Aug 2023 at 12:22, Sumit Garg <sumit.garg@linaro.org> wrote:
>
> On Wed, 16 Aug 2023 at 19:37, Jan Kiszka <jan.kiszka@siemens.com> wrote:
> >
> > On 16.08.23 13:58, Ilias Apalodimas wrote:
> > > On Tue, 15 Aug 2023 at 05:41, Masahisa Kojima
> > > <masahisa.kojima@linaro.org> wrote:
> > >>
> > >> Hi Jan,
> > >>
> > >> 2023年8月15日(火) 2:23 Jan Kiszka <jan.kiszka@siemens.com>:
> > >>>
> > >>> On 14.08.23 11:24, Ilias Apalodimas wrote:
> > >>>> Hi Jan,
> > >>>>
> > >>>> On Mon, 7 Aug 2023 at 05:53, Masahisa Kojima <masahisa.kojima@linaro.org> wrote:
> > >>>>>
> > >>>>> This series introduces the 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.
> > >>>>>
> > >>>>> Changelog:
> > >>>>> v7 -> v8
> > >>>>> Only patch #3 "efi: Add tee-based EFI variable driver" is updated.
> > >>>>> - fix typos
> > >>>>> - refactor error handling, direct return if applicable
> > >>>>> - use devm_add_action_or_reset() for closing of tee context/session
> > >>>>> - remove obvious comment
> > >>>>
> > >>>> Any chance you can run this and see if it solves your issues?
> > >>>>
> > >>>
> > >>> I also need [1], and I still need a cleanup script before terminating
> > >>> the tee-supplicant, right?
> > >>
> > >>
> > >> Yes, we need patch[1] and a cleanup script.
> > >> Sorry, I should note in the cover letter.
> > >>
> > >>> And if need some service in the initrd
> > >>> already, I still need to start the supplicant there and transfer its
> > >>> ownership to systemd later on?
> > >>
> > >> Yes.
> > >>
> > >>> These patches here only make life easier
> > >>> if the supplicant is started by systemd, after efivarfs has been
> > >>> mounted, correct?
> > >
> > > Not systemd specifically.  Any tool that can signal
> > > <dev>/driver/unbind would work.  Sumit is just reusing the default
> > > unbind notification mechanism
> > >
> >
> > I was referring to the boot ordering topic, not the shutdown issue.
> >
> > The latter has now a nicer way to trigger the device shutdown prior to
> > killing tee-supplicant, but you still need to do that explicitly, no?
> >
>
> Yeah it has to be done explicitly in user-space. As you have already
> seen, my first try (v1 patch) to do it in kernel space failed. The
> reason being that when those devices are being removed, the
> tee-supplicant has to be alive to handle RPC calls. The kernel only
> gets notified once "/dev/teepriv0" fd is closed and by that time
> tee-supplicant is already dead.

Yea, that was along the lines of what I asked in a past mail.  IOW
bind the cleanups needed on the opening/closing of the
"/dev/teepriv0", but unfortunately that's not doable.  What we could
do as a future enhancement is add a signal handler in the supplicant
that signals the events without relying on a different userspace app
to do that.

Sumit pointed out a few things we need to be cautious about on the
signal handler, but in any case, that's orthogonal to the current
approach.

Thanks
/Ilias

>
> -Sumit
>
> > Jan
> >
> > --
> > Siemens AG, Technology
> > Linux Expert Center
> >