Re: [PATCH 3/5] firmware: qcom: Add an TEE bus based ICE back-end

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

Hi Konrad,

On 22-10-2026 01:21 pm, Konrad Dybcio wrote:
> On 10/21/26 5:43 PM, Harshal Dev wrote:
>> Add support for a TEE client driver which forwards calls from the Inline
>> Crypto Engine (ICE) service (invoked by the ICE driver) to the OPTEE ICE
>> Psuedo-TA.
>>
>> The ICE PTA supports both ICE Legacy and Standard modes. In Standard mode
>> Hardware Key Manager v1 is supported.
>>
>> Signed-off-by: Harshal Dev <harshal.dev@oss.qualcomm.com>
>> ---
> [...]
> 
>> +	ret = tee_client_invoke_func(data->ctx, &inv_arg, param);
>> +	if (ret < 0 || inv_arg.ret != 0) {
>> +		dev_err(dev,
>> +			"ICE generate_key failed, ret=%d, err=0x%x\n",
>> +			ret, inv_arg.ret);
>> +		ret = ret ?: -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	buf = tee_shm_get_va(key_shm, 0);
> 
> This can return an ERR_PTR()
> 
>

Ack. 
>> +	out_size = param[0].u.memref.size;
> 
> This returned size is not validated, there are a couple occurences
> of that
> 
> 
>

Ack, will fix in qcom_ice_tee_wrapped_key_call() as well. 
>> +
>> +	memcpy(lt_key, buf, lt_key_size);
>> +	memzero_explicit(buf, out_size);
>> +out:
>> +	tee_shm_free(key_shm);
> 
> LLM is concerned about freeing the key without zeroing the memory
> first
>

When the TEE invoke call fails, the TEE must not touch the out_buf.
We trust the TEE to behave correctly, since it is a 'Trusted' EE after all. :)

When we fail after a successful call from the TEE, we must clear the buffer.
I will check for returned size and ERR_PTR() and clear the out_buf in that
case.

Ack. 
>> +static int qcom_ice_tee_probe(struct tee_client_device *ice_dev)
>> +{
>> +	struct device *dev = &ice_dev->dev;
>> +	struct qcom_ice_tee_private *data;
>> +	struct tee_ioctl_open_session_arg sess_arg = {
>> +		.clnt_login = TEE_IOCTL_LOGIN_REE_KERNEL,
>> +	};
>> +	int ret, err = -ENODEV;
>> +
>> +	data = devm_kzalloc(dev, sizeof(*data), GFP_KERNEL);
>> +	if (!data)
>> +		return -ENOMEM;
>> +
>> +	data->ctx = tee_client_open_context(NULL, optee_ctx_match, NULL, NULL);
>> +	if (IS_ERR(data->ctx))
>> +		return -ENODEV;
>> +
>> +	export_uuid(sess_arg.uuid, &ice_dev->id.uuid);
>> +	ret = tee_client_open_session(data->ctx, &sess_arg, NULL);
>> +	if (ret < 0 || sess_arg.ret != 0) {
>> +		dev_err(dev,
>> +			"tee_client_open_session failed, ret=%d, err=0x%x\n",
>> +			ret, sess_arg.ret);
>> +		err = ret ?: -EINVAL;
>> +		goto out_ctx;
> 
> Should we care about the specific retval that comes from the TEE?
>

Traditionally TEE client drivers do not map the TEE error codes to kernel ones
since a 1-1 mapping is not always possible. Also, since the ICE client driver whose
calls land into this API via the intermediate ICE service would not know whether
it is calling into QTEE or OPTEE, any error-code mapping we create here would
possibly conflict with the ones returned from QTEE.

So returning a generic error code while printing the specific TEE error in the log
helps keep things simple and easy to debug.

I'm open to any alternative suggestions.

 
>> +	}
>> +
>> +	data->session_id = sess_arg.session;
>> +	dev_set_drvdata(dev, data);
>> +
>> +	qcom_ice_ops_tee.dev = dev;
>> +	qcom_ice_svc_ops_register(&qcom_ice_ops_tee);
> 
> This can fail (but it currently returns void)
>

Practically it can never fail because if the platform boots with OPTEE, QCOM_SCM never
calls this. Alternatively if QTEE boots this TEE client driver never makes this call.
This is why the QCOM_PAS driver also returns a void from a similar function:
https://elixir.bootlin.com/linux/v7.3-rc5/source/drivers/firmware/qcom/qcom_pas.c#L279

Hypothetically it could fail. If it fails:
1. QCOM_SCM should ignore the failure and continue to probe because it provides a
lot of other functionalities apart from the ICE SCM calls.
2. This TEE client driver should fail probe because it only provides ICE functionality. 

I can cover this hypothetical scenario by making the function return a failure which
QCOM_SCM ignores but this driver interprets as fatal. But it could open another Pandora's
box of compiler/static analysis tool warnings.

Let me know.
Regards,
Harshal 
> Konrad