[PATCH v1 0/2] iommu/riscv: Keep page table pages when IOTLB invalidation fails

[PATCH v1 0/2] iommu/riscv: Keep page table pages when IOTLB invalidation fails

[Yuho Choi]

riscv_iommu_iotlb_sync() frees the page table pages on gather->freelist
without knowing whether the invalidation that should have removed the
cached references to them was queued and completed. Patch 1 makes the
command helpers report a failure and leaks the freelist in that case.
Patch 2 fixes riscv_iommu_queue_wait() reporting success when the
command queue stopped on an error before the IOFENCE.C ran, so that
patch 1 also catches that case.

Neither failure is expected on working hardware, but when one happens,
freeing the pages lets the IOMMU walk memory that has a new owner.

The pages that the dma-iommu flush queue frees after flush_iotlb_all()
and the data pages released by iommu_unmap() callers are not covered,
as the IOMMU core does not let those callbacks report a failure.

The issues were found with a static analysis tool from ongoing, not yet
published research, and confirmed by code inspection. Compile-tested
only (ARCH=riscv defconfig with RISCV_IOMMU=y, W=1, each patch).

Yuho Choi (2):
  iommu/riscv: Keep page table pages when IOTLB invalidation fails
  iommu/riscv: Do not report a stalled command queue as completed

 drivers/iommu/riscv/iommu.c | 74 ++++++++++++++++++++++++++-----------
 1 file changed, 53 insertions(+), 21 deletions(-)


base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

[PATCH v1 1/2] iommu/riscv: Keep page table pages when IOTLB invalidation fails

[Yuho Choi]

riscv_iommu_iotlb_sync() frees the page table pages on gather->freelist
right after riscv_iommu_iotlb_inval(), which does not report whether the
invalidation commands could be queued or whether IOFENCE.C completed.
If either fails, the pages go back to the page allocator while the IOMMU
may still hold cached entries pointing to them, and whatever their next
owner writes there can then be walked as page table entries.

The kernel-doc of struct iommu_iotlb_gather requires these pages to be
freed only after the cached references are gone. Return an error from
the command helpers and riscv_iommu_iotlb_inval(), and leak the freelist
with a one-time warning when the invalidation was not confirmed. The
other callers of these helpers keep ignoring the result.

This does not cover the pages freed by the dma-iommu flush queue after
flush_iotlb_all(), nor the data pages released by iommu_unmap() callers,
as the IOMMU core does not let those callbacks report a failure.

Fixes: 488ffbf18171 ("iommu/riscv: Paging domain support")
Assisted-by: LLM
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
This was found with a static analysis tool from ongoing, not yet
published research, and confirmed by code inspection. It has not been
reproduced on hardware or in an emulator.

 drivers/iommu/riscv/iommu.c | 63 +++++++++++++++++++++++++------------
 1 file changed, 43 insertions(+), 20 deletions(-)

diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
index fe8e6d0f8a23..816d50c0f508 100644
--- a/drivers/iommu/riscv/iommu.c
+++ b/drivers/iommu/riscv/iommu.c
@@ -508,15 +508,15 @@ static irqreturn_t riscv_iommu_cmdq_process(int irq, void *data)
 }
 
 /* Send command to the IOMMU command queue */
-static void riscv_iommu_cmd_send(struct riscv_iommu_device *iommu,
-				 struct riscv_iommu_command *cmd)
+static int riscv_iommu_cmd_send(struct riscv_iommu_device *iommu,
+				struct riscv_iommu_command *cmd)
 {
-	riscv_iommu_queue_send(&iommu->cmdq, cmd, sizeof(*cmd), NULL);
+	return riscv_iommu_queue_send(&iommu->cmdq, cmd, sizeof(*cmd), NULL);
 }
 
 /* Send IOFENCE.C command and wait for all scheduled commands to complete. */
-static void riscv_iommu_cmd_sync(struct riscv_iommu_device *iommu,
-				 unsigned int timeout_us)
+static int riscv_iommu_cmd_sync(struct riscv_iommu_device *iommu,
+				unsigned int timeout_us)
 {
 	struct riscv_iommu_command cmd;
 	unsigned int prod;
@@ -525,14 +525,17 @@ static void riscv_iommu_cmd_sync(struct riscv_iommu_device *iommu,
 	riscv_iommu_cmd_iofence(&cmd);
 	ret = riscv_iommu_queue_send(&iommu->cmdq, &cmd, sizeof(cmd), &prod);
 	if (ret)
-		return;
+		return ret;
 
 	if (!timeout_us)
-		return;
+		return 0;
 
-	if (riscv_iommu_queue_wait(&iommu->cmdq, prod, timeout_us))
+	ret = riscv_iommu_queue_wait(&iommu->cmdq, prod, timeout_us);
+	if (ret)
 		dev_err_once(iommu->dev,
 			     "Hardware error: command execution timeout\n");
+
+	return ret;
 }
 
 /*
@@ -1002,14 +1005,15 @@ static void riscv_iommu_tlbi_calc(struct riscv_iommu_tlbi *tlbi,
 	}
 }
 
-static void riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
-					  int pscid,
-					  struct riscv_iommu_tlbi *tlbi)
+static int riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
+					 int pscid,
+					 struct riscv_iommu_tlbi *tlbi)
 {
 	bool use_nl = tlbi->non_leaf &&
 		      (iommu->caps & RISCV_IOMMU_CAPABILITIES_NL);
 	struct riscv_iommu_command cmd;
 	unsigned int i;
+	int err, ret = 0;
 
 	riscv_iommu_cmd_inval_vma(&cmd);
 	riscv_iommu_cmd_inval_set_pscid(&cmd, pscid);
@@ -1027,7 +1031,7 @@ static void riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
 						tlbi->range.sz_lg2);
 		if (use_nl)
 			riscv_iommu_cmd_inval_set_nl(&cmd);
-		riscv_iommu_cmd_send(iommu, &cmd);
+		ret = riscv_iommu_cmd_send(iommu, &cmd);
 	} else {
 		unsigned long iova;
 
@@ -1039,21 +1043,24 @@ static void riscv_iommu_iotlb_inval_iommu(struct riscv_iommu_device *iommu,
 			riscv_iommu_cmd_inval_set_addr(&cmd, iova);
 			if (use_nl)
 				riscv_iommu_cmd_inval_set_nl(&cmd);
-			riscv_iommu_cmd_send(iommu, &cmd);
+			err = riscv_iommu_cmd_send(iommu, &cmd);
+			if (err)
+				ret = err;
 			iova += 1ULL << tlbi->single.stride_lg2;
 		}
 	}
-	return;
+	return ret;
 global:
-	riscv_iommu_cmd_send(iommu, &cmd);
+	return riscv_iommu_cmd_send(iommu, &cmd);
 }
 
-static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
-				    struct iommu_iotlb_gather *gather)
+static int riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
+				   struct iommu_iotlb_gather *gather)
 {
 	struct riscv_iommu_device *iommu, *prev;
 	struct riscv_iommu_bond *bond;
 	struct riscv_iommu_tlbi tlbi;
+	int err, ret = 0;
 
 	riscv_iommu_tlbi_calc(&tlbi, gather);
 
@@ -1096,7 +1103,9 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
 		if (iommu == prev)
 			continue;
 
-		riscv_iommu_iotlb_inval_iommu(iommu, domain->pscid, &tlbi);
+		err = riscv_iommu_iotlb_inval_iommu(iommu, domain->pscid, &tlbi);
+		if (err)
+			ret = err;
 		prev = iommu;
 	}
 
@@ -1106,10 +1115,14 @@ static void riscv_iommu_iotlb_inval(struct riscv_iommu_domain *domain,
 		if (iommu == prev)
 			continue;
 
-		riscv_iommu_cmd_sync(iommu, RISCV_IOMMU_IOTINVAL_TIMEOUT);
+		err = riscv_iommu_cmd_sync(iommu, RISCV_IOMMU_IOTINVAL_TIMEOUT);
+		if (err)
+			ret = err;
 		prev = iommu;
 	}
 	rcu_read_unlock();
+
+	return ret;
 }
 
 #define RISCV_IOMMU_FSC_BARE 0
@@ -1270,7 +1283,17 @@ static void riscv_iommu_iotlb_sync(struct iommu_domain *iommu_domain,
 {
 	struct riscv_iommu_domain *domain = iommu_domain_to_riscv(iommu_domain);
 
-	riscv_iommu_iotlb_inval(domain, gather);
+	/*
+	 * The IOMMU may keep walking the page table pages on the freelist
+	 * until the invalidation has completed. If that is not confirmed,
+	 * leak them instead of handing them back to the page allocator.
+	 */
+	if (riscv_iommu_iotlb_inval(domain, gather)) {
+		if (!iommu_pages_list_empty(&gather->freelist))
+			pr_warn_once("IOTLB invalidation failed, leaking page table pages\n");
+		return;
+	}
+
 	iommu_put_pages_list(&gather->freelist);
 }
 
-- 
2.43.0

[PATCH v1 2/2] iommu/riscv: Do not report a stalled command queue as completed

[Yuho Choi]

Since commit b2e5684558ed ("iommu/riscv: Stop polling when CQCSR reports
an error"), riscv_iommu_queue_wait() stops polling when CQCSR has cqmf,
cmd_to or cmd_ill set, but then returns 0 as if the waited-for command
had been executed. The hardware stops processing the queue at the
faulting command, so an IOFENCE.C queued behind it has not completed,
yet riscv_iommu_iotlb_sync() goes on to free the page table pages.

Return -EIO when the queue stopped before reaching the waited-for entry,
and keep the "command execution timeout" message for real timeouts; the
interrupt handler already reports the CQCSR error bits.

Fixes: b2e5684558ed ("iommu/riscv: Stop polling when CQCSR reports an error")
Assisted-by: LLM
Signed-off-by: Yuho Choi <oss.patchbox@gmail.com>
---
This was found with a static analysis tool from ongoing, not yet
published research, and confirmed by code inspection. It has not been
reproduced on hardware or in an emulator.

 drivers/iommu/riscv/iommu.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/iommu/riscv/iommu.c b/drivers/iommu/riscv/iommu.c
index 816d50c0f508..2a95cb8c9e4c 100644
--- a/drivers/iommu/riscv/iommu.c
+++ b/drivers/iommu/riscv/iommu.c
@@ -371,15 +371,24 @@ static int riscv_iommu_queue_wait(struct riscv_iommu_queue *queue,
 	unsigned int cons = atomic_read(&queue->head);
 	unsigned int flags = RISCV_IOMMU_CQCSR_CQMF | RISCV_IOMMU_CQCSR_CMD_TO |
 			     RISCV_IOMMU_CQCSR_CMD_ILL;
+	int ret;
 
 	/* Already processed by the consumer */
 	if ((int)(cons - index) > 0)
 		return 0;
 
 	/* Monitor consumer index */
-	return readx_poll_timeout(riscv_iommu_queue_cons, queue, cons,
+	ret = readx_poll_timeout(riscv_iommu_queue_cons, queue, cons,
 				 (riscv_iommu_readl(queue->iommu, queue->qcr) & flags) ||
 				 (int)(cons - index) > 0, 0, timeout_us);
+	if (ret)
+		return ret;
+
+	/* The queue stopped on an error before it reached the item. */
+	if ((int)(cons - index) <= 0)
+		return -EIO;
+
+	return 0;
 }
 
 static int riscv_iommu_queue_wait_for_space(struct riscv_iommu_queue *queue,
@@ -531,7 +540,7 @@ static int riscv_iommu_cmd_sync(struct riscv_iommu_device *iommu,
 		return 0;
 
 	ret = riscv_iommu_queue_wait(&iommu->cmdq, prod, timeout_us);
-	if (ret)
+	if (ret == -ETIMEDOUT)
 		dev_err_once(iommu->dev,
 			     "Hardware error: command execution timeout\n");
 
-- 
2.43.0