Re: [PATCH v7 03/15] firmware: qcom_scm: Migrate to generic PAS service

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] The metadata release function removes the safety check for `ctx->ptr`, leading to a regression (NULL pointer dereference or kernel WARN) in error paths of legacy clients.
- [High] The global `ops_ptr` in the generic PAS service is accessed without locking or RCU, causing a race condition and TOCTOU vulnerability.

Pre-existing issues:
- [High] The resource table size returned by TrustZone is trusted without validating it against the allocated buffer size, leading to an out-of-bounds read of kernel memory.
--

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

firmware: qcom_scm: Migrate to generic PAS service
    
This patch adds SCM calls as a backend for the generic PAS service to keep
supporting legacy QTEE interfaces. The exported qcom_scm wrappers are retained
but 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 9b06a69d3a6d7..d87a962e93da1 100644
> --- a/drivers/firmware/qcom/qcom_scm.c
> +++ b/drivers/firmware/qcom/qcom_scm.c
[ ... ]
> @@ -671,36 +671,35 @@ int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
>  
>  	return ret ? : res.result[0];
>  }
> -EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>  
> -/**
> - * qcom_scm_pas_metadata_release() - release metadata context
> - * @ctx:	pas context
> - */
> -void qcom_scm_pas_metadata_release(struct qcom_scm_pas_context *ctx)
> +int qcom_scm_pas_init_image(u32 pas_id, const void *metadata, size_t size,
> +			    struct qcom_scm_pas_context *ctx)
>  {
> -	if (!ctx->ptr)
> -		return;
> +	return __qcom_scm_pas_init_image2(__scm->dev, pas_id, metadata, size,
> +					  (struct qcom_pas_context *)ctx);
> +}
> +EXPORT_SYMBOL_GPL(qcom_scm_pas_init_image);
>  
> +static void __qcom_scm_pas_metadata_release(struct device *dev,
> +					    struct qcom_pas_context *ctx)
> +{

[Severity: High]
Does this removal of the !ctx->ptr safety check cause a regression? In
legacy clients like qcom_q6v5_pas, this function might be unconditionally
called during unprepare or error paths where init_image failed and ctx->ptr
is NULL. This can result in qcom_tzmem_free() warning on a NULL pointer or
dma_free_coherent() crashing. Note that this appears to be fixed later in the
series when clients migrate to the new qcom_pas_metadata_release which
restores the check, but it poses an issue for this intermediate commit.

>  	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);
>  
>  	ctx->ptr = NULL;
>  }
[ ... ]
> @@ -909,11 +828,12 @@ struct resource_table *qcom_scm_pas_get_rsc_table(struct qcom_scm_pas_context *c
>  
>  	memcpy(input_rt_tzm, input_rt, input_rt_size);
>  
> -	output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id, input_rt_tzm,
> +	output_rt_tzm = __qcom_scm_pas_get_rsc_table(dev, ctx->pas_id,
>  						     input_rt_tzm,
>  						     input_rt_size, &size);

[Severity: High]
This is a pre-existing issue, but does this function trust the resource table
size returned by TrustZone without validating it against the allocated buffer
size?

If TrustZone returns success and specifies a size larger than the initial
16KB allocation (but smaller than 1GB), the size variable is updated to this
larger size. Later in this function, kmemdup() is called with this updated
size, which would read past the end of the 16KB buffer allocated by
qcom_tzmem_alloc(). Can this leak adjacent kernel heap data into the returned
resource table?

>  	if (PTR_ERR(output_rt_tzm) == -EOVERFLOW)
>  		/* Try again with the size requested by the TZ */
> -		output_rt_tzm = __qcom_scm_pas_get_rsc_table(ctx->pas_id,
> +		output_rt_tzm = __qcom_scm_pas_get_rsc_table(dev, ctx->pas_id,
>  							     input_rt_tzm,
>  							     input_rt_size,
>  							     &size);
[ ... ]
> @@ -2836,6 +2803,11 @@ static int qcom_scm_probe(struct platform_device *pdev)
>  
>  	__get_convention();
>  
> +	if (qcom_scm_is_pas_available()) {
> +		qcom_pas_ops_scm.dev = scm->dev;
> +		qcom_pas_ops_register(&qcom_pas_ops_scm);
> +	}
> +

[Severity: High]
Is there a race condition when accessing the global ops_ptr in the generic
PAS service?

Functions like qcom_pas_shutdown() check if (!ops_ptr) and then dereference
ops_ptr->shutdown. Since ops_ptr can be modified concurrently (for instance,
if the PAS TEE service is unbound via sysfs calling
qcom_pas_ops_unregister()), could a compiler reload or concurrent execution
lead to a NULL pointer dereference or use-after-free of the backend provider
data? There appears to be no module referencing or locking to prevent the PAS
provider from unbinding while in use.

>  	/*
>  	 * If "download mode" is requested, from this point on warmboot
>  	 * will cause the boot stages to enter download mode, unless

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