[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

Hi Jan,

On 23/12/2020 16:35, Jan Beulich wrote:
On 23.12.2020 17:19, Julien Grall wrote:
On 23/12/2020 16:15, Jan Beulich wrote:
On 23.12.2020 17:07, Julien Grall wrote:
I think I prefer the small race introduced (the pages will still be
freed) over this solution.

The "will still be freed" is because of the 2nd round of freeing
you add in an earlier patch? I'd prefer to avoid the race to in
turn avoid that 2nd round of freeing.

The "2nd round" of freeing is necessary at least for the domain creation
failure case (it would be the 1rst round).

If we can avoid IOMMU page-table allocation in the domain creation path,
then yes I agree that we want to avoid that "2nd round". Otherwise, I
think it is best to take advantage of it.

Well, I'm not really certain either way here. If it's really just
VMX'es APIC access page that's the problem here, custom cleanup
of this "fallout" from VMX code would certainly be an option.

From my testing, it looks like the VMX APIC page is the only entry added while the domain is created.

Furthermore I consider it wrong to insert this page in the IOMMU
page tables in the first place. This page overlaps with the MSI
special address range, and hence accesses will be dealt with by
interrupt remapping machinery (if enabled). If interrupt
remapping is disabled, this page should be no different for I/O
purposes than all other pages in this window - they shouldn't
be mapped at all.

That's a fair point. I will have a look to see if I can avoid the IOMMU mapping for the VMX APIC page.

Perhaps, along the lines of epte_get_entry_emt(), ept_set_entry()
should gain an is_special_page() check to prevent propagation to
the IOMMU for such pages (we can't do anything about them in
shared-page-table setups)? See also the (PV related) comment in
cleanup_page_mappings(), a few lines ahead of a respective use of

There seems to be a mismatch between what the comment says and the code adding the page in the IOMMU for PV domain.

AFAICT, the IOMMU mapping will be added via _get_page_type():

        /* Special pages should not be accessible from devices. */
        struct domain *d = page_get_owner(page);

        if ( d && unlikely(need_iommu_pt_sync(d)) && is_pv_domain(d) )
            mfn_t mfn = page_to_mfn(page);

            if ( (x & PGT_type_mask) == PGT_writable_page )
                rc = iommu_legacy_unmap(d, _dfn(mfn_x(mfn)),
                                        1ul << PAGE_ORDER_4K);
                rc = iommu_legacy_map(d, _dfn(mfn_x(mfn)), mfn,
                                      1ul << PAGE_ORDER_4K,
                                      IOMMUF_readable | IOMMUF_writable);

In this snippet, "special page" is interpreted as a page with no owner. However is_special_page() will return true when PGC_extra is set and the flag implies there is an owner (see assign_pages()).

So it looks like to me, any pages with PGC_extra would end up to have a mapping in the IOMMU pages-tables if they are writable.

This statement also seems to apply for xenheap pages shared with a domain (e.g grant-table).

Did I miss anything?


Julien Grall



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