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

[Julien Grall <julien@xxxxxxx>]

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
> is_special_page(),
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);
            else
                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?

Cheers,

--
Julien Grall