[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [for-4.15][PATCH v2 4/5] xen/iommu: x86: Don't leak the IOMMU page-tables





On 10/02/2021 14:32, Jan Beulich wrote:
On 09.02.2021 16:28, Julien Grall wrote:
From: Julien Grall <jgrall@xxxxxxxxxx>

The new IOMMU page-tables allocator will release the pages when
relinquish the domain resources. However, this is not sufficient when
the domain is dying because nothing prevents page-table to be allocated.

iommu_alloc_pgtable() is now checking if the domain is dying before
adding the page in the list. We are relying on &hd->arch.pgtables.lock
to synchronize d->is_dying.

As said in reply to an earlier patch, I think suppressing
(really: ignoring) new mappings would be better.

This is exactly what I suggested in v1 but you wrote:

"Ignoring requests there seems fragile to me. Paul - what are your
thoughts about bailing early from hvm_add_ioreq_gfn() when the
domain is dying?"

Are you know saying that the following snipped would be fine:

if ( d->is_dying )
  return 0;

You could
utilize the same lock, but you'd need to duplicate the
checking in {amd,intel}_iommu_map_page().

I'm not entirely certain in the case about unmap requests:
It may be possible to also suppress/ignore them, but this
may require some further thought.

I think the unmap part is quite risky to d->is_dying because the PCI devices may not quiesced and still assigned to the domain.


Apart from this, just in case we settle on your current
approach, a few spelling nits:

--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -149,6 +149,13 @@ int arch_iommu_domain_init(struct domain *d)
void arch_iommu_domain_destroy(struct domain *d)
  {
+    /*
+     * There should be not page-tables left allocated by the time the

... should be no ...

+     * domain is destroyed. Note that arch_iommu_domain_destroy() is
+     * called unconditionally, so pgtables may be unitialized.

uninitialized

@@ -303,9 +317,29 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
      unmap_domain_page(p);
spin_lock(&hd->arch.pgtables.lock);
-    page_list_add(pg, &hd->arch.pgtables.list);
+    /*
+     * The IOMMU page-tables are freed when relinquishing the domain, but
+     * nothing prevent allocation to happen afterwards. There is no valid

prevents

+     * reasons to continue to update the IOMMU page-tables while the

reason

+     * domain is dying.
+     *
+     * So prevent page-table allocation when the domain is dying.
+     *
+     * We relying on &hd->arch.pgtables.lock to synchronize d->is_dying.

rely

Jan


--
Julien Grall



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.