iommu/iova: Unsafe locking in find_iova()

RFC In review Dmitry Safonov 3 participants 9 messages Jun 21, 2018 – Jul 9, 2018

Subsystems (2)

IOMMU IOMMU DMA-API layer

lore

Share & tools

lore

Dmitry Safonov

author

2018-06-21 18:08 UTC #0 lore raw

cover iommu/iova: Unsafe locking in find_iova()

find_iova() looks to be using a bad locking practice: it locks the returned iova only for the search time.
And looking in code, the element can be removed from the tree and freed under rbtree lock. That happens during memory hot-unplug and cleanup on module removal.
Here I cleanup users of the function and delete it.

Dmitry Safonov (3):
iommu/iova: Find and split iova under rbtree's lock iommu/iova: Make free_iova() atomic iommu/iova: Remove find_iova()

 drivers/iommu/intel-iommu.c | 14 +++----------
 drivers/iommu/iova.c        | 48 +++++++++++++++++----------------------------
 include/linux/iova.h        | 17 ++++------------
 3 files changed, 25 insertions(+), 54 deletions(-)

Signature · 1 line

2.13.6

Dmitry Safonov

author

2018-06-21 18:08 UTC #1 lore raw

RFC 1/3 iommu/iova: Find and split iova under rbtree's lock

find_iova() holds iova_rbtree_lock only during the traversing rbtree. After the lock is released, returned iova may be freed (e.g., during module's release).
Hold the spinlock during search and removal of iova from the rbtree, eleminating possible use-after-free or/and double-free of iova.

Cc David Woodhouse <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc [email protected]

Cc Dmitry Safonov <[email protected]>

Signed-off-by Dmitry Safonov <[email protected]>

---
 drivers/iommu/intel-iommu.c | 14 +++-----------
 drivers/iommu/iova.c        | 19 ++++++++++++-------
 include/linux/iova.h        | 10 ++++------
 3 files changed, 19 insertions(+), 24 deletions(-)

drivers/iommu/intel-iommu.c +3 ~~-11~~

index 14e4b3722428..494394ef0f92 100644
@@ -4530,19 +4530,11 @@ static int intel_iommu_memory_notifier(struct notifier_block *nb,
 			struct intel_iommu *iommu;
 			struct page *freelist;
 
-			iova = find_iova(&si_domain->iovad, start_vpfn);
+			iova = iova_split_and_pop(&si_domain->iovad, start_vpfn, last_vpfn);
 			if (iova == NULL) {
-				pr_debug("Failed get IOVA for PFN %lx\n",
-					 start_vpfn);
-				break;
-			}
-
-			iova = split_and_remove_iova(&si_domain->iovad, iova,
-						     start_vpfn, last_vpfn);
-			if (iova == NULL) {
-				pr_warn("Failed to split IOVA PFN [%lx-%lx]\n",
+				pr_warn("Failed to split & pop IOVA PFN [%lx-%lx]\n",
 					start_vpfn, last_vpfn);
-				return NOTIFY_BAD;
+				break;
 			}
 
 			freelist = domain_unmap(si_domain, iova->pfn_lo,

drivers/iommu/iova.c +12 -7

index 83fe2621effe..4b38eb507670 100644
@@ -715,23 +715,27 @@ copy_reserved_iova(struct iova_domain *from, struct iova_domain *to)
 }
 EXPORT_SYMBOL_GPL(copy_reserved_iova);
 
-struct iova *
-split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
+struct iova *iova_split_and_pop(struct iova_domain *iovad,
 		      unsigned long pfn_lo, unsigned long pfn_hi)
 {
-	unsigned long flags;
 	struct iova *prev = NULL, *next = NULL;
+	unsigned long flags;
+	struct iova *iova;
 
 	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+	iova = private_find_iova(iovad, pfn_lo);
+	if (iova == NULL)
+		goto err_unlock;
+
 	if (iova->pfn_lo < pfn_lo) {
 		prev = alloc_and_init_iova(iova->pfn_lo, pfn_lo - 1);
 		if (prev == NULL)
-			goto error;
+			goto err_unlock;
 	}
 	if (iova->pfn_hi > pfn_hi) {
 		next = alloc_and_init_iova(pfn_hi + 1, iova->pfn_hi);
 		if (next == NULL)
-			goto error;
+			goto err_free;
 	}
 
 	__cached_rbnode_delete_update(iovad, iova);

@@ -749,10 +753,11 @@ split_and_remove_iova(struct iova_domain *iovad, struct iova *iova,
 
 	return iova;
 
-error:
-	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
+err_free:
 	if (prev)
 		free_iova_mem(prev);
+err_unlock:
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 	return NULL;
 }

include/linux/iova.h +4 -6

index 928442dda565..803472b77919 100644
@@ -160,8 +160,8 @@ int init_iova_flush_queue(struct iova_domain *iovad,
 			  iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
 struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
-struct iova *split_and_remove_iova(struct iova_domain *iovad,
-	struct iova *iova, unsigned long pfn_lo, unsigned long pfn_hi);
+struct iova *iova_split_and_pop(struct iova_domain *iovad,
+		unsigned long pfn_lo, unsigned long pfn_hi);
 void free_cpu_cached_iovas(unsigned int cpu, struct iova_domain *iovad);
 #else
 static inline int iova_cache_get(void)

@@ -253,10 +253,8 @@ static inline void put_iova_domain(struct iova_domain *iovad)
 {
 }
 
-static inline struct iova *split_and_remove_iova(struct iova_domain *iovad,
-						 struct iova *iova,
-						 unsigned long pfn_lo,
-						 unsigned long pfn_hi)
+static inline struct iova *iova_split_and_pop(struct iova_domain *iovad,
+		unsigned long pfn_lo, unsigned long pfn_hi)
 {
 	return NULL;
 }

Signature · 1 line

2.13.6

Dmitry Safonov

author

2018-06-21 18:08 UTC #2 lore raw

RFC 2/3 iommu/iova: Make free_iova() atomic

In reply to Dmitry Safonov

find_iova() grabs rbtree's spinlock only for the search time. Nothing guaranties that returned iova still exist for __free_iova(). Prevent potential use-after-free and double-free by holding the spinlock all the time iova is being searched and freed.

Cc David Woodhouse <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc [email protected]

Cc Dmitry Safonov <[email protected]>

Signed-off-by Dmitry Safonov <[email protected]>

---
 drivers/iommu/iova.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

drivers/iommu/iova.c +6 -3

index 4b38eb507670..4c63d92afaf7 100644
@@ -382,11 +382,14 @@ EXPORT_SYMBOL_GPL(__free_iova);
 void
 free_iova(struct iova_domain *iovad, unsigned long pfn)
 {
-	struct iova *iova = find_iova(iovad, pfn);
+	unsigned long flags;
+	struct iova *iova;
 
+	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
+	iova = private_find_iova(iovad, pfn);
 	if (iova)
-		__free_iova(iovad, iova);
-
+		private_free_iova(iovad, iova);
+	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
 }
 EXPORT_SYMBOL_GPL(free_iova);

Signature · 1 line

2.13.6

Dmitry Safonov

author

2018-06-21 18:08 UTC #3 lore raw

RFC 3/3 iommu/iova: Remove find_iova()

In reply to Dmitry Safonov

This function is potentially dangerous: nothing protects returned iova. As there is no user in tree anymore, delete it.

Cc David Woodhouse <[email protected]>

Cc Joerg Roedel <[email protected]>

Cc [email protected]

Cc Dmitry Safonov <[email protected]>

Signed-off-by Dmitry Safonov <[email protected]>

---
 drivers/iommu/iova.c | 20 --------------------
 include/linux/iova.h |  7 -------
 2 files changed, 27 deletions(-)

drivers/iommu/iova.c +0 ~~-20~~

index 4c63d92afaf7..4a568e28a633 100644
@@ -336,26 +336,6 @@ static void private_free_iova(struct iova_domain *iovad, struct iova *iova)
 }
 
 /**
- * find_iova - finds an iova for a given pfn
- * @iovad: - iova domain in question.
- * @pfn: - page frame number
- * This function finds and returns an iova belonging to the
- * given doamin which matches the given pfn.
- */
-struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn)
-{
-	unsigned long flags;
-	struct iova *iova;
-
-	/* Take the lock so that no other thread is manipulating the rbtree */
-	spin_lock_irqsave(&iovad->iova_rbtree_lock, flags);
-	iova = private_find_iova(iovad, pfn);
-	spin_unlock_irqrestore(&iovad->iova_rbtree_lock, flags);
-	return iova;
-}
-EXPORT_SYMBOL_GPL(find_iova);
-
-/**
  * __free_iova - frees the given iova
  * @iovad: iova domain in question.
  * @iova: iova in question.

include/linux/iova.h +0 -7

index 803472b77919..006911306a84 100644
@@ -158,7 +158,6 @@ void init_iova_domain(struct iova_domain *iovad, unsigned long granule,
 	unsigned long start_pfn);
 int init_iova_flush_queue(struct iova_domain *iovad,
 			  iova_flush_cb flush_cb, iova_entry_dtor entry_dtor);
-struct iova *find_iova(struct iova_domain *iovad, unsigned long pfn);
 void put_iova_domain(struct iova_domain *iovad);
 struct iova *iova_split_and_pop(struct iova_domain *iovad,
 		unsigned long pfn_lo, unsigned long pfn_hi);

@@ -243,12 +242,6 @@ static inline int init_iova_flush_queue(struct iova_domain *iovad,
 	return -ENODEV;
 }
 
-static inline struct iova *find_iova(struct iova_domain *iovad,
-				     unsigned long pfn)
-{
-	return NULL;
-}
-
 static inline void put_iova_domain(struct iova_domain *iovad)
 {
 }

Signature · 1 line

2.13.6

Dmitry Safonov

2018-07-03 18:59 UTC #4 lore raw

In reply to Dmitry Safonov

Hi,

Any opinion on that?
It looks to remove a source of possible issues and has a nice diffstat.

2018-06-21 19:08 GMT+01:00 Dmitry Safonov <[email protected]>:

Quoted 12 lines

find_iova() looks to be using a bad locking practice: it locks the returned iova only for the search time.
And looking in code, the element can be removed from the tree and freed under rbtree lock. That happens during memory hot-unplug and cleanup on module removal.
Here I cleanup users of the function and delete it.
Dmitry Safonov (3):
iommu/iova: Find and split iova under rbtree's lock iommu/iova: Make free_iova() atomic iommu/iova: Remove find_iova()

Quoted 5 lines

 drivers/iommu/intel-iommu.c | 14 +++----------
 drivers/iommu/iova.c        | 48 +++++++++++++++++----------------------------
 include/linux/iova.h        | 17 ++++------------
 3 files changed, 25 insertions(+), 54 deletions(-)

Thanks,
Dmitry

Joerg Roedel

maintainer

2018-07-06 13:16 UTC #5 lore raw

In reply to Dmitry Safonov

Quoted 5 lines · Dmitry Safonov

find_iova() looks to be using a bad locking practice: it locks the returned iova only for the search time. And looking in code, the element can be removed from the tree and freed under rbtree lock. That happens during memory hot-unplug and cleanup on module removal. Here I cleanup users of the function and delete it.

But this is only a problem if more than one code-path uses tries to handle a given iova at the same time, no?

Regards,

Joerg

Dmitry Safonov

author

2018-07-06 14:10 UTC #6 lore raw

Quoted 11 lines · Joerg Roedel

Quoted 7 lines · Dmitry Safonov
find_iova() looks to be using a bad locking practice: it locks the returned iova only for the search time. And looking in code, the element can be removed from the tree and freed under rbtree lock. That
happens during memory hot-unplug and cleanup on module removal. Here
I cleanup users of the function and delete it.
But this is only a problem if more than one code-path uses tries to handle a given iova at the same time, no?

Yes, as far as I can see, there are code-paths which may try to handle it at the same time:
o memory notifiers for hot-unplug (intel-iommu.c)
o drivers unloading calls free_iova(), which in the result calls find_iova()
o I see at least one driver that frees iova during it's normal work
too: scif_rma.c:scif_free_window_offset()

So, I decided to fix the interface while it's not widely used instead of all callers.
Looks worth for me even as it's all corner-cases like unplugging the memory.

Anyway, just found it while some college wrote a debug sysfs interface for iovas and used find_iova().
So, if you think it's not worth to change - that's fine for me, but I thought I'll nip this in the bud, preventing other people to misuse it.

Signature · 2 lines

Thanks,
             Dmitry

Joerg Roedel

maintainer

2018-07-06 15:13 UTC #7 lore raw

Quoted 7 lines · Dmitry Safonov

Yes, as far as I can see, there are code-paths which may try to handle it at the same time:
o memory notifiers for hot-unplug (intel-iommu.c)
o drivers unloading calls free_iova(), which in the result calls find_iova()
o I see at least one driver that frees iova during it's normal work
too: scif_rma.c:scif_free_window_offset()

Yeah, but the IOVAs freed in the memory notifiers are just the ones for the direct-mapped RMRR regions requested by firmware, not the IOVAs allocated by any driver, so I think this shouldn't be a problem.

Regards,

Joerg

Dmitry Safonov

author

2018-07-09 17:57 UTC #8 lore raw

Quoted 10 lines · Joerg Roedel

Quoted 8 lines · Dmitry Safonov
Yes, as far as I can see, there are code-paths which may try to handle
it at the same time:
o memory notifiers for hot-unplug (intel-iommu.c)
o drivers unloading calls free_iova(), which in the result calls find_iova()
o I see at least one driver that frees iova during it's normal work
too: scif_rma.c:scif_free_window_offset()

Quoted 4 lines

Yeah, but the IOVAs freed in the memory notifiers are just the ones for
the direct-mapped RMRR regions requested by firmware, not the IOVAs allocated by any driver, so I think this shouldn't be a problem.

Ok, than drop it, please.
I thought that safer API + nice diffstat can justify the change, but whatever %)

Signature · 2 lines

Thanks,
             Dmitry