Re: [for-4.15][PATCH v2 5/5] xen/iommu: x86: Clear the root page-table before freeing the page-tables

[Julien Grall <julien@xxxxxxx>]

Hi Kevin,

On 10/02/2021 02:21, Tian, Kevin wrote:
> > From: Julien Grall <julien@xxxxxxx>
> > Sent: Tuesday, February 9, 2021 11:28 PM
> >
> > From: Julien Grall <jgrall@xxxxxxxxxx>
> >
> > The new per-domain IOMMU page-table allocator will now free the
> > page-tables when domain's resources are relinquished. However, the root
> > page-table (i.e. hd->arch.pg_maddr) will not be cleared.
> It's the pointer not the table itself.
I don't think the sentence is incorrect. One can view clearing as either 
clearing the page table itself or the pointer.
> > Xen may access the IOMMU page-tables afterwards at least in the case of
> > PV domain:
> >
> > (XEN) Xen call trace:
> > (XEN)    [<ffff82d04025b4b2>] R
> > iommu.c#addr_to_dma_page_maddr+0x12e/0x1d8
> > (XEN)    [<ffff82d04025b695>] F
> > iommu.c#intel_iommu_unmap_page+0x5d/0xf8
> > (XEN)    [<ffff82d0402695f3>] F iommu_unmap+0x9c/0x129
> > (XEN)    [<ffff82d0402696a6>] F iommu_legacy_unmap+0x26/0x63
> > (XEN)    [<ffff82d04033c5c7>] F mm.c#cleanup_page_mappings+0x139/0x144
> > (XEN)    [<ffff82d04033c61d>] F put_page+0x4b/0xb3
> > (XEN)    [<ffff82d04033c87f>] F put_page_from_l1e+0x136/0x13b
> > (XEN)    [<ffff82d04033cada>] F devalidate_page+0x256/0x8dc
> > (XEN)    [<ffff82d04033d396>] F mm.c#_put_page_type+0x236/0x47e
> > (XEN)    [<ffff82d04033d64d>] F mm.c#put_pt_page+0x6f/0x80
> > (XEN)    [<ffff82d04033d8d6>] F mm.c#put_page_from_l2e+0x8a/0xcf
> > (XEN)    [<ffff82d04033cc27>] F devalidate_page+0x3a3/0x8dc
> > (XEN)    [<ffff82d04033d396>] F mm.c#_put_page_type+0x236/0x47e
> > (XEN)    [<ffff82d04033d64d>] F mm.c#put_pt_page+0x6f/0x80
> > (XEN)    [<ffff82d04033d807>] F mm.c#put_page_from_l3e+0x8a/0xcf
> > (XEN)    [<ffff82d04033cdf0>] F devalidate_page+0x56c/0x8dc
> > (XEN)    [<ffff82d04033d396>] F mm.c#_put_page_type+0x236/0x47e
> > (XEN)    [<ffff82d04033d64d>] F mm.c#put_pt_page+0x6f/0x80
> > (XEN)    [<ffff82d04033d6c7>] F mm.c#put_page_from_l4e+0x69/0x6d
> > (XEN)    [<ffff82d04033cf24>] F devalidate_page+0x6a0/0x8dc
> > (XEN)    [<ffff82d04033d396>] F mm.c#_put_page_type+0x236/0x47e
> > (XEN)    [<ffff82d04033d92e>] F put_page_type_preemptible+0x13/0x15
> > (XEN)    [<ffff82d04032598a>] F domain.c#relinquish_memory+0x1ff/0x4e9
> > (XEN)    [<ffff82d0403295f2>] F domain_relinquish_resources+0x2b6/0x36a
> > (XEN)    [<ffff82d040205cdf>] F domain_kill+0xb8/0x141
> > (XEN)    [<ffff82d040236cac>] F do_domctl+0xb6f/0x18e5
> > (XEN)    [<ffff82d04031d098>] F pv_hypercall+0x2f0/0x55f
> > (XEN)    [<ffff82d04039b432>] F lstar_enter+0x112/0x120
> >
> > This will result to a use after-free and possibly an host crash or
> > memory corruption.
> >
> > Freeing the page-tables further down in domain_relinquish_resources()
> > would not work because pages may not be released until later if another
> > domain hold a reference on them.
> >
> > Once all the PCI devices have been de-assigned, it is actually pointless
> > to access modify the IOMMU page-tables. So we can simply clear the root
> > page-table address.
> Above two paragraphs are confusing to me. I don't know what exact change
> is proposed until looking over the whole patch. Isn't it clearer to just say
> "We should clear the root page table pointer in iommu_free_pgtables to
> avoid use-after-free after all pgtables are moved to the free list"?
Your sentence explain the approach taken but not the rational behind the 
approach. Both are equally important for future reference.
I will try to reword it.

Cheers,

--
Julien Grall