Skip to contentSkip to conversation

remoteproc: qcom_q6v5_mss: Leak metadata buffer if reclaim fails

PATCH Applied Yuho Choi 2 participants 2 messages Nov 5 – Nov 8

Subsystems (2)
lore
Share & tools
lore

Yuho Choi

author
#0 lore raw

PATCH 1/1 remoteproc: qcom_q6v5_mss: Leak metadata buffer if reclaim fails

When the device tree describes no "metadata" memory region, q6v5_mpss_init_image() allocates the MPSS metadata buffer with dma_alloc_attrs(), assigns it to the modem and asks the MBA to authenticate it. It then calls q6v5_xfer_mem_ownership() again to assign the buffer back to HLOS.

If that second call fails, the buffer is still assigned to the modem, but the function only warns and then returns the buffer to the DMA allocator. The pages can then be reused by another part of the kernel while the modem can still access them.

Leak the buffer in that case instead. The driver has no later point at which it could retry the reassignment. The MBA and MPSS regions and a "metadata" region from the device tree are never freed, so this is the only place where a buffer that failed to be reclaimed goes back to the allocator.

Fixes 6c5a9dc2481b ("remoteproc: qcom: Make secure world call for mem ownership switch")

---
This was found with a static analysis tool from ongoing, not yet published research, and confirmed by code inspection. Compile-tested only (arm64 defconfig, W=1); I have no Qualcomm board with a modem to test it on.

 drivers/remoteproc/qcom_q6v5_mss.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/remoteproc/qcom_q6v5_mss.c +4 -1
index 2f71ed2feff6..1a5afce4e6c1 100644
@@ -1128,9 +1128,12 @@ static int q6v5_mpss_init_image(struct q6v5 *qproc, const struct firmware *fw,
 	/* Metadata authentication done, remove modem access */
 	xferop_ret = q6v5_xfer_mem_ownership(qproc, &mdata_perm, true, false,
 					     phys, size);
-	if (xferop_ret)
+	if (xferop_ret) {
 		dev_warn(qproc->dev,
 			 "mdt buffer not reclaimed system may become unstable\n");
+		/* The modem may still have access to the buffer, so leak it */
+		goto free_metadata;
+	}
 
 free_dma_attrs:
 	if (!qproc->mdata_phys)

base-commit: 8396089f91fa07dd77b580e045334e670f782ff8

Signature · 1 line
2.43.0
maintainer Applied
#1 lore raw
Quoted 6 lines · Yuho Choi

When the device tree describes no "metadata" memory region, q6v5_mpss_init_image() allocates the MPSS metadata buffer with dma_alloc_attrs(), assigns it to the modem and asks the MBA to authenticate it. It then calls q6v5_xfer_mem_ownership() again to assign the buffer back to HLOS.

Quoted 6 lines

If that second call fails, the buffer is still assigned to the modem, but the function only warns and then returns the buffer to the DMA allocator. The pages can then be reused by another part of the kernel while the modem can still access them.

[...]

Applied, thanks!

[1/1] remoteproc: qcom_q6v5_mss: Leak metadata buffer if reclaim fails

      commit: 3f4f7571c001b04622be50c288c73ec8f320beca

Best regards,

Signature · 1 line
Bjorn Andersson <[email protected]>