Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Lockless double-read of global ops_ptr causes TOCTOU NULL pointer dereference. Pre-existing issues: - [High] Repeated calls to devm_memremap() cause memory/vmap leaks during firmware recovery. - [High] Missing qcom_pas_shutdown() on error paths leaves remote processor running. -- commit e338489a518c55193a64126b6633cfb19d87e0ba Author: Sumit Garg <[email protected]> wifi: ath12k: Switch to generic PAS TZ APIs This commit switches the ath12k client driver over to generic PAS TZ APIs to support multiple TZ implementation backends. > diff --git a/drivers/net/wireless/ath/ath12k/ahb.c > b/drivers/net/wireless/ath/ath12k/ahb.c > index 30733a244454e..69e21214e6290 100644 > --- a/drivers/net/wireless/ath/ath12k/ahb.c > +++ b/drivers/net/wireless/ath/ath12k/ahb.c [ ... ] > @@ -420,7 +420,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_scm_pas_auth_and_reset(pasid); > + ret = qcom_pas_auth_and_reset(pasid); [Severity: High] This is a pre-existing issue, but does ath12k_ahb_power_up() leak memory during firmware recovery? Earlier in this function, devm_memremap() is used: mem_region = devm_memremap(dev, mem_phys, mem_size, MEMREMAP_WC); When the ath12k device undergoes a firmware crash recovery or reset, ath12k_core_reset() calls ath12k_hif_power_up(), which calls ath12k_ahb_power_up(). Because devm_memremap() allocates device-managed memory, and the device itself is not destroyed or unbound during a reset, the memory mapping is permanently leaked into the device's resource list. [Severity: High] Does calling this new API introduce a TOCTOU NULL pointer dereference? Looking at qcom_pas_auth_and_reset() in drivers/firmware/qcom/qcom_pas.c: int qcom_pas_auth_and_reset(u32 pas_id) { if (!ops_ptr) return -ENODEV; return ops_ptr->auth_and_reset(ops_ptr->dev, pas_id); } If a concurrent module unload of the PAS backend occurs, ops_ptr could be set to NULL between the check and the dereference. Even with READ_ONCE(), the lack of module reference counting allows the backend to be unloaded while its functions are executing. The same issue exists in qcom_pas_shutdown(). > if (ret) { > ath12k_err(ab, "failed to boot the remote processor > %d\n", ret); > goto err_fw2; [Severity: High] This is a pre-existing issue, but does this error path leave the remote processor running? If qcom_pas_auth_and_reset() succeeds, but a subsequent step like updating the SMEM state or waiting for completion fails, the code jumps to err_fw2 and returns an error without shutting down the PAS. This can lead to memory corruption if the driver unloads and the remote processor continues to DMA into freed kernel memory. [ ... ] > @@ -485,10 +485,10 @@ static void ath12k_ahb_power_down(struct ath12k_base > *ab, bool is_suspend) > pasid = (u32_encode_bits(ab_ahb->userpd_id, > ATH12K_USERPD_ID_MASK)) | > ATH12K_AHB_UPD_SWID; > /* Release the firmware */ > - ret = qcom_scm_pas_shutdown(pasid); > + ret = qcom_pas_shutdown(pasid); [Severity: High] This is also a pre-existing issue, but does ath12k_ahb_power_down() skip shutting down the PAS on timeout? If the wait for userpd_stopped times out earlier in this function: if (!time_left) { ath12k_err(ab, "UserPD stop wait timed out\n"); return; } The function returns early without invoking qcom_pas_shutdown(), which can also leave the remote processor running. > if (ret) > - ath12k_err(ab, "scm pas shutdown failed for userPD%d\n", > - ab_ahb->userpd_id); > + ath12k_err(ab, "PAS shutdown failed for userPD%d: %d\n", > + ab_ahb->userpd_id, ret); > } > } -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=12
