Re: [PATCH v8 02/14] firmware: qcom_scm: Migrate to generic PAS service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Removing the `if (!ctx->ptr)` check from the metadata release path causes a kernel crash when called with a `NULL` pointer on error paths or during teardown.
--

commit e2ee3fe2c5f5a59a30d27512b70afb597cba4bec
Author: Sumit Garg <sumit.garg@oss.qualcomm.com>

firmware: qcom_scm: Migrate to generic PAS service

With the availability of generic PAS service, this patch adds SCM calls as
a backend to keep supporting legacy QTEE interfaces. The exported qcom_scm*
wrappers will get dropped once all the client drivers get migrated as part
of future patches.

> diff --git a/drivers/firmware/qcom/qcom_scm.c b/drivers/firmware/qcom/qcom_scm.c
> index 6b601a4b89dbf..7933e55803dc2 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c

[ ... ]

> -/**
> - * qcom_scm_pas_metadata_release() - release metadata context
> - * @ctx:	pas context
> - */
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx)
> +static void __qcom_scm_pas_metadata_release(struct device *dev,
> +					    struct qcom_pas_context *ctx)
>  {
> -	if (!ctx->ptr)
> -		return;
> +	if (ctx->use_tzmem)
> +		qcom_tzmem_free(ctx->ptr);
>  	else
> -		dma_free_coherent(__scm->dev, ctx->size, ctx->ptr, ctx->phys);
> +		dma_free_coherent(dev, ctx->size, ctx->ptr, ctx->phys);

[Severity: High]
Will this cause a kernel crash by passing a NULL pointer to
dma_free_coherent() on error paths?

If a client driver calls qcom_scm_pas_init_image() and it fails before
setting ctx->ptr (for example, if dma_alloc_coherent() fails), the
client's error handler will then call qcom_scm_pas_metadata_release().
Since the !ctx->ptr check was removed, this would execute
dma_free_coherent() with a NULL pointer.

I note that a later commit in the series (commit 823d5bd22444cb98ee7ba)
removes these SCM PAS wrappers entirely after clients are migrated.
However, does removing this safety check here break bisection for
existing client drivers in the meantime?

>  
>  	ctx->ptr = NULL;
>  }
> +
> +void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx)
> +{
> +	__qcom_scm_pas_metadata_release(__scm->dev,
> +					(struct qcom_pas_context *)ctx);
> +}
>  EXPORT_SYMBOL_GPL(qcom_scm_pas_metadata_release);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260626133440.692849-1-sumit.garg@kernel.org?part=2