| Message ID | 20210217142458.3769-2-julien@xen.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen/iommu: Collection of bug fixes for IOMMU teadorwn | expand |
On 17.02.2021 15:24, Julien Grall wrote: > --- 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 iommu_clear_root_pgtable(struct domain *d) Nit: amd_iommu_ as a prefix would be okay here considering other (static) functions also use it. Since it is a static function, no prefix at all would also do (my personal preference). But iommu_ as a prefix isn't helpful and results in needless re-use of VT-d's name. > --- a/xen/drivers/passthrough/x86/iommu.c > +++ b/xen/drivers/passthrough/x86/iommu.c > @@ -267,6 +267,15 @@ int iommu_free_pgtables(struct domain *d) > struct page_info *pg; > unsigned int done = 0; > > + if ( !is_iommu_enabled(d) ) > + return 0; > + > + /* > + * 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); Taking amd_iommu_alloc_root() as example, is this really correct prior to what is now patch 2? What guarantees a new root table won't get allocated subsequently? Jan
Hi Jan, On 17/02/2021 14:54, Jan Beulich wrote: > On 17.02.2021 15:24, Julien Grall wrote: >> --- a/xen/drivers/passthrough/x86/iommu.c >> +++ b/xen/drivers/passthrough/x86/iommu.c >> @@ -267,6 +267,15 @@ int iommu_free_pgtables(struct domain *d) >> struct page_info *pg; >> unsigned int done = 0; >> >> + if ( !is_iommu_enabled(d) ) >> + return 0; >> + >> + /* >> + * 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); > > Taking amd_iommu_alloc_root() as example, is this really correct > prior to what is now patch 2? Yes, there are no more use-after-free... > What guarantees a new root table > won't get allocated subsequently? It doesn't prevent root table allocation. I view the two as distincts issues, hence the two patches. Cheers,
On 17.02.2021 16:00, Julien Grall wrote: > Hi Jan, > > On 17/02/2021 14:54, Jan Beulich wrote: >> On 17.02.2021 15:24, Julien Grall wrote: >>> --- a/xen/drivers/passthrough/x86/iommu.c >>> +++ b/xen/drivers/passthrough/x86/iommu.c >>> @@ -267,6 +267,15 @@ int iommu_free_pgtables(struct domain *d) >>> struct page_info *pg; >>> unsigned int done = 0; >>> >>> + if ( !is_iommu_enabled(d) ) >>> + return 0; >>> + >>> + /* >>> + * 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); >> >> Taking amd_iommu_alloc_root() as example, is this really correct >> prior to what is now patch 2? > > Yes, there are no more use-after-free... And this is because of ...? The necessary lock isn't being held here, so on another CPU allocation of a new root and then of new page tables could happen before you make enough progress here, and hence it looks to me as if there might then still be pages which get freed while present in the page tables (and hence accessible by devices). Jan >> What guarantees a new root table >> won't get allocated subsequently? > > It doesn't prevent root table allocation. I view the two as distincts > issues, hence the two patches. > > Cheers, >
On 17/02/2021 15:17, Jan Beulich wrote: > On 17.02.2021 16:00, Julien Grall wrote: >> Hi Jan, >> >> On 17/02/2021 14:54, Jan Beulich wrote: >>> On 17.02.2021 15:24, Julien Grall wrote: >>>> --- a/xen/drivers/passthrough/x86/iommu.c >>>> +++ b/xen/drivers/passthrough/x86/iommu.c >>>> @@ -267,6 +267,15 @@ int iommu_free_pgtables(struct domain *d) >>>> struct page_info *pg; >>>> unsigned int done = 0; >>>> >>>> + if ( !is_iommu_enabled(d) ) >>>> + return 0; >>>> + >>>> + /* >>>> + * 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); >>> >>> Taking amd_iommu_alloc_root() as example, is this really correct >>> prior to what is now patch 2? >> >> Yes, there are no more use-after-free... > > And this is because of ...? The necessary lock isn't being held > here, so on another CPU allocation of a new root and then of new > page tables could happen before you make enough progress here, > and hence it looks to me as if there might then still be pages > which get freed while present in the page tables (and hence > accessible by devices). Ah yes. I forgot that now patch #3 is not first anymore. I can move again patch #3 first, although I know you dislike the approach taken there... Cheers,
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c index 42b5a5a9bec4..81add0ba26b4 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 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 = 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 d136fe36883b..e1871f6c2bc1 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, @@ -2719,6 +2728,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 cea1032b3d02..f54fc8093f18 100644 --- a/xen/drivers/passthrough/x86/iommu.c +++ b/xen/drivers/passthrough/x86/iommu.c @@ -267,6 +267,15 @@ int iommu_free_pgtables(struct domain *d) struct page_info *pg; unsigned int done = 0; + if ( !is_iommu_enabled(d) ) + return 0; + + /* + * 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);