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

[Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>]

Hi Harshal,

On 11/3/2026 7:35 PM, Harshal Dev wrote:
> 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.
> 

I do not believe it is comment issue. I understand attr is an optional output value
which could be NULL. However, this implementation also uses attr as input. 

In your provided link:
https://elixir.bootlin.com/linux/v7.3/source/fs/efivarfs/file.c#L108
attr that passed to this function is from stack and uninitialized, so the
TA will get garbage in in_attr based on bellow code. It this acceptable?


>>> +	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
>  

Yeah, you are right. Let's ignore it for now.

Best Regards,
Amir

>>
>> - Amir
>>