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

[Harshal Dev]

Hi Konrad,

On 24-10-2026 07:16 pm, Harshal Dev wrote:
> 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@xxxxxxxxxxxxxxxx>
>>> ---
>> [...]
>>
>>> +	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

[...]

> Hypothetically it could fail. If it fails:

[...]

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

I'd like to backtrack on this one particular comment. I think it's better to simply
return a value as suggested by you and print it when it isn't fatal in the case of
QCOM_SCM driver. In the case of ICE TEE, I'll fail probe here when it returns error.

Ack.

Regards,
Harshal
> Regards,
> Harshal 
>> Konrad
>