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

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





On 23/12/2020 14:34, Jan Beulich wrote:
On 22.12.2020 16:43, 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 in two
cases:
     1) domain_relinquish_resources() is not called when the domain
     creation fails.

Could you remind me of what IOMMU page table insertions there
are during domain creation? No memory got allocated to the
domain at that point yet, so it would seem to me there simply
is nothing to map.

The P2M is first modified in hvm_domain_initialise():

(XEN) Xen call trace:
(XEN)    [<ffff82d04026b9ec>] R iommu_alloc_pgtable+0x11/0x137
(XEN) [<ffff82d04025f9f5>] F drivers/passthrough/vtd/iommu.c#addr_to_dma_page_maddr+0x146/0x1d8 (XEN) [<ffff82d04025fcc5>] F drivers/passthrough/vtd/iommu.c#intel_iommu_map_page+0x6a/0x14b
(XEN)    [<ffff82d04026d949>] F iommu_map+0x6d/0x16f
(XEN)    [<ffff82d04026da71>] F iommu_legacy_map+0x26/0x63
(XEN) [<ffff82d040301bdc>] F arch/x86/mm/p2m-ept.c#ept_set_entry+0x6b2/0x730
(XEN)    [<ffff82d0402f67e7>] F p2m_set_entry+0x91/0x128
(XEN) [<ffff82d0402f6b5c>] F arch/x86/mm/p2m.c#set_typed_p2m_entry+0xfe/0x3f7
(XEN)    [<ffff82d0402f7f4c>] F set_mmio_p2m_entry+0x65/0x6e
(XEN) [<ffff82d04029a080>] F arch/x86/hvm/vmx/vmx.c#vmx_domain_initialise+0xf6/0x137
(XEN)    [<ffff82d0402af421>] F hvm_domain_initialise+0x357/0x4c7
(XEN)    [<ffff82d04031eae7>] F arch_domain_create+0x478/0x4ff
(XEN)    [<ffff82d04020476e>] F domain_create+0x4f2/0x778
(XEN)    [<ffff82d04023b0d2>] F do_domctl+0xb1e/0x18b8
(XEN)    [<ffff82d040311dbf>] F pv_hypercall+0x2f0/0x55f
(XEN)    [<ffff82d040390432>] F lstar_enter+0x112/0x120


     2) There is nothing preventing page-table allocations when the
     domain is dying.

In both cases, this can be solved by freeing the page-tables again
when the domain destruction. Although, this may result to an high
number of page-tables to free.

Since I've seen this before in this series, and despite me also
not being a native speaker, as a nit: I don't think it can
typically be other than "result in".

I think you are right.


--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2290,7 +2290,7 @@ int domain_relinquish_resources(struct domain *d)
PROGRESS(iommu_pagetables): - ret = iommu_free_pgtables(d);
+        ret = iommu_free_pgtables(d, false);

I suppose you mean "true" here, but I also think the other
approach (checking for DOMDYING_dead, which you don't seem to
like very much) is better, if for no other reason than it
already being used elsewhere.

I think "don't like very much" is an understatement :). There seems to be more function using an extra parameter (such as hap_set_allocation() which was introduced before your DOMDYING_dead). So I only followed what they did.


@@ -305,6 +320,19 @@ struct page_info *iommu_alloc_pgtable(struct domain *d)
          memflags = MEMF_node(hd->node);
  #endif
+ /*
+     * The IOMMU page-tables are freed when relinquishing the domain, but
+     * nothing prevent allocation to happen afterwards. There is no valid
+     * reasons to continue to update the IOMMU page-tables while the
+     * domain is dying.
+     *
+     * So prevent page-table allocation when the domain is dying. Note
+     * this doesn't fully prevent the race because d->is_dying may not
+     * yet be seen.
+     */
+    if ( d->is_dying )
+        return NULL;
+
      pg = alloc_domheap_page(NULL, memflags);
      if ( !pg )
          return NULL;

As said in reply to an earlier patch - with a suitable
spin_barrier() you can place your check further down, along the
lines of

     spin_lock(&hd->arch.pgtables.lock);
     if ( likely(!d->is_dying) )
     {
         page_list_add(pg, &hd->arch.pgtables.list);
         p = NULL;
     }
     spin_unlock(&hd->arch.pgtables.lock);

     if ( p )
     {
         free_domheap_page(pg);
         pg = NULL;
     }

(albeit I'm relatively sure you won't like the re-purposing of
p, but that's a minor detail). (FREE_DOMHEAP_PAGE() would be
nice to use here, but we seem to only have FREE_XENHEAP_PAGE()
so far.)

In fact I don't mind the re-purposing of p. However, I dislike the allocation and then freeing when the domain is dying.

I think I prefer the small race introduced (the pages will still be freed) over this solution.

Note that Paul's IOMMU series will completely rework the function. So this is only temporary.

Cheers,

--
Julien Grall



 


Rackspace

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