Re: [PATCH RFC v3 1/9] tee: optee: share the global shared-memory ID mapping

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] tee: optee: UAF race in SHM free via unreferenced lookup
--

commit 39c6675fd3cd9df49f5b14024b5f0c43a9129fe8
Author: Amirreza Zarrabi <amirreza.zarrabi@oss.qualcomm.com>

tee: optee: share the global shared-memory ID mapping

This commit moves the FF-A backend's hash table mapping secure-world memory
handles to TEE shared-memory objects into common helpers. It also converts
the FF-A backend to use this new shared mapping API.

> diff --git a/drivers/tee/optee/ffa_abi.c b/drivers/tee/optee/ffa_abi.c
> --- a/drivers/tee/optee/ffa_abi.c
> +++ b/drivers/tee/optee/ffa_abi.c
> @@ -488,7 +404,8 @@ static void handle_ffa_rpc_func_cmd_shm_free(struct tee_context *ctx,
>  	    arg->params[0].attr != OPTEE_MSG_ATTR_TYPE_VALUE_INPUT)
>  		goto err_bad_param;
>  
> -	shm = optee_shm_from_ffa_handle(optee, arg->params[0].u.value.b);
> +	shm = optee_shm_handle_map_lookup(&optee->ffa.shm_map,
> +					  arg->params[0].u.value.b);
>  	if (!shm)
>  		goto err_bad_param;

[Severity: High]
Could this lookup introduce a Use-After-Free race if multiple RPCs are
processed concurrently?

If a compromised Secure World sends concurrent OPTEE_RPC_CMD_SHM_FREE RPCs
for the same shared memory global_id, both threads can look up the pointer
before either has a chance to free it.

Since optee_shm_handle_map_lookup() returns the tee_shm pointer without
incrementing its reference count, both threads will then proceed down to
tee_shm_free() or optee_rpc_cmd_free_suppl(). The first thread will drop
the reference to 0, freeing the object, and the second thread will
subsequently access shm->refcount on the freed pointer.

> diff --git a/drivers/tee/optee/shm_handle_map.c b/drivers/tee/optee/shm_handle_map.c
> --- /dev/null
> +++ b/drivers/tee/optee/shm_handle_map.c
[ ... ]
> +struct tee_shm *
> +optee_shm_handle_map_lookup(struct optee_shm_handle_map *map, u64 global_id)
> +{
> +	struct optee_shm_handle_map_entry *entry;
> +	struct tee_shm *shm = NULL;
> +
> +	mutex_lock(&map->mutex);
> +	entry = rhashtable_lookup_fast(&map->rht, &global_id,
> +				       shm_handle_map_params);
> +	if (entry)
> +		shm = entry->shm;
> +	mutex_unlock(&map->mutex);
> +
> +	return shm;
> +}

[Severity: High]
Since this new API helper retrieves the shared memory pointer without acquiring
a reference, does it allow the object to be freed concurrently by another
thread once the mutex is released?

The kerneldoc above this function mentions "The caller must keep the SHM
object alive; no reference is acquired here", but as seen in
handle_ffa_rpc_func_cmd_shm_free(), the caller isn't guaranteed to hold a
reference when handling incoming RPCs from the secure world.

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