Re: [PATCH v5 6/7] firmware: qcom: Add support for TEE based EFI-var client driver

[Harshal Dev <harshal.dev@oss.qualcomm.com>]

Hi Amir,

On 26-10-2026 10:14 am, Amirreza Zarrabi wrote:
> Hi Harshal,
> 
> On 10/15/2026 10:39 PM, Harshal Dev wrote:
>> On Qualcomm SoC based platforms, UEFI stores EFI variables within the
>> Replay Protected Memory Block (RPMB) located within either the UFS,
>> eMMC or SPI-NOR storage. The RPMB key which is one-time programmed into
>> the storage controller to allow authentication of the RPMB frames is
>> generated by and only available to the Qualcomm Trusted Execution
>> Environment (QTEE).
>>
>> The legacy QSEECOM protocol used for communicating with the QTEE is
>> deprecated and replaced with the use-case agnostic SMCInvoke protocol
>> starting with the Qualcomm SM8x50 series. On platforms where the QSEECOM
>> driver still probes, it does not support a listener interface with QTEE
>> to enable writing of non-volatile EFI variables to the RPMB for UFS and
>> eMMC storage.
>> Therefore on such platforms, a TEE client driver which communicates with
>> QTEE via the SMCInvoke protocol implemented by the QCOMTEE driver (and
>> registered with the TEE subsystem) must be used to update such EFI
>> variables.
>>
>> Add support for a TEE based uefisecapp client driver which installs efivar
>> operations after obtaining an object reference to the uefisecapp service.
>> This enables the kernel/user-space to access or modify both volatile EFI
>> variables stored by the Secure Application (in-memory) and non-volatile
>> ones stored within RPMB.
>>
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>> ---
>>  MAINTAINERS                                 |   6 +
>>  drivers/firmware/qcom/Kconfig               |  31 ++
>>  drivers/firmware/qcom/Makefile              |   1 +
>>  drivers/firmware/qcom/qcom_tee_uefisecapp.c | 644 ++++++++++++++++++++++++++++
>>  4 files changed, 682 insertions(+)
>>

[...]

>> diff --git a/drivers/firmware/qcom/qcom_tee_uefisecapp.c b/drivers/firmware/qcom/qcom_tee_uefisecapp.c
>> new file mode 100644
>> index 000000000000..bcd025f929fc
>> --- /dev/null
>> +++ b/drivers/firmware/qcom/qcom_tee_uefisecapp.c
>> @@ -0,0 +1,644 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * Copyright (c) Qualcomm Technologies, Inc. and/or its subsidiaries.
>> + */
>> +
>> +#include <linux/efi.h>
>> +#include <linux/tee.h>
>> +#include <linux/tee_drv.h>
>> +#include <linux/ucs2_string.h>

[...]

>> +
>> +static efi_status_t qcomtee_uefi_get_variable(efi_char16_t *name, efi_guid_t *guid,
>> +					      u32 *attr, unsigned long *data_size,
>> +					      void *data)
>> +{
>> +	int ret;
>> +	u32 in_attr, out_attributes, out_errno;
>> +	struct tee_param_ubuf in_data, in_var, in_attributes;
>> +
>> +	if (!name || !guid)
>> +		return EFI_INVALID_PARAMETER;
>> +
>> +	/*
>> +	 * 'attr' can be NULL, however an input attribute is always expected
>> +	 * by UefiSecApp TA
>> +	 */
> 
> You are enforceing a poliy on the arguments of a cb which is called publicly, are you
> sure all callers understand that this output value is optional? it is not specified
> anywhere as far as I can see. This seesm to be a TA thing.
>

I think the way this comment is written is a bit misleading. The comment is not about
the output value of attr, it is about the input value passed to this callback.

The callers of this callback either pass a NULL value for attribute or a valid value.
When they pass NULL, they ignore the output since it's expected to not be set by the
implementation: https://elixir.bootlin.com/linux/v7.3/source/fs/efivarfs/vars.c#L505

When they pass a valid value, they expect a valid output in return and operate on it:
https://elixir.bootlin.com/linux/v7.3/source/fs/efivarfs/file.c#L108
https://elixir.bootlin.com/linux/v7.3/source/fs/efivarfs/vars.c#L531
So the value is not optional, it is set or not set as per the expectation of the
caller. 
 
>> +	in_attr = 0;

However, the SMCInvoke transport here needs this to not be NULL, and so we force
set a 0 here which is interpreted corrected by the TA as attribute not set.

I will re-phrase this comment to avoid ambiguity.

>> +	if (attr)
>> +		in_attr = *attr;
>> +
>> +	in_data = (struct tee_param_ubuf){ .addr = data, *data_size };
>> +	in_var = (struct tee_param_ubuf){ .addr = name,
>> +					  (ucs2_strlen(name) + 1) * sizeof(*name) };
>> +	in_attributes = (struct tee_param_ubuf){ .addr = &in_attr, sizeof(u32) };
>> +
>> +	/* On SUCCESS, 'data' member of 'in_data' has already been updated. */
>> +	ret = qcuefi_get_variable(in_var, guid, in_attributes, &in_data,
>> +				  &out_attributes, &out_errno);
>> +
>> +	if (ret)
>> +		return EFI_DEVICE_ERROR;
>> +
>> +	if (!out_errno || out_errno == QCOMTEE_UEFI_SEC_ERROR_SIZE_OUT) {
>> +		/* If 'attr' is NULL 'out_attributes' is not updated. */
>> +		if (attr)
>> +			*attr = out_attributes;
>> +
>> +		*data_size = in_data.size;
>> +	}
>> +
>> +	return uefisecapp_err_to_efi_status(out_errno);
>> +}
>> +

[...]

>> +
>> +static const struct efivar_operations qcom_efivar_ops = {
>> +	.get_variable = qcomtee_uefi_get_variable,
>> +	.set_variable = qcomtee_uefi_set_variable,
> 
> Not sure how much it is relevant, but I was looking into the efi and in
> efivar_set_variable_locked, saw this comment "If no _nonblocking variant exists,
> the ordinary one is assumed to be non-blocking." which in not the case fo us.
> 
> Does it make sense to introduce something like
> qcomtee_uefi_set_variable_nonblocking and return UNSUPPORTED!?
>

Ack, this ensures that if a caller expects the set_variable() to land in a
non-blocking call, they get returned -ENOTSUPPORTED instead of being misled.
As far as I can see only the x86 pstore implementation explicitly requests
a call into a non-blocking variant:
https://elixir.bootlin.com/linux/v7.3/source/drivers/firmware/efi/efi-pstore.c#L227
 
>> +	.get_next_variable = qcomtee_uefi_get_next_variable,
>> +	.query_variable_info = qcomtee_uefi_query_variable_info,
>> +};
>> +
>> +static int qcomtee_ctx_match(struct tee_ioctl_version_data *ver,
>> +			     const void *data)
>> +{
>> +	return (ver->impl_id == TEE_IMPL_ID_QTEE);
>> +}
>> +
>> +static int qcomtee_uefisecapp_probe(struct tee_client_device *tee_dev)
>> +{
>> +	int ret, err;
>> +	struct tee_param_objref client_env_obj;
>> +	struct tee_param_objref uefisec_svc_obj;
>> +
>> +	uefisec_app.dev = &tee_dev->dev;
>> +	/* Open context with QCOMTEE driver */
>> +	uefisec_app.ctx = tee_client_open_context(NULL, qcomtee_ctx_match, NULL,
>> +						  NULL);
>> +	if (IS_ERR(uefisec_app.ctx))
>> +		return -ENODEV;
>> +
>> +	ret = qcomtee_get_client_env_obj(uefisec_app.ctx, &client_env_obj);
>> +	if (ret) {
>> +		err = -EINVAL;
>> +		goto err_get_client_env;
>> +	}
>> +
>> +	ret = qcomtee_get_uefisec_svc_obj(uefisec_app.ctx, client_env_obj,
>> +					  &uefisec_svc_obj);
>> +	if (ret) {
>> +		err = -EINVAL;
>> +		goto err_get_uefisec_svc;
>> +	}
>> +	uefisec_app.uefisec_svc_obj = uefisec_svc_obj;
>> +
>> +	ret = efivars_register(&uefisec_app.efivars, &qcom_efivar_ops);
>> +	if (ret) {
>> +		err = ret;
>> +		goto err_efi_vars_reg;
>> +	}
>> +
>> +	/*
>> +	 * We don't need to keep a reference to this object anymore, we only
>> +	 * needed it to obtain the uefisec_svc object.
>> +	 */
>> +	qcomtee_release_object(uefisec_app.ctx, client_env_obj);
>> +	return 0;
>> +
>> +err_efi_vars_reg:
>> +	qcomtee_release_object(uefisec_app.ctx, uefisec_svc_obj);
>> +err_get_uefisec_svc:
>> +	qcomtee_release_object(uefisec_app.ctx, client_env_obj);
>> +err_get_client_env:
>> +	tee_client_close_context(uefisec_app.ctx);
>> +
>> +	return err;
>> +}
>> +
>> +static void qcomtee_uefisecapp_remove(struct tee_client_device *tee_dev)
>> +{
>> +	efivars_unregister(&uefisec_app.efivars);
> 
> I still have concerns about the removal path. efivars_unregister() acquires the
> semaphore interruptibly and can return -EINTR, for example when a signal arrives while
> it waits for an active EFI operation. Its return value is ignored here, so removal can
> release the service object and close the TEE context while the EFI provider remains registered.
>

This is a valid point. But it seems like no easy solution exists here. The driver core
doesn't check the value being returned from remove(), so it will always detach the driver from
the device.

Currently when -EINTR is returned, the driver is removed but __efivar->ops remain set, so a
subsequent call to __efivars->ops would cause a kernel panic/crash. If we check for error here
and return without releasing the service object/TEE context, we still have the same issues but
now also a leak; QTEE's object table still has the service object and the TEE context remains
allocated (while the driver is removed and __efivars->ops remain set to a random pointers).

Everyone else is also suffering the same race:
https://elixir.bootlin.com/linux/v7.3/source/drivers/firmware/efi/stmm/tee_stmm_efi.c#L518
https://elixir.bootlin.com/linux/v7.3/source/drivers/firmware/qcom/qcom_qseecom_uefisecapp.c#L842
https://elixir.bootlin.com/linux/v7.3/source/drivers/firmware/google/gsmi.c#L1075

This is a framework level issue, where a fail-safe fix is to use down() instead of down_interruptible()
in efivar_unregister() and pray that whoever is invoking the callback comes out soon. But at
least it replaces a sure-shot kernel panic with a low probability deadlock which can be
diagnosed.

I thought about other ways, like using try_module_get() from either the callbacks or the
efivar_register/unregister(), but neither solve this.
I can try initiating a discussion on this separately on the efivar mailing list, unless there
are any other innovative suggestions for solving it from within the driver. :)

Let me know your thoughts.

> Separately, efivar_lock() appears to have an existing bug: it checks __efivars->ops
> without first checking whether __efivars is NULL. Successful unregister sets that pointer to NULL,
> so subsequent access through an existing efivarfs can dereference NULL.
> I Feel, that needs an EFI-core fix as well. what do you think?
>

Ack, this is true and a fix is straightforward. I will send a patch separately if someone hasn't
already sent one.

Regards,
Harshal
 
> 
> - Amir
>