Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: - [High] leak: devm_qcom_pas_context_alloc memory leak in recovery loop - [High] leak: TrustZone memory leak via missing qcom_pas_metadata_release --
commit 8e4c5b119cfde717dbade4e353783f865e47fb32 Author: Mukesh Ojha <[email protected]> wifi: ath12k: migrate firmware loading to qcom_mdt_pas_load() and qcom_pas_prepare_and_auth_reset() ath12k_ahb_power_up() and ath12k_ahb_load_auth_shared_fw() call qcom_mdt_load() for SCM-authenticated paths and then qcom_pas_auth_and_reset() which skips the shmbridge prepare step required before TrustZone authentication. > diff --git a/drivers/net/wireless/ath/ath12k/ahb.c > b/drivers/net/wireless/ath/ath12k/ahb.c > index 85360365aa4b2..6f0ad29a83f85 100644 > --- a/drivers/net/wireless/ath/ath12k/ahb.c > +++ b/drivers/net/wireless/ath/ath12k/ahb.c [ ... ] > @@ -424,14 +425,20 @@ 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) { > + ctx = devm_qcom_pas_context_alloc(dev, pasid, > + rproc_info->mem_phys, > + rproc_info->mem_size); [Severity: High] Is it safe to use a devm-managed allocation here? Since ath12k_ahb_power_up() is called dynamically during normal device operation, such as in firmware crash recovery loops, and the platform device does not unbind during this process, does this permanently leak a struct qcom_pas_context on every power-up cycle? > + if (IS_ERR(ctx)) { > + ret = PTR_ERR(ctx); > + goto err_fw; > + } > + ret = qcom_mdt_pas_load(ctx, fw, fw_name, NULL); [Severity: High] Does this code leak TrustZone memory? qcom_mdt_pas_load() allocates firmware metadata inside TrustZone. The caller is typically required to invoke qcom_pas_metadata_release() to free this secure memory after authentication is complete or on error. Without a call to qcom_pas_metadata_release() here or later in the function, is this secure memory permanently lost on every firmware load? > + } else { > ret = qcom_mdt_load_no_init(dev, fw, fw_name, > rproc_info->mem_region, > rproc_info->mem_phys, > rproc_info->mem_size, > NULL); > + } [ ... ] > @@ -1012,14 +1020,19 @@ 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); > + ctx = devm_qcom_pas_context_alloc(ab->dev, pasid, > + rproc_info->mem_phys, > + rproc_info->mem_size); > + if (IS_ERR(ctx)) > + return PTR_ERR(ctx); > + > + ret = qcom_mdt_pas_load(ctx, fw, fw_name, NULL); [Severity: High] Similar to the question above, is a call to qcom_pas_metadata_release() needed here to prevent leaking TrustZone memory after the shared firmware is loaded and authenticated? > if (ret) { > ath12k_err(ab, "failed to load RO firmware %s: %d\n", fw_name, > ret); > return ret; > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=6
