Re: [PATCH v2 7/9] wifi: ath12k: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] wifi: ath12k: PAS metadata leak in ath12k_ahb_power_up() and
  shared FW load
--

commit 7ea070a02821a21185ed124f14f936a8b5576395
Author: Mukesh Ojha <mukesh.ojha@oss.qualcomm.com>

wifi: ath12k: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset()

This commit replaces qcom_mdt_load() and qcom_pas_auth_and_reset() in
ath12k_ahb_power_up() and ath12k_ahb_load_auth_shared_fw() with
qcom_pas_context_init(), qcom_mdt_pas_load(), and
qcom_pas_prepare_and_auth_reset().

> diff --git a/drivers/net/wireless/ath/ath12k/ahb.c b/drivers/net/wireless/ath/ath12k/ahb.c
> index 2c55aae06dd06..355f1f5509975 100644
> --- a/drivers/net/wireless/ath/ath12k/ahb.c
> +++ b/drivers/net/wireless/ath/ath12k/ahb.c
> @@ -411,6 +411,7 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  	struct ath12k_ahb_rproc_info *rproc_info = ab_ahb->rproc_info;
>  	char fw_name[ATH12K_USERPD_FW_NAME_LEN];
>  	char fw2_name[ATH12K_USERPD_FW_NAME_LEN];
> +	struct qcom_pas_context ctx;
>  	struct device *dev = ab->dev;
>  	const struct firmware *fw, *fw2;
>  	unsigned long time_left;
> @@ -444,14 +445,15 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  		ATH12K_AHB_UPD_SWID;
>  
>  	/* Load FW image to a reserved memory location */
> -	if (ab_ahb->scm_auth_enabled)
> -		ret = qcom_mdt_load(dev, fw, fw_name, pasid, rproc_info->mem_region,
> -				    rproc_info->mem_phys, rproc_info->mem_size,
> -				    NULL);
> -	else
> +	if (ab_ahb->scm_auth_enabled) {
> +		qcom_pas_context_init(&ctx, dev, pasid,
> +				      rproc_info->mem_phys, rproc_info->mem_size);
> +		ret = qcom_mdt_pas_load(&ctx, fw, fw_name, NULL);
> +	} else {
>  		ret = qcom_mdt_load_no_init(dev, fw, fw_name, rproc_info->mem_region,
>  					    rproc_info->mem_phys, rproc_info->mem_size,
>  					    NULL);
> +	}
>  	if (ret) {
>  		ath12k_err(ab, "Failed to load MDT segments: %d\n", ret);
>  		goto err_fw;
> @@ -485,7 +487,7 @@ static int ath12k_ahb_power_up(struct ath12k_base *ab)
>  
>  	if (ab_ahb->scm_auth_enabled) {
>  		/* Authenticate FW image using peripheral ID */
> -		ret = qcom_pas_auth_and_reset(pasid);
> +		ret = qcom_pas_prepare_and_auth_reset(&ctx);
>  		if (ret) {
>  			ath12k_err(ab, "failed to boot the remote processor %d\n", ret);
>  			goto err_fw2;
> @@ -1023,6 +1025,7 @@ static int ath12k_ahb_load_auth_shared_fw(struct ath12k_base *ab,
>  					  struct ath12k_ahb_rproc_info *rproc_info,
>  					  const char *fw_name, u32 pasid)
>  {
> +	struct qcom_pas_context ctx;
>  	int ret;
>  
>  	const struct firmware *fw __free(firmware) = NULL;
> @@ -1041,14 +1044,16 @@ static int ath12k_ahb_load_auth_shared_fw(struct ath12k_base *ab,
>  	ath12k_dbg(ab, ATH12K_DBG_AHB, "loading firmware %s, size %zd\n", fw_name,
>  		   fw->size);
>  
> -	ret = qcom_mdt_load(ab->dev, fw, fw_name, pasid, rproc_info->mem_region,
> -			    rproc_info->mem_phys, rproc_info->mem_size, NULL);
> +	qcom_pas_context_init(&ctx, ab->dev, pasid,
> +			      rproc_info->mem_phys, rproc_info->mem_size);
> +
> +	ret = qcom_mdt_pas_load(&ctx, fw, fw_name, NULL);
>  	if (ret) {
>  		ath12k_err(ab, "failed to load RO firmware %s: %d\n", fw_name, ret);
>  		return ret;
>  	}
>  
> -	ret = qcom_pas_auth_and_reset(pasid);
> +	ret = qcom_pas_prepare_and_auth_reset(&ctx);
>  	if (ret)
>  		ath12k_err(ab, "failed to authenticate and boot shared firmware: %d\n",
>  			   ret);

[Severity: High]
Does this leak the PAS metadata buffer stored in ctx.ptr in both
ath12k_ahb_power_up() and ath12k_ahb_load_auth_shared_fw()?

Previously, qcom_mdt_load() passed a NULL context pointer to
__qcom_mdt_pas_init() -> qcom_pas_init_image(), so the underlying backend
freed the metadata allocation immediately.  qcom_mdt_pas_load() instead
passes &ctx, which retains the allocated metadata buffer in ctx.ptr until
the caller releases it with qcom_pas_metadata_release(&ctx):

drivers/firmware/qcom/qcom_scm.c:qcom_scm_pas_init_image() {
	...
	if (ret < 0 || !ctx) {
		dma_free_coherent(dev, size, mdata_buf, mdata_phys);
	} else if (ctx) {
		ctx->ptr = mdata_buf;
		ctx->phys = mdata_phys;
		ctx->size = size;
	}
	...
}

Because neither function calls qcom_pas_metadata_release(&ctx) after
qcom_pas_prepare_and_auth_reset(&ctx) or on the error paths after
qcom_mdt_pas_load() (including if qcom_mdt_pas_load() fails after
qcom_pas_init_image() succeeds), the stack-allocated ctx goes out of scope
and leaks the metadata buffer on every firmware load.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113185009.157893-1-mukesh.ojha@oss.qualcomm.com?part=7