| Message ID | 20210224094356.7606-3-julien@xen.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | xen/iommu: Collection of bug fixes for IOMMU teardown | expand |
On 24.02.2021 16:58, Jan Beulich wrote: > On 24.02.2021 10:43, Julien Grall wrote: >> --- 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 >> + * domain is destroyed. Note that arch_iommu_domain_destroy() is >> + * called unconditionally, so pgtables may be unitialized. >> + */ >> + ASSERT(dom_iommu(d)->platform_ops == NULL || > > Since you've used the preferred ! instead of "== 0" / > "== NULL" in the ASSERT()s you add further up, may I ask that > you do so here as well? Oh, and I meant to provide Reviewed-by: Jan Beulich <jbeulich@suse.com> preferably with that cosmetic adjustment (and ideally also with "uninitialized" in the comment, as I notice only now). Jan
Hi Jan, On 24/02/2021 16:00, Jan Beulich wrote: > On 24.02.2021 16:58, Jan Beulich wrote: >> On 24.02.2021 10:43, Julien Grall wrote: >>> --- 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 >>> + * domain is destroyed. Note that arch_iommu_domain_destroy() is >>> + * called unconditionally, so pgtables may be unitialized. >>> + */ >>> + ASSERT(dom_iommu(d)->platform_ops == NULL || >> >> Since you've used the preferred ! instead of "== 0" / >> "== NULL" in the ASSERT()s you add further up, may I ask that >> you do so here as well? > > Oh, and I meant to provide > Reviewed-by: Jan Beulich <jbeulich@suse.com> > preferably with that cosmetic adjustment (and ideally also with > "uninitialized" in the comment, as I notice only now). I don't seem to find your original answer in my inbox and on lore [1]. Could you confirm if the two comments visible in this thread are the only one you made on this patch? Thanks for the review! Cheers, [1] https://lore.kernel.org/xen-devel/20210224094356.7606-3-julien@xen.org/ > > Jan >
On 25.02.2021 13:01, Julien Grall wrote: > On 24/02/2021 16:00, Jan Beulich wrote: >> On 24.02.2021 16:58, Jan Beulich wrote: >>> On 24.02.2021 10:43, Julien Grall wrote: >>>> --- 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 >>>> + * domain is destroyed. Note that arch_iommu_domain_destroy() is >>>> + * called unconditionally, so pgtables may be unitialized. >>>> + */ >>>> + ASSERT(dom_iommu(d)->platform_ops == NULL || >>> >>> Since you've used the preferred ! instead of "== 0" / >>> "== NULL" in the ASSERT()s you add further up, may I ask that >>> you do so here as well? >> >> Oh, and I meant to provide >> Reviewed-by: Jan Beulich <jbeulich@suse.com> >> preferably with that cosmetic adjustment (and ideally also with >> "uninitialized" in the comment, as I notice only now). > > I don't seem to find your original answer in my inbox and on lore [1]. > Could you confirm if the two comments visible in this thread are the > only one you made on this patch? Oh, yes - what I look to have done is to reply to a draft that was never sent. There indeed was just what's visible above - thanks for double checking. Jan
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 42b5a5a9bec4..085fe2f5771e 100644 --- a/xen/drivers/passthrough/amd/pci_amd_iommu.c +++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c @@ -381,9 +381,18 @@ static int amd_iommu_assign_device(struct domain *d, u8 devfn, return reassign_device(pdev->domain, d, devfn, pdev); } +static void amd_iommu_clear_root_pgtable(struct domain *d) +{ + struct domain_iommu *hd = dom_iommu(d); + + spin_lock(&hd->arch.mapping_lock); + hd->arch.amd.root_table = NULL; + spin_unlock(&hd->arch.mapping_lock); +} + static void amd_iommu_domain_destroy(struct domain *d) { - dom_iommu(d)->arch.amd.root_table = NULL; + ASSERT(!dom_iommu(d)->arch.amd.root_table); } static int amd_iommu_add_device(u8 devfn, struct pci_dev *pdev) @@ -565,6 +574,7 @@ static const struct iommu_ops __initconstrel _iommu_ops = { .remove_device = amd_iommu_remove_device, .assign_device = amd_iommu_assign_device, .teardown = amd_iommu_domain_destroy, + .clear_root_pgtable = amd_iommu_clear_root_pgtable, .map_page = amd_iommu_map_page, .unmap_page = amd_iommu_unmap_page, .iotlb_flush = amd_iommu_flush_iotlb_pages, diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c index b549a71530d5..475efb3be3bd 100644 --- a/xen/drivers/passthrough/vtd/iommu.c +++ b/xen/drivers/passthrough/vtd/iommu.c @@ -1726,6 +1726,15 @@ out: return ret; } +static void iommu_clear_root_pgtable(struct domain *d) +{ + struct domain_iommu *hd = dom_iommu(d); + + spin_lock(&hd->arch.mapping_lock); + hd->arch.vtd.pgd_maddr = 0; + spin_unlock(&hd->arch.mapping_lock); +} + static void iommu_domain_teardown(struct domain *d) { struct domain_iommu *hd = dom_iommu(d); @@ -1740,7 +1749,7 @@ static void iommu_domain_teardown(struct domain *d) xfree(mrmrr); } - hd->arch.vtd.pgd_maddr = 0; + ASSERT(!hd->arch.vtd.pgd_maddr); } static int __must_check intel_iommu_map_page(struct domain *d, dfn_t dfn, @@ -2731,6 +2740,7 @@ static struct iommu_ops __initdata vtd_ops = { .remove_device = intel_iommu_remove_device, .assign_device = intel_iommu_assign_device, .teardown = iommu_domain_teardown, + .clear_root_pgtable = iommu_clear_root_pgtable, .map_page = intel_iommu_map_page, .unmap_page = intel_iommu_unmap_page, .lookup_page = intel_iommu_lookup_page, diff --git a/xen/drivers/passthrough/x86/iommu.c b/xen/drivers/passthrough/x86/iommu.c index c6b03624fe28..faeb549591d8 100644 --- 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 + * domain is destroyed. Note that arch_iommu_domain_destroy() is + * called unconditionally, so pgtables may be unitialized. + */ + ASSERT(dom_iommu(d)->platform_ops == NULL || + page_list_empty(&dom_iommu(d)->arch.pgtables.list)); } static bool __hwdom_init hwdom_iommu_map(const struct domain *d, @@ -273,6 +280,12 @@ int iommu_free_pgtables(struct domain *d) /* After this barrier, no more IOMMU mapping can happen */ spin_barrier(&hd->arch.mapping_lock); + /* + * Pages will be moved to the free list below. So we want to + * clear the root page-table to avoid any potential use after-free. + */ + hd->platform_ops->clear_root_pgtable(d); + while ( (pg = page_list_remove_head(&hd->arch.pgtables.list)) ) { free_domheap_page(pg); diff --git a/xen/include/xen/iommu.h b/xen/include/xen/iommu.h index 863a68fe1622..d59ed7cbad43 100644 --- a/xen/include/xen/iommu.h +++ b/xen/include/xen/iommu.h @@ -272,6 +272,7 @@ struct iommu_ops { int (*adjust_irq_affinities)(void); void (*sync_cache)(const void *addr, unsigned int size); + void (*clear_root_pgtable)(struct domain *d); #endif /* CONFIG_X86 */ int __must_check (*suspend)(void);