Re: [PATCH RFC v3 8/9] tee: optee: bind RPMI services and negotiate backend capabilities

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tee: optee: buffer overflow in optee_get_msg_arg() due to unbounded rpc_param_count
- [Medium] optee: missing optee_shm_arg_cache_uninit() on probe error paths
--

commit 8a4dd6e2611ab6b4ab7ba8d46b447719a90842db
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

tee: optee: bind RPMI services and negotiate backend capabilities

Register an RPMI service driver matching the OP-TEE service UUID and
integrate it with OP-TEE module initialization and removal. Check the service
API version, query the trusted OS revision, and obtain the RPC parameter and
logical notification counts.

> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
> index 9a6439e15a33f..0ed2de96758e2 100644
> --- a/drivers/tee/optee/rpmi_abi.c
> +++ b/drivers/tee/optee/rpmi_abi.c
[ ... ]
> +static int optee_rpmi_exchange_caps(struct optee *optee, u32 *rpc_param_count,
> +				    u32 *max_notif_key)
> +{
[ ... ]
> +	rc = optee_rpmi_call(optee, &req, sizeof(req), &caps, sizeof(caps));
> +	if (rc)
> +		return rc;
> +
> +	rpc_count = le32_to_cpu(caps.rpc_param_count);
> +	if (!rpc_count)
> +		return -EPROTO;
> +
> +	*rpc_param_count = rpc_count;

[Severity: High]
Does this code overflow the shared memory buffer in optee_get_msg_arg()?

Here rpc_count is read directly from the TEE response and assigned without
any upper bounds check.

Later, in optee_get_msg_arg(), sz = optee_msg_arg_size(optee->rpc_param_count)
is calculated. If sz exceeds SHM_ENTRY_SIZE (which is typically 4096),
the code falls back to allocating a new shared memory buffer using
tee_shm_alloc_kernel_buf(ctx, SHM_ENTRY_SIZE), which strictly allocates 4096
bytes regardless of the calculated sz.

Finally, memset(ma, 0, sz) is called, which writes sz bytes into the newly
allocated 4096-byte buffer.

Since rpc_param_count is unbounded, a large value from a compromised or
buggy secure world could lead to an out-of-bounds write.

[ ... ]
> +static int optee_rpmi_probe(struct rpmi_tee_device *rdev)
> +{
[ ... ]
> +	optee_cq_init(&optee->call_queue, 0);
> +	optee_supp_init(&optee->supp);
> +	optee_shm_arg_cache_init(optee, OPTEE_SHM_ARG_SHARED);
> +	mutex_init(&optee->rpmb_dev_mutex);
> +	INIT_WORK(&optee->rpmb_scan_bus_work, optee_bus_scan_rpmb);
> +	optee->rpmb_intf.notifier_call = optee_rpmb_intf_rdev;
> +	rc = optee_notif_init(optee, max_notif_key);
> +	if (rc)
> +		goto err_common;
> +
> +	teedev = tee_device_alloc(&optee_rpmi_clnt_desc, &rdev->dev,
> +				  optee->pool, optee);
> +	if (IS_ERR(teedev)) {
> +		rc = PTR_ERR(teedev);
> +		goto err_notif;
> +	}
[ ... ]
> +err_devices:
> +	tee_device_unregister(optee->supp_teedev);
> +	tee_device_unregister(optee->teedev);
> +	optee_shm_arg_cache_uninit(optee);
> +err_notif:
> +	optee_notif_uninit(optee);
> +err_common:
> +	optee_supp_uninit(&optee->supp);

[Severity: Medium]
Does this error path leak the argument cache resources?

optee_shm_arg_cache_init() is called unconditionally earlier in this function,
which allocates resources and initializes a mutex (optee->shm_arg_cache.mutex).

If optee_notif_init() fails, execution jumps to the err_common label. If
tee_device_alloc() fails, execution jumps to the err_notif label.

Both of these labels bypass the required cleanup call
optee_shm_arg_cache_uninit() which is located under the err_devices label
above them, leading to a leaked mutex and other argument cache resources.

> +	mutex_destroy(&optee->call_queue.mutex);
> +	rpmb_dev_put(optee->rpmb_dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-rpmi-tee-service-grp-dev-v3-0-975b574667e2@oss.qualcomm.com?part=8