From: Paul Durrant <xadimgnik@gmail.com>
To: "'Julien Grall'" <julien@xen.org>, <xen-devel@lists.xenproject.org>
Cc: <hongyxia@amazon.co.uk>, <iwj@xenproject.org>,
"'Julien Grall'" <jgrall@amazon.com>,
"'Jan Beulich'" <jbeulich@suse.com>,
"'Andrew Cooper'" <andrew.cooper3@citrix.com>,
"'Kevin Tian'" <kevin.tian@intel.com>
Subject: RE: [for-4.15][PATCH v2 5/5] xen/iommu: x86: Clear the root page-table before freeing the page-tables
Date: Tue, 9 Feb 2021 20:36:49 -0000 [thread overview]
Message-ID: <04f901d6ff23$4b2b5e80$e1821b80$@xen.org> (raw)
In-Reply-To: <20210209152816.15792-6-julien@xen.org>
> -----Original Message-----
> From: Julien Grall <julien@xen.org>
> Sent: 09 February 2021 15:28
> To: xen-devel@lists.xenproject.org
> Cc: hongyxia@amazon.co.uk; iwj@xenproject.org; Julien Grall <jgrall@amazon.com>; Jan Beulich
> <jbeulich@suse.com>; Andrew Cooper <andrew.cooper3@citrix.com>; Kevin Tian <kevin.tian@intel.com>;
> Paul Durrant <paul@xen.org>
> Subject: [for-4.15][PATCH v2 5/5] xen/iommu: x86: Clear the root page-table before freeing the page-
> tables
>
> From: Julien Grall <jgrall@amazon.com>
>
> 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.
>
> 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.
>
> Fixes: 3eef6d07d722 ("x86/iommu: convert VT-d code to use new page table allocator")
> Signed-off-by: Julien Grall <jgrall@amazon.com>
>
> ---
> Changes in v2:
> - Introduce clear_root_pgtable()
> - Move the patch later in the series
> ---
> xen/drivers/passthrough/amd/pci_amd_iommu.c | 12 +++++++++++-
> xen/drivers/passthrough/vtd/iommu.c | 12 +++++++++++-
> xen/drivers/passthrough/x86/iommu.c | 6 ++++++
> xen/include/xen/iommu.h | 1 +
> 4 files changed, 29 insertions(+), 2 deletions(-)
>
> 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 82d770107a47..d3cdec6ee83f 100644
> --- a/xen/drivers/passthrough/x86/iommu.c
> +++ b/xen/drivers/passthrough/x86/iommu.c
> @@ -280,6 +280,12 @@ int iommu_free_pgtables(struct domain *d)
> /* After this barrier no new page allocations can occur. */
> spin_barrier(&hd->arch.pgtables.lock);
>
> + /*
> + * Pages will be moved to the free list in a bit. So we want to
'in a bit' sounds a little odd. I think you could just say 'below'. The code looks fine though so...
Reviewed-by: Paul Durrant <paul@xen.org>
> + * 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);
> --
> 2.17.1
next prev parent reply other threads:[~2021-02-09 20:37 UTC|newest]
Thread overview: 47+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-02-09 15:28 [for-4.15][PATCH v2 0/5] xen/iommu: Collection of bug fixes for IOMMU teadorwn Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 1/5] xen/x86: p2m: Don't map the special pages in the IOMMU page-tables Julien Grall
2021-02-09 20:19 ` Paul Durrant
2021-02-10 8:29 ` Roger Pau Monné
2021-02-10 8:50 ` Julien Grall
2021-02-10 9:34 ` Julien Grall
2021-02-10 11:10 ` Jan Beulich
2021-02-10 11:34 ` Roger Pau Monné
2021-02-10 11:38 ` Jan Beulich
2021-02-10 11:40 ` Julien Grall
2021-02-10 11:45 ` Jan Beulich
2021-02-10 11:48 ` Julien Grall
2021-02-10 11:54 ` Roger Pau Monné
2021-02-10 13:12 ` Jan Beulich
2021-02-10 15:24 ` Roger Pau Monné
2021-02-10 15:53 ` Jan Beulich
2021-02-10 13:08 ` Jan Beulich
2021-02-10 11:26 ` Jan Beulich
2021-02-15 11:38 ` Julien Grall
2021-02-15 12:36 ` Jan Beulich
2021-02-15 12:54 ` Julien Grall
2021-02-15 13:14 ` Jan Beulich
2021-02-17 11:21 ` Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 2/5] xen/iommu: Check if the IOMMU was initialized before tearing down Julien Grall
2021-02-09 20:22 ` Paul Durrant
2021-02-17 11:25 ` Julien Grall
2021-02-09 15:28 ` [for-4.15][PATCH v2 3/5] xen/iommu: iommu_map: Don't crash the domain if it is dying Julien Grall
2021-02-09 20:28 ` Paul Durrant
2021-02-09 21:14 ` Julien Grall
2021-02-10 14:14 ` Jan Beulich
2021-02-10 14:58 ` Julien Grall
2021-02-10 15:56 ` Jan Beulich
2021-02-09 15:28 ` [for-4.15][PATCH v2 4/5] xen/iommu: x86: Don't leak the IOMMU page-tables Julien Grall
2021-02-09 20:33 ` Paul Durrant
2021-02-10 14:32 ` Jan Beulich
2021-02-10 15:04 ` Julien Grall
2021-02-10 16:12 ` Jan Beulich
2021-02-17 11:49 ` Julien Grall
2021-02-17 12:57 ` Jan Beulich
2021-02-10 14:44 ` Jan Beulich
2021-02-09 15:28 ` [for-4.15][PATCH v2 5/5] xen/iommu: x86: Clear the root page-table before freeing the page-tables Julien Grall
2021-02-09 20:36 ` Paul Durrant [this message]
2021-02-10 2:21 ` Tian, Kevin
2021-02-17 13:54 ` Julien Grall
2021-02-10 14:39 ` Jan Beulich
2021-02-09 16:47 ` [for-4.15][PATCH v2 0/5] xen/iommu: Collection of bug fixes for IOMMU teadorwn Ian Jackson
2021-02-17 11:33 ` Julien Grall
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='04f901d6ff23$4b2b5e80$e1821b80$@xen.org' \
--to=xadimgnik@gmail.com \
--cc=andrew.cooper3@citrix.com \
--cc=hongyxia@amazon.co.uk \
--cc=iwj@xenproject.org \
--cc=jbeulich@suse.com \
--cc=jgrall@amazon.com \
--cc=julien@xen.org \
--cc=kevin.tian@intel.com \
--cc=paul@xen.org \
--cc=xen-devel@lists.xenproject.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).