Re: [PATCH RFC v3 6/9] tee: optee: add RPMI RPC handling

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] tee/optee: Use-After-Free race in SHM_FREE RPC due to missing refcount
- [Medium] tee/optee: Memory leak via type confusion in SHM_FREE RPC
--

commit 3440aa8b7411752d7f5d5849d15211a269875fce
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

tee: optee: add RPMI RPC handling

OP-TEE can suspend a yielding call to request services from normal world
through RPC. This patch adds handlers for shared-memory allocation and
free requests, returning RPMI parcel references. It routes other
commands through the common OP-TEE RPC dispatcher.

> diff --git a/drivers/tee/optee/rpmi_abi.c b/drivers/tee/optee/rpmi_abi.c
> index bbb219ddab381..8b90c3653e80d 100644
> --- a/drivers/tee/optee/rpmi_abi.c
> +++ b/drivers/tee/optee/rpmi_abi.c

[ ... ]

> +static void optee_rpmi_handle_rpc_shm_free(struct tee_context *ctx,
> +					   struct optee *optee,
> +					   struct optee_msg_arg *arg)
> +{
> +	struct tee_shm *shm;
> +	u64 type, global_id;
> +
> +	if (arg->num_params != 1 ||
> +	    arg->params[0].attr != OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
> +		goto err_bad_param;
> +
> +	type = arg->params[0].u.value.a;
> +	global_id = arg->params[0].u.value.b;
> +	/* SHM_FREE requires a non-NULL identity. */
> +	if (!optee_rpmi_nonce(global_id) || arg->params[0].u.value.c)
> +		goto err_bad_param;
> +
> +	shm = optee_shm_handle_map_lookup(&optee->rpmi.shm_map, global_id);

[Severity: High]
Does this code allow a use-after-free if concurrent free requests are made?

If a compromised or buggy TEE issues concurrent OPTEE_RPC_CMD_SHM_FREE RPC
requests for the same global_id, multiple kernel threads could obtain the same
unrefcounted shm pointer from optee_shm_handle_map_lookup() before it is
removed from the map.

Both threads would then concurrently call tee_shm_free() (or
optee_rpc_cmd_free_suppl()), causing a use-after-free when the second thread
attempts to drop the reference count.

> +	if (!shm)
> +		goto err_bad_param;
> +
> +	switch (type) {

[Severity: Medium]
Could this code cause a memory leak by trusting the type from the message?

The type is read directly from the untrusted secure world message without
verifying that the shm object actually corresponds to that allocation type.

If the secure world requests to free a supplicant-allocated buffer but spoofs
the type as OPTEE_RPC_SHM_TYPE_KERNEL, the kernel will execute
tee_shm_free(shm).

This bypasses optee_rpc_cmd_free_suppl(), preventing the free message from
reaching the user-space supplicant, which will then permanently leak the
memory.

> +	case OPTEE_RPC_SHM_TYPE_APPL:
> +		optee_rpc_cmd_free_suppl(ctx, shm);
> +		break;
> +	case OPTEE_RPC_SHM_TYPE_KERNEL:
> +		tee_shm_free(shm);
> +		break;
> +	default:
> +		goto err_bad_param;
> +	}
> +
> +	arg->ret = TEEC_SUCCESS;
> +	return;
> +
> +err_bad_param:
> +	arg->ret = TEEC_ERROR_BAD_PARAMETERS;
> +}

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