Re: [PATCH v9 10/10] optee: Add support for arm,ffa-lend-pool

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free during driver removal when releasing the reserved memory / lend pool device.
- [High] Memory leak and dangling pointer in global lists on probe error paths.
--

commit a9ed483b0a922615c4b6f53cf658491bee8ed6a1
Author: Vincent Donnefort <vdonnefort@google.com>

optee: Add support for arm,ffa-lend-pool

This commit hooks OP-TEE dynamically allocated protected memory pools to the
"arm,ffa-lend-pool" driver. It also adds reserved memory device
initialization and release for SMC transport platform devices.

> 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
> @@ -979,6 +979,7 @@ static void optee_ffa_remove(struct ffa_device *ffa_dev)
>  
>  	mutex_destroy(&optee->ffa.mutex);
>  	rhashtable_free_and_destroy(&optee->ffa.global_ids, rh_free_fn, NULL);
> +	ffa_lend_pool_detach(&optee->teedev->dev);

[Severity: High]
Can this cause a use-after-free when unbinding the driver?

The call to optee_remove_common() slightly earlier in this function will
unregister and eventually free optee->teedev. Does this subsequent call to
ffa_lend_pool_detach() dereference the already-freed optee->teedev pointer?

>  
>  	kfree(optee);
>  }

[ ... ]

> @@ -1042,13 +1043,21 @@ static int optee_ffa_protmem_pool_init(struct optee *optee, u32 sec_caps)
>  	int rc = 0;
>  
>  	if (sec_caps & OPTEE_FFA_SEC_CAP_PROTMEM) {
> +		rc = ffa_lend_pool_attach(&optee->teedev->dev);

[Severity: High]
Does this introduce a resource leak and dangling pointer on the probe error
paths?

If optee_ffa_probe() fails after this pool is successfully attached, the
error paths will unregister and free optee->teedev. However, it doesn't
appear that the error paths call ffa_lend_pool_detach() to clean this up.

Could this leave the freed device pointer tracked in the lend pool tracking,
leading to a use-after-free later if list operations occur?

> +		if (rc && rc != -ENODEV)
>  			return rc;
>  
>  		pool = optee_protmem_alloc_dyn_pool(optee, id);
> -		if (IS_ERR(pool))
> +		if (IS_ERR(pool)) {
> +			ffa_lend_pool_detach(&optee->teedev->dev);
>  			return PTR_ERR(pool);
> +		}

[ ... ]

> diff --git a/drivers/tee/optee/smc_abi.c b/drivers/tee/optee/smc_abi.c
> --- a/drivers/tee/optee/smc_abi.c
> +++ b/drivers/tee/optee/smc_abi.c
> @@ -1528,6 +1529,8 @@ static void optee_smc_remove(struct platform_device *pdev)
>  	if (optee->smc.memremaped_shm)
>  		memunmap(optee->smc.memremaped_shm);
>  
> +	of_reserved_mem_device_release(&optee->teedev->dev);

[Severity: High]
Does this also trigger a use-after-free during driver removal?

Similar to optee_ffa_remove(), optee_smc_remove() calls
optee_remove_common() before reaching this code, which frees optee->teedev.
Is it safe to access optee->teedev->dev here after it has been freed?

> +
>  	kfree(optee);
>  }

[ ... ]

> @@ -1712,16 +1715,22 @@ static int optee_protmem_pool_init(struct optee *optee)
>  	if (!protm && !dyn_protm)
>  		return 0;
>  
> +	of_reserved_mem_device_init_by_idx(&optee->teedev->dev,
> +					   dev_of_node(optee->teedev->dev.parent), 0);

[Severity: High]
Is there a missing of_reserved_mem_device_release() on the optee_probe()
error paths?

If a failure occurs later in optee_probe() (for instance, if
optee_enumerate_devices() fails), the error handling jumps to labels like
err_disable_shm_cache and err_unreg_teedev. Those paths unregister and free
optee->teedev, but do not release this reserved memory device.

Could this leave the freed dev pointer tracked in the global
of_rmem_assigned_device_list, risking memory corruption if another device
requests memory later?

>  	if (protm)
>  		pool = static_protmem_pool_init(optee);
>  	if (dyn_protm && IS_ERR(pool))
>  		pool = optee_protmem_alloc_dyn_pool(optee, heap_id);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260902104712.2399797-1-vdonnefort@google.com?part=10