LinuxLists.cc - [PATCH v5 3/3] efi: Add tee-based EFI variable driver

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 26.05.23 03:07, Masahisa Kojima wrote:
> When the flash is not owned by the non-secure world, accessing the EFI
> variables is straightforward and done via EFI Runtime Variable Services.
> In this case, critical variables for system integrity and security
> are normally stored in the dedicated secure storage and only accessible
> from the secure world.
>
> On the other hand, the small embedded devices don't have the special
> dedicated secure storage. The eMMC device with an RPMB partition is
> becoming more common, we can use an RPMB partition to store the
> EFI Variables.
>
> The eMMC device is typically owned by the non-secure world(linux in
> this case). There is an existing solution utilizing eMMC RPMB partition
> for EFI Variables, it is implemented by interacting with
> 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 <[email protected]>
> Co-developed-by: Ilias Apalodimas <[email protected]>
> Signed-off-by: Ilias Apalodimas <[email protected]>
> Signed-off-by: Masahisa Kojima <[email protected]>
> ---
> 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 | 638 +++++++++++++++++++
> 4 files changed, 890 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 043ca31c114e..aa38089d1e4a 100644
> --- a/drivers/firmware/efi/Kconfig
> +++ b/drivers/firmware/efi/Kconfig
> @@ -287,3 +287,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 b51f2a4c821e..2ca8ee6ab490 100644
> --- a/drivers/firmware/efi/Makefile
> +++ b/drivers/firmware/efi/Makefile
> @@ -41,3 +41,4 @@ obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
> obj-$(CONFIG_EFI_EARLYCON) += earlycon.o
> obj-$(CONFIG_UEFI_CPER_ARM) += cper-arm.o
> obj-$(CONFIG_UEFI_CPER_X86) += cper-x86.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..f6623171ae04
> --- /dev/null
> +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> @@ -0,0 +1,638 @@
> +// 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;
> + efi_status_t ret;
> + 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:
> + ret = EFI_SUCCESS;
> + break;
> +
> + case ARM_SVC_SPM_RET_INVALID_PARAMS:
> + ret = EFI_INVALID_PARAMETER;
> + break;
> +
> + case ARM_SVC_SPM_RET_DENIED:
> + ret = EFI_ACCESS_DENIED;
> + break;
> +
> + case ARM_SVC_SPM_RET_NO_MEMORY:
> + ret = EFI_OUT_OF_RESOURCES;
> + break;
> +
> + default:
> + ret = EFI_ACCESS_DENIED;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * mm_communicate() - Adjust the 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) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + payload_size = sizeof(*var_payload);
> + var_payload = setup_mm_hdr(&comm_buf, payload_size,
> + SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
> + &ret);
> + if (!comm_buf)
> + goto out;
> +
> + ret = mm_communicate(comm_buf, payload_size);
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> + /* Make sure the buffer is big enough for storing variables */
> + if (var_payload->size < MM_VARIABLE_ACCESS_HEADER_SIZE + 0x20) {
> + ret = EFI_DEVICE_ERROR;
> + goto out;
> + }
> + *size = var_payload->size;
> + /*
> + * There seems to be a bug in EDK2 miscalculating the boundaries and
> + * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
> + * it up here to ensure backwards compatibility with older versions
> + * (cf. StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c.
> + * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the
> + * flexible array member).
> + *
> + * size is guaranteed to be > 2 due to checks on the beginning.
> + */
> + *size -= 2;
> +out:
> + kfree(comm_buf);
> + return ret;
> +}
> +
> +static efi_status_t get_property_int(u16 *name, size_t name_size,
> + const efi_guid_t *vendor,
> + struct var_check_property *var_property)
> +{
> + struct smm_variable_var_check_property *smm_property;
> + size_t payload_size;
> + u8 *comm_buf = NULL;
> + efi_status_t ret;
> +
> + memset(var_property, 0, sizeof(*var_property));
> + payload_size = sizeof(*smm_property) + name_size;
> + if (payload_size > max_payload_size) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + smm_property = setup_mm_hdr(
> + &comm_buf, payload_size,
> + SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> + if (!comm_buf)
> + goto out;
> +
> + memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
> + smm_property->name_size = name_size;
> + memcpy(smm_property->name, name, name_size);
> +
> + ret = mm_communicate(comm_buf, payload_size);
> + /*
> + * Currently only R/O property is supported in StMM.
> + * Variables that are not set to R/O will not set the property in StMM
> + * and the call will return EFI_NOT_FOUND. We are setting the
> + * properties to 0x0 so checking against that is enough for the
> + * EFI_NOT_FOUND case.
> + */
> + if (ret == EFI_NOT_FOUND)
> + ret = EFI_SUCCESS;
> + if (ret != EFI_SUCCESS)
> + goto out;
> + memcpy(var_property, &smm_property->property, sizeof(*var_property));
> +
> +out:
> + kfree(comm_buf);
> + return ret;
> +}
> +
> +static efi_status_t 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) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> + if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /* Trim output buffer size */
> + tmp_dsize = *data_size;
> + if (name_size + tmp_dsize >
> + max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> + tmp_dsize = max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE -
> + name_size;
> + }
> +
> + /* Get communication buffer and initialize header */
> + payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> + var_acc = setup_mm_hdr(&comm_buf, payload_size,
> + SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> + if (!comm_buf)
> + goto out;
> +
> + /* Fill in contents */
> + memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> + var_acc->data_size = tmp_dsize;
> + var_acc->name_size = name_size;
> + var_acc->attr = attributes ? *attributes : 0;
> + memcpy(var_acc->name, name, name_size);
> +
> + /* Communicate */
> + ret = mm_communicate(comm_buf, payload_size);
> + if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL)
> + /* Update with reported data size for trimmed case */
> + *data_size = var_acc->data_size;
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> + ret = get_property_int(name, name_size, vendor, &var_property);
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> + if (attributes)
> + *attributes = var_acc->attr;
> +
> + if (data)
> + memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> + var_acc->data_size);
> + else
> + ret = EFI_INVALID_PARAMETER;
> +
> +out:
> + kfree(comm_buf);
> + return ret;
> +}
> +
> +static efi_status_t 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) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + out_name_size = *name_size;
> + in_name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> +
> + if (out_name_size < in_name_size) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + if (in_name_size >
> + max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /* Trim output buffer size */
> + if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> + out_name_size =
> + max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
> +
> + payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
> + var_getnext = setup_mm_hdr(&comm_buf, payload_size,
> + SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> + &ret);
> + if (!comm_buf)
> + goto out;
> +
> + /* Fill in contents */
> + memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> + var_getnext->name_size = out_name_size;
> + memcpy(var_getnext->name, name, in_name_size);
> + memset((u8 *)var_getnext->name + in_name_size, 0x0,
> + out_name_size - in_name_size);
> +
> + /* Communicate */
> + ret = mm_communicate(comm_buf, payload_size);
> + if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> + /* Update with reported data size for trimmed case */
> + *name_size = var_getnext->name_size;
> + }
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> + memcpy(guid, &var_getnext->guid, sizeof(*guid));
> + memcpy(name, var_getnext->name, var_getnext->name_size);
> +
> +out:
> + kfree(comm_buf);
> + return ret;
> +}
> +
> +static efi_status_t 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) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + if (data_size > 0 && !data) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> + /* Check payload size */
> + name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> + payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> + if (payload_size > max_payload_size) {
> + ret = EFI_INVALID_PARAMETER;
> + goto out;
> + }
> +
> + /*
> + * Allocate the buffer early, before switching to RW (if needed)
> + * so we won't need to account for any failures in reading/setting
> + * the properties, if the allocation fails
> + */
> + var_acc = setup_mm_hdr(&comm_buf, payload_size,
> + SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> + if (!comm_buf)
> + goto out;
> +
> + /*
> + * The API has the ability to override RO flags. If no RO check was
> + * requested switch the variable to RW for the duration of this call
> + */
> + ret = get_property_int(name, name_size, vendor, &var_property);
> + if (ret != EFI_SUCCESS) {
> + 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);
> +
> +
> + /* Communicate */
> + 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 (!comm_buf)
> + goto out;
> +
> + 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 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;
> +
> + /* 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);
> + rc = -EINVAL;
> + goto out_ctx;
> + }
> + pvt_data.session = sess_arg.session;
> + pvt_data.dev = dev;
> +
> + ret = get_max_payload(&max_payload_size);
> + if (ret != EFI_SUCCESS) {
> + rc = -EIO;
> + goto out_sess;
> + }
> +
> + 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;
> +
> +out_sess:
> + tee_client_close_session(pvt_data.ctx, pvt_data.session);
> +out_ctx:
> + tee_client_close_context(pvt_data.ctx);
> +
> + return rc;
> +}
> +
> +static int tee_stmm_efi_remove(struct device *dev)
> +{
> + efivars_unregister(&tee_efivars);
> + efivars_generic_ops_register();
> +
> + tee_client_close_session(pvt_data.ctx, pvt_data.session);
> + tee_client_close_context(pvt_data.ctx);
> +
> + 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 <[email protected]>");
> +MODULE_AUTHOR("Masahisa Kojima <[email protected]>");
> +MODULE_DESCRIPTION("TEE based EFI runtime variable service driver");

I think we have a probe ordering issue with this driver:
efivarfs_fill_super() may be called before the TEE bus was probed, thus
with the default efivar ops still registered. And that means
efivar_supports_writes() will return false, and the fs declares itself
as readonly. I've seen systemd mounting it r/o initialling, and you need
to remount the fs to enable writability.

Is there anything that could be done to re-order things reliably, probe
the tee bus earlier etc.?

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-06 07:28:58

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan,

On Tue, 6 Jun 2023 at 12:09, Jan Kiszka <[email protected]> wrote:
>
> On 26.05.23 03:07, Masahisa Kojima wrote:
> > When the flash is not owned by the non-secure world, accessing the EFI
> > variables is straightforward and done via EFI Runtime Variable Services.
> > In this case, critical variables for system integrity and security
> > are normally stored in the dedicated secure storage and only accessible
> > from the secure world.
> >
> > On the other hand, the small embedded devices don't have the special
> > dedicated secure storage. The eMMC device with an RPMB partition is
> > becoming more common, we can use an RPMB partition to store the
> > EFI Variables.
> >
> > The eMMC device is typically owned by the non-secure world(linux in
> > this case). There is an existing solution utilizing eMMC RPMB partition
> > for EFI Variables, it is implemented by interacting with
> > 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 <[email protected]>
> > Co-developed-by: Ilias Apalodimas <[email protected]>
> > Signed-off-by: Ilias Apalodimas <[email protected]>
> > Signed-off-by: Masahisa Kojima <[email protected]>
> > ---
> > 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 | 638 +++++++++++++++++++
> > 4 files changed, 890 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 043ca31c114e..aa38089d1e4a 100644
> > --- a/drivers/firmware/efi/Kconfig
> > +++ b/drivers/firmware/efi/Kconfig
> > @@ -287,3 +287,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 b51f2a4c821e..2ca8ee6ab490 100644
> > --- a/drivers/firmware/efi/Makefile
> > +++ b/drivers/firmware/efi/Makefile
> > @@ -41,3 +41,4 @@ obj-$(CONFIG_EFI_CAPSULE_LOADER) += capsule-loader.o
> > obj-$(CONFIG_EFI_EARLYCON) += earlycon.o
> > obj-$(CONFIG_UEFI_CPER_ARM) += cper-arm.o
> > obj-$(CONFIG_UEFI_CPER_X86) += cper-x86.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..f6623171ae04
> > --- /dev/null
> > +++ b/drivers/firmware/efi/stmm/tee_stmm_efi.c
> > @@ -0,0 +1,638 @@
> > +// 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;
> > + efi_status_t ret;
> > + 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:
> > + ret = EFI_SUCCESS;
> > + break;
> > +
> > + case ARM_SVC_SPM_RET_INVALID_PARAMS:
> > + ret = EFI_INVALID_PARAMETER;
> > + break;
> > +
> > + case ARM_SVC_SPM_RET_DENIED:
> > + ret = EFI_ACCESS_DENIED;
> > + break;
> > +
> > + case ARM_SVC_SPM_RET_NO_MEMORY:
> > + ret = EFI_OUT_OF_RESOURCES;
> > + break;
> > +
> > + default:
> > + ret = EFI_ACCESS_DENIED;
> > + }
> > +
> > + return ret;
> > +}
> > +
> > +/**
> > + * mm_communicate() - Adjust the 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) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + payload_size = sizeof(*var_payload);
> > + var_payload = setup_mm_hdr(&comm_buf, payload_size,
> > + SMM_VARIABLE_FUNCTION_GET_PAYLOAD_SIZE,
> > + &ret);
> > + if (!comm_buf)
> > + goto out;
> > +
> > + ret = mm_communicate(comm_buf, payload_size);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + /* Make sure the buffer is big enough for storing variables */
> > + if (var_payload->size < MM_VARIABLE_ACCESS_HEADER_SIZE + 0x20) {
> > + ret = EFI_DEVICE_ERROR;
> > + goto out;
> > + }
> > + *size = var_payload->size;
> > + /*
> > + * There seems to be a bug in EDK2 miscalculating the boundaries and
> > + * size checks, so deduct 2 more bytes to fulfill this requirement. Fix
> > + * it up here to ensure backwards compatibility with older versions
> > + * (cf. StandaloneMmPkg/Drivers/StandaloneMmCpu/AArch64/EventHandle.c.
> > + * sizeof (EFI_MM_COMMUNICATE_HEADER) instead the size minus the
> > + * flexible array member).
> > + *
> > + * size is guaranteed to be > 2 due to checks on the beginning.
> > + */
> > + *size -= 2;
> > +out:
> > + kfree(comm_buf);
> > + return ret;
> > +}
> > +
> > +static efi_status_t get_property_int(u16 *name, size_t name_size,
> > + const efi_guid_t *vendor,
> > + struct var_check_property *var_property)
> > +{
> > + struct smm_variable_var_check_property *smm_property;
> > + size_t payload_size;
> > + u8 *comm_buf = NULL;
> > + efi_status_t ret;
> > +
> > + memset(var_property, 0, sizeof(*var_property));
> > + payload_size = sizeof(*smm_property) + name_size;
> > + if (payload_size > max_payload_size) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + smm_property = setup_mm_hdr(
> > + &comm_buf, payload_size,
> > + SMM_VARIABLE_FUNCTION_VAR_CHECK_VARIABLE_PROPERTY_GET, &ret);
> > + if (!comm_buf)
> > + goto out;
> > +
> > + memcpy(&smm_property->guid, vendor, sizeof(smm_property->guid));
> > + smm_property->name_size = name_size;
> > + memcpy(smm_property->name, name, name_size);
> > +
> > + ret = mm_communicate(comm_buf, payload_size);
> > + /*
> > + * Currently only R/O property is supported in StMM.
> > + * Variables that are not set to R/O will not set the property in StMM
> > + * and the call will return EFI_NOT_FOUND. We are setting the
> > + * properties to 0x0 so checking against that is enough for the
> > + * EFI_NOT_FOUND case.
> > + */
> > + if (ret == EFI_NOT_FOUND)
> > + ret = EFI_SUCCESS;
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > + memcpy(var_property, &smm_property->property, sizeof(*var_property));
> > +
> > +out:
> > + kfree(comm_buf);
> > + return ret;
> > +}
> > +
> > +static efi_status_t 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) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > + if (name_size > max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /* Trim output buffer size */
> > + tmp_dsize = *data_size;
> > + if (name_size + tmp_dsize >
> > + max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE) {
> > + tmp_dsize = max_payload_size - MM_VARIABLE_ACCESS_HEADER_SIZE -
> > + name_size;
> > + }
> > +
> > + /* Get communication buffer and initialize header */
> > + payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + tmp_dsize;
> > + var_acc = setup_mm_hdr(&comm_buf, payload_size,
> > + SMM_VARIABLE_FUNCTION_GET_VARIABLE, &ret);
> > + if (!comm_buf)
> > + goto out;
> > +
> > + /* Fill in contents */
> > + memcpy(&var_acc->guid, vendor, sizeof(var_acc->guid));
> > + var_acc->data_size = tmp_dsize;
> > + var_acc->name_size = name_size;
> > + var_acc->attr = attributes ? *attributes : 0;
> > + memcpy(var_acc->name, name, name_size);
> > +
> > + /* Communicate */
> > + ret = mm_communicate(comm_buf, payload_size);
> > + if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL)
> > + /* Update with reported data size for trimmed case */
> > + *data_size = var_acc->data_size;
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + ret = get_property_int(name, name_size, vendor, &var_property);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + if (attributes)
> > + *attributes = var_acc->attr;
> > +
> > + if (data)
> > + memcpy(data, (u8 *)var_acc->name + var_acc->name_size,
> > + var_acc->data_size);
> > + else
> > + ret = EFI_INVALID_PARAMETER;
> > +
> > +out:
> > + kfree(comm_buf);
> > + return ret;
> > +}
> > +
> > +static efi_status_t 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) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + out_name_size = *name_size;
> > + in_name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > +
> > + if (out_name_size < in_name_size) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + if (in_name_size >
> > + max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /* Trim output buffer size */
> > + if (out_name_size > max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE)
> > + out_name_size =
> > + max_payload_size - MM_VARIABLE_GET_NEXT_HEADER_SIZE;
> > +
> > + payload_size = MM_VARIABLE_GET_NEXT_HEADER_SIZE + out_name_size;
> > + var_getnext = setup_mm_hdr(&comm_buf, payload_size,
> > + SMM_VARIABLE_FUNCTION_GET_NEXT_VARIABLE_NAME,
> > + &ret);
> > + if (!comm_buf)
> > + goto out;
> > +
> > + /* Fill in contents */
> > + memcpy(&var_getnext->guid, guid, sizeof(var_getnext->guid));
> > + var_getnext->name_size = out_name_size;
> > + memcpy(var_getnext->name, name, in_name_size);
> > + memset((u8 *)var_getnext->name + in_name_size, 0x0,
> > + out_name_size - in_name_size);
> > +
> > + /* Communicate */
> > + ret = mm_communicate(comm_buf, payload_size);
> > + if (ret == EFI_SUCCESS || ret == EFI_BUFFER_TOO_SMALL) {
> > + /* Update with reported data size for trimmed case */
> > + *name_size = var_getnext->name_size;
> > + }
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > + memcpy(guid, &var_getnext->guid, sizeof(*guid));
> > + memcpy(name, var_getnext->name, var_getnext->name_size);
> > +
> > +out:
> > + kfree(comm_buf);
> > + return ret;
> > +}
> > +
> > +static efi_status_t 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) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + if (data_size > 0 && !data) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > + /* Check payload size */
> > + name_size = (ucs2_strnlen(name, EFI_VAR_NAME_LEN) + 1) * sizeof(u16);
> > + payload_size = MM_VARIABLE_ACCESS_HEADER_SIZE + name_size + data_size;
> > + if (payload_size > max_payload_size) {
> > + ret = EFI_INVALID_PARAMETER;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Allocate the buffer early, before switching to RW (if needed)
> > + * so we won't need to account for any failures in reading/setting
> > + * the properties, if the allocation fails
> > + */
> > + var_acc = setup_mm_hdr(&comm_buf, payload_size,
> > + SMM_VARIABLE_FUNCTION_SET_VARIABLE, &ret);
> > + if (!comm_buf)
> > + goto out;
> > +
> > + /*
> > + * The API has the ability to override RO flags. If no RO check was
> > + * requested switch the variable to RW for the duration of this call
> > + */
> > + ret = get_property_int(name, name_size, vendor, &var_property);
> > + if (ret != EFI_SUCCESS) {
> > + 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);
> > +
> > +
> > + /* Communicate */
> > + 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 (!comm_buf)
> > + goto out;
> > +
> > + 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 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;
> > +
> > + /* 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);
> > + rc = -EINVAL;
> > + goto out_ctx;
> > + }
> > + pvt_data.session = sess_arg.session;
> > + pvt_data.dev = dev;
> > +
> > + ret = get_max_payload(&max_payload_size);
> > + if (ret != EFI_SUCCESS) {
> > + rc = -EIO;
> > + goto out_sess;
> > + }
> > +
> > + 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;
> > +
> > +out_sess:
> > + tee_client_close_session(pvt_data.ctx, pvt_data.session);
> > +out_ctx:
> > + tee_client_close_context(pvt_data.ctx);
> > +
> > + return rc;
> > +}
> > +
> > +static int tee_stmm_efi_remove(struct device *dev)
> > +{
> > + efivars_unregister(&tee_efivars);
> > + efivars_generic_ops_register();
> > +
> > + tee_client_close_session(pvt_data.ctx, pvt_data.session);
> > + tee_client_close_context(pvt_data.ctx);
> > +
> > + 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 <[email protected]>");
> > +MODULE_AUTHOR("Masahisa Kojima <[email protected]>");
> > +MODULE_DESCRIPTION("TEE based EFI runtime variable service driver");
>
> I think we have a probe ordering issue with this driver:
> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> with the default efivar ops still registered. And that means
> efivar_supports_writes() will return false, and the fs declares itself
> as readonly. I've seen systemd mounting it r/o initialling, and you need
> to remount the fs to enable writability.
>
> Is there anything that could be done to re-order things reliably, probe
> the tee bus earlier etc.?

This driver has a dependency on user-space daemon: tee-supplicant to
be running for RPMB access. So once you start that daemon the
corresponding device will be enumerated on the TEE bus and this driver
probe will be invoked. So I would suggest you to load this daemon very
early in the boot process or better to make it a part of initramfs.

-Sumit

>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-06 07:41:28

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Tue, 6 Jun 2023 at 08:52, Sumit Garg <[email protected]> wrote:
>
> Hi Jan,
>
> On Tue, 6 Jun 2023 at 12:09, Jan Kiszka <[email protected]> wrote:
> >
> > On 26.05.23 03:07, Masahisa Kojima wrote:
> > > When the flash is not owned by the non-secure world, accessing the EFI
> > > variables is straightforward and done via EFI Runtime Variable Services.
> > > In this case, critical variables for system integrity and security
> > > are normally stored in the dedicated secure storage and only accessible
> > > from the secure world.
> > >
> > > On the other hand, the small embedded devices don't have the special
> > > dedicated secure storage. The eMMC device with an RPMB partition is
> > > becoming more common, we can use an RPMB partition to store the
> > > EFI Variables.
> > >
> > > The eMMC device is typically owned by the non-secure world(linux in
> > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > for EFI Variables, it is implemented by interacting with
> > > 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 <[email protected]>
> > > Co-developed-by: Ilias Apalodimas <[email protected]>
> > > Signed-off-by: Ilias Apalodimas <[email protected]>
> > > Signed-off-by: Masahisa Kojima <[email protected]>
> > > ---
> > > 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 | 638 +++++++++++++++++++
> > > 4 files changed, 890 insertions(+)
> > > create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
> > > create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c
> > >
...
> >
> > I think we have a probe ordering issue with this driver:
> > efivarfs_fill_super() may be called before the TEE bus was probed, thus
> > with the default efivar ops still registered. And that means
> > efivar_supports_writes() will return false, and the fs declares itself
> > as readonly. I've seen systemd mounting it r/o initialling, and you need
> > to remount the fs to enable writability.
> >
> > Is there anything that could be done to re-order things reliably, probe
> > the tee bus earlier etc.?
>
> This driver has a dependency on user-space daemon: tee-supplicant to
> be running for RPMB access. So once you start that daemon the
> corresponding device will be enumerated on the TEE bus and this driver
> probe will be invoked. So I would suggest you to load this daemon very
> early in the boot process or better to make it a part of initramfs.
>

That is not the point, really.

If this dependency exists, the code should be aware of that, and made
to work correctly in spite of it. Requiring a module to be part of
initramfs is not a reasonable fix.

IIUC, this also means that the efivar ops are updated while there is
already a client. This seems less than ideal as well

2023-06-06 08:15:11

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 06.06.23 08:57, Ard Biesheuvel wrote:
> On Tue, 6 Jun 2023 at 08:52, Sumit Garg <[email protected]> wrote:
>>
>> Hi Jan,
>>
>> On Tue, 6 Jun 2023 at 12:09, Jan Kiszka <[email protected]> wrote:
>>>
>>> On 26.05.23 03:07, Masahisa Kojima wrote:
>>>> When the flash is not owned by the non-secure world, accessing the EFI
>>>> variables is straightforward and done via EFI Runtime Variable Services.
>>>> In this case, critical variables for system integrity and security
>>>> are normally stored in the dedicated secure storage and only accessible
>>>> from the secure world.
>>>>
>>>> On the other hand, the small embedded devices don't have the special
>>>> dedicated secure storage. The eMMC device with an RPMB partition is
>>>> becoming more common, we can use an RPMB partition to store the
>>>> EFI Variables.
>>>>
>>>> The eMMC device is typically owned by the non-secure world(linux in
>>>> this case). There is an existing solution utilizing eMMC RPMB partition
>>>> for EFI Variables, it is implemented by interacting with
>>>> 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 <[email protected]>
>>>> Co-developed-by: Ilias Apalodimas <[email protected]>
>>>> Signed-off-by: Ilias Apalodimas <[email protected]>
>>>> Signed-off-by: Masahisa Kojima <[email protected]>
>>>> ---
>>>> 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 | 638 +++++++++++++++++++
>>>> 4 files changed, 890 insertions(+)
>>>> create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
>>>> create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c
>>>>
> ...
>>>
>>> I think we have a probe ordering issue with this driver:
>>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
>>> with the default efivar ops still registered. And that means
>>> efivar_supports_writes() will return false, and the fs declares itself
>>> as readonly. I've seen systemd mounting it r/o initialling, and you need
>>> to remount the fs to enable writability.
>>>
>>> Is there anything that could be done to re-order things reliably, probe
>>> the tee bus earlier etc.?
>>
>> This driver has a dependency on user-space daemon: tee-supplicant to
>> be running for RPMB access. So once you start that daemon the
>> corresponding device will be enumerated on the TEE bus and this driver
>> probe will be invoked. So I would suggest you to load this daemon very
>> early in the boot process or better to make it a part of initramfs.
>>
>
> That is not the point, really.
>
> If this dependency exists, the code should be aware of that, and made
> to work correctly in spite of it. Requiring a module to be part of
> initramfs is not a reasonable fix.

In fact, I've tested a non-modularized build as well, just to exclude
that issue. The daemon dependency is more likely the problem here.

>
> IIUC, this also means that the efivar ops are updated while there is
> already a client. This seems less than ideal as well

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-06 08:21:40

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Tue, 6 Jun 2023 at 12:28, Ard Biesheuvel <[email protected]> wrote:
>
> On Tue, 6 Jun 2023 at 08:52, Sumit Garg <[email protected]> wrote:
> >
> > Hi Jan,
> >
> > On Tue, 6 Jun 2023 at 12:09, Jan Kiszka <[email protected]> wrote:
> > >
> > > On 26.05.23 03:07, Masahisa Kojima wrote:
> > > > When the flash is not owned by the non-secure world, accessing the EFI
> > > > variables is straightforward and done via EFI Runtime Variable Services.
> > > > In this case, critical variables for system integrity and security
> > > > are normally stored in the dedicated secure storage and only accessible
> > > > from the secure world.
> > > >
> > > > On the other hand, the small embedded devices don't have the special
> > > > dedicated secure storage. The eMMC device with an RPMB partition is
> > > > becoming more common, we can use an RPMB partition to store the
> > > > EFI Variables.
> > > >
> > > > The eMMC device is typically owned by the non-secure world(linux in
> > > > this case). There is an existing solution utilizing eMMC RPMB partition
> > > > for EFI Variables, it is implemented by interacting with
> > > > 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 <[email protected]>
> > > > Co-developed-by: Ilias Apalodimas <[email protected]>
> > > > Signed-off-by: Ilias Apalodimas <[email protected]>
> > > > Signed-off-by: Masahisa Kojima <[email protected]>
> > > > ---
> > > > 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 | 638 +++++++++++++++++++
> > > > 4 files changed, 890 insertions(+)
> > > > create mode 100644 drivers/firmware/efi/stmm/mm_communication.h
> > > > create mode 100644 drivers/firmware/efi/stmm/tee_stmm_efi.c
> > > >
> ...
> > >
> > > I think we have a probe ordering issue with this driver:
> > > efivarfs_fill_super() may be called before the TEE bus was probed, thus
> > > with the default efivar ops still registered. And that means
> > > efivar_supports_writes() will return false, and the fs declares itself
> > > as readonly. I've seen systemd mounting it r/o initialling, and you need
> > > to remount the fs to enable writability.
> > >
> > > Is there anything that could be done to re-order things reliably, probe
> > > the tee bus earlier etc.?
> >
> > This driver has a dependency on user-space daemon: tee-supplicant to
> > be running for RPMB access. So once you start that daemon the
> > corresponding device will be enumerated on the TEE bus and this driver
> > probe will be invoked. So I would suggest you to load this daemon very
> > early in the boot process or better to make it a part of initramfs.
> >
>
> That is not the point, really.
>
> If this dependency exists, the code should be aware of that, and made
> to work correctly in spite of it. Requiring a module to be part of
> initramfs is not a reasonable fix.

I am not sure if I followed you here. Until we have the tee-stmm-efi
module loaded we won't get corresponding EFI operations registered.
The key here is that the underlying OP-TEE device won't be available
until tee-supplicant (see enumeration with PTA_CMD_GET_DEVICES_SUPP)
is running.

Do you have any better ideas regarding how this should be handled?

>
> IIUC, this also means that the efivar ops are updated while there is
> already a client. This seems less than ideal as well

That's true. An ideal situation would be to allow in-kernel RPMB
access APIs for OP-TEE or other kernel drivers to use. With that we
should be able to remove this tee-supplicant dependency. I hope you
are already aware about the earlier efforts to add the RPMB subsystem
to the kernel. But with these real world use-cases emerging like EFI
variables and fTPM, we should be able to convince corresponding
subsystem maintainers.

-Sumit

2023-06-07 06:50:58

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Wed, 7 Jun 2023 at 09:34, Ilias Apalodimas
<[email protected]> wrote:
>
> Hi Jan,
>
> [...]
>
> > >>>>
> > > ...
> > >>>
> > >>> I think we have a probe ordering issue with this driver:
> > >>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> > >>> with the default efivar ops still registered. And that means
> > >>> efivar_supports_writes() will return false, and the fs declares itself
> > >>> as readonly. I've seen systemd mounting it r/o initialling, and you need
> > >>> to remount the fs to enable writability.
> > >>>
> > >>> Is there anything that could be done to re-order things reliably, probe
> > >>> the tee bus earlier etc.?
> > >>
> > >> This driver has a dependency on user-space daemon: tee-supplicant to
> > >> be running for RPMB access. So once you start that daemon the
> > >> corresponding device will be enumerated on the TEE bus and this driver
> > >> probe will be invoked. So I would suggest you to load this daemon very
> > >> early in the boot process or better to make it a part of initramfs.
> > >>
> > >
> > > That is not the point, really.
> > >
> > > If this dependency exists, the code should be aware of that, and made
> > > to work correctly in spite of it. Requiring a module to be part of
> > > initramfs is not a reasonable fix.
> >
> > In fact, I've tested a non-modularized build as well, just to exclude
> > that issue. The daemon dependency is more likely the problem here.
> >
> > >
> > > IIUC, this also means that the efivar ops are updated while there is
> > > already a client. This seems less than ideal as well
>
> As Sumit pointed out, the 'device' won't be available from OP-TEE
> until the supplicant is up and running and as a result, the module
> _probe() function won't run. Unfortunately, this isn't something we
> can avoid since the supplicant is responsible for the RPMB writes.
> The only thing I can think of is moving parts of the supplicant to the
> kernel and wiring up the RPC calls for reading/writing data to the
> eMMC subsystem. There was another discussion here [0] requesting the
> same thing for different reasons. But unless I am missing something
> this won't solve the problem completely either. You still have a
> timing dependency of "when did the RT callbacks change" -- "when was
> my efivarfs mounted".
>

Forgot to attach the link... apologies for the noise.
[0] https://lore.kernel.org/all/CAC_iWjLOhUvp5ggCCkHN5MRNfB_h6FZ2Z14yrtR3aqGn0Ovxig@mail.gmail.com/

> Thanks
> /Ilias
> >
> > Jan
> >
> > --
> > Siemens AG, Technology
> > Competence Center Embedded Linux
> >

2023-06-07 06:52:06

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan,

[...]

> >>>>
> > ...
> >>>
> >>> I think we have a probe ordering issue with this driver:
> >>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> >>> with the default efivar ops still registered. And that means
> >>> efivar_supports_writes() will return false, and the fs declares itself
> >>> as readonly. I've seen systemd mounting it r/o initialling, and you need
> >>> to remount the fs to enable writability.
> >>>
> >>> Is there anything that could be done to re-order things reliably, probe
> >>> the tee bus earlier etc.?
> >>
> >> This driver has a dependency on user-space daemon: tee-supplicant to
> >> be running for RPMB access. So once you start that daemon the
> >> corresponding device will be enumerated on the TEE bus and this driver
> >> probe will be invoked. So I would suggest you to load this daemon very
> >> early in the boot process or better to make it a part of initramfs.
> >>
> >
> > That is not the point, really.
> >
> > If this dependency exists, the code should be aware of that, and made
> > to work correctly in spite of it. Requiring a module to be part of
> > initramfs is not a reasonable fix.
>
> In fact, I've tested a non-modularized build as well, just to exclude
> that issue. The daemon dependency is more likely the problem here.
>
> >
> > IIUC, this also means that the efivar ops are updated while there is
> > already a client. This seems less than ideal as well

As Sumit pointed out, the 'device' won't be available from OP-TEE
until the supplicant is up and running and as a result, the module
_probe() function won't run. Unfortunately, this isn't something we
can avoid since the supplicant is responsible for the RPMB writes.
The only thing I can think of is moving parts of the supplicant to the
kernel and wiring up the RPC calls for reading/writing data to the
eMMC subsystem. There was another discussion here [0] requesting the
same thing for different reasons. But unless I am missing something
this won't solve the problem completely either. You still have a
timing dependency of "when did the RT callbacks change" -- "when was
my efivarfs mounted".

Thanks
/Ilias
>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-07 07:52:23

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Ilias,

On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
<[email protected]> wrote:
>
> Hi Jan,
>
> [...]
>
> > >>>>
> > > ...
> > >>>
> > >>> I think we have a probe ordering issue with this driver:
> > >>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> > >>> with the default efivar ops still registered. And that means
> > >>> efivar_supports_writes() will return false, and the fs declares itself
> > >>> as readonly. I've seen systemd mounting it r/o initialling, and you need
> > >>> to remount the fs to enable writability.
> > >>>
> > >>> Is there anything that could be done to re-order things reliably, probe
> > >>> the tee bus earlier etc.?
> > >>
> > >> This driver has a dependency on user-space daemon: tee-supplicant to
> > >> be running for RPMB access. So once you start that daemon the
> > >> corresponding device will be enumerated on the TEE bus and this driver
> > >> probe will be invoked. So I would suggest you to load this daemon very
> > >> early in the boot process or better to make it a part of initramfs.
> > >>
> > >
> > > That is not the point, really.
> > >
> > > If this dependency exists, the code should be aware of that, and made
> > > to work correctly in spite of it. Requiring a module to be part of
> > > initramfs is not a reasonable fix.
> >
> > In fact, I've tested a non-modularized build as well, just to exclude
> > that issue. The daemon dependency is more likely the problem here.
> >
> > >
> > > IIUC, this also means that the efivar ops are updated while there is
> > > already a client. This seems less than ideal as well
>
> As Sumit pointed out, the 'device' won't be available from OP-TEE
> until the supplicant is up and running and as a result, the module
> _probe() function won't run. Unfortunately, this isn't something we
> can avoid since the supplicant is responsible for the RPMB writes.
> The only thing I can think of is moving parts of the supplicant to the
> kernel and wiring up the RPC calls for reading/writing data to the
> eMMC subsystem. There was another discussion here [0] requesting the
> same thing for different reasons. But unless I am missing something
> this won't solve the problem completely either. You still have a
> timing dependency of "when did the RT callbacks change" -- "when was
> my efivarfs mounted".

With the RPMB writes wired through the kernel [1], the only dependency
left is when do you load the tee-stmm-efi driver to have real EFI
runtime variables support. IMO, tee-stmm-efi driver should be built-in
to support systems without initramfs. The distro installers may choose
to bundle it in initramfs. Do you still see a timing dependency with
this approach?

[1] Of Course here we need the eMMC and TEE/OPTEE drivers to be built-in too.

-Sumit

>
> Thanks
> /Ilias
> >
> > Jan
> >
> > --
> > Siemens AG, Technology
> > Competence Center Embedded Linux
> >

2023-06-07 08:34:12

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Sumit,

On Wed, 7 Jun 2023 at 10:25, Sumit Garg <[email protected]> wrote:
>
> Hi Ilias,
>
> On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
> <[email protected]> wrote:
> >
> > Hi Jan,
> >
> > [...]
> >
> > > >>>>
> > > > ...
> > > >>>
> > > >>> I think we have a probe ordering issue with this driver:
> > > >>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> > > >>> with the default efivar ops still registered. And that means
> > > >>> efivar_supports_writes() will return false, and the fs declares itself
> > > >>> as readonly. I've seen systemd mounting it r/o initialling, and you need
> > > >>> to remount the fs to enable writability.
> > > >>>
> > > >>> Is there anything that could be done to re-order things reliably, probe
> > > >>> the tee bus earlier etc.?
> > > >>
> > > >> This driver has a dependency on user-space daemon: tee-supplicant to
> > > >> be running for RPMB access. So once you start that daemon the
> > > >> corresponding device will be enumerated on the TEE bus and this driver
> > > >> probe will be invoked. So I would suggest you to load this daemon very
> > > >> early in the boot process or better to make it a part of initramfs.
> > > >>
> > > >
> > > > That is not the point, really.
> > > >
> > > > If this dependency exists, the code should be aware of that, and made
> > > > to work correctly in spite of it. Requiring a module to be part of
> > > > initramfs is not a reasonable fix.
> > >
> > > In fact, I've tested a non-modularized build as well, just to exclude
> > > that issue. The daemon dependency is more likely the problem here.
> > >
> > > >
> > > > IIUC, this also means that the efivar ops are updated while there is
> > > > already a client. This seems less than ideal as well
> >
> > As Sumit pointed out, the 'device' won't be available from OP-TEE
> > until the supplicant is up and running and as a result, the module
> > _probe() function won't run. Unfortunately, this isn't something we
> > can avoid since the supplicant is responsible for the RPMB writes.
> > The only thing I can think of is moving parts of the supplicant to the
> > kernel and wiring up the RPC calls for reading/writing data to the
> > eMMC subsystem. There was another discussion here [0] requesting the
> > same thing for different reasons. But unless I am missing something
> > this won't solve the problem completely either. You still have a
> > timing dependency of "when did the RT callbacks change" -- "when was
> > my efivarfs mounted".
>
> With the RPMB writes wired through the kernel [1], the only dependency
> left is when do you load the tee-stmm-efi driver to have real EFI
> runtime variables support. IMO, tee-stmm-efi driver should be built-in
> to support systems without initramfs. The distro installers may choose
> to bundle it in initramfs. Do you still see a timing dependency with
> this approach?

No I don't, this will work reliably without the need to remount the efivarfs.
As you point out you will still have this dependency if you end up
building them as modules and you manage to mount the efivarfs before
those get inserted. Does anyone see a reasonable workaround?
Deceiving the kernel and making the bootloader set the RT property bit
to force the filesystem being mounted as rw is a nasty hack that we
should avoid. Maybe adding a kernel command line parameter that says
"Ignore the RTPROP I know what I am doing"? I don't particularly love
this either, but it's not unreasonable.

Thanks
/Ilias
>
> [1] Of Course here we need the eMMC and TEE/OPTEE drivers to be built-in too.
>
> -Sumit
>
> >
> > Thanks
> > /Ilias
> > >
> > > Jan
> > >
> > > --
> > > Siemens AG, Technology
> > > Competence Center Embedded Linux
> > >

2023-06-07 12:48:54

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 07.06.23 10:25, Ilias Apalodimas wrote:
> Hi Sumit,
>
> On Wed, 7 Jun 2023 at 10:25, Sumit Garg <[email protected]> wrote:
>>
>> Hi Ilias,
>>
>> On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
>> <[email protected]> wrote:
>>>
>>> Hi Jan,
>>>
>>> [...]
>>>
>>>>>>>>
>>>>> ...
>>>>>>>
>>>>>>> I think we have a probe ordering issue with this driver:
>>>>>>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
>>>>>>> with the default efivar ops still registered. And that means
>>>>>>> efivar_supports_writes() will return false, and the fs declares itself
>>>>>>> as readonly. I've seen systemd mounting it r/o initialling, and you need
>>>>>>> to remount the fs to enable writability.
>>>>>>>
>>>>>>> Is there anything that could be done to re-order things reliably, probe
>>>>>>> the tee bus earlier etc.?
>>>>>>
>>>>>> This driver has a dependency on user-space daemon: tee-supplicant to
>>>>>> be running for RPMB access. So once you start that daemon the
>>>>>> corresponding device will be enumerated on the TEE bus and this driver
>>>>>> probe will be invoked. So I would suggest you to load this daemon very
>>>>>> early in the boot process or better to make it a part of initramfs.
>>>>>>
>>>>>
>>>>> That is not the point, really.
>>>>>
>>>>> If this dependency exists, the code should be aware of that, and made
>>>>> to work correctly in spite of it. Requiring a module to be part of
>>>>> initramfs is not a reasonable fix.
>>>>
>>>> In fact, I've tested a non-modularized build as well, just to exclude
>>>> that issue. The daemon dependency is more likely the problem here.
>>>>
>>>>>
>>>>> IIUC, this also means that the efivar ops are updated while there is
>>>>> already a client. This seems less than ideal as well
>>>
>>> As Sumit pointed out, the 'device' won't be available from OP-TEE
>>> until the supplicant is up and running and as a result, the module
>>> _probe() function won't run. Unfortunately, this isn't something we
>>> can avoid since the supplicant is responsible for the RPMB writes.
>>> The only thing I can think of is moving parts of the supplicant to the
>>> kernel and wiring up the RPC calls for reading/writing data to the
>>> eMMC subsystem. There was another discussion here [0] requesting the
>>> same thing for different reasons. But unless I am missing something
>>> this won't solve the problem completely either. You still have a
>>> timing dependency of "when did the RT callbacks change" -- "when was
>>> my efivarfs mounted".
>>
>> With the RPMB writes wired through the kernel [1], the only dependency
>> left is when do you load the tee-stmm-efi driver to have real EFI
>> runtime variables support. IMO, tee-stmm-efi driver should be built-in
>> to support systems without initramfs. The distro installers may choose
>> to bundle it in initramfs. Do you still see a timing dependency with
>> this approach?
>
> No I don't, this will work reliably without the need to remount the efivarfs.
> As you point out you will still have this dependency if you end up
> building them as modules and you manage to mount the efivarfs before
> those get inserted. Does anyone see a reasonable workaround?
> Deceiving the kernel and making the bootloader set the RT property bit
> to force the filesystem being mounted as rw is a nasty hack that we
> should avoid. Maybe adding a kernel command line parameter that says
> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> this either, but it's not unreasonable.

In the context of https://github.com/OP-TEE/optee_os/issues/6094,
basically this issue mapped on reboot/shutdown, I would really love to
see the unhandy tee-supplicant daemon to be overcome.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-07 15:32:49

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Wed, 7 Jun 2023 at 18:10, Jan Kiszka <[email protected]> wrote:
>
> On 07.06.23 10:25, Ilias Apalodimas wrote:
> > Hi Sumit,
> >
> > On Wed, 7 Jun 2023 at 10:25, Sumit Garg <[email protected]> wrote:
> >>
> >> Hi Ilias,
> >>
> >> On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
> >> <[email protected]> wrote:
> >>>
> >>> Hi Jan,
> >>>
> >>> [...]
> >>>
> >>>>>>>>
> >>>>> ...
> >>>>>>>
> >>>>>>> I think we have a probe ordering issue with this driver:
> >>>>>>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
> >>>>>>> with the default efivar ops still registered. And that means
> >>>>>>> efivar_supports_writes() will return false, and the fs declares itself
> >>>>>>> as readonly. I've seen systemd mounting it r/o initialling, and you need
> >>>>>>> to remount the fs to enable writability.
> >>>>>>>
> >>>>>>> Is there anything that could be done to re-order things reliably, probe
> >>>>>>> the tee bus earlier etc.?
> >>>>>>
> >>>>>> This driver has a dependency on user-space daemon: tee-supplicant to
> >>>>>> be running for RPMB access. So once you start that daemon the
> >>>>>> corresponding device will be enumerated on the TEE bus and this driver
> >>>>>> probe will be invoked. So I would suggest you to load this daemon very
> >>>>>> early in the boot process or better to make it a part of initramfs.
> >>>>>>
> >>>>>
> >>>>> That is not the point, really.
> >>>>>
> >>>>> If this dependency exists, the code should be aware of that, and made
> >>>>> to work correctly in spite of it. Requiring a module to be part of
> >>>>> initramfs is not a reasonable fix.
> >>>>
> >>>> In fact, I've tested a non-modularized build as well, just to exclude
> >>>> that issue. The daemon dependency is more likely the problem here.
> >>>>
> >>>>>
> >>>>> IIUC, this also means that the efivar ops are updated while there is
> >>>>> already a client. This seems less than ideal as well
> >>>
> >>> As Sumit pointed out, the 'device' won't be available from OP-TEE
> >>> until the supplicant is up and running and as a result, the module
> >>> _probe() function won't run. Unfortunately, this isn't something we
> >>> can avoid since the supplicant is responsible for the RPMB writes.
> >>> The only thing I can think of is moving parts of the supplicant to the
> >>> kernel and wiring up the RPC calls for reading/writing data to the
> >>> eMMC subsystem. There was another discussion here [0] requesting the
> >>> same thing for different reasons. But unless I am missing something
> >>> this won't solve the problem completely either. You still have a
> >>> timing dependency of "when did the RT callbacks change" -- "when was
> >>> my efivarfs mounted".
> >>
> >> With the RPMB writes wired through the kernel [1], the only dependency
> >> left is when do you load the tee-stmm-efi driver to have real EFI
> >> runtime variables support. IMO, tee-stmm-efi driver should be built-in
> >> to support systems without initramfs. The distro installers may choose
> >> to bundle it in initramfs. Do you still see a timing dependency with
> >> this approach?
> >
> > No I don't, this will work reliably without the need to remount the efivarfs.
> > As you point out you will still have this dependency if you end up
> > building them as modules and you manage to mount the efivarfs before
> > those get inserted. Does anyone see a reasonable workaround?
> > Deceiving the kernel and making the bootloader set the RT property bit
> > to force the filesystem being mounted as rw is a nasty hack that we
> > should avoid. Maybe adding a kernel command line parameter that says
> > "Ignore the RTPROP I know what I am doing"? I don't particularly love
> > this either, but it's not unreasonable.
>
> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> basically this issue mapped on reboot/shutdown, I would really love to
> see the unhandy tee-supplicant daemon to be overcome.

I have seen this error before and it has been on my todo list. So I
have tried to fix it here [1]. Feel free to test it and let me know if
you see any further issues.

[1] https://lkml.org/lkml/2023/6/7/927

-Sumit

>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-07 16:20:53

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 07.06.23 17:18, Sumit Garg wrote:
> On Wed, 7 Jun 2023 at 18:10, Jan Kiszka <[email protected]> wrote:
>>
>> On 07.06.23 10:25, Ilias Apalodimas wrote:
>>> Hi Sumit,
>>>
>>> On Wed, 7 Jun 2023 at 10:25, Sumit Garg <[email protected]> wrote:
>>>>
>>>> Hi Ilias,
>>>>
>>>> On Wed, 7 Jun 2023 at 12:05, Ilias Apalodimas
>>>> <[email protected]> wrote:
>>>>>
>>>>> Hi Jan,
>>>>>
>>>>> [...]
>>>>>
>>>>>>>>>>
>>>>>>> ...
>>>>>>>>>
>>>>>>>>> I think we have a probe ordering issue with this driver:
>>>>>>>>> efivarfs_fill_super() may be called before the TEE bus was probed, thus
>>>>>>>>> with the default efivar ops still registered. And that means
>>>>>>>>> efivar_supports_writes() will return false, and the fs declares itself
>>>>>>>>> as readonly. I've seen systemd mounting it r/o initialling, and you need
>>>>>>>>> to remount the fs to enable writability.
>>>>>>>>>
>>>>>>>>> Is there anything that could be done to re-order things reliably, probe
>>>>>>>>> the tee bus earlier etc.?
>>>>>>>>
>>>>>>>> This driver has a dependency on user-space daemon: tee-supplicant to
>>>>>>>> be running for RPMB access. So once you start that daemon the
>>>>>>>> corresponding device will be enumerated on the TEE bus and this driver
>>>>>>>> probe will be invoked. So I would suggest you to load this daemon very
>>>>>>>> early in the boot process or better to make it a part of initramfs.
>>>>>>>>
>>>>>>>
>>>>>>> That is not the point, really.
>>>>>>>
>>>>>>> If this dependency exists, the code should be aware of that, and made
>>>>>>> to work correctly in spite of it. Requiring a module to be part of
>>>>>>> initramfs is not a reasonable fix.
>>>>>>
>>>>>> In fact, I've tested a non-modularized build as well, just to exclude
>>>>>> that issue. The daemon dependency is more likely the problem here.
>>>>>>
>>>>>>>
>>>>>>> IIUC, this also means that the efivar ops are updated while there is
>>>>>>> already a client. This seems less than ideal as well
>>>>>
>>>>> As Sumit pointed out, the 'device' won't be available from OP-TEE
>>>>> until the supplicant is up and running and as a result, the module
>>>>> _probe() function won't run. Unfortunately, this isn't something we
>>>>> can avoid since the supplicant is responsible for the RPMB writes.
>>>>> The only thing I can think of is moving parts of the supplicant to the
>>>>> kernel and wiring up the RPC calls for reading/writing data to the
>>>>> eMMC subsystem. There was another discussion here [0] requesting the
>>>>> same thing for different reasons. But unless I am missing something
>>>>> this won't solve the problem completely either. You still have a
>>>>> timing dependency of "when did the RT callbacks change" -- "when was
>>>>> my efivarfs mounted".
>>>>
>>>> With the RPMB writes wired through the kernel [1], the only dependency
>>>> left is when do you load the tee-stmm-efi driver to have real EFI
>>>> runtime variables support. IMO, tee-stmm-efi driver should be built-in
>>>> to support systems without initramfs. The distro installers may choose
>>>> to bundle it in initramfs. Do you still see a timing dependency with
>>>> this approach?
>>>
>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>> As you point out you will still have this dependency if you end up
>>> building them as modules and you manage to mount the efivarfs before
>>> those get inserted. Does anyone see a reasonable workaround?
>>> Deceiving the kernel and making the bootloader set the RT property bit
>>> to force the filesystem being mounted as rw is a nasty hack that we
>>> should avoid. Maybe adding a kernel command line parameter that says
>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>> this either, but it's not unreasonable.
>>
>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>> basically this issue mapped on reboot/shutdown, I would really love to
>> see the unhandy tee-supplicant daemon to be overcome.
>
> I have seen this error before and it has been on my todo list. So I
> have tried to fix it here [1]. Feel free to test it and let me know if
> you see any further issues.
>
> [1] https://lkml.org/lkml/2023/6/7/927
>

Ah, nice, will test ASAP!

Meanwhile more food: I managed to build a firmware that was missing
STMM. But the driver loaded, and I got this:

root@iot2050-debian:~# efi-updatevar -f PK.auth PK
[ 243.407097] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
[ 243.415959] Mem abort info:
[ 243.418801] ESR = 0x86000004
[ 243.422099] EC = 0x21: IABT (current EL), IL = 32 bits
[ 243.427529] SET = 0, FnV = 0
[ 243.430755] EA = 0, S1PTW = 0
[ 243.433931] user pgtable: 4k pages, 48-bit VAs, pgdp=000000008b74e000
[ 243.440438] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
[ 243.447274] Internal error: Oops: 86000004 [#1] PREEMPT SMP
[ 243.452835] Modules linked in: ctr ccm mt7601u mac80211 cfg80211 rfkill libarc4 cp210x usbserial pci_endpoint_test ti_k3_r5_remoteproc optee_rng rng_core ti_cal ti_am335x_adc videobuf2_dma_contig kfifo_buf v4l2_fwnode videobuf2_memops videobuf2_v4l2 videobuf2_common irq_pruss_intc at24 fuse ip_tables x_tables ipv6 tpm_ftpm_tee icssg_prueth pru_rproc icss_iep ptp pps_core ti_am335x_tscadc pruss
[ 243.487733] CPU: 0 PID: 875 Comm: efi-updatevar Not tainted 5.10.162-cip24 #1
[ 243.494851] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.07-rc3-00018-g0afdaac6505 07/01/2023
[ 243.505180] pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
[ 243.511179] pc : 0x0
[ 243.513366] lr : efivar_entry_set_get_size+0xd4/0x1e0
[ 243.518404] sp : ffff8000127a3d00
[ 243.521708] x29: ffff8000127a3d00 x28: 0000000000000f87
[ 243.527012] x27: ffff000005bae400 x26: ffff800011254000
[ 243.532315] x25: ffff000005baf000 x24: ffff800011254aa0
[ 243.537618] x23: ffff8000127a3dab x22: ffff8000111d0268
[ 243.542921] x21: ffff8000127a3db0 x20: 0000000000000000
[ 243.548224] x19: ffff000005bae000 x18: 0000000000000000
[ 243.553527] x17: 0000000000000000 x16: 0000000000000000
[ 243.558829] x15: 0000aaab0876f264 x14: 35cd6a1025d11a20
[ 243.564132] x13: ac6f8dda3945638d x12: fb482642e3487f2d
[ 243.569435] x11: 0000000000000003 x10: ffff00000b792a80
[ 243.574738] x9 : a098e2bf989ff097 x8 : 0000000000000010
[ 243.580041] x7 : ffff800010e35c00 x6 : 4ddcbe2ecfc8fc79
[ 243.585345] x5 : 0000000000000000 x4 : ffff000005baf000
[ 243.590647] x3 : 0000000000000f87 x2 : 0000000000000027
[ 243.595950] x1 : ffff000005bae400 x0 : ffff000005bae000
[ 243.601254] Call trace:
[ 243.603695] 0x0
[ 243.605531] efivarfs_file_write+0xa4/0x170
[ 243.609709] vfs_write+0xf0/0x2a4
[ 243.613016] ksys_write+0x68/0xf4
[ 243.616323] __arm64_sys_write+0x1c/0x2c
[ 243.620241] el0_svc_common.constprop.0+0x78/0x1c4
[ 243.625022] do_el0_svc+0x24/0x8c
[ 243.628331] el0_svc+0x14/0x20
[ 243.631378] el0_sync_handler+0xb0/0xb4
[ 243.635206] el0_sync+0x180/0x1c0
[ 243.638523] Code: bad PC value
[ 243.641573] ---[ end trace 369e4632cb003adc ]---

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-07 16:31:18

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan,

[...]
> >>> No I don't, this will work reliably without the need to remount the efivarfs.
> >>> As you point out you will still have this dependency if you end up
> >>> building them as modules and you manage to mount the efivarfs before
> >>> those get inserted. Does anyone see a reasonable workaround?
> >>> Deceiving the kernel and making the bootloader set the RT property bit
> >>> to force the filesystem being mounted as rw is a nasty hack that we
> >>> should avoid. Maybe adding a kernel command line parameter that says
> >>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> >>> this either, but it's not unreasonable.
> >>
> >> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> >> basically this issue mapped on reboot/shutdown, I would really love to
> >> see the unhandy tee-supplicant daemon to be overcome.
> >
> > I have seen this error before and it has been on my todo list. So I
> > have tried to fix it here [1]. Feel free to test it and let me know if
> > you see any further issues.
> >
> > [1] https://lkml.org/lkml/2023/6/7/927
> >
>
> Ah, nice, will test ASAP!
>
> Meanwhile more food: I managed to build a firmware that was missing
> STMM. But the driver loaded, and I got this:

Thanks for the testing. I'll try to reproduce it locally and get back to you

/Ilias
>
> root@iot2050-debian:~# efi-updatevar -f PK.auth PK
> [ 243.407097] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> [ 243.415959] Mem abort info:
> [ 243.418801] ESR = 0x86000004
> [ 243.422099] EC = 0x21: IABT (current EL), IL = 32 bits
> [ 243.427529] SET = 0, FnV = 0
> [ 243.430755] EA = 0, S1PTW = 0
> [ 243.433931] user pgtable: 4k pages, 48-bit VAs, pgdp=000000008b74e000
> [ 243.440438] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> [ 243.447274] Internal error: Oops: 86000004 [#1] PREEMPT SMP
> [ 243.452835] Modules linked in: ctr ccm mt7601u mac80211 cfg80211 rfkill libarc4 cp210x usbserial pci_endpoint_test ti_k3_r5_remoteproc optee_rng rng_core ti_cal ti_am335x_adc videobuf2_dma_contig kfifo_buf v4l2_fwnode videobuf2_memops videobuf2_v4l2 videobuf2_common irq_pruss_intc at24 fuse ip_tables x_tables ipv6 tpm_ftpm_tee icssg_prueth pru_rproc icss_iep ptp pps_core ti_am335x_tscadc pruss
> [ 243.487733] CPU: 0 PID: 875 Comm: efi-updatevar Not tainted 5.10.162-cip24 #1
> [ 243.494851] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.07-rc3-00018-g0afdaac6505 07/01/2023
> [ 243.505180] pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
> [ 243.511179] pc : 0x0
> [ 243.513366] lr : efivar_entry_set_get_size+0xd4/0x1e0
> [ 243.518404] sp : ffff8000127a3d00
> [ 243.521708] x29: ffff8000127a3d00 x28: 0000000000000f87
> [ 243.527012] x27: ffff000005bae400 x26: ffff800011254000
> [ 243.532315] x25: ffff000005baf000 x24: ffff800011254aa0
> [ 243.537618] x23: ffff8000127a3dab x22: ffff8000111d0268
> [ 243.542921] x21: ffff8000127a3db0 x20: 0000000000000000
> [ 243.548224] x19: ffff000005bae000 x18: 0000000000000000
> [ 243.553527] x17: 0000000000000000 x16: 0000000000000000
> [ 243.558829] x15: 0000aaab0876f264 x14: 35cd6a1025d11a20
> [ 243.564132] x13: ac6f8dda3945638d x12: fb482642e3487f2d
> [ 243.569435] x11: 0000000000000003 x10: ffff00000b792a80
> [ 243.574738] x9 : a098e2bf989ff097 x8 : 0000000000000010
> [ 243.580041] x7 : ffff800010e35c00 x6 : 4ddcbe2ecfc8fc79
> [ 243.585345] x5 : 0000000000000000 x4 : ffff000005baf000
> [ 243.590647] x3 : 0000000000000f87 x2 : 0000000000000027
> [ 243.595950] x1 : ffff000005bae400 x0 : ffff000005bae000
> [ 243.601254] Call trace:
> [ 243.603695] 0x0
> [ 243.605531] efivarfs_file_write+0xa4/0x170
> [ 243.609709] vfs_write+0xf0/0x2a4
> [ 243.613016] ksys_write+0x68/0xf4
> [ 243.616323] __arm64_sys_write+0x1c/0x2c
> [ 243.620241] el0_svc_common.constprop.0+0x78/0x1c4
> [ 243.625022] do_el0_svc+0x24/0x8c
> [ 243.628331] el0_svc+0x14/0x20
> [ 243.631378] el0_sync_handler+0xb0/0xb4
> [ 243.635206] el0_sync+0x180/0x1c0
> [ 243.638523] Code: bad PC value
> [ 243.641573] ---[ end trace 369e4632cb003adc ]---
>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-07 17:20:36

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 07.06.23 18:59, Ilias Apalodimas wrote:
> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> <[email protected]> wrote:
>>
>> Hi Jan,
>>
>> [...]
>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>>>>> As you point out you will still have this dependency if you end up
>>>>>> building them as modules and you manage to mount the efivarfs before
>>>>>> those get inserted. Does anyone see a reasonable workaround?
>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
>>>>>> should avoid. Maybe adding a kernel command line parameter that says
>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>>>>> this either, but it's not unreasonable.
>>>>>
>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>>>>> basically this issue mapped on reboot/shutdown, I would really love to
>>>>> see the unhandy tee-supplicant daemon to be overcome.
>>>>
>>>> I have seen this error before and it has been on my todo list. So I
>>>> have tried to fix it here [1]. Feel free to test it and let me know if
>>>> you see any further issues.
>>>>
>>>> [1] https://lkml.org/lkml/2023/6/7/927
>>>>
>>>
>>> Ah, nice, will test ASAP!
>>>
>>> Meanwhile more food: I managed to build a firmware that was missing
>>> STMM. But the driver loaded, and I got this:
>>
>> Thanks for the testing. I'll try to reproduce it locally and get back to you
>
> Can you provide a bit more info on how that was triggered btw? I would
> be helpful to know
>
> - OP-TEE version

Today's master, 145953d55.

> - was it compiled as a module or built-in?

Sorry, not sure anymore, switching back and forth right now. I think it
was built-in.

> - was the supplicant running?

Yes.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-07 17:25:50

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
<[email protected]> wrote:
>
> Hi Jan,
>
> [...]
> > >>> No I don't, this will work reliably without the need to remount the efivarfs.
> > >>> As you point out you will still have this dependency if you end up
> > >>> building them as modules and you manage to mount the efivarfs before
> > >>> those get inserted. Does anyone see a reasonable workaround?
> > >>> Deceiving the kernel and making the bootloader set the RT property bit
> > >>> to force the filesystem being mounted as rw is a nasty hack that we
> > >>> should avoid. Maybe adding a kernel command line parameter that says
> > >>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> > >>> this either, but it's not unreasonable.
> > >>
> > >> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> > >> basically this issue mapped on reboot/shutdown, I would really love to
> > >> see the unhandy tee-supplicant daemon to be overcome.
> > >
> > > I have seen this error before and it has been on my todo list. So I
> > > have tried to fix it here [1]. Feel free to test it and let me know if
> > > you see any further issues.
> > >
> > > [1] https://lkml.org/lkml/2023/6/7/927
> > >
> >
> > Ah, nice, will test ASAP!
> >
> > Meanwhile more food: I managed to build a firmware that was missing
> > STMM. But the driver loaded, and I got this:
>
> Thanks for the testing. I'll try to reproduce it locally and get back to you

Can you provide a bit more info on how that was triggered btw? I would
be helpful to know

- OP-TEE version
- was it compiled as a module or built-in?
- was the supplicant running?

Thanks
/Ilias
>
> /Ilias
> >
> > root@iot2050-debian:~# efi-updatevar -f PK.auth PK
> > [ 243.407097] Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> > [ 243.415959] Mem abort info:
> > [ 243.418801] ESR = 0x86000004
> > [ 243.422099] EC = 0x21: IABT (current EL), IL = 32 bits
> > [ 243.427529] SET = 0, FnV = 0
> > [ 243.430755] EA = 0, S1PTW = 0
> > [ 243.433931] user pgtable: 4k pages, 48-bit VAs, pgdp=000000008b74e000
> > [ 243.440438] [0000000000000000] pgd=0000000000000000, p4d=0000000000000000
> > [ 243.447274] Internal error: Oops: 86000004 [#1] PREEMPT SMP
> > [ 243.452835] Modules linked in: ctr ccm mt7601u mac80211 cfg80211 rfkill libarc4 cp210x usbserial pci_endpoint_test ti_k3_r5_remoteproc optee_rng rng_core ti_cal ti_am335x_adc videobuf2_dma_contig kfifo_buf v4l2_fwnode videobuf2_memops videobuf2_v4l2 videobuf2_common irq_pruss_intc at24 fuse ip_tables x_tables ipv6 tpm_ftpm_tee icssg_prueth pru_rproc icss_iep ptp pps_core ti_am335x_tscadc pruss
> > [ 243.487733] CPU: 0 PID: 875 Comm: efi-updatevar Not tainted 5.10.162-cip24 #1
> > [ 243.494851] Hardware name: Unknown Unknown Product/Unknown Product, BIOS 2023.07-rc3-00018-g0afdaac6505 07/01/2023
> > [ 243.505180] pstate: 00000005 (nzcv daif -PAN -UAO -TCO BTYPE=--)
> > [ 243.511179] pc : 0x0
> > [ 243.513366] lr : efivar_entry_set_get_size+0xd4/0x1e0
> > [ 243.518404] sp : ffff8000127a3d00
> > [ 243.521708] x29: ffff8000127a3d00 x28: 0000000000000f87
> > [ 243.527012] x27: ffff000005bae400 x26: ffff800011254000
> > [ 243.532315] x25: ffff000005baf000 x24: ffff800011254aa0
> > [ 243.537618] x23: ffff8000127a3dab x22: ffff8000111d0268
> > [ 243.542921] x21: ffff8000127a3db0 x20: 0000000000000000
> > [ 243.548224] x19: ffff000005bae000 x18: 0000000000000000
> > [ 243.553527] x17: 0000000000000000 x16: 0000000000000000
> > [ 243.558829] x15: 0000aaab0876f264 x14: 35cd6a1025d11a20
> > [ 243.564132] x13: ac6f8dda3945638d x12: fb482642e3487f2d
> > [ 243.569435] x11: 0000000000000003 x10: ffff00000b792a80
> > [ 243.574738] x9 : a098e2bf989ff097 x8 : 0000000000000010
> > [ 243.580041] x7 : ffff800010e35c00 x6 : 4ddcbe2ecfc8fc79
> > [ 243.585345] x5 : 0000000000000000 x4 : ffff000005baf000
> > [ 243.590647] x3 : 0000000000000f87 x2 : 0000000000000027
> > [ 243.595950] x1 : ffff000005bae400 x0 : ffff000005bae000
> > [ 243.601254] Call trace:
> > [ 243.603695] 0x0
> > [ 243.605531] efivarfs_file_write+0xa4/0x170
> > [ 243.609709] vfs_write+0xf0/0x2a4
> > [ 243.613016] ksys_write+0x68/0xf4
> > [ 243.616323] __arm64_sys_write+0x1c/0x2c
> > [ 243.620241] el0_svc_common.constprop.0+0x78/0x1c4
> > [ 243.625022] do_el0_svc+0x24/0x8c
> > [ 243.628331] el0_svc+0x14/0x20
> > [ 243.631378] el0_sync_handler+0xb0/0xb4
> > [ 243.635206] el0_sync+0x180/0x1c0
> > [ 243.638523] Code: bad PC value
> > [ 243.641573] ---[ end trace 369e4632cb003adc ]---
> >
> > Jan
> >
> > --
> > Siemens AG, Technology
> > Competence Center Embedded Linux
> >

2023-06-07 18:22:29

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
>
> On 07.06.23 18:59, Ilias Apalodimas wrote:
> > On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> > <[email protected]> wrote:
> >>
> >> Hi Jan,
> >>
> >> [...]
> >>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
> >>>>>> As you point out you will still have this dependency if you end up
> >>>>>> building them as modules and you manage to mount the efivarfs before
> >>>>>> those get inserted. Does anyone see a reasonable workaround?
> >>>>>> Deceiving the kernel and making the bootloader set the RT property bit
> >>>>>> to force the filesystem being mounted as rw is a nasty hack that we
> >>>>>> should avoid. Maybe adding a kernel command line parameter that says
> >>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> >>>>>> this either, but it's not unreasonable.
> >>>>>
> >>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> >>>>> basically this issue mapped on reboot/shutdown, I would really love to
> >>>>> see the unhandy tee-supplicant daemon to be overcome.
> >>>>
> >>>> I have seen this error before and it has been on my todo list. So I
> >>>> have tried to fix it here [1]. Feel free to test it and let me know if
> >>>> you see any further issues.
> >>>>
> >>>> [1] https://lkml.org/lkml/2023/6/7/927
> >>>>
> >>>
> >>> Ah, nice, will test ASAP!
> >>>
> >>> Meanwhile more food: I managed to build a firmware that was missing
> >>> STMM. But the driver loaded, and I got this:
> >>
> >> Thanks for the testing. I'll try to reproduce it locally and get back to you
> >
> > Can you provide a bit more info on how that was triggered btw? I would
> > be helpful to know
> >
> > - OP-TEE version
>
> Today's master, 145953d55.
>
> > - was it compiled as a module or built-in?
>
> Sorry, not sure anymore, switching back and forth right now. I think it
> was built-in.
>
> > - was the supplicant running?
>
> Yes.
>

Ok thanks, that helps. I guess this also means U-Boot was compiled to
store the variables in a file in the ESP instead of the RPMB right?
Otherwise, I can't see how the device booted in the first place.

Thanks
/Ilias

> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-07 20:00:44

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 07.06.23 20:17, Ilias Apalodimas wrote:
> On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
>>
>> On 07.06.23 18:59, Ilias Apalodimas wrote:
>>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
>>> <[email protected]> wrote:
>>>>
>>>> Hi Jan,
>>>>
>>>> [...]
>>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>>>>>>> As you point out you will still have this dependency if you end up
>>>>>>>> building them as modules and you manage to mount the efivarfs before
>>>>>>>> those get inserted. Does anyone see a reasonable workaround?
>>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
>>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
>>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
>>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>>>>>>> this either, but it's not unreasonable.
>>>>>>>
>>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
>>>>>>> see the unhandy tee-supplicant daemon to be overcome.
>>>>>>
>>>>>> I have seen this error before and it has been on my todo list. So I
>>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
>>>>>> you see any further issues.
>>>>>>
>>>>>> [1] https://lkml.org/lkml/2023/6/7/927
>>>>>>
>>>>>
>>>>> Ah, nice, will test ASAP!
>>>>>
>>>>> Meanwhile more food: I managed to build a firmware that was missing
>>>>> STMM. But the driver loaded, and I got this:
>>>>
>>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
>>>
>>> Can you provide a bit more info on how that was triggered btw? I would
>>> be helpful to know
>>>
>>> - OP-TEE version
>>
>> Today's master, 145953d55.
>>
>>> - was it compiled as a module or built-in?
>>
>> Sorry, not sure anymore, switching back and forth right now. I think it
>> was built-in.
>>
>>> - was the supplicant running?
>>
>> Yes.
>>
>
> Ok thanks, that helps. I guess this also means U-Boot was compiled to
> store the variables in a file in the ESP instead of the RPMB right?
> Otherwise, I can't see how the device booted in the first place.

U-Boot was not configured to perform secure booting in this case. It had
RPMB support enabled, just didn't have to use it.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-08 06:37:18

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan

On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
>
> On 07.06.23 20:17, Ilias Apalodimas wrote:
> > On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
> >>
> >> On 07.06.23 18:59, Ilias Apalodimas wrote:
> >>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> >>> <[email protected]> wrote:
> >>>>
> >>>> Hi Jan,
> >>>>
> >>>> [...]
> >>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
> >>>>>>>> As you point out you will still have this dependency if you end up
> >>>>>>>> building them as modules and you manage to mount the efivarfs before
> >>>>>>>> those get inserted. Does anyone see a reasonable workaround?
> >>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
> >>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
> >>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
> >>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> >>>>>>>> this either, but it's not unreasonable.
> >>>>>>>
> >>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> >>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
> >>>>>>> see the unhandy tee-supplicant daemon to be overcome.
> >>>>>>
> >>>>>> I have seen this error before and it has been on my todo list. So I
> >>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
> >>>>>> you see any further issues.
> >>>>>>
> >>>>>> [1] https://lkml.org/lkml/2023/6/7/927
> >>>>>>
> >>>>>
> >>>>> Ah, nice, will test ASAP!
> >>>>>
> >>>>> Meanwhile more food: I managed to build a firmware that was missing
> >>>>> STMM. But the driver loaded, and I got this:
> >>>>
> >>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
> >>>
> >>> Can you provide a bit more info on how that was triggered btw? I would
> >>> be helpful to know
> >>>
> >>> - OP-TEE version
> >>
> >> Today's master, 145953d55.
> >>
> >>> - was it compiled as a module or built-in?
> >>
> >> Sorry, not sure anymore, switching back and forth right now. I think it
> >> was built-in.
> >>
> >>> - was the supplicant running?
> >>
> >> Yes.
> >>
> >
> > Ok thanks, that helps. I guess this also means U-Boot was compiled to
> > store the variables in a file in the ESP instead of the RPMB right?
> > Otherwise, I can't see how the device booted in the first place.
>
> U-Boot was not configured to perform secure booting in this case. It had
> RPMB support enabled, just didn't have to use it.

In your initial mail you said you managed to build a firmware without
StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
variable storage in an RPMB, the EFI subsystem will fail to start.

In any case, I don't think the ooops you are seeing is not connected
to this patchset. Looking at the kernel EFI stub we only set the
SetVariableRT if the RTPROP table is set accordingly by the firmware.
U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
can't support it. What you are doing is remount the efivarfs as rw
and then trying to set a variable, but the callback for it is NULL.
I think you'll be able to replicate the same behavior on the kernel
without even inserting the new module.

Thanks
/Ilias

>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-08 14:26:56

by Ard Biesheuvel

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
<[email protected]> wrote:
>
> Hi Jan
>
>
> On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
> >
> > On 07.06.23 20:17, Ilias Apalodimas wrote:
> > > On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
> > >>
> > >> On 07.06.23 18:59, Ilias Apalodimas wrote:
> > >>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> > >>> <[email protected]> wrote:
> > >>>>
> > >>>> Hi Jan,
> > >>>>
> > >>>> [...]
> > >>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
> > >>>>>>>> As you point out you will still have this dependency if you end up
> > >>>>>>>> building them as modules and you manage to mount the efivarfs before
> > >>>>>>>> those get inserted. Does anyone see a reasonable workaround?
> > >>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
> > >>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
> > >>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
> > >>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> > >>>>>>>> this either, but it's not unreasonable.
> > >>>>>>>
> > >>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> > >>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
> > >>>>>>> see the unhandy tee-supplicant daemon to be overcome.
> > >>>>>>
> > >>>>>> I have seen this error before and it has been on my todo list. So I
> > >>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
> > >>>>>> you see any further issues.
> > >>>>>>
> > >>>>>> [1] https://lkml.org/lkml/2023/6/7/927
> > >>>>>>
> > >>>>>
> > >>>>> Ah, nice, will test ASAP!
> > >>>>>
> > >>>>> Meanwhile more food: I managed to build a firmware that was missing
> > >>>>> STMM. But the driver loaded, and I got this:
> > >>>>
> > >>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
> > >>>
> > >>> Can you provide a bit more info on how that was triggered btw? I would
> > >>> be helpful to know
> > >>>
> > >>> - OP-TEE version
> > >>
> > >> Today's master, 145953d55.
> > >>
> > >>> - was it compiled as a module or built-in?
> > >>
> > >> Sorry, not sure anymore, switching back and forth right now. I think it
> > >> was built-in.
> > >>
> > >>> - was the supplicant running?
> > >>
> > >> Yes.
> > >>
> > >
> > > Ok thanks, that helps. I guess this also means U-Boot was compiled to
> > > store the variables in a file in the ESP instead of the RPMB right?
> > > Otherwise, I can't see how the device booted in the first place.
> >
> > U-Boot was not configured to perform secure booting in this case. It had
> > RPMB support enabled, just didn't have to use it.
>
> In your initial mail you said you managed to build a firmware without
> StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
> variable storage in an RPMB, the EFI subsystem will fail to start.
>
> In any case, I don't think the ooops you are seeing is not connected
> to this patchset. Looking at the kernel EFI stub we only set the
> SetVariableRT if the RTPROP table is set accordingly by the firmware.
> U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
> can't support it. What you are doing is remount the efivarfs as rw
> and then trying to set a variable, but the callback for it is NULL.
> I think you'll be able to replicate the same behavior on the kernel
> without even inserting the new module.
>

I have dropped this series from efi/next for now, given that it
obviously has problems in its current state.

The risk of merging this now and fixing it later is that it may cause
regressions for early adopters that rely on the behavior we are
introducing here. Better to get this in shape first.

2023-06-08 19:58:57

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On Thu, 8 Jun 2023 at 16:52, Ard Biesheuvel <[email protected]> wrote:
>
> On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
> <[email protected]> wrote:
> >
> > Hi Jan
> >
> >
> > On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
> > >
> > > On 07.06.23 20:17, Ilias Apalodimas wrote:
> > > > On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
> > > >>
> > > >> On 07.06.23 18:59, Ilias Apalodimas wrote:
> > > >>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> > > >>> <[email protected]> wrote:
> > > >>>>
> > > >>>> Hi Jan,
> > > >>>>
> > > >>>> [...]
> > > >>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
> > > >>>>>>>> As you point out you will still have this dependency if you end up
> > > >>>>>>>> building them as modules and you manage to mount the efivarfs before
> > > >>>>>>>> those get inserted. Does anyone see a reasonable workaround?
> > > >>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
> > > >>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
> > > >>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
> > > >>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> > > >>>>>>>> this either, but it's not unreasonable.
> > > >>>>>>>
> > > >>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> > > >>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
> > > >>>>>>> see the unhandy tee-supplicant daemon to be overcome.
> > > >>>>>>
> > > >>>>>> I have seen this error before and it has been on my todo list. So I
> > > >>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
> > > >>>>>> you see any further issues.
> > > >>>>>>
> > > >>>>>> [1] https://lkml.org/lkml/2023/6/7/927
> > > >>>>>>
> > > >>>>>
> > > >>>>> Ah, nice, will test ASAP!
> > > >>>>>
> > > >>>>> Meanwhile more food: I managed to build a firmware that was missing
> > > >>>>> STMM. But the driver loaded, and I got this:
> > > >>>>
> > > >>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
> > > >>>
> > > >>> Can you provide a bit more info on how that was triggered btw? I would
> > > >>> be helpful to know
> > > >>>
> > > >>> - OP-TEE version
> > > >>
> > > >> Today's master, 145953d55.
> > > >>
> > > >>> - was it compiled as a module or built-in?
> > > >>
> > > >> Sorry, not sure anymore, switching back and forth right now. I think it
> > > >> was built-in.
> > > >>
> > > >>> - was the supplicant running?
> > > >>
> > > >> Yes.
> > > >>
> > > >
> > > > Ok thanks, that helps. I guess this also means U-Boot was compiled to
> > > > store the variables in a file in the ESP instead of the RPMB right?
> > > > Otherwise, I can't see how the device booted in the first place.
> > >
> > > U-Boot was not configured to perform secure booting in this case. It had
> > > RPMB support enabled, just didn't have to use it.
> >
> > In your initial mail you said you managed to build a firmware without
> > StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
> > variable storage in an RPMB, the EFI subsystem will fail to start.
> >
> > In any case, I don't think the ooops you are seeing is not connected
> > to this patchset. Looking at the kernel EFI stub we only set the
> > SetVariableRT if the RTPROP table is set accordingly by the firmware.
> > U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
> > can't support it. What you are doing is remount the efivarfs as rw
> > and then trying to set a variable, but the callback for it is NULL.
> > I think you'll be able to replicate the same behavior on the kernel
> > without even inserting the new module.
> >
>
> I have dropped this series from efi/next for now, given that it
> obviously has problems in its current state.
>
> The risk of merging this now and fixing it later is that it may cause
> regressions for early adopters that rely on the behavior we are
> introducing here. Better to get this in shape first.

The ooops Jan was seeing is reproducible in the current code with
$ sudo mount -o remount,rw /sys/firmware/efi/efivars
$ sudo efi-updatevar -f PK.auth PK

So the only real problem we are discussing here is users having to
remount the efivarfs once the module is inserted and the supplicant is
running right? We could do something along the lines of the patch
below. That would solve both of the problems.

However, the patch changes the way efivarfs is mounted -- it's now
always mounted as RW. What I am worried about is userspace tools that
rely on this. I know fwupd uses it and looks for a RO mounted
efivarfs on it's initial checking, but since userspace apps are
already dealing with firmware that exposes SetVariableRT but always
returns EFI_UNSUPPORTED this should be safe ?(famous last words)

Jan can you please apply this and see if it solves your problem?

diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4cc8d0e7d0fd..37accd9e885c 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -206,6 +206,13 @@ static bool generic_ops_supported(void)
return true;
}

+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
+ u32 attributes, unsigned long data_size,
+ void *data)
+{
+ return EFI_UNSUPPORTED;
+}
+
static int generic_ops_register(void)
{
if (!generic_ops_supported())
@@ -219,6 +226,9 @@ static int generic_ops_register(void)
if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
generic_ops.set_variable = efi.set_variable;
generic_ops.set_variable_nonblocking =
efi.set_variable_nonblocking;
+ } else {
+ generic_ops.set_variable = set_variable_int;
+ generic_ops.set_variable_nonblocking = set_variable_int;
}
return efivars_register(&generic_efivars, &generic_ops);
}
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index e9dc7116daf1..c75eff3f409d 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);

bool efivar_supports_writes(void)
{
- return __efivars && __efivars->ops->set_variable;
+ return __efivars && __efivars->ops->set_variable != set_variable_int;
}
EXPORT_SYMBOL_GPL(efivar_supports_writes);

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index e028fafa04f3..e40b5c4c5106 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
*sb, struct fs_context *fc)
sb->s_d_op = &efivarfs_d_ops;
sb->s_time_gran = 1;

- if (!efivar_supports_writes())
- sb->s_flags |= SB_RDONLY;
-
inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
if (!inode)
return -ENOMEM;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 58d1c271d3b0..ec0ac6ef50a3 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
const struct efivar_operations *ops);
int efivars_unregister(struct efivars *efivars);

+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
+ u32 attributes, unsigned long data_size,
+ void *data);
+
void efivars_generic_ops_register(void);
void efivars_generic_ops_unregister(void);

apalos@hades:~/work/linux-next>;
apalos@hades:~/work/linux-next>git diff
diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
index 4cc8d0e7d0fd..37accd9e885c 100644
--- a/drivers/firmware/efi/efi.c
+++ b/drivers/firmware/efi/efi.c
@@ -206,6 +206,13 @@ static bool generic_ops_supported(void)
return true;
}

+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
+ u32 attributes, unsigned long data_size,
+ void *data)
+{
+ return EFI_UNSUPPORTED;
+}
+
static int generic_ops_register(void)
{
if (!generic_ops_supported())
@@ -219,6 +226,9 @@ static int generic_ops_register(void)
if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
generic_ops.set_variable = efi.set_variable;
generic_ops.set_variable_nonblocking =
efi.set_variable_nonblocking;
+ } else {
+ generic_ops.set_variable = set_variable_int;
+ generic_ops.set_variable_nonblocking = set_variable_int;
}
return efivars_register(&generic_efivars, &generic_ops);
}
diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
index e9dc7116daf1..c75eff3f409d 100644
--- a/drivers/firmware/efi/vars.c
+++ b/drivers/firmware/efi/vars.c
@@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);

bool efivar_supports_writes(void)
{
- return __efivars && __efivars->ops->set_variable;
+ return __efivars && __efivars->ops->set_variable != set_variable_int;
}
EXPORT_SYMBOL_GPL(efivar_supports_writes);

diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
index e028fafa04f3..e40b5c4c5106 100644
--- a/fs/efivarfs/super.c
+++ b/fs/efivarfs/super.c
@@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
*sb, struct fs_context *fc)
sb->s_d_op = &efivarfs_d_ops;
sb->s_time_gran = 1;

- if (!efivar_supports_writes())
- sb->s_flags |= SB_RDONLY;
-
inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
if (!inode)
return -ENOMEM;
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 58d1c271d3b0..ec0ac6ef50a3 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
const struct efivar_operations *ops);
int efivars_unregister(struct efivars *efivars);

+efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
+ u32 attributes, unsigned long data_size,
+ void *data);
+
void efivars_generic_ops_register(void);
void efivars_generic_ops_unregister(void);

Thanks
/Ilias

2023-06-09 06:30:23

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 08.06.23 15:52, Ard Biesheuvel wrote:
> On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
> <[email protected]> wrote:
>>
>> Hi Jan
>>
>>
>> On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
>>>
>>> On 07.06.23 20:17, Ilias Apalodimas wrote:
>>>> On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
>>>>>
>>>>> On 07.06.23 18:59, Ilias Apalodimas wrote:
>>>>>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
>>>>>> <[email protected]> wrote:
>>>>>>>
>>>>>>> Hi Jan,
>>>>>>>
>>>>>>> [...]
>>>>>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>>>>>>>>>> As you point out you will still have this dependency if you end up
>>>>>>>>>>> building them as modules and you manage to mount the efivarfs before
>>>>>>>>>>> those get inserted. Does anyone see a reasonable workaround?
>>>>>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
>>>>>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
>>>>>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
>>>>>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>>>>>>>>>> this either, but it's not unreasonable.
>>>>>>>>>>
>>>>>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>>>>>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
>>>>>>>>>> see the unhandy tee-supplicant daemon to be overcome.
>>>>>>>>>
>>>>>>>>> I have seen this error before and it has been on my todo list. So I
>>>>>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
>>>>>>>>> you see any further issues.
>>>>>>>>>
>>>>>>>>> [1] https://lkml.org/lkml/2023/6/7/927
>>>>>>>>>
>>>>>>>>
>>>>>>>> Ah, nice, will test ASAP!
>>>>>>>>
>>>>>>>> Meanwhile more food: I managed to build a firmware that was missing
>>>>>>>> STMM. But the driver loaded, and I got this:
>>>>>>>
>>>>>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
>>>>>>
>>>>>> Can you provide a bit more info on how that was triggered btw? I would
>>>>>> be helpful to know
>>>>>>
>>>>>> - OP-TEE version
>>>>>
>>>>> Today's master, 145953d55.
>>>>>
>>>>>> - was it compiled as a module or built-in?
>>>>>
>>>>> Sorry, not sure anymore, switching back and forth right now. I think it
>>>>> was built-in.
>>>>>
>>>>>> - was the supplicant running?
>>>>>
>>>>> Yes.
>>>>>
>>>>
>>>> Ok thanks, that helps. I guess this also means U-Boot was compiled to
>>>> store the variables in a file in the ESP instead of the RPMB right?
>>>> Otherwise, I can't see how the device booted in the first place.
>>>
>>> U-Boot was not configured to perform secure booting in this case. It had
>>> RPMB support enabled, just didn't have to use it.
>>
>> In your initial mail you said you managed to build a firmware without
>> StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
>> variable storage in an RPMB, the EFI subsystem will fail to start.
>>
>> In any case, I don't think the ooops you are seeing is not connected
>> to this patchset. Looking at the kernel EFI stub we only set the
>> SetVariableRT if the RTPROP table is set accordingly by the firmware.
>> U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
>> can't support it. What you are doing is remount the efivarfs as rw
>> and then trying to set a variable, but the callback for it is NULL.
>> I think you'll be able to replicate the same behavior on the kernel
>> without even inserting the new module.

Might be true. I'll try to look into that again when the other dust settled.

>
> I have dropped this series from efi/next for now, given that it
> obviously has problems in its current state.
>
> The risk of merging this now and fixing it later is that it may cause
> regressions for early adopters that rely on the behavior we are
> introducing here. Better to get this in shape first.

On the one side, I'm sorry having ruined the merge, but my gut feeling
is as well that this really needs to be reworked to get rid of the
unfortunate tee-supplicant daemon. So far, we have to start the daemon
from initrd, write a systemd service to adopt that instance, and make
ftpm modular to remove it before terminating tee-supplicant (e.g. on
system shutdown) - Sumit's patch didn't help there.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-09 06:38:51

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 08.06.23 21:38, Ilias Apalodimas wrote:
> On Thu, 8 Jun 2023 at 16:52, Ard Biesheuvel <[email protected]> wrote:
>>
>> On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
>> <[email protected]> wrote:
>>>
>>> Hi Jan
>>>
>>>
>>> On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
>>>>
>>>> On 07.06.23 20:17, Ilias Apalodimas wrote:
>>>>> On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
>>>>>>
>>>>>> On 07.06.23 18:59, Ilias Apalodimas wrote:
>>>>>>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
>>>>>>> <[email protected]> wrote:
>>>>>>>>
>>>>>>>> Hi Jan,
>>>>>>>>
>>>>>>>> [...]
>>>>>>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>>>>>>>>>>> As you point out you will still have this dependency if you end up
>>>>>>>>>>>> building them as modules and you manage to mount the efivarfs before
>>>>>>>>>>>> those get inserted. Does anyone see a reasonable workaround?
>>>>>>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
>>>>>>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
>>>>>>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
>>>>>>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>>>>>>>>>>> this either, but it's not unreasonable.
>>>>>>>>>>>
>>>>>>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>>>>>>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
>>>>>>>>>>> see the unhandy tee-supplicant daemon to be overcome.
>>>>>>>>>>
>>>>>>>>>> I have seen this error before and it has been on my todo list. So I
>>>>>>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
>>>>>>>>>> you see any further issues.
>>>>>>>>>>
>>>>>>>>>> [1] https://lkml.org/lkml/2023/6/7/927
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> Ah, nice, will test ASAP!
>>>>>>>>>
>>>>>>>>> Meanwhile more food: I managed to build a firmware that was missing
>>>>>>>>> STMM. But the driver loaded, and I got this:
>>>>>>>>
>>>>>>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
>>>>>>>
>>>>>>> Can you provide a bit more info on how that was triggered btw? I would
>>>>>>> be helpful to know
>>>>>>>
>>>>>>> - OP-TEE version
>>>>>>
>>>>>> Today's master, 145953d55.
>>>>>>
>>>>>>> - was it compiled as a module or built-in?
>>>>>>
>>>>>> Sorry, not sure anymore, switching back and forth right now. I think it
>>>>>> was built-in.
>>>>>>
>>>>>>> - was the supplicant running?
>>>>>>
>>>>>> Yes.
>>>>>>
>>>>>
>>>>> Ok thanks, that helps. I guess this also means U-Boot was compiled to
>>>>> store the variables in a file in the ESP instead of the RPMB right?
>>>>> Otherwise, I can't see how the device booted in the first place.
>>>>
>>>> U-Boot was not configured to perform secure booting in this case. It had
>>>> RPMB support enabled, just didn't have to use it.
>>>
>>> In your initial mail you said you managed to build a firmware without
>>> StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
>>> variable storage in an RPMB, the EFI subsystem will fail to start.
>>>
>>> In any case, I don't think the ooops you are seeing is not connected
>>> to this patchset. Looking at the kernel EFI stub we only set the
>>> SetVariableRT if the RTPROP table is set accordingly by the firmware.
>>> U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
>>> can't support it. What you are doing is remount the efivarfs as rw
>>> and then trying to set a variable, but the callback for it is NULL.
>>> I think you'll be able to replicate the same behavior on the kernel
>>> without even inserting the new module.
>>>
>>
>> I have dropped this series from efi/next for now, given that it
>> obviously has problems in its current state.
>>
>> The risk of merging this now and fixing it later is that it may cause
>> regressions for early adopters that rely on the behavior we are
>> introducing here. Better to get this in shape first.
>
> The ooops Jan was seeing is reproducible in the current code with
> $ sudo mount -o remount,rw /sys/firmware/efi/efivars
> $ sudo efi-updatevar -f PK.auth PK
>
> So the only real problem we are discussing here is users having to
> remount the efivarfs once the module is inserted and the supplicant is
> running right? We could do something along the lines of the patch
> below. That would solve both of the problems.
>
> However, the patch changes the way efivarfs is mounted -- it's now
> always mounted as RW. What I am worried about is userspace tools that
> rely on this. I know fwupd uses it and looks for a RO mounted
> efivarfs on it's initial checking, but since userspace apps are
> already dealing with firmware that exposes SetVariableRT but always
> returns EFI_UNSUPPORTED this should be safe ?(famous last words)
>
> Jan can you please apply this and see if it solves your problem?
>
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4cc8d0e7d0fd..37accd9e885c 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -206,6 +206,13 @@ static bool generic_ops_supported(void)
> return true;
> }
>
> +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
> + u32 attributes, unsigned long data_size,
> + void *data)
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> static int generic_ops_register(void)
> {
> if (!generic_ops_supported())
> @@ -219,6 +226,9 @@ static int generic_ops_register(void)
> if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> generic_ops.set_variable = efi.set_variable;
> generic_ops.set_variable_nonblocking =
> efi.set_variable_nonblocking;
> + } else {
> + generic_ops.set_variable = set_variable_int;
> + generic_ops.set_variable_nonblocking = set_variable_int;
> }
> return efivars_register(&generic_efivars, &generic_ops);
> }
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index e9dc7116daf1..c75eff3f409d 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
>
> bool efivar_supports_writes(void)
> {
> - return __efivars && __efivars->ops->set_variable;
> + return __efivars && __efivars->ops->set_variable != set_variable_int;
> }
> EXPORT_SYMBOL_GPL(efivar_supports_writes);
>
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index e028fafa04f3..e40b5c4c5106 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
> *sb, struct fs_context *fc)
> sb->s_d_op = &efivarfs_d_ops;
> sb->s_time_gran = 1;
>
> - if (!efivar_supports_writes())
> - sb->s_flags |= SB_RDONLY;
> -
> inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
> if (!inode)
> return -ENOMEM;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 58d1c271d3b0..ec0ac6ef50a3 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
> const struct efivar_operations *ops);
> int efivars_unregister(struct efivars *efivars);
>
> +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
> + u32 attributes, unsigned long data_size,
> + void *data);
> +
> void efivars_generic_ops_register(void);
> void efivars_generic_ops_unregister(void);
>
> apalos@hades:~/work/linux-next>;
> apalos@hades:~/work/linux-next>git diff
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4cc8d0e7d0fd..37accd9e885c 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -206,6 +206,13 @@ static bool generic_ops_supported(void)
> return true;
> }
>
> +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
> + u32 attributes, unsigned long data_size,
> + void *data)
> +{
> + return EFI_UNSUPPORTED;
> +}
> +
> static int generic_ops_register(void)
> {
> if (!generic_ops_supported())
> @@ -219,6 +226,9 @@ static int generic_ops_register(void)
> if (efi_rt_services_supported(EFI_RT_SUPPORTED_SET_VARIABLE)) {
> generic_ops.set_variable = efi.set_variable;
> generic_ops.set_variable_nonblocking =
> efi.set_variable_nonblocking;
> + } else {
> + generic_ops.set_variable = set_variable_int;
> + generic_ops.set_variable_nonblocking = set_variable_int;
> }
> return efivars_register(&generic_efivars, &generic_ops);
> }
> diff --git a/drivers/firmware/efi/vars.c b/drivers/firmware/efi/vars.c
> index e9dc7116daf1..c75eff3f409d 100644
> --- a/drivers/firmware/efi/vars.c
> +++ b/drivers/firmware/efi/vars.c
> @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
>
> bool efivar_supports_writes(void)
> {
> - return __efivars && __efivars->ops->set_variable;
> + return __efivars && __efivars->ops->set_variable != set_variable_int;
> }
> EXPORT_SYMBOL_GPL(efivar_supports_writes);
>
> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> index e028fafa04f3..e40b5c4c5106 100644
> --- a/fs/efivarfs/super.c
> +++ b/fs/efivarfs/super.c
> @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
> *sb, struct fs_context *fc)
> sb->s_d_op = &efivarfs_d_ops;
> sb->s_time_gran = 1;
>
> - if (!efivar_supports_writes())
> - sb->s_flags |= SB_RDONLY;
> -
> inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
> if (!inode)
> return -ENOMEM;
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 58d1c271d3b0..ec0ac6ef50a3 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
> const struct efivar_operations *ops);
> int efivars_unregister(struct efivars *efivars);
>
> +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
> + u32 attributes, unsigned long data_size,
> + void *data);
> +
> void efivars_generic_ops_register(void);
> void efivars_generic_ops_unregister(void);
>
> Thanks
> /Ilias

As just written in my other reply: The root cause is the dependency on
tee-supplicant daemon. That needs to be resolved, and then also r/w
mounting will just work.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-09 06:42:55

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 09.06.23 08:34, Ilias Apalodimas wrote:
> Hi Jan,
>
> [...]
>
>>> --- a/drivers/firmware/efi/vars.c
>>> +++ b/drivers/firmware/efi/vars.c
>>> @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
>>>
>>> bool efivar_supports_writes(void)
>>> {
>>> - return __efivars && __efivars->ops->set_variable;
>>> + return __efivars && __efivars->ops->set_variable != set_variable_int;
>>> }
>>> EXPORT_SYMBOL_GPL(efivar_supports_writes);
>>>
>>> diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
>>> index e028fafa04f3..e40b5c4c5106 100644
>>> --- a/fs/efivarfs/super.c
>>> +++ b/fs/efivarfs/super.c
>>> @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
>>> *sb, struct fs_context *fc)
>>> sb->s_d_op = &efivarfs_d_ops;
>>> sb->s_time_gran = 1;
>>>
>>> - if (!efivar_supports_writes())
>>> - sb->s_flags |= SB_RDONLY;
>>> -
>>> inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
>>> if (!inode)
>>> return -ENOMEM;
>>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>>> index 58d1c271d3b0..ec0ac6ef50a3 100644
>>> --- a/include/linux/efi.h
>>> +++ b/include/linux/efi.h
>>> @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
>>> const struct efivar_operations *ops);
>>> int efivars_unregister(struct efivars *efivars);
>>>
>>> +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
>>> + u32 attributes, unsigned long data_size,
>>> + void *data);
>>> +
>>> void efivars_generic_ops_register(void);
>>> void efivars_generic_ops_unregister(void);
>>>
>>> Thanks
>>> /Ilias
>>
>> As just written in my other reply: The root cause is the dependency on
>> tee-supplicant daemon. That needs to be resolved, and then also r/w
>> mounting will just work.
>
> That's partially true. If we solve the dependency your problem will
> go away only if everything gets compiled as built in. But if you have
> them as modules there's still a chance you mount the efivarfs before
> installing all the modules. In that case, you'll end up with the same
> problem no?

Obviously, this will need proper probing of the TA services in the
proper order so that the STMM driver is pulled in before efivarfs gets used.

>
> That's why I think this patch (or a variation of it) is useful. It
> solves the kernel panic you are seeing if you remount the efivarfs as
> RW and It unifies the way the kernel responds to userspace no matter
> what the firmware does with its setvariableRT service.

I'm not against fixes crashes, but the r/w issue is a different thing IMHO.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-09 06:53:43

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

On 09.06.23 08:22, Ilias Apalodimas wrote:
> Hi Jan,
>
> On Fri, 9 Jun 2023 at 09:16, Jan Kiszka <[email protected]> wrote:
>>
>> On 08.06.23 15:52, Ard Biesheuvel wrote:
>>> On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
>>> <[email protected]> wrote:
>>>>
>>>> Hi Jan
>>>>
>>>>
>>>> On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
>>>>>
>>>>> On 07.06.23 20:17, Ilias Apalodimas wrote:
>>>>>> On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
>>>>>>>
>>>>>>> On 07.06.23 18:59, Ilias Apalodimas wrote:
>>>>>>>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
>>>>>>>> <[email protected]> wrote:
>>>>>>>>>
>>>>>>>>> Hi Jan,
>>>>>>>>>
>>>>>>>>> [...]
>>>>>>>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
>>>>>>>>>>>>> As you point out you will still have this dependency if you end up
>>>>>>>>>>>>> building them as modules and you manage to mount the efivarfs before
>>>>>>>>>>>>> those get inserted. Does anyone see a reasonable workaround?
>>>>>>>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
>>>>>>>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
>>>>>>>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
>>>>>>>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
>>>>>>>>>>>>> this either, but it's not unreasonable.
>>>>>>>>>>>>
>>>>>>>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
>>>>>>>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
>>>>>>>>>>>> see the unhandy tee-supplicant daemon to be overcome.
>>>>>>>>>>>
>>>>>>>>>>> I have seen this error before and it has been on my todo list. So I
>>>>>>>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
>>>>>>>>>>> you see any further issues.
>>>>>>>>>>>
>>>>>>>>>>> [1] https://lkml.org/lkml/2023/6/7/927
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> Ah, nice, will test ASAP!
>>>>>>>>>>
>>>>>>>>>> Meanwhile more food: I managed to build a firmware that was missing
>>>>>>>>>> STMM. But the driver loaded, and I got this:
>>>>>>>>>
>>>>>>>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
>>>>>>>>
>>>>>>>> Can you provide a bit more info on how that was triggered btw? I would
>>>>>>>> be helpful to know
>>>>>>>>
>>>>>>>> - OP-TEE version
>>>>>>>
>>>>>>> Today's master, 145953d55.
>>>>>>>
>>>>>>>> - was it compiled as a module or built-in?
>>>>>>>
>>>>>>> Sorry, not sure anymore, switching back and forth right now. I think it
>>>>>>> was built-in.
>>>>>>>
>>>>>>>> - was the supplicant running?
>>>>>>>
>>>>>>> Yes.
>>>>>>>
>>>>>>
>>>>>> Ok thanks, that helps. I guess this also means U-Boot was compiled to
>>>>>> store the variables in a file in the ESP instead of the RPMB right?
>>>>>> Otherwise, I can't see how the device booted in the first place.
>>>>>
>>>>> U-Boot was not configured to perform secure booting in this case. It had
>>>>> RPMB support enabled, just didn't have to use it.
>>>>
>>>> In your initial mail you said you managed to build a firmware without
>>>> StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
>>>> variable storage in an RPMB, the EFI subsystem will fail to start.
>>>>
>>>> In any case, I don't think the ooops you are seeing is not connected
>>>> to this patchset. Looking at the kernel EFI stub we only set the
>>>> SetVariableRT if the RTPROP table is set accordingly by the firmware.
>>>> U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
>>>> can't support it. What you are doing is remount the efivarfs as rw
>>>> and then trying to set a variable, but the callback for it is NULL.
>>>> I think you'll be able to replicate the same behavior on the kernel
>>>> without even inserting the new module.
>>
>> Might be true. I'll try to look into that again when the other dust settled.
>>
>>>
>>> I have dropped this series from efi/next for now, given that it
>>> obviously has problems in its current state.
>>>
>>> The risk of merging this now and fixing it later is that it may cause
>>> regressions for early adopters that rely on the behavior we are
>>> introducing here. Better to get this in shape first.
>>
>> On the one side, I'm sorry having ruined the merge, but my gut feeling
>> is as well that this really needs to be reworked to get rid of the
>> unfortunate tee-supplicant daemon. So far, we have to start the daemon
>> from initrd, write a systemd service to adopt that instance, and make
>> ftpm modular to remove it before terminating tee-supplicant (e.g. on
>> system shutdown) - Sumit's patch didn't help there.
>>
>
> No worries, the whole functionality is intrusive, so I prefer going
> through some iterations until everyone is happy. OTOH the
> 'supplicant' problem isn't going away soon. We will try to move it to
> the kernel but that has some difficulties as well and it's going to
> take some time. In any case, we've lived with the supplicant for
> quite some time and the ftpm module has a similar set of problems.
> IOW there are kernel modules that depend on it.
> From a functionality point of view nothing will change if the
> supplicant gets moved to kernel space. It will just make distros
> life easier and remove the supplicant dependency. I've attached a
> patch that solves both the kernel panic and the fortunate side-effect
> is that you don't have to remount the efivarfs. If people like it, I
> can send it as a fix
>

Distro friendliness is the key aspect here. If we were just continuing
with the classic embedded customization hell, we could also continue to
live with all those vendor-specific ways of booting, validating,
encrypting etc. so that distros will widely stay away from embedded
boards. That's why you invested into the UEFI way of doing things, to
change that.

Jan

--
Siemens AG, Technology
Competence Center Embedded Linux

2023-06-09 06:54:48

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan,

[...]

> > --- a/drivers/firmware/efi/vars.c
> > +++ b/drivers/firmware/efi/vars.c
> > @@ -123,7 +123,7 @@ EXPORT_SYMBOL_GPL(efivars_unregister);
> >
> > bool efivar_supports_writes(void)
> > {
> > - return __efivars && __efivars->ops->set_variable;
> > + return __efivars && __efivars->ops->set_variable != set_variable_int;
> > }
> > EXPORT_SYMBOL_GPL(efivar_supports_writes);
> >
> > diff --git a/fs/efivarfs/super.c b/fs/efivarfs/super.c
> > index e028fafa04f3..e40b5c4c5106 100644
> > --- a/fs/efivarfs/super.c
> > +++ b/fs/efivarfs/super.c
> > @@ -242,9 +242,6 @@ static int efivarfs_fill_super(struct super_block
> > *sb, struct fs_context *fc)
> > sb->s_d_op = &efivarfs_d_ops;
> > sb->s_time_gran = 1;
> >
> > - if (!efivar_supports_writes())
> > - sb->s_flags |= SB_RDONLY;
> > -
> > inode = efivarfs_get_inode(sb, NULL, S_IFDIR | 0755, 0, true);
> > if (!inode)
> > return -ENOMEM;
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index 58d1c271d3b0..ec0ac6ef50a3 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1084,6 +1084,10 @@ int efivars_register(struct efivars *efivars,
> > const struct efivar_operations *ops);
> > int efivars_unregister(struct efivars *efivars);
> >
> > +efi_status_t set_variable_int(efi_char16_t *name, efi_guid_t *vendor,
> > + u32 attributes, unsigned long data_size,
> > + void *data);
> > +
> > void efivars_generic_ops_register(void);
> > void efivars_generic_ops_unregister(void);
> >
> > Thanks
> > /Ilias
>
> As just written in my other reply: The root cause is the dependency on
> tee-supplicant daemon. That needs to be resolved, and then also r/w
> mounting will just work.

That's partially true. If we solve the dependency your problem will
go away only if everything gets compiled as built in. But if you have
them as modules there's still a chance you mount the efivarfs before
installing all the modules. In that case, you'll end up with the same
problem no?

That's why I think this patch (or a variation of it) is useful. It
solves the kernel panic you are seeing if you remount the efivarfs as
RW and It unifies the way the kernel responds to userspace no matter
what the firmware does with its setvariableRT service.

Thanks
/Ilias
>
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-09 06:55:10

[permalink] [raw]

Subject: Re: [PATCH v5 3/3] efi: Add tee-based EFI variable driver

Hi Jan,

On Fri, 9 Jun 2023 at 09:16, Jan Kiszka <[email protected]> wrote:
>
> On 08.06.23 15:52, Ard Biesheuvel wrote:
> > On Thu, 8 Jun 2023 at 08:22, Ilias Apalodimas
> > <[email protected]> wrote:
> >>
> >> Hi Jan
> >>
> >>
> >> On Wed, 7 Jun 2023 at 22:46, Jan Kiszka <[email protected]> wrote:
> >>>
> >>> On 07.06.23 20:17, Ilias Apalodimas wrote:
> >>>> On Wed, 7 Jun 2023 at 20:14, Jan Kiszka <[email protected]> wrote:
> >>>>>
> >>>>> On 07.06.23 18:59, Ilias Apalodimas wrote:
> >>>>>> On Wed, 7 Jun 2023 at 19:09, Ilias Apalodimas
> >>>>>> <[email protected]> wrote:
> >>>>>>>
> >>>>>>> Hi Jan,
> >>>>>>>
> >>>>>>> [...]
> >>>>>>>>>>> No I don't, this will work reliably without the need to remount the efivarfs.
> >>>>>>>>>>> As you point out you will still have this dependency if you end up
> >>>>>>>>>>> building them as modules and you manage to mount the efivarfs before
> >>>>>>>>>>> those get inserted. Does anyone see a reasonable workaround?
> >>>>>>>>>>> Deceiving the kernel and making the bootloader set the RT property bit
> >>>>>>>>>>> to force the filesystem being mounted as rw is a nasty hack that we
> >>>>>>>>>>> should avoid. Maybe adding a kernel command line parameter that says
> >>>>>>>>>>> "Ignore the RTPROP I know what I am doing"? I don't particularly love
> >>>>>>>>>>> this either, but it's not unreasonable.
> >>>>>>>>>>
> >>>>>>>>>> In the context of https://github.com/OP-TEE/optee_os/issues/6094,
> >>>>>>>>>> basically this issue mapped on reboot/shutdown, I would really love to
> >>>>>>>>>> see the unhandy tee-supplicant daemon to be overcome.
> >>>>>>>>>
> >>>>>>>>> I have seen this error before and it has been on my todo list. So I
> >>>>>>>>> have tried to fix it here [1]. Feel free to test it and let me know if
> >>>>>>>>> you see any further issues.
> >>>>>>>>>
> >>>>>>>>> [1] https://lkml.org/lkml/2023/6/7/927
> >>>>>>>>>
> >>>>>>>>
> >>>>>>>> Ah, nice, will test ASAP!
> >>>>>>>>
> >>>>>>>> Meanwhile more food: I managed to build a firmware that was missing
> >>>>>>>> STMM. But the driver loaded, and I got this:
> >>>>>>>
> >>>>>>> Thanks for the testing. I'll try to reproduce it locally and get back to you
> >>>>>>
> >>>>>> Can you provide a bit more info on how that was triggered btw? I would
> >>>>>> be helpful to know
> >>>>>>
> >>>>>> - OP-TEE version
> >>>>>
> >>>>> Today's master, 145953d55.
> >>>>>
> >>>>>> - was it compiled as a module or built-in?
> >>>>>
> >>>>> Sorry, not sure anymore, switching back and forth right now. I think it
> >>>>> was built-in.
> >>>>>
> >>>>>> - was the supplicant running?
> >>>>>
> >>>>> Yes.
> >>>>>
> >>>>
> >>>> Ok thanks, that helps. I guess this also means U-Boot was compiled to
> >>>> store the variables in a file in the ESP instead of the RPMB right?
> >>>> Otherwise, I can't see how the device booted in the first place.
> >>>
> >>> U-Boot was not configured to perform secure booting in this case. It had
> >>> RPMB support enabled, just didn't have to use it.
> >>
> >> In your initial mail you said you managed to build a firmware without
> >> StMM. If U-boot isn't reconfigured accordingly -- iow skip the EFI
> >> variable storage in an RPMB, the EFI subsystem will fail to start.
> >>
> >> In any case, I don't think the ooops you are seeing is not connected
> >> to this patchset. Looking at the kernel EFI stub we only set the
> >> SetVariableRT if the RTPROP table is set accordingly by the firmware.
> >> U-Boot never sets the EFI_RT_SUPPORTED_SET_VARIABLE property since it
> >> can't support it. What you are doing is remount the efivarfs as rw
> >> and then trying to set a variable, but the callback for it is NULL.
> >> I think you'll be able to replicate the same behavior on the kernel
> >> without even inserting the new module.
>
> Might be true. I'll try to look into that again when the other dust settled.
>
> >
> > I have dropped this series from efi/next for now, given that it
> > obviously has problems in its current state.
> >
> > The risk of merging this now and fixing it later is that it may cause
> > regressions for early adopters that rely on the behavior we are
> > introducing here. Better to get this in shape first.
>
> On the one side, I'm sorry having ruined the merge, but my gut feeling
> is as well that this really needs to be reworked to get rid of the
> unfortunate tee-supplicant daemon. So far, we have to start the daemon
> from initrd, write a systemd service to adopt that instance, and make
> ftpm modular to remove it before terminating tee-supplicant (e.g. on
> system shutdown) - Sumit's patch didn't help there.
>

No worries, the whole functionality is intrusive, so I prefer going
through some iterations until everyone is happy. OTOH the
'supplicant' problem isn't going away soon. We will try to move it to
the kernel but that has some difficulties as well and it's going to
take some time. In any case, we've lived with the supplicant for
quite some time and the ftpm module has a similar set of problems.
IOW there are kernel modules that depend on it.
From a functionality point of view nothing will change if the
supplicant gets moved to kernel space. It will just make distros
life easier and remove the supplicant dependency. I've attached a
patch that solves both the kernel panic and the fortunate side-effect
is that you don't have to remount the efivarfs. If people like it, I
can send it as a fix

Thanks
/Ilias
> Jan
>
> --
> Siemens AG, Technology
> Competence Center Embedded Linux
>

2023-06-09 08:05:58