[PATCH 0/2] iommu/vt-d: Fix SVA teardown races with process exit

[PATCH 0/2] iommu/vt-d: Fix SVA teardown races with process exit

[Lu Baolu]

When a process using SVA exits while a device still has DMA and page
requests in flight for its PASID, the Intel IOMMU driver can mishandle
the teardown of that PASID. This series aims to fix two such issues.

The first is a use-after-free in the iopf handling path. After the
process exits, the later SVA unbind no longer drains the page request
queue for the PASID before the SVA attach handle is freed. Page
requests queued before the exit are then handled by
iommu_sva_handle_iopf() on the freed handle.

The second is a race between process exit and an SVA unbind of the
PASID. The mm release callback can return before the unbind completes
invalidating the PASID cache, IOTLB and device TLB. The exiting process
may then free its page tables while the IOMMU and the device still hold
translations that reference them.

Please review and comments.

Lu Baolu (2):
  iommu/vt-d: Set FPD when tearing down a PASID entry with fault_ignore
  iommu/vt-d: Serialize SVA unbind teardown with intel_mm_release()

 drivers/iommu/intel/pasid.h | 11 +++++++++++
 drivers/iommu/intel/iommu.c | 10 ++++++++++
 drivers/iommu/intel/pasid.c | 30 +++++++++++-------------------
 3 files changed, 32 insertions(+), 19 deletions(-)

-- 
2.43.0

[PATCH 1/2] iommu/vt-d: Set FPD when tearing down a PASID entry with fault_ignore

[Lu Baolu]

When an SVA process exits, intel_mm_release() tears down its PASID entry
with fault_ignore=true. It does not drain the PRQ at that point. Instead,
it is supposed to leave the FPD bit set. Later, when the device is
unbound, the driver checks for this bit and drains the PRQ.

Commit 75ed00055c05 ("iommu/vt-d: Clear Present bit before tearing down
PASID entry") changed the teardown order. It clears the Present bit
before calling intel_pasid_clear_entry(), which sets the FPD bit only
if the entry is still present. As a result, FPD is not set and the
entire entry is cleared, so the PRQ is not drained for that PASID
during unbind:

  intel_mm_release()
    intel_pasid_tear_down_entry(fault_ignore=true)
      pasid_clear_present()
      intel_pasid_clear_entry()       <- P=0, so FPD is not set
  ...
  iommu_sva_unbind_device()
    iommu_detach_device_pasid()
      blocking_domain_set_dev_pasid()
        intel_pasid_tear_down_entry(fault_ignore=false)
          !present && !FPD            <- return, no PRQ drain
        intel_iommu_disable_iopf()
          iopf_queue_remove_device()  <- groups still queued
  kfree(handle)                       <- handle still used by iopf work

Page requests queued before process exit may later be handled by
iommu_sva_handle_iopf() after the SVA attach handle has been freed,
causing a use-after-free. Fix this by setting FPD in the same write
that clears the Present bit when fault_ignore is requested.

Fixes: 75ed00055c05 ("iommu/vt-d: Clear Present bit before tearing down PASID entry")
Cc: stable@vger.kernel.org
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/pasid.h | 11 +++++++++++
 drivers/iommu/intel/pasid.c | 28 ++++++++++------------------
 2 files changed, 21 insertions(+), 18 deletions(-)

diff --git a/drivers/iommu/intel/pasid.h b/drivers/iommu/intel/pasid.h
index 48d3bb6b68de..af798a6ee121 100644
--- a/drivers/iommu/intel/pasid.h
+++ b/drivers/iommu/intel/pasid.h
@@ -251,6 +251,17 @@ static inline void pasid_clear_present(struct pasid_entry *pe)
 	dma_wmb();
 }
 
+/*
+ * Clear the Present bit and set the FPD bit in a single write, so that
+ * hardware never sees the entry non-present without FPD.
+ */
+static inline void pasid_clear_present_set_fpd(struct pasid_entry *pe)
+{
+	pasid_set_bits(&pe->val[0], PASID_PTE_PRESENT | PASID_PTE_FPD,
+		       PASID_PTE_FPD);
+	dma_wmb();
+}
+
 /*
  * Setup Page Walk Snoop bit (Bit 87) of a scalable mode PASID
  * entry.
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index e4f24d3f19a6..d1c08d180788 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -178,21 +178,6 @@ static struct pasid_entry *intel_pasid_get_entry(struct device *dev, u32 pasid)
 /*
  * Interfaces for PASID table entry manipulation:
  */
-static void
-intel_pasid_clear_entry(struct device *dev, u32 pasid, bool fault_ignore)
-{
-	struct pasid_entry *pe;
-
-	pe = intel_pasid_get_entry(dev, pasid);
-	if (WARN_ON(!pe))
-		return;
-
-	if (fault_ignore && pasid_pte_is_present(pe))
-		pasid_clear_entry_with_fpd(pe);
-	else
-		pasid_clear_entry(pe);
-}
-
 static void
 pasid_cache_invalidation_with_pasid(struct intel_iommu *iommu,
 				    u16 did, u32 pasid)
@@ -253,7 +238,6 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 
 	if (!pasid_pte_is_present(pte)) {
 		if (!pasid_pte_is_fault_disabled(pte)) {
-			WARN_ON(READ_ONCE(pte->val[0]) != 0);
 			spin_unlock(&iommu->lock);
 			return;
 		}
@@ -266,6 +250,8 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 		 */
 		pasid_clear_entry(pte);
 		spin_unlock(&iommu->lock);
+		if (!ecap_coherent(iommu->ecap))
+			clflush_cache_range(pte, sizeof(*pte));
 		intel_iommu_drain_pasid_prq(dev, pasid);
 
 		return;
@@ -273,7 +259,10 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 
 	did = pasid_get_domain_id(pte);
 	pgtt = pasid_pte_get_pgtt(pte);
-	pasid_clear_present(pte);
+	if (fault_ignore)
+		pasid_clear_present_set_fpd(pte);
+	else
+		pasid_clear_present(pte);
 	spin_unlock(&iommu->lock);
 
 	if (!ecap_coherent(iommu->ecap))
@@ -287,7 +276,10 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 		iommu->flush.flush_iotlb(iommu, did, 0, 0, DMA_TLB_DSI_FLUSH);
 
 	devtlb_invalidation_with_pasid(iommu, dev, pasid);
-	intel_pasid_clear_entry(dev, pasid, fault_ignore);
+	if (fault_ignore)
+		pasid_clear_entry_with_fpd(pte);
+	else
+		pasid_clear_entry(pte);
 	if (!ecap_coherent(iommu->ecap))
 		clflush_cache_range(pte, sizeof(*pte));
 
-- 
2.43.0

[PATCH 2/2] iommu/vt-d: Serialize SVA unbind teardown with intel_mm_release()

[Lu Baolu]

intel_mm_release() can run concurrently with an unbind of the same PASID.
The unbind clears the Present bit, drops iommu->lock and then invalidates
the PASID cache, IOTLB and device TLB:

  iommu_sva_unbind_device()              exit_mmap()
    blocking_domain_set_dev_pasid()        intel_mm_release()
      intel_pasid_tear_down_entry()
        pasid_clear_present()
        spin_unlock(&iommu->lock)
                                             intel_pasid_tear_down_entry()
                                               !present && !FPD -> return
                                           ... free page tables ...
        ... invalidate caches ...

intel_mm_release() then returns before the invalidations complete, and
exit_mmap() can free the page tables while the IOMMU and the device still
hold translations that reference them.

intel_mm_release() walks domain->dev_pasids and tears down each entry
under domain->lock. Do the same in the blocking attach for an SVA domain:
tear down the entry with fault_ignore under domain->lock, so that either
intel_mm_release() completes the invalidations first, or it waits for the
unbind to complete them.

Fixes: 4f0bdab175d6 ("iommu/vt-d: Make the blocked domain support PASID")
Signed-off-by: Lu Baolu <baolu.lu@linux.intel.com>
---
 drivers/iommu/intel/iommu.c | 10 ++++++++++
 drivers/iommu/intel/pasid.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/drivers/iommu/intel/iommu.c b/drivers/iommu/intel/iommu.c
index 5a2e705a2f2e..02d2c97da86b 100644
--- a/drivers/iommu/intel/iommu.c
+++ b/drivers/iommu/intel/iommu.c
@@ -3699,6 +3699,16 @@ static int blocking_domain_set_dev_pasid(struct iommu_domain *domain,
 {
 	struct device_domain_info *info = dev_iommu_priv_get(dev);
 
+	if (old && old->type == IOMMU_DOMAIN_SVA) {
+		struct dmar_domain *dmar_domain = to_dmar_domain(old);
+		unsigned long flags;
+
+		/* intel_mm_release() must not return before invalidation. */
+		spin_lock_irqsave(&dmar_domain->lock, flags);
+		intel_pasid_tear_down_entry(info->iommu, dev, pasid, true);
+		spin_unlock_irqrestore(&dmar_domain->lock, flags);
+	}
+
 	intel_pasid_tear_down_entry(info->iommu, dev, pasid, false);
 	iopf_for_domain_remove(old, dev);
 	domain_remove_dev_pasid(old, dev, pasid);
diff --git a/drivers/iommu/intel/pasid.c b/drivers/iommu/intel/pasid.c
index d1c08d180788..00f9b0722eb5 100644
--- a/drivers/iommu/intel/pasid.c
+++ b/drivers/iommu/intel/pasid.c
@@ -237,7 +237,7 @@ void intel_pasid_tear_down_entry(struct intel_iommu *iommu, struct device *dev,
 	}
 
 	if (!pasid_pte_is_present(pte)) {
-		if (!pasid_pte_is_fault_disabled(pte)) {
+		if (!pasid_pte_is_fault_disabled(pte) || fault_ignore) {
 			spin_unlock(&iommu->lock);
 			return;
 		}
-- 
2.43.0