|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 1/3] x86/p2m: tighten conditions of IOMMU mapping updates
>>> On 21.09.15 at 16:02, <JBeulich@xxxxxxxx> wrote:
> In the EPT case permission changes should also result in updates or
> TLB flushes.
>
> In the NPT case the old MFN does not depend on the new entry being
> valid (but solely on the old one), and the need to update or TLB-flush
> again also depends on permission changes.
>
> In the shadow mode case, iommu_hap_pt_share should be ignored.
>
> Furthermore in the NPT/shadow case old intermediate page tables must
> be freed only after IOMMU side updates/flushes have got carried out.
>
> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
Wei,
I'm sorry, I forgot to Cc you on the patch and state that I think this
should be considered for 4.6.
Jan
> ---
> In addition to the fixes here it looks to me as if both EPT and
> NPT/shadow code lack invalidation of IOMMU side paging structure
> caches, i.e. further changes may be needed. Am I overlooking something?
>
> --- a/xen/arch/x86/mm/p2m-ept.c
> +++ b/xen/arch/x86/mm/p2m-ept.c
> @@ -668,6 +668,7 @@ ept_set_entry(struct p2m_domain *p2m, un
> uint8_t ipat = 0;
> int need_modify_vtd_table = 1;
> int vtd_pte_present = 0;
> + unsigned int iommu_flags = p2m_get_iommu_flags(p2mt);
> enum { sync_off, sync_on, sync_check } needs_sync = sync_check;
> ept_entry_t old_entry = { .epte = 0 };
> ept_entry_t new_entry = { .epte = 0 };
> @@ -798,8 +799,9 @@ ept_set_entry(struct p2m_domain *p2m, un
> new_entry.mfn = mfn_x(mfn);
>
> /* Safe to read-then-write because we hold the p2m lock */
> - if ( ept_entry->mfn == new_entry.mfn )
> - need_modify_vtd_table = 0;
> + if ( ept_entry->mfn == new_entry.mfn &&
> + p2m_get_iommu_flags(ept_entry->sa_p2mt) == iommu_flags )
> + need_modify_vtd_table = 0;
>
> ept_p2m_type_to_flags(p2m, &new_entry, p2mt, p2ma);
> }
> @@ -830,11 +832,9 @@ out:
> iommu_pte_flush(d, gfn, &ept_entry->epte, order,
> vtd_pte_present);
> else
> {
> - unsigned int flags = p2m_get_iommu_flags(p2mt);
> -
> - if ( flags != 0 )
> + if ( iommu_flags )
> for ( i = 0; i < (1 << order); i++ )
> - iommu_map_page(d, gfn + i, mfn_x(mfn) + i, flags);
> + iommu_map_page(d, gfn + i, mfn_x(mfn) + i,
> iommu_flags);
> else
> for ( i = 0; i < (1 << order); i++ )
> iommu_unmap_page(d, gfn + i);
> --- a/xen/arch/x86/mm/p2m-pt.c
> +++ b/xen/arch/x86/mm/p2m-pt.c
> @@ -488,12 +488,13 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> void *table;
> unsigned long i, gfn_remainder = gfn;
> l1_pgentry_t *p2m_entry;
> - l1_pgentry_t entry_content;
> + l1_pgentry_t entry_content, old_entry = l1e_empty();
> l2_pgentry_t l2e_content;
> l3_pgentry_t l3e_content;
> int rc;
> unsigned int iommu_pte_flags = p2m_get_iommu_flags(p2mt);
> - unsigned long old_mfn = 0;
> + unsigned int old_flags = 0;
> + unsigned long old_mfn = INVALID_MFN;
>
> ASSERT(sve != 0);
>
> @@ -538,17 +539,20 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> */
> if ( page_order == PAGE_ORDER_1G )
> {
> - l1_pgentry_t old_entry = l1e_empty();
> p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
> L3_PAGETABLE_SHIFT - PAGE_SHIFT,
> L3_PAGETABLE_ENTRIES);
> ASSERT(p2m_entry);
> - if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> - !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> + old_flags = l1e_get_flags(*p2m_entry);
> + if ( old_flags & _PAGE_PRESENT )
> {
> - /* We're replacing a non-SP page with a superpage. Make sure to
> - * handle freeing the table properly. */
> - old_entry = *p2m_entry;
> + if ( old_flags & _PAGE_PSE )
> + old_mfn = l1e_get_pfn(*p2m_entry);
> + else
> + {
> + old_entry = *p2m_entry;
> + old_flags = ~0U;
> + }
> }
>
> ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -559,17 +563,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> entry_content.l1 = l3e_content.l3;
>
> if ( entry_content.l1 != 0 )
> - {
> p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> - old_mfn = l1e_get_pfn(*p2m_entry);
> - }
>
> p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 3);
> /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> -
> - /* Free old intermediate tables if necessary */
> - if ( l1e_get_flags(old_entry) & _PAGE_PRESENT )
> - p2m_free_entry(p2m, &old_entry, page_order);
> }
> else
> {
> @@ -591,7 +588,9 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
> 0, L1_PAGETABLE_ENTRIES);
> ASSERT(p2m_entry);
> -
> + old_flags = l1e_get_flags(*p2m_entry);
> + old_mfn = l1e_get_pfn(*p2m_entry);
> +
> if ( mfn_valid(mfn) || (p2mt == p2m_mmio_direct)
> || p2m_is_paging(p2mt) )
> entry_content = p2m_l1e_from_pfn(mfn_x(mfn),
> @@ -600,29 +599,28 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> entry_content = l1e_empty();
>
> if ( entry_content.l1 != 0 )
> - {
> p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> - old_mfn = l1e_get_pfn(*p2m_entry);
> - }
> +
> /* level 1 entry */
> p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 1);
> /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> }
> else if ( page_order == PAGE_ORDER_2M )
> {
> - l1_pgentry_t old_entry = l1e_empty();
> p2m_entry = p2m_find_entry(table, &gfn_remainder, gfn,
> L2_PAGETABLE_SHIFT - PAGE_SHIFT,
> L2_PAGETABLE_ENTRIES);
> ASSERT(p2m_entry);
> -
> - /* FIXME: Deal with 4k replaced by 2meg pages */
> - if ( (l1e_get_flags(*p2m_entry) & _PAGE_PRESENT) &&
> - !(l1e_get_flags(*p2m_entry) & _PAGE_PSE) )
> - {
> - /* We're replacing a non-SP page with a superpage. Make sure to
> - * handle freeing the table properly. */
> - old_entry = *p2m_entry;
> + old_flags = l1e_get_flags(*p2m_entry);
> + if ( old_flags & _PAGE_PRESENT )
> + {
> + if ( old_flags & _PAGE_PSE )
> + old_mfn = l1e_get_pfn(*p2m_entry);
> + else
> + {
> + old_entry = *p2m_entry;
> + old_flags = ~0U;
> + }
> }
>
> ASSERT(!mfn_valid(mfn) || p2mt != p2m_mmio_direct);
> @@ -636,17 +634,10 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> entry_content.l1 = l2e_content.l2;
>
> if ( entry_content.l1 != 0 )
> - {
> p2m_add_iommu_flags(&entry_content, 0, iommu_pte_flags);
> - old_mfn = l1e_get_pfn(*p2m_entry);
> - }
>
> p2m->write_p2m_entry(p2m, gfn, p2m_entry, entry_content, 2);
> /* NB: paging_write_p2m_entry() handles tlb flushes properly */
> -
> - /* Free old intermediate tables if necessary */
> - if ( l1e_get_flags(old_entry) & _PAGE_PRESENT )
> - p2m_free_entry(p2m, &old_entry, page_order);
> }
>
> /* Track the highest gfn for which we have ever had a valid mapping */
> @@ -654,11 +645,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> && (gfn + (1UL << page_order) - 1 > p2m->max_mapped_pfn) )
> p2m->max_mapped_pfn = gfn + (1UL << page_order) - 1;
>
> - if ( iommu_enabled && need_iommu(p2m->domain) )
> + if ( iommu_enabled && need_iommu(p2m->domain) &&
> + (p2m_get_iommu_flags(p2m_flags_to_type(old_flags)) !=
> + iommu_pte_flags ||
> + old_mfn != mfn_x(mfn)) )
> {
> - if ( iommu_hap_pt_share )
> + if ( iommu_use_hap_pt(p2m->domain) )
> {
> - if ( old_mfn && (old_mfn != mfn_x(mfn)) )
> + if ( old_flags & _PAGE_PRESENT )
> amd_iommu_flush_pages(p2m->domain, gfn, page_order);
> }
> else
> @@ -674,6 +668,14 @@ p2m_pt_set_entry(struct p2m_domain *p2m,
> }
> }
>
> + /*
> + * Free old intermediate tables if necessary. This has to be the
> + * last thing we do, after removal from the IOMMU tables, so as to
> + * avoid a potential use-after-free.
> + */
> + if ( l1e_get_flags(old_entry) & _PAGE_PRESENT )
> + p2m_free_entry(p2m, &old_entry, page_order);
> +
> out:
> unmap_domain_page(table);
> return rc;
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |