Re: [PATCH RFC v2 7/8] 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:
- [Medium] tee: optee: mixing goto and __free() violates cleanup guidelines
- [Low] tee: optee: missing cleanup of shm_arg_cache on probe error paths
--

commit 3ef22394684ff6d8b2126c91fa4c8bb2d1960d68
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. It checks the service API
version, queries the trusted OS revision, initializes shared-memory tracking,
and connects the RPMI backend to the common OP-TEE operations.

> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
> index 6e76316794c18..db541f3de4250 100644
> --- a/drivers/tee/optee/rpmi_abi.c
> +++ b/drivers/tee/optee/rpmi_abi.c
[ ... ]
> +static int optee_rpmi_probe(struct rpmi_tee_device *rdev)
> +{
> +	struct tee_device *teedev;
> +	struct tee_context *ctx;
> +	int ret;
> +
> +	struct optee *optee __free(kfree) = kzalloc_obj(*optee);

[Severity: Medium]
Does this function mix goto-based error handling and scope-based cleanup?

According to the kernel cleanup subsystem guidelines, mixing goto and
scope-based cleanups like __free() in the same function creates confusing
ownership semantics and raises the risk of missed cleanups. The guidelines
suggest converting all resources to scope-based cleanup or none at all.

> +	if (!optee)
> +		return -ENOMEM;
> +
[ ... ]
> +	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;
> +	ret = optee_notif_init(optee, optee->rpmi.notification_count);
> +	if (ret)
> +		goto err_common;
[ ... ]
> +err_initialized:
> +	/* The remove path owns and frees the published backend state. */
> +	retain_and_null_ptr(optee);
> +	optee_rpmi_remove(rdev);
> +
> +	return ret;
> +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:

[Severity: Low]
Are we missing the cleanup of the argument cache on the err_common path?

If optee_notif_init() fails, the code jumps to err_common, bypassing
err_devices where optee_shm_arg_cache_uninit() is called. This skips the
mutex_destroy() for the argument cache. The actual memory for the optee
struct is freed via the scope-based cleanup, but the logical teardown
is bypassed.

> +	optee_supp_uninit(&optee->supp);
> +	mutex_destroy(&optee->call_queue.mutex);
> +	rpmb_dev_put(optee->rpmb_dev);
> +	mutex_destroy(&optee->rpmb_dev_mutex);
> +	optee_rpmi_shm_rht_uninit(optee);
> +err_pool:
> +	tee_shm_pool_free(optee->pool);
> +
> +	return ret;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261005-rpmi-tee-service-grp-dev-v2-0-72f222e23ec1@oss.qualcomm.com?part=7