Compare commits

...
Sign in to create a new pull request.

5 commits

Author SHA1 Message Date
Michael Young
f522170871 6 security fixes
VCPUOP_initialise DoS [XSA-296, CVE-2019-18420]
missing descriptor table limit checking in x86 PV emulation [XSA-298,
        CVE-2019-18425]
Issues with restartable PV type change operations [XSA-299, CVE-2019-18421]
        (#1767726)
add-to-physmap can be abused to DoS Arm hosts [XSA-301, CVE-2019-18423]
passed through PCI devices may corrupt host memory after deassignment
        [XSA-302, CVE-2019-18424] (#1767731)
ARM: Interrupts are unconditionally unmasked in exception handlers
        [XSA-303, CVE-2019-18422]
2019-11-01 19:28:17 +00:00
Michael Young
cf804a7f3f delete dropped patches 2019-07-01 22:00:35 +01:00
Michael Young
842cba0ff7 update to xen-4.11.2 2019-07-01 21:56:32 +01:00
Michael Young
780a4075dc Unlimited Arm Atomics Operations [XSA-295] (#1720760) 2019-06-15 19:13:39 +01:00
Michael Young
e41f4de679 Microarchitectural Data Sampling speculative side channel [XSA-297,
CVE-2018-12126, CVE-2018-12127, CVE-2018-12130, CVE-2019-11091]
additional patches so above applies cleanly
work around grub2 issues in dom0
2019-05-14 22:51:28 +01:00
38 changed files with 4055 additions and 1841 deletions

2
.gitignore vendored
View file

@ -6,4 +6,4 @@ lwip-1.3.0.tar.gz
pciutils-2.2.9.tar.bz2
zlib-1.2.3.tar.gz
polarssl-1.1.4-gpl.tgz
/xen-4.11.1.tar.gz
/xen-4.11.2.tar.gz

View file

@ -4,4 +4,4 @@ SHA512 (newlib-1.16.0.tar.gz) = 40eb96bbc6736a16b6399e0cdb73e853d0d90b685c967e77
SHA512 (zlib-1.2.3.tar.gz) = 021b958fcd0d346c4ba761bcf0cc40f3522de6186cf5a0a6ea34a70504ce9622b1c2626fce40675bc8282cf5f5ade18473656abc38050f72f5d6480507a2106e
SHA512 (polarssl-1.1.4-gpl.tgz) = 88da614e4d3f4409c4fd3bb3e44c7587ba051e3fed4e33d526069a67e8180212e1ea22da984656f50e290049f60ddca65383e5983c0f8884f648d71f698303ad
SHA512 (pciutils-2.2.9.tar.bz2) = 2b3d98d027e46d8c08037366dde6f0781ca03c610ef2b380984639e4ef39899ed8d8b8e4cd9c9dc54df101279b95879bd66bfd4d04ad07fef41e847ea7ae32b5
SHA512 (xen-4.11.1.tar.gz) = c1655c5decdaed95a2b9a99652318cfc72f6cfdae957cfe60d635f7787e8850f33e8fafc4c4b8d61fb579c9b9d93028a6382903e71808a0418b931e76d72a649
SHA512 (xen-4.11.2.tar.gz) = 48d3d926d35eb56c79c06d0abc6e6be2564fadb43367cc7f46881c669a75016707672179c2cca1c4cfb14af2cefd46e2e7f99470cddf7df2886d8435a2de814e

View file

@ -9,17 +9,6 @@
union {
u64 val;
struct {
--- xen-4.11.1/xen/common/trace.c.orig 2018-11-29 14:04:11.000000000 +0000
+++ xen-4.11.1/xen/common/trace.c 2019-02-06 20:31:27.370256971 +0000
@@ -819,7 +819,7 @@
void __trace_hypercall(uint32_t event, unsigned long op,
const xen_ulong_t *args)
{
- struct __packed {
+ struct {
uint32_t op;
uint32_t args[6];
} d;
--- xen-4.11.1/xen/arch/x86/cpu/mtrr/generic.c.orig 2018-11-29 14:04:11.000000000 +0000
+++ xen-4.11.1/xen/arch/x86/cpu/mtrr/generic.c 2019-02-10 19:24:09.378805103 +0000
@@ -171,6 +171,9 @@

105
xen.spec
View file

@ -66,8 +66,8 @@
Summary: Xen is a virtual machine monitor
Name: xen
Version: 4.11.1
Release: 4%{?dist}
Version: 4.11.2
Release: 2%{?dist}
License: GPLv2+ and LGPLv2+ and BSD
URL: http://xen.org/
Source0: https://downloads.xenproject.org/release/xen/%{version}/xen-%{version}.tar.gz
@ -126,17 +126,18 @@ Patch42: xen.stubdom.build.patch
Patch44: xen.vwprintw.fix.patch
Patch45: xen.python.env.patch
Patch46: xen.gcc9.fixes.patch
Patch47: xsa284.patch
Patch48: xsa285-4.11.patch
Patch49: xsa287-4.11.patch
Patch50: xsa288-4.11.patch
Patch51: xsa290-4.11-1.patch
Patch52: xsa290-4.11-2.patch
Patch53: xsa291-4.11.patch
Patch54: xsa292.patch
Patch55: xsa293-4.11-1.patch
Patch56: xsa293-4.11-2.patch
Patch57: xsa294-4.11.patch
Patch47: xsa296.patch
Patch48: xsa298-4.11.patch
Patch49: xsa299-4.11-0001-x86-mm-L1TF-checks-don-t-leave-a-partial-entry.patch
Patch50: xsa301-4.11-1.patch
Patch51: xsa301-4.11-2.patch
Patch52: xsa301-4.11-3.patch
Patch53: xsa302-4.11-0001-IOMMU-add-missing-HVM-check.patch
Patch54: xsa302-4.11-0002-passthrough-quarantine-PCI-devices.patch
Patch55: xsa303-0001-xen-arm32-entry-Split-__DEFINE_ENTRY_TRAP-in-two.patch
Patch56: xsa303-0002-xen-arm32-entry-Fold-the-macro-SAVE_ALL-in-the-macro.patch
Patch57: xsa303-0003-xen-arm32-Don-t-blindly-unmask-interrupts-on-trap-wi.patch
Patch58: xsa303-0004-xen-arm64-Don-t-blindly-unmask-interrupts-on-trap-wi.patch
%if %build_qemutrad
@ -349,11 +350,14 @@ manage Xen virtual machines.
%patch50 -p1
%patch51 -p1
%patch52 -p1
%ifarch %{ix86} x86_64
%patch53 -p1
%patch54 -p1
%endif
%patch55 -p1
%patch56 -p1
%patch57 -p1
%patch58 -p1
# qemu-xen-traditional patches
pushd tools/qemu-xen-traditional
@ -633,9 +637,33 @@ fi
if [ $1 == 1 -a -f /sbin/grub2-mkconfig ]; then
if [ -f /boot/grub2/grub.cfg ]; then
/sbin/grub2-mkconfig -o /boot/grub2/grub.cfg
sed -i -e '/insmod module2/d' /boot/grub2/grub.cfg
if [ -d /usr/lib/grub/i386-pc ]; then
if [ ! -d /boot/grub2/i386-pc ]; then
mkdir /boot/grub2/i386-pc
fi
if [ -f /usr/lib/grub/i386-pc/relocator.mod -a ! -f /boot/grub2/i386-pc/relocator.mod ]; then
cp -p /usr/lib/grub/i386-pc/relocator.mod /boot/grub2/i386-pc/relocator.mod
fi
if [ -f /usr/lib/grub/i386-pc/multiboot2.mod -a ! -f /boot/grub2/i386-pc/multiboot2.mod ]; then
cp -p /usr/lib/grub/i386-pc/multiboot2.mod /boot/grub2/i386-pc/multiboot2.mod
fi
fi
fi
if [ -f /boot/efi/EFI/fedora/grub.cfg ]; then
/sbin/grub2-mkconfig -o /boot/efi/EFI/fedora/grub.cfg
sed -i -e '/insmod module2/d' /boot/efi/EFI/fedora/grub.cfg
if [ -d /usr/lib/grub/x86_64-efi ]; then
if [ ! -d /boot/efi/EFI/fedora/x86_64-efi ]; then
mkdir /boot/efi/EFI/fedora/x86_64-efi
fi
if [ -f /usr/lib/grub/x86_64-efi/relocator.mod -a ! -f /boot/efi/EFI/fedora/x86_64-efi/relocator.mod ]; then
cp -p /usr/lib/grub/x86_64-efi/relocator.mod /boot/efi/EFI/fedora/x86_64-efi/relocator.mod
fi
if [ -f /usr/lib/grub/x86_64-efi/multiboot2.mod -a ! -f /boot/efi/EFI/fedora/x86_64-efi/multiboot2.mod ]; then
cp -p /usr/lib/grub/x86_64-efi/multiboot2.mod /boot/efi/EFI/fedora/x86_64-efi/multiboot2.mod
fi
fi
fi
fi
@ -643,9 +671,33 @@ fi
if [ -f /sbin/grub2-mkconfig ]; then
if [ -f /boot/grub2/grub.cfg ]; then
/sbin/grub2-mkconfig -o /boot/grub2/grub.cfg
sed -i -e '/insmod module2/d' /boot/grub2/grub.cfg
if [ -d /usr/lib/grub/i386-pc -a $1 == 1 ]; then
if [ ! -d /boot/grub2/i386-pc ]; then
mkdir /boot/grub2/i386-pc
fi
if [ -f /usr/lib/grub/i386-pc/relocator.mod -a ! -f /boot/grub2/i386-pc/relocator.mod ]; then
cp -p /usr/lib/grub/i386-pc/relocator.mod /boot/grub2/i386-pc/relocator.mod
fi
if [ -f /usr/lib/grub/i386-pc/multiboot2.mod -a ! -f /boot/grub2/i386-pc/multiboot2.mod ]; then
cp -p /usr/lib/grub/i386-pc/multiboot2.mod /boot/grub2/i386-pc/multiboot2.mod
fi
fi
fi
if [ -f /boot/efi/EFI/fedora/grub.cfg ]; then
/sbin/grub2-mkconfig -o /boot/efi/EFI/fedora/grub.cfg
sed -i -e '/insmod module2/d' /boot/efi/EFI/fedora/grub.cfg
if [ -d /usr/lib/grub/x86_64-efi -a $1 == 1 ]; then
if [ ! -d /boot/efi/EFI/fedora/x86_64-efi ]; then
mkdir /boot/efi/EFI/fedora/x86_64-efi
fi
if [ -f /usr/lib/grub/x86_64-efi/relocator.mod -a ! -f /boot/efi/EFI/fedora/x86_64-efi/relocator.mod ]; then
cp -p /usr/lib/grub/x86_64-efi/relocator.mod /boot/efi/EFI/fedora/x86_64-efi/relocator.mod
fi
if [ -f /usr/lib/grub/x86_64-efi/multiboot2.mod -a ! -f /boot/efi/EFI/fedora/x86_64-efi/multiboot2.mod ]; then
cp -p /usr/lib/grub/x86_64-efi/multiboot2.mod /boot/efi/EFI/fedora/x86_64-efi/multiboot2.mod
fi
fi
fi
fi
%endif
@ -905,6 +957,33 @@ fi
%endif
%changelog
* Fri Nov 01 2019 Michael Young <m.a.young@durham.ac.uk> - 4.11.2-2
- VCPUOP_initialise DoS [XSA-296, CVE-2019-18420]
- missing descriptor table limit checking in x86 PV emulation [XSA-298,
CVE-2019-18425]
- Issues with restartable PV type change operations [XSA-299, CVE-2019-18421]
(#1767726)
- add-to-physmap can be abused to DoS Arm hosts [XSA-301, CVE-2019-18423]
- passed through PCI devices may corrupt host memory after deassignment
[XSA-302, CVE-2019-18424] (#1767731)
- ARM: Interrupts are unconditionally unmasked in exception handlers
[XSA-303, CVE-2019-18422]
* Mon Jul 01 2019 Michael Young <m.a.young@durham.ac.uk> - 4.11.2-1
- update to 4.11.2
remove patches now fixed upstream
adjust xen.use.fedora.ipxe.patch
drop parts of xen.gcc9.fixes.patch
* Sat Jun 15 2019 Michael Young <m.a.young@durham.ac.uk> - 4.11.1-6
- Unlimited Arm Atomics Operations [XSA-295] (#1720760)
* Tue May 14 2019 Michael Young <m.a.young@durham.ac.uk> - 4.11.1-5
- Microarchitectural Data Sampling speculative side channel [XSA-297,
CVE-2018-12126, CVE-2018-12127, CVE-2018-12130, CVE-2019-11091]
- additional patches so above applies cleanly
- work around grub2 issues in dom0
* Tue Mar 05 2019 Michael Young <m.a.young@durham.ac.uk> - 4.11.1-4
- xen: various flaws (#1685577)
grant table transfer issues on large hosts [XSA-284]

View file

@ -19,7 +19,7 @@
+ETHERBOOT_NICS ?= 10ec8139 8086100e
QEMU_TRADITIONAL_REVISION ?= xen-4.11.1
QEMU_TRADITIONAL_REVISION ?= xen-4.11.2
--- xen-4.2.0/tools/firmware/Makefile.orig 2012-05-27 21:57:04.480812871 +0100
+++ xen-4.2.0/tools/firmware/Makefile 2012-06-02 19:03:52.254691484 +0100
@@ -10,7 +10,7 @@

View file

@ -1,31 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: gnttab: set page refcount for copy-on-grant-transfer
Commit 5cc77f9098 ("32-on-64: Fix domain address-size clamping,
implement"), which introduced this functionality, took care of clearing
the old page's PGC_allocated, but failed to set the bit (and install the
associated reference) on the newly allocated one. Furthermore the "mfn"
local variable was never updated, and hence the wrong MFN was passed to
guest_physmap_add_page() (and back to the destination domain) in this
case, leading to an IOMMU mapping into an unowned page.
Ideally the code would use assign_pages(), but the call to
gnttab_prepare_for_transfer() sits in the middle of the actions
mirroring that function.
This is XSA-284.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: George Dunlap <george.dunlap@citrix.com>
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2183,6 +2183,8 @@ gnttab_transfer(
page->count_info &= ~(PGC_count_mask|PGC_allocated);
free_domheap_page(page);
page = new_page;
+ page->count_info = PGC_allocated | 1;
+ mfn = page_to_mfn(page);
}
spin_lock(&e->page_alloc_lock);

View file

@ -1,43 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: IOMMU/x86: fix type ref-counting race upon IOMMU page table construction
When arch_iommu_populate_page_table() gets invoked for an already
running guest, simply looking at page types once isn't enough, as they
may change at any time. Add logic to re-check the type after having
mapped the page, unmapping it again if needed.
This is XSA-285.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tentatively-Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/drivers/passthrough/x86/iommu.c
+++ b/xen/drivers/passthrough/x86/iommu.c
@@ -68,6 +68,27 @@ int arch_iommu_populate_page_table(struct domain *d)
rc = hd->platform_ops->map_page(d, gfn, mfn,
IOMMUF_readable |
IOMMUF_writable);
+
+ /*
+ * We may be working behind the back of a running guest, which
+ * may change the type of a page at any time. We can't prevent
+ * this (for instance, by bumping the type count while mapping
+ * the page) without causing legitimate guest type-change
+ * operations to fail. So after adding the page to the IOMMU,
+ * check again to make sure this is still valid. NB that the
+ * writable entry in the iommu is harmless until later, when
+ * the actual device gets assigned.
+ */
+ if ( !rc && !is_hvm_domain(d) &&
+ ((page->u.inuse.type_info & PGT_type_mask) !=
+ PGT_writable_page) )
+ {
+ rc = hd->platform_ops->unmap_page(d, gfn);
+ /* If the type changed yet again, simply force a retry. */
+ if ( !rc && ((page->u.inuse.type_info & PGT_type_mask) ==
+ PGT_writable_page) )
+ rc = -ERESTART;
+ }
}
if ( rc )
{

View file

@ -1,328 +0,0 @@
From 67620c1ccb13f7b58645f48248ba1f408b021fdc Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 18 Jan 2019 15:00:34 +0000
Subject: [PATCH] steal_page: Get rid of bogus struct page states
The original rules for `struct page` required the following invariants
at all times:
- refcount > 0 implies owner != NULL
- PGC_allocated implies refcount > 0
steal_page, in a misguided attempt to protect against unknown races,
violates both of these rules, thus introducing other races:
- Temporarily, the count_info has the refcount go to 0 while
PGC_allocated is set
- It explicitly returns the page PGC_allocated set, but owner == NULL
and page not on the page_list.
The second one meant that page_get_owner_and_reference() could return
NULL even after having successfully grabbed a reference on the page,
leading the caller to leak the reference (since "couldn't get ref" and
"got ref but no owner" look the same).
Furthermore, rather than grabbing a page reference to ensure that the
owner doesn't change under its feet, it appears to rely on holding
d->page_alloc lock to prevent this.
Unfortunately, this is ineffective: page->owner remains non-NULL for
some time after the count has been set to 0; meaning that it would be
entirely possible for the page to be freed and re-allocated to a
different domain between the page_get_owner() check and the count_info
check.
Modify steal_page to instead follow the appropriate access discipline,
taking the page through series of states similar to being freed and
then re-allocated with MEMF_no_owner:
- Grab an extra reference to make sure we don't race with anyone else
freeing the page
- Drop both references and PGC_allocated atomically, so that (if
successful), anyone else trying to grab a reference will fail
- Attempt to reset Xen's mappings
- Reset the rest of the state.
Then, modify the two callers appropriately:
- Leave count_info alone (it's already been cleared)
- Call free_domheap_page() directly if appropriate
- Call assign_pages() rather than open-coding a partial assign
With all callers to assign_pages() now passing in pages with the
type_info field clear, tighten the respective assertion there.
This is XSA-287.
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 84 ++++++++++++++++++++++++++++------------
xen/common/grant_table.c | 20 +++++-----
xen/common/memory.c | 19 +++++----
xen/common/page_alloc.c | 2 +-
4 files changed, 83 insertions(+), 42 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6509035a5c..d8ff58c901 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -3966,70 +3966,106 @@ int donate_page(
return -EINVAL;
}
+/*
+ * Steal page will attempt to remove `page` from domain `d`. Upon
+ * return, `page` will be in a state similar to the state of a page
+ * returned from alloc_domheap_page() with MEMF_no_owner set:
+ * - refcount 0
+ * - type count cleared
+ * - owner NULL
+ * - page caching attributes cleaned up
+ * - removed from the domain's page_list
+ *
+ * If MEMF_no_refcount is not set, the domain's tot_pages will be
+ * adjusted. If this results in the page count falling to 0,
+ * put_domain() will be called.
+ *
+ * The caller should either call free_domheap_page() to free the
+ * page, or assign_pages() to put it back on some domain's page list.
+ */
int steal_page(
struct domain *d, struct page_info *page, unsigned int memflags)
{
unsigned long x, y;
bool drop_dom_ref = false;
- const struct domain *owner = dom_xen;
+ const struct domain *owner;
+ int rc;
if ( paging_mode_external(d) )
return -EOPNOTSUPP;
- spin_lock(&d->page_alloc_lock);
-
- if ( is_xen_heap_page(page) || ((owner = page_get_owner(page)) != d) )
+ /* Grab a reference to make sure the page doesn't change under our feet */
+ rc = -EINVAL;
+ if ( !(owner = page_get_owner_and_reference(page)) )
goto fail;
+ if ( owner != d || is_xen_heap_page(page) )
+ goto fail_put;
+
/*
- * We require there is just one reference (PGC_allocated). We temporarily
- * drop this reference now so that we can safely swizzle the owner.
+ * We require there are exactly two references -- the one we just
+ * took, and PGC_allocated. We temporarily drop both these
+ * references so that the page becomes effectively non-"live" for
+ * the domain.
*/
y = page->count_info;
do {
x = y;
- if ( (x & (PGC_count_mask|PGC_allocated)) != (1 | PGC_allocated) )
- goto fail;
- y = cmpxchg(&page->count_info, x, x & ~PGC_count_mask);
+ if ( (x & (PGC_count_mask|PGC_allocated)) != (2 | PGC_allocated) )
+ goto fail_put;
+ y = cmpxchg(&page->count_info, x, x & ~(PGC_count_mask|PGC_allocated));
} while ( y != x );
/*
- * With the sole reference dropped temporarily, no-one can update type
- * information. Type count also needs to be zero in this case, but e.g.
- * PGT_seg_desc_page may still have PGT_validated set, which we need to
- * clear before transferring ownership (as validation criteria vary
- * depending on domain type).
+ * NB this is safe even if the page ends up being given back to
+ * the domain, because the count is zero: subsequent mappings will
+ * cause the cache attributes to be re-instated inside
+ * get_page_from_l1e().
+ */
+ if ( (rc = cleanup_page_cacheattr(page)) )
+ {
+ /*
+ * Couldn't fixup Xen's mappings; put things the way we found
+ * it and return an error
+ */
+ page->count_info |= PGC_allocated | 1;
+ goto fail;
+ }
+
+ /*
+ * With the reference count now zero, nobody can grab references
+ * to do anything else with the page. Return the page to a state
+ * that it might be upon return from alloc_domheap_pages with
+ * MEMF_no_owner set.
*/
+ spin_lock(&d->page_alloc_lock);
+
BUG_ON(page->u.inuse.type_info & (PGT_count_mask | PGT_locked |
PGT_pinned));
page->u.inuse.type_info = 0;
-
- /* Swizzle the owner then reinstate the PGC_allocated reference. */
page_set_owner(page, NULL);
- y = page->count_info;
- do {
- x = y;
- BUG_ON((x & (PGC_count_mask|PGC_allocated)) != PGC_allocated);
- } while ( (y = cmpxchg(&page->count_info, x, x | 1)) != x );
+ page_list_del(page, &d->page_list);
/* Unlink from original owner. */
if ( !(memflags & MEMF_no_refcount) && !domain_adjust_tot_pages(d, -1) )
drop_dom_ref = true;
- page_list_del(page, &d->page_list);
spin_unlock(&d->page_alloc_lock);
+
if ( unlikely(drop_dom_ref) )
put_domain(d);
+
return 0;
+ fail_put:
+ put_page(page);
fail:
- spin_unlock(&d->page_alloc_lock);
gdprintk(XENLOG_WARNING, "Bad steal mfn %" PRI_mfn
" from d%d (owner d%d) caf=%08lx taf=%" PRtype_info "\n",
mfn_x(page_to_mfn(page)), d->domain_id,
owner ? owner->domain_id : DOMID_INVALID,
page->count_info, page->u.inuse.type_info);
- return -EINVAL;
+ return rc;
}
static int __do_update_va_mapping(
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index c0585d33f4..656fad1b42 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2179,7 +2179,7 @@ gnttab_transfer(
rcu_unlock_domain(e);
put_gfn_and_copyback:
put_gfn(d, gop.mfn);
- page->count_info &= ~(PGC_count_mask|PGC_allocated);
+ /* The count_info has already been cleaned */
free_domheap_page(page);
goto copyback;
}
@@ -2202,10 +2202,9 @@ gnttab_transfer(
copy_domain_page(page_to_mfn(new_page), mfn);
- page->count_info &= ~(PGC_count_mask|PGC_allocated);
+ /* The count_info has already been cleared */
free_domheap_page(page);
page = new_page;
- page->count_info = PGC_allocated | 1;
mfn = page_to_mfn(page);
}
@@ -2245,12 +2244,17 @@ gnttab_transfer(
*/
spin_unlock(&e->page_alloc_lock);
okay = gnttab_prepare_for_transfer(e, d, gop.ref);
- spin_lock(&e->page_alloc_lock);
- if ( unlikely(!okay) || unlikely(e->is_dying) )
+ if ( unlikely(!okay || assign_pages(e, page, 0, MEMF_no_refcount)) )
{
- bool_t drop_dom_ref = !domain_adjust_tot_pages(e, -1);
+ bool drop_dom_ref;
+ /*
+ * Need to grab this again to safely free our "reserved"
+ * page in the page total
+ */
+ spin_lock(&e->page_alloc_lock);
+ drop_dom_ref = !domain_adjust_tot_pages(e, -1);
spin_unlock(&e->page_alloc_lock);
if ( okay /* i.e. e->is_dying due to the surrounding if() */ )
@@ -2263,10 +2267,6 @@ gnttab_transfer(
goto unlock_and_copyback;
}
- page_list_add_tail(page, &e->page_list);
- page_set_owner(page, e);
-
- spin_unlock(&e->page_alloc_lock);
put_gfn(d, gop.mfn);
TRACE_1D(TRC_MEM_PAGE_GRANT_TRANSFER, e->domain_id);
diff --git a/xen/common/memory.c b/xen/common/memory.c
index 4fb7962c79..f71163221f 100644
--- a/xen/common/memory.c
+++ b/xen/common/memory.c
@@ -675,20 +675,22 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
* Success! Beyond this point we cannot fail for this chunk.
*/
- /* Destroy final reference to each input page. */
+ /*
+ * These pages have already had owner and reference cleared.
+ * Do the final two steps: Remove from the physmap, and free
+ * them.
+ */
while ( (page = page_list_remove_head(&in_chunk_list)) )
{
unsigned long gfn;
- if ( !test_and_clear_bit(_PGC_allocated, &page->count_info) )
- BUG();
mfn = page_to_mfn(page);
gfn = mfn_to_gmfn(d, mfn_x(mfn));
/* Pages were unshared above */
BUG_ON(SHARED_M2P(gfn));
if ( guest_physmap_remove_page(d, _gfn(gfn), mfn, 0) )
domain_crash(d);
- put_page(page);
+ free_domheap_page(page);
}
/* Assign each output page to the domain. */
@@ -761,13 +763,16 @@ static long memory_exchange(XEN_GUEST_HANDLE_PARAM(xen_memory_exchange_t) arg)
* chunks succeeded.
*/
fail:
- /* Reassign any input pages we managed to steal. */
+ /*
+ * Reassign any input pages we managed to steal. NB that if the assign
+ * fails again, we're on the hook for freeing the page, since we've already
+ * cleared PGC_allocated.
+ */
while ( (page = page_list_remove_head(&in_chunk_list)) )
if ( assign_pages(d, page, 0, MEMF_no_refcount) )
{
BUG_ON(!d->is_dying);
- if ( test_and_clear_bit(_PGC_allocated, &page->count_info) )
- put_page(page);
+ free_domheap_page(page);
}
dying:
diff --git a/xen/common/page_alloc.c b/xen/common/page_alloc.c
index 482f0988f7..52da7762e3 100644
--- a/xen/common/page_alloc.c
+++ b/xen/common/page_alloc.c
@@ -2221,7 +2221,7 @@ int assign_pages(
for ( i = 0; i < (1 << order); i++ )
{
ASSERT(page_get_owner(&pg[i]) == NULL);
- ASSERT((pg[i].count_info & ~(PGC_allocated | 1)) == 0);
+ ASSERT(!pg[i].count_info);
page_set_owner(&pg[i], d);
smp_wmb(); /* Domain pointer must be visible before updating refcnt. */
pg[i].count_info = PGC_allocated | 1;
--
2.20.1

View file

@ -1,308 +0,0 @@
From 5d3a02e320f88747b75e3794c2e694284ae64c3e Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Wed, 23 Jan 2019 11:57:46 +0000
Subject: [PATCH] xen: Make coherent PV IOMMU discipline
In order for a PV domain to set up DMA from a passed-through device to
one of its pages, the page must be mapped in the IOMMU. On the other
hand, before a PV page may be used as a "special" page type (such as a
pagetable or descriptor table), it _must not_ be writable in the IOMMU
(otherwise a malicious guest could DMA arbitrary page tables into the
memory, bypassing Xen's safety checks); and Xen's current rule is to
have such pages not in the IOMMU at all.
At the moment, in order to accomplish this, the code borrows HVM
domain's "physmap" concept: When a page is assigned to a guest,
guess_physmap_add_entry() is called, which for PV guests, will create
a writable IOMMU mapping; and when a page is removed,
guest_physmap_remove_entry() is called, which will remove the mapping.
Additionally, when a page gains the PGT_writable page type, the page
will be added into the IOMMU; and when the page changes away from a
PGT_writable type, the page will be removed from the IOMMU.
Unfortunately, borrowing the "physmap" concept from HVM domains is
problematic. HVM domains have a lock on their p2m tables, ensuring
synchronization between modifications to the p2m; and all hypercall
parameters must first be translated through the p2m before being used.
Trying to mix this locked-and-gated approach with PV's lock-free
approach leads to several races and inconsistencies:
* A race between a page being assigned and it being put into the
physmap; for example:
- P1: call populate_physmap() { A = allocate_domheap_pages() }
- P2: Guess page A's mfn, and call decrease_reservation(A). A is owned by the domain,
and so Xen will clear the PGC_allocated bit and free the page
- P1: finishes populate_physmap() { guest_physmap_add_entry() }
Now the domain has a writable IOMMU mapping to a page it no longer owns.
* Pages start out as type PGT_none, but with a writable IOMMU mapping.
If a guest uses a page as a page table without ever having created a
writable mapping, the IOMMU mapping will not be removed; the guest
will have a writable IOMMU mapping to a page it is currently using
as a page table.
* A newly-allocated page can be DMA'd into with no special actions on
the part of the guest; However, if a page is promoted to a
non-writable type, the page must be mapped with a writable type before
DMA'ing to it again, or the transaction will fail.
To fix this, do away with the "PV physmap" concept entirely, and
replace it with the following IOMMU discipline for PV guests:
- (type == PGT_writable) <=> in iommu (even if type_count == 0)
- Upon a final put_page(), check to see if type is PGT_writable; if so,
iommu_unmap.
In order to achieve that:
- Remove PV IOMMU related code from guest_physmap_*
- Repurpose cleanup_page_cacheattr() into a general
cleanup_page_mappings() function, which will both fix up Xen
mappings for pages with special cache attributes, and also check for
a PGT_writable type and remove pages if appropriate.
- For compatibility with current guests, grab-and-release a
PGT_writable_page type for PV guests in guest_physmap_add_entry().
This will cause most "normal" guest pages to start out life with
PGT_writable_page type (and thus an IOMMU mapping), but no type
count (so that they can be used as special cases at will).
Also, note that there is one exception to to the "PGT_writable => in
iommu" rule: xenheap pages shared with guests may be given a
PGT_writable type with one type reference. This reference prevents
the type from changing, which in turn prevents page from gaining an
IOMMU mapping in get_page_type(). It's not clear whether this was
intentional or not, but it's not something to change in a security
update.
This is XSA-288.
Reported-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 95 +++++++++++++++++++++++++++++++++++++++----
xen/arch/x86/mm/p2m.c | 57 ++++++++++++--------------
2 files changed, 111 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index d8ff58c901..ad8aacad68 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -81,6 +81,22 @@
* OS's, which will generally use the WP bit to simplify copy-on-write
* implementation (in that case, OS wants a fault when it writes to
* an application-supplied buffer).
+ *
+ * PV domUs and IOMMUs:
+ * --------------------
+ * For a guest to be able to DMA into a page, that page must be in the
+ * domain's IOMMU. However, we *must not* allow DMA into 'special'
+ * pages (such as page table pages, descriptor tables, &c); and we
+ * must also ensure that mappings are removed from the IOMMU when the
+ * page is freed. Finally, it is inherently racy to make any changes
+ * based on a page with a non-zero type count.
+ *
+ * To that end, we put the page in the IOMMU only when a page gains
+ * the PGT_writeable type; and we remove the page when it loses the
+ * PGT_writeable type (not when the type count goes to zero). This
+ * effectively protects the IOMMU status update with the type count we
+ * have just acquired. We must also check for PGT_writable type when
+ * doing the final put_page(), and remove it from the iommu if so.
*/
#include <xen/init.h>
@@ -2275,19 +2291,79 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
return rc;
}
-static int cleanup_page_cacheattr(struct page_info *page)
+/*
+ * In the course of a page's use, it may have caused other secondary
+ * mappings to have changed:
+ * - Xen's mappings may have been changed to accomodate the requested
+ * cache attibutes
+ * - A page may have been put into the IOMMU of a PV guest when it
+ * gained a writable mapping.
+ *
+ * Now that the page is being freed, clean up these mappings if
+ * appropriate. NB that at this point the page is still "allocated",
+ * but not "live" (i.e., its refcount is 0), so it's safe to read the
+ * count_info, owner, and type_info without synchronization.
+ */
+static int cleanup_page_mappings(struct page_info *page)
{
unsigned int cacheattr =
(page->count_info & PGC_cacheattr_mask) >> PGC_cacheattr_base;
+ int rc = 0;
+ unsigned long mfn = mfn_x(page_to_mfn(page));
- if ( likely(cacheattr == 0) )
- return 0;
+ /*
+ * If we've modified xen mappings as a result of guest cache
+ * attributes, restore them to the "normal" state.
+ */
+ if ( unlikely(cacheattr) )
+ {
+ page->count_info &= ~PGC_cacheattr_mask;
- page->count_info &= ~PGC_cacheattr_mask;
+ BUG_ON(is_xen_heap_page(page));
- BUG_ON(is_xen_heap_page(page));
+ rc = update_xen_mappings(mfn, 0);
+ }
- return update_xen_mappings(mfn_x(page_to_mfn(page)), 0);
+ /*
+ * If this may be in a PV domain's IOMMU, remove it.
+ *
+ * NB that writable xenheap pages have their type set and cleared by
+ * implementation-specific code, rather than by get_page_type(). As such:
+ * - They aren't expected to have an IOMMU mapping, and
+ * - We don't necessarily expect the type count to be zero when the final
+ * put_page happens.
+ *
+ * Go ahead and attemp to call iommu_unmap() on xenheap pages anyway, just
+ * in case; but only ASSERT() that the type count is zero and remove the
+ * PGT_writable type for non-xenheap pages.
+ */
+ if ( (page->u.inuse.type_info & PGT_type_mask) == PGT_writable_page )
+ {
+ struct domain *d = page_get_owner(page);
+
+ if ( d && is_pv_domain(d) && unlikely(need_iommu(d)) )
+ {
+ int rc2 = iommu_unmap_page(d, mfn);
+
+ if ( !rc )
+ rc = rc2;
+ }
+
+ if ( likely(!is_xen_heap_page(page)) )
+ {
+ ASSERT((page->u.inuse.type_info &
+ (PGT_type_mask | PGT_count_mask)) == PGT_writable_page);
+ /*
+ * Clear the type to record the fact that all writable mappings
+ * have been removed. But if either operation failed, leave
+ * type_info alone.
+ */
+ if ( likely(!rc) )
+ page->u.inuse.type_info &= ~(PGT_type_mask | PGT_count_mask);
+ }
+ }
+
+ return rc;
}
void put_page(struct page_info *page)
@@ -2303,7 +2379,7 @@ void put_page(struct page_info *page)
if ( unlikely((nx & PGC_count_mask) == 0) )
{
- if ( cleanup_page_cacheattr(page) == 0 )
+ if ( !cleanup_page_mappings(page) )
free_domheap_page(page);
else
gdprintk(XENLOG_WARNING,
@@ -4020,9 +4096,10 @@ int steal_page(
* NB this is safe even if the page ends up being given back to
* the domain, because the count is zero: subsequent mappings will
* cause the cache attributes to be re-instated inside
- * get_page_from_l1e().
+ * get_page_from_l1e(), or the page to be added back to the IOMMU
+ * upon the type changing to PGT_writeable, as appropriate.
*/
- if ( (rc = cleanup_page_cacheattr(page)) )
+ if ( (rc = cleanup_page_mappings(page)) )
{
/*
* Couldn't fixup Xen's mappings; put things the way we found
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index c53cab44d9..2b62bc61dd 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -708,23 +708,9 @@ p2m_remove_page(struct p2m_domain *p2m, unsigned long gfn_l, unsigned long mfn,
p2m_type_t t;
p2m_access_t a;
+ /* IOMMU for PV guests is handled in get_page_type() and put_page(). */
if ( !paging_mode_translate(p2m->domain) )
- {
- int rc = 0;
-
- if ( need_iommu(p2m->domain) )
- {
- for ( i = 0; i < (1 << page_order); i++ )
- {
- int ret = iommu_unmap_page(p2m->domain, mfn + i);
-
- if ( !rc )
- rc = ret;
- }
- }
-
- return rc;
- }
+ return 0;
ASSERT(gfn_locked_by_me(p2m, gfn));
P2M_DEBUG("removing gfn=%#lx mfn=%#lx\n", gfn_l, mfn);
@@ -769,26 +755,33 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
int pod_count = 0;
int rc = 0;
+ /* IOMMU for PV guests is handled in get_page_type() and put_page(). */
if ( !paging_mode_translate(d) )
{
- if ( need_iommu(d) && t == p2m_ram_rw )
- {
- for ( i = 0; i < (1 << page_order); i++ )
- {
- rc = iommu_map_page(d, mfn_x(mfn_add(mfn, i)),
- mfn_x(mfn_add(mfn, i)),
- IOMMUF_readable|IOMMUF_writable);
- if ( rc != 0 )
- {
- while ( i-- > 0 )
- /* If statement to satisfy __must_check. */
- if ( iommu_unmap_page(d, mfn_x(mfn_add(mfn, i))) )
- continue;
+ struct page_info *page = mfn_to_page(mfn);
- return rc;
- }
- }
+ /*
+ * Our interface for PV guests wrt IOMMU entries hasn't been very
+ * clear; but historically, pages have started out with IOMMU mappings,
+ * and only lose them when changed to a different page type.
+ *
+ * Retain this property by grabbing a writable type ref and then
+ * dropping it immediately. The result will be pages that have a
+ * writable type (and an IOMMU entry), but a count of 0 (such that
+ * any guest-requested type changes succeed and remove the IOMMU
+ * entry).
+ */
+ if ( !need_iommu(d) || t != p2m_ram_rw )
+ return 0;
+
+ for ( i = 0; i < (1UL << page_order); ++i, ++page )
+ {
+ if ( get_page_and_type(page, d, PGT_writable_page) )
+ put_page_and_type(page);
+ else
+ return -EINVAL;
}
+
return 0;
}
--
2.20.1

View file

@ -1,237 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/mm: also allow L2 (un)validation to be preemptible
Commit c612481d1c ("x86/mm: Plumbing to allow any PTE update to fail
with -ERESTART") added assertions next to the {alloc,free}_l2_table()
invocations to document (and validate in debug builds) that L2
(un)validations are always preemptible.
The assertion in free_page_type() was now observed to trigger when
recursive L2 page tables get cleaned up.
In particular put_page_from_l2e()'s assumption that _put_page_type()
would always succeed is now wrong, resulting in a partially un-validated
page left in a domain, which has no other means of getting cleaned up
later on. If not causing any problems earlier, this would ultimately
trigger the check for ->u.inuse.type_info having a zero count when
freeing the page during cleanup after the domain has died.
As a result it should be considered a mistake to not have extended
preemption fully to L2 when it was added to L3/L4 table handling, which
this change aims to correct.
The validation side additions are done just for symmetry.
This is part of XSA-290.
Reported-by: Manuel Bouyer <bouyer@antioche.eu.org>
Tested-by: Manuel Bouyer <bouyer@antioche.eu.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1126,7 +1126,7 @@ get_page_from_l1e(
define_get_linear_pagetable(l2);
static int
get_page_from_l2e(
- l2_pgentry_t l2e, unsigned long pfn, struct domain *d)
+ l2_pgentry_t l2e, unsigned long pfn, struct domain *d, int partial)
{
unsigned long mfn = l2e_get_pfn(l2e);
int rc;
@@ -1141,7 +1141,8 @@ get_page_from_l2e(
return -EINVAL;
}
- rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, 0, 0);
+ rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d,
+ partial, false);
if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
rc = 0;
@@ -1295,8 +1296,11 @@ void put_page_from_l1e(l1_pgentry_t l1e,
* NB. Virtual address 'l2e' maps to a machine address within frame 'pfn'.
* Note also that this automatically deals correctly with linear p.t.'s.
*/
-static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn)
+static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
+ int partial, bool defer)
{
+ int rc = 0;
+
if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) || (l2e_get_pfn(l2e) == pfn) )
return 1;
@@ -1311,13 +1315,27 @@ static int put_page_from_l2e(l2_pgentry_
else
{
struct page_info *pg = l2e_get_page(l2e);
- int rc = _put_page_type(pg, false, mfn_to_page(_mfn(pfn)));
+ struct page_info *ptpg = mfn_to_page(_mfn(pfn));
- ASSERT(!rc);
- put_page(pg);
+ if ( unlikely(partial > 0) )
+ {
+ ASSERT(!defer);
+ rc = _put_page_type(pg, true, ptpg);
+ }
+ else if ( defer )
+ {
+ current->arch.old_guest_ptpg = ptpg;
+ current->arch.old_guest_table = pg;
+ }
+ else
+ {
+ rc = _put_page_type(pg, true, ptpg);
+ if ( likely(!rc) )
+ put_page(pg);
+ }
}
- return 0;
+ return rc;
}
static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
@@ -1487,11 +1505,12 @@ static int alloc_l2_table(struct page_in
unsigned long pfn = mfn_x(page_to_mfn(page));
l2_pgentry_t *pl2e;
unsigned int i;
- int rc = 0;
+ int rc = 0, partial = page->partial_pte;
pl2e = map_domain_page(_mfn(pfn));
- for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES; i++ )
+ for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES;
+ i++, partial = 0 )
{
if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
{
@@ -1501,23 +1520,33 @@ static int alloc_l2_table(struct page_in
}
if ( !is_guest_l2_slot(d, type, i) ||
- (rc = get_page_from_l2e(pl2e[i], pfn, d)) > 0 )
+ (rc = get_page_from_l2e(pl2e[i], pfn, d, partial)) > 0 )
continue;
- if ( unlikely(rc == -ERESTART) )
+ if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- break;
+ page->partial_pte = partial ?: 1;
}
-
- if ( rc < 0 )
+ else if ( rc == -EINTR && i )
+ {
+ page->nr_validated_ptes = i;
+ page->partial_pte = 0;
+ rc = -ERESTART;
+ }
+ else if ( rc < 0 && rc != -EINTR )
{
gdprintk(XENLOG_WARNING, "Failure in alloc_l2_table: slot %#x\n", i);
- while ( i-- > 0 )
- if ( is_guest_l2_slot(d, type, i) )
- put_page_from_l2e(pl2e[i], pfn);
- break;
+ if ( i )
+ {
+ page->nr_validated_ptes = i;
+ page->partial_pte = 0;
+ current->arch.old_guest_ptpg = NULL;
+ current->arch.old_guest_table = page;
+ }
}
+ if ( rc < 0 )
+ break;
pl2e[i] = adjust_guest_l2e(pl2e[i], d);
}
@@ -1797,28 +1826,50 @@ static int free_l2_table(struct page_inf
struct domain *d = page_get_owner(page);
unsigned long pfn = mfn_x(page_to_mfn(page));
l2_pgentry_t *pl2e;
- unsigned int i = page->nr_validated_ptes - 1;
- int err = 0;
+ int rc = 0, partial = page->partial_pte;
+ unsigned int i = page->nr_validated_ptes - !partial;
pl2e = map_domain_page(_mfn(pfn));
- ASSERT(page->nr_validated_ptes);
- do {
- if ( is_guest_l2_slot(d, page->u.inuse.type_info, i) &&
- put_page_from_l2e(pl2e[i], pfn) == 0 &&
- i && hypercall_preempt_check() )
+ for ( ; ; )
+ {
+ if ( is_guest_l2_slot(d, page->u.inuse.type_info, i) )
+ rc = put_page_from_l2e(pl2e[i], pfn, partial, false);
+ if ( rc < 0 )
+ break;
+
+ partial = 0;
+
+ if ( !i-- )
+ break;
+
+ if ( hypercall_preempt_check() )
{
- page->nr_validated_ptes = i;
- err = -ERESTART;
+ rc = -EINTR;
+ break;
}
- } while ( !err && i-- );
+ }
unmap_domain_page(pl2e);
- if ( !err )
+ if ( rc >= 0 )
+ {
page->u.inuse.type_info &= ~PGT_pae_xen_l2;
+ rc = 0;
+ }
+ else if ( rc == -ERESTART )
+ {
+ page->nr_validated_ptes = i;
+ page->partial_pte = partial ?: -1;
+ }
+ else if ( rc == -EINTR && i < L2_PAGETABLE_ENTRIES - 1 )
+ {
+ page->nr_validated_ptes = i + 1;
+ page->partial_pte = 0;
+ rc = -ERESTART;
+ }
- return err;
+ return rc;
}
static int free_l3_table(struct page_info *page)
@@ -2138,7 +2189,7 @@ static int mod_l2_entry(l2_pgentry_t *pl
return -EBUSY;
}
- if ( unlikely((rc = get_page_from_l2e(nl2e, pfn, d)) < 0) )
+ if ( unlikely((rc = get_page_from_l2e(nl2e, pfn, d, 0)) < 0) )
return rc;
nl2e = adjust_guest_l2e(nl2e, d);
@@ -2157,7 +2208,8 @@ static int mod_l2_entry(l2_pgentry_t *pl
return -EBUSY;
}
- put_page_from_l2e(ol2e, pfn);
+ put_page_from_l2e(ol2e, pfn, 0, true);
+
return rc;
}

View file

@ -1,71 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/mm: add explicit preemption checks to L3 (un)validation
When recursive page tables are used at the L3 level, unvalidation of a
single L4 table may incur unvalidation of two levels of L3 tables, i.e.
a maximum iteration count of 512^3 for unvalidating an L4 table. The
preemption check in free_l2_table() as well as the one in
_put_page_type() may never be reached, so explicit checking is needed in
free_l3_table().
When recursive page tables are used at the L4 level, the iteration count
at L4 alone is capped at 512^2. As soon as a present L3 entry is hit
which itself needs unvalidation (and hence requiring another nested loop
with 512 iterations), the preemption checks added here kick in, so no
further preemption checking is needed at L4 (until we decide to permit
5-level paging for PV guests).
The validation side additions are done just for symmetry.
This is part of XSA-290.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1581,6 +1581,13 @@ static int alloc_l3_table(struct page_in
for ( i = page->nr_validated_ptes; i < L3_PAGETABLE_ENTRIES;
i++, partial = 0 )
{
+ if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
+ {
+ page->nr_validated_ptes = i;
+ rc = -ERESTART;
+ break;
+ }
+
if ( is_pv_32bit_domain(d) && (i == 3) )
{
if ( !(l3e_get_flags(pl3e[i]) & _PAGE_PRESENT) ||
@@ -1882,15 +1889,25 @@ static int free_l3_table(struct page_inf
pl3e = map_domain_page(_mfn(pfn));
- do {
+ for ( ; ; )
+ {
rc = put_page_from_l3e(pl3e[i], pfn, partial, 0);
if ( rc < 0 )
break;
+
partial = 0;
- if ( rc > 0 )
- continue;
- pl3e[i] = unadjust_guest_l3e(pl3e[i], d);
- } while ( i-- );
+ if ( rc == 0 )
+ pl3e[i] = unadjust_guest_l3e(pl3e[i], d);
+
+ if ( !i-- )
+ break;
+
+ if ( hypercall_preempt_check() )
+ {
+ rc = -EINTR;
+ break;
+ }
+ }
unmap_domain_page(pl3e);

View file

@ -1,53 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/mm: don't retain page type reference when IOMMU operation fails
The IOMMU update in _get_page_type() happens between recording of the
new reference and validation of the page for its new type (if
necessary). If the IOMMU operation fails, there's no point in actually
carrying out validation. Furthermore, with this resulting in failure
getting indicated to the caller, the recorded type reference also needs
to be dropped again.
Note that in case of failure of alloc_page_type() there's no need to
undo the IOMMU operation: Only special types get handed to the function.
The function, upon failure, clears ->u.inuse.type_info, effectively
converting the page to PGT_none. The IOMMU mapping, however, solely
depends on whether the type is PGT_writable_page.
This is XSA-291.
Reported-by: Igor Druzhinin <igor.druzhinin@citrix.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -2751,6 +2751,13 @@ static int _get_page_type(struct page_in
iommu_ret = iommu_map_page(d, gfn_x(gfn),
mfn_x(page_to_mfn(page)),
IOMMUF_readable|IOMMUF_writable);
+
+ if ( unlikely(iommu_ret) )
+ {
+ _put_page_type(page, false, NULL);
+ rc = iommu_ret;
+ goto out;
+ }
}
}
@@ -2765,12 +2772,10 @@ static int _get_page_type(struct page_in
rc = alloc_page_type(page, type, preemptible);
}
+ out:
if ( (x & PGT_partial) && !(nx & PGT_partial) )
put_page(page);
- if ( !rc )
- rc = iommu_ret;
-
return rc;
}

View file

@ -1,95 +0,0 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/mm: properly flush TLB in switch_cr3_cr4()
The CR3 values used for contexts run with PCID enabled uniformly have
CR3.NOFLUSH set, resulting in the CR3 write itself to not cause any
flushing at all. When the second CR4 write is skipped or doesn't do any
flushing, there's nothing so far which would purge TLB entries which may
have accumulated again if the PCID doesn't change; the "just in case"
flush only affects the case where the PCID actually changes. (There may
be particularly many TLB entries re-accumulated in case of a watchdog
NMI kicking in during the critical time window.)
Suppress the no-flush behavior of the CR3 write in this particular case.
Similarly the second CR4 write may not cause any flushing of TLB entries
established again while the original PCID was still in use - it may get
performed because of unrelated bits changing. The flush of the old PCID
needs to happen nevertheless.
At the same time also eliminate a possible race with lazy context
switch: Just like for CR4, CR3 may change at any time while interrupts
are enabled, due to the __sync_local_execstate() invocation from the
flush IPI handler. It is for that reason that the CR3 read, just like
the CR4 one, must happen only after interrupts have been turned off.
This is XSA-292.
Reported-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Tested-by: Sergey Dyasli <sergey.dyasli@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
v3: Adjust comments. Drop old_cr4 from the PGE check in the expression
controlling the invocation of invpcid_flush_single_context(), as PGE
is always clear there.
v2: Decouple invpcid_flush_single_context() from 2nd CR4 write.
--- a/xen/arch/x86/flushtlb.c
+++ b/xen/arch/x86/flushtlb.c
@@ -103,9 +103,8 @@ static void do_tlb_flush(void)
void switch_cr3_cr4(unsigned long cr3, unsigned long cr4)
{
- unsigned long flags, old_cr4;
+ unsigned long flags, old_cr4, old_pcid;
u32 t;
- unsigned long old_pcid = cr3_pcid(read_cr3());
/* This non-reentrant function is sometimes called in interrupt context. */
local_irq_save(flags);
@@ -133,15 +132,38 @@ void switch_cr3_cr4(unsigned long cr3, u
*/
invpcid_flush_all_nonglobals();
+ /*
+ * If we don't change PCIDs, the CR3 write below needs to flush this very
+ * PCID, even when a full flush was performed above, as we are currently
+ * accumulating TLB entries again from the old address space.
+ * NB: Clearing the bit when we don't use PCID is benign (as it is clear
+ * already in that case), but allows the if() to be more simple.
+ */
+ old_pcid = cr3_pcid(read_cr3());
+ if ( old_pcid == cr3_pcid(cr3) )
+ cr3 &= ~X86_CR3_NOFLUSH;
+
write_cr3(cr3);
if ( old_cr4 != cr4 )
write_cr4(cr4);
- else if ( old_pcid != cr3_pcid(cr3) )
- /*
- * Make sure no TLB entries related to the old PCID created between
- * flushing the TLB and writing the new %cr3 value remain in the TLB.
- */
+
+ /*
+ * Make sure no TLB entries related to the old PCID created between
+ * flushing the TLB and writing the new %cr3 value remain in the TLB.
+ *
+ * The write to CR4 just above has performed a wider flush in certain
+ * cases, which therefore get excluded here. Since that write is
+ * conditional, note in particular that it won't be skipped if PCIDE
+ * transitions from 1 to 0. This is because the CR4 write further up will
+ * have been skipped in this case, as PCIDE and PGE won't both be set at
+ * the same time.
+ *
+ * Note also that PGE is always clear in old_cr4.
+ */
+ if ( old_pcid != cr3_pcid(cr3) &&
+ !(cr4 & X86_CR4_PGE) &&
+ (old_cr4 & X86_CR4_PCIDE) <= (cr4 & X86_CR4_PCIDE) )
invpcid_flush_single_context(old_pcid);
post_flush(t);

View file

@ -1,317 +0,0 @@
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/pv: Rewrite guest %cr4 handling from scratch
The PV cr4 logic is almost impossible to follow, and leaks bits into guest
context which definitely shouldn't be visible (in particular, VMXE).
The biggest problem however, and source of the complexity, is that it derives
new real and guest cr4 values from the current value in hardware - this is
context dependent and an inappropriate source of information.
Rewrite the cr4 logic to be invariant of the current value in hardware.
First of all, modify write_ptbase() to always use mmu_cr4_features for IDLE
and HVM contexts. mmu_cr4_features *is* the correct value to use, and makes
the ASSERT() obviously redundant.
For PV guests, curr->arch.pv.ctrlreg[4] remains the guests view of cr4, but
all logic gets reworked in terms of this and mmu_cr4_features only.
Two masks are introduced; bits which the guest has control over, and bits
which are forwarded from Xen's settings. One guest-visible change here is
that Xen's VMXE setting is no longer visible at all.
pv_make_cr4() follows fairly closely from pv_guest_cr4_to_real_cr4(), but
deliberately starts with mmu_cr4_features, and only alters the minimal subset
of bits.
The boot-time {compat_,}pv_cr4_mask variables are removed, as they are a
remnant of the pre-CPUID policy days. pv_fixup_guest_cr4() gains a related
derivation from the policy.
Another guest visible change here is that a 32bit PV guest can now flip
FSGSBASE in its view of CR4. While the {RD,WR}{FS,GS}BASE instructions are
unusable outside of a 64bit code segment, the ability to modify FSGSBASE
matches real hardware behaviour, and avoids the need for any 32bit/64bit
differences in the logic.
Overall, this patch shouldn't have a practical change in guest behaviour.
VMXE will disappear from view, and an inquisitive 32bit kernel can now see
FSGSBASE changing, but this new logic is otherwise bug-compatible with before.
This is part of XSA-293
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index b1e50d1..675152a 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -733,49 +733,6 @@ int arch_domain_soft_reset(struct domain *d)
return ret;
}
-/*
- * These are the masks of CR4 bits (subject to hardware availability) which a
- * PV guest may not legitimiately attempt to modify.
- */
-static unsigned long __read_mostly pv_cr4_mask, compat_pv_cr4_mask;
-
-static int __init init_pv_cr4_masks(void)
-{
- unsigned long common_mask = ~X86_CR4_TSD;
-
- /*
- * All PV guests may attempt to modify TSD, DE and OSXSAVE.
- */
- if ( cpu_has_de )
- common_mask &= ~X86_CR4_DE;
- if ( cpu_has_xsave )
- common_mask &= ~X86_CR4_OSXSAVE;
-
- pv_cr4_mask = compat_pv_cr4_mask = common_mask;
-
- /*
- * 64bit PV guests may attempt to modify FSGSBASE.
- */
- if ( cpu_has_fsgsbase )
- pv_cr4_mask &= ~X86_CR4_FSGSBASE;
-
- return 0;
-}
-__initcall(init_pv_cr4_masks);
-
-unsigned long pv_guest_cr4_fixup(const struct vcpu *v, unsigned long guest_cr4)
-{
- unsigned long hv_cr4 = real_cr4_to_pv_guest_cr4(read_cr4());
- unsigned long mask = is_pv_32bit_vcpu(v) ? compat_pv_cr4_mask : pv_cr4_mask;
-
- if ( (guest_cr4 & mask) != (hv_cr4 & mask) )
- printk(XENLOG_G_WARNING
- "d%d attempted to change %pv's CR4 flags %08lx -> %08lx\n",
- current->domain->domain_id, v, hv_cr4, guest_cr4);
-
- return (hv_cr4 & mask) | (guest_cr4 & ~mask);
-}
-
#define xen_vcpu_guest_context vcpu_guest_context
#define fpu_ctxt fpu_ctxt.x
CHECK_FIELD_(struct, vcpu_guest_context, fpu_ctxt);
@@ -789,7 +746,7 @@ int arch_set_info_guest(
struct domain *d = v->domain;
unsigned long cr3_gfn;
struct page_info *cr3_page;
- unsigned long flags, cr4;
+ unsigned long flags;
unsigned int i;
int rc = 0, compat;
@@ -978,9 +935,8 @@ int arch_set_info_guest(
v->arch.pv_vcpu.ctrlreg[0] &= X86_CR0_TS;
v->arch.pv_vcpu.ctrlreg[0] |= read_cr0() & ~X86_CR0_TS;
- cr4 = v->arch.pv_vcpu.ctrlreg[4];
- v->arch.pv_vcpu.ctrlreg[4] = cr4 ? pv_guest_cr4_fixup(v, cr4) :
- real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+ v->arch.pv_vcpu.ctrlreg[4] =
+ pv_fixup_guest_cr4(v, v->arch.pv_vcpu.ctrlreg[4]);
memset(v->arch.debugreg, 0, sizeof(v->arch.debugreg));
for ( i = 0; i < 8; i++ )
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 6509035..08634b7 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -505,33 +505,13 @@ void make_cr3(struct vcpu *v, mfn_t mfn)
v->arch.cr3 |= get_pcid_bits(v, false);
}
-unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v)
-{
- const struct domain *d = v->domain;
- unsigned long cr4;
-
- cr4 = v->arch.pv_vcpu.ctrlreg[4] & ~X86_CR4_DE;
- cr4 |= mmu_cr4_features & (X86_CR4_PSE | X86_CR4_SMEP | X86_CR4_SMAP |
- X86_CR4_OSXSAVE | X86_CR4_FSGSBASE);
-
- if ( d->arch.pv_domain.pcid )
- cr4 |= X86_CR4_PCIDE;
- else if ( !d->arch.pv_domain.xpti )
- cr4 |= X86_CR4_PGE;
-
- cr4 |= d->arch.vtsc ? X86_CR4_TSD : 0;
-
- return cr4;
-}
-
void write_ptbase(struct vcpu *v)
{
struct cpu_info *cpu_info = get_cpu_info();
unsigned long new_cr4;
new_cr4 = (is_pv_vcpu(v) && !is_idle_vcpu(v))
- ? pv_guest_cr4_to_real_cr4(v)
- : ((read_cr4() & ~(X86_CR4_PCIDE | X86_CR4_TSD)) | X86_CR4_PGE);
+ ? pv_make_cr4(v) : mmu_cr4_features;
if ( is_pv_vcpu(v) && v->domain->arch.pv_domain.xpti )
{
@@ -550,8 +530,6 @@ void write_ptbase(struct vcpu *v)
switch_cr3_cr4(v->arch.cr3, new_cr4);
cpu_info->pv_cr3 = 0;
}
-
- ASSERT(is_pv_vcpu(v) || read_cr4() == mmu_cr4_features);
}
/*
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index b75ff6b..3965959 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -97,6 +97,52 @@ static void release_compat_l4(struct vcpu *v)
v->arch.guest_table_user = pagetable_null();
}
+unsigned long pv_fixup_guest_cr4(const struct vcpu *v, unsigned long cr4)
+{
+ const struct cpuid_policy *p = v->domain->arch.cpuid;
+
+ /* Discard attempts to set guest controllable bits outside of the policy. */
+ cr4 &= ~((p->basic.tsc ? 0 : X86_CR4_TSD) |
+ (p->basic.de ? 0 : X86_CR4_DE) |
+ (p->feat.fsgsbase ? 0 : X86_CR4_FSGSBASE) |
+ (p->basic.xsave ? 0 : X86_CR4_OSXSAVE));
+
+ /* Masks expected to be disjoint sets. */
+ BUILD_BUG_ON(PV_CR4_GUEST_MASK & PV_CR4_GUEST_VISIBLE_MASK);
+
+ /*
+ * A guest sees the policy subset of its own choice of guest controllable
+ * bits, and a subset of Xen's choice of certain hardware settings.
+ */
+ return ((cr4 & PV_CR4_GUEST_MASK) |
+ (mmu_cr4_features & PV_CR4_GUEST_VISIBLE_MASK));
+}
+
+unsigned long pv_make_cr4(const struct vcpu *v)
+{
+ const struct domain *d = v->domain;
+ unsigned long cr4 = mmu_cr4_features &
+ ~(X86_CR4_PCIDE | X86_CR4_PGE | X86_CR4_TSD);
+
+ /*
+ * PCIDE or PGE depends on the PCID/XPTI settings, but must not both be
+ * set, as it impacts the safety of TLB flushing.
+ */
+ if ( d->arch.pv_domain.pcid )
+ cr4 |= X86_CR4_PCIDE;
+ else if ( !d->arch.pv_domain.xpti )
+ cr4 |= X86_CR4_PGE;
+
+ /*
+ * TSD is needed if either the guest has elected to use it, or Xen is
+ * virtualising the TSC value the guest sees.
+ */
+ if ( d->arch.vtsc || (v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_TSD) )
+ cr4 |= X86_CR4_TSD;
+
+ return cr4;
+}
+
int switch_compat(struct domain *d)
{
struct vcpu *v;
@@ -191,7 +237,7 @@ int pv_vcpu_initialise(struct vcpu *v)
/* PV guests by default have a 100Hz ticker. */
v->periodic_period = MILLISECS(10);
- v->arch.pv_vcpu.ctrlreg[4] = real_cr4_to_pv_guest_cr4(mmu_cr4_features);
+ v->arch.pv_vcpu.ctrlreg[4] = pv_fixup_guest_cr4(v, 0);
if ( is_pv_32bit_domain(d) )
{
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index ce2ec76..4abbc14 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -32,6 +32,7 @@
#include <asm/hypercall.h>
#include <asm/mc146818rtc.h>
#include <asm/p2m.h>
+#include <asm/pv/domain.h>
#include <asm/pv/traps.h>
#include <asm/shared.h>
#include <asm/traps.h>
@@ -785,8 +786,8 @@ static int write_cr(unsigned int reg, unsigned long val,
}
case 4: /* Write CR4 */
- curr->arch.pv_vcpu.ctrlreg[4] = pv_guest_cr4_fixup(curr, val);
- write_cr4(pv_guest_cr4_to_real_cr4(curr));
+ curr->arch.pv_vcpu.ctrlreg[4] = pv_fixup_guest_cr4(curr, val);
+ write_cr4(pv_make_cr4(curr));
ctxt_switch_levelling(curr);
return X86EMUL_OKAY;
}
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index ec81d78..c8aa8a5 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -610,17 +610,6 @@ bool update_secondary_system_time(struct vcpu *,
void vcpu_show_execution_state(struct vcpu *);
void vcpu_show_registers(const struct vcpu *);
-/* Clean up CR4 bits that are not under guest control. */
-unsigned long pv_guest_cr4_fixup(const struct vcpu *, unsigned long guest_cr4);
-
-/* Convert between guest-visible and real CR4 values. */
-unsigned long pv_guest_cr4_to_real_cr4(const struct vcpu *v);
-
-#define real_cr4_to_pv_guest_cr4(c) \
- ((c) & ~(X86_CR4_PGE | X86_CR4_PSE | X86_CR4_TSD | \
- X86_CR4_OSXSAVE | X86_CR4_SMEP | \
- X86_CR4_FSGSBASE | X86_CR4_SMAP | X86_CR4_PCIDE))
-
#define domain_max_vcpus(d) (is_hvm_domain(d) ? HVM_MAX_VCPUS : MAX_VIRT_CPUS)
static inline struct vcpu_guest_context *alloc_vcpu_guest_context(void)
diff --git a/xen/include/asm-x86/pv/domain.h b/xen/include/asm-x86/pv/domain.h
index 4fea764..4e4710c 100644
--- a/xen/include/asm-x86/pv/domain.h
+++ b/xen/include/asm-x86/pv/domain.h
@@ -59,6 +59,23 @@ int pv_vcpu_initialise(struct vcpu *v);
void pv_domain_destroy(struct domain *d);
int pv_domain_initialise(struct domain *d);
+/*
+ * Bits which a PV guest can toggle in its view of cr4. Some are loaded into
+ * hardware, while some are fully emulated.
+ */
+#define PV_CR4_GUEST_MASK \
+ (X86_CR4_TSD | X86_CR4_DE | X86_CR4_FSGSBASE | X86_CR4_OSXSAVE)
+
+/* Bits which a PV guest may observe from the real hardware settings. */
+#define PV_CR4_GUEST_VISIBLE_MASK \
+ (X86_CR4_PAE | X86_CR4_MCE | X86_CR4_OSFXSR | X86_CR4_OSXMMEXCPT)
+
+/* Given a new cr4 value, construct the resulting guest-visible cr4 value. */
+unsigned long pv_fixup_guest_cr4(const struct vcpu *v, unsigned long cr4);
+
+/* Create a cr4 value to load into hardware, based on vcpu settings. */
+unsigned long pv_make_cr4(const struct vcpu *v);
+
#else /* !CONFIG_PV */
#include <xen/errno.h>
@@ -68,6 +85,8 @@ static inline int pv_vcpu_initialise(struct vcpu *v) { return -EOPNOTSUPP; }
static inline void pv_domain_destroy(struct domain *d) {}
static inline int pv_domain_initialise(struct domain *d) { return -EOPNOTSUPP; }
+static inline unsigned long pv_make_cr4(const struct vcpu *v) { return ~0ul; }
+
#endif /* CONFIG_PV */
void paravirt_ctxt_switch_from(struct vcpu *v);

View file

@ -1,260 +0,0 @@
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: x86/pv: Don't have %cr4.fsgsbase active behind a guest kernels back
Currently, a 64bit PV guest can appear to set and clear FSGSBASE in %cr4, but
the bit remains set in hardware. Therefore, the {RD,WR}{FS,GS}BASE are usable
even when the guest kernel believes that they are disabled.
The FSGSBASE feature isn't currently supported in Linux, and its context
switch path has some optimisations which rely on userspace being unable to use
the WR{FS,GS}BASE instructions. Xen's current behaviour undermines this
expectation.
In 64bit PV guest context, always load the guest kernels setting of FSGSBASE
into %cr4. This requires adjusting how Xen uses the {RD,WR}{FS,GS}BASE
instructions.
* Delete the cpu_has_fsgsbase helper. It is no longer safe, as users need to
check %cr4 directly.
* The raw __rd{fs,gs}base() helpers are only safe to use when %cr4.fsgsbase
is set. Comment this property.
* The {rd,wr}{fs,gs}{base,shadow}() and read_msr() helpers are updated to use
the current %cr4 value to determine which mechanism to use.
* toggle_guest_mode() and save_segments() are update to avoid reading
fs/gsbase if the values in hardware cannot be stale WRT struct vcpu. A
consequence of this is that the write_cr() path needs to cache the current
bases, as subsequent context switches will skip saving the values.
* write_cr4() is updated to ensure that the shadow %cr4.fsgsbase value is
observed in a safe way WRT the hardware setting, if an interrupt happens to
hit in the middle.
* pv_make_cr4() is updated for 64bit PV guests to use the guest kernels
choice of FSGSBASE.
This is part of XSA-293
Reported-by: Andy Lutomirski <luto@kernel.org>
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 675152a..29f892c 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1433,7 +1433,8 @@ static void save_segments(struct vcpu *v)
regs->fs = read_sreg(fs);
regs->gs = read_sreg(gs);
- if ( cpu_has_fsgsbase && !is_pv_32bit_vcpu(v) )
+ /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
+ if ( (read_cr4() & X86_CR4_FSGSBASE) && !is_pv_32bit_vcpu(v) )
{
v->arch.pv_vcpu.fs_base = __rdfsbase();
if ( v->arch.flags & TF_kernel_mode )
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index 3965959..228a174 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -140,6 +140,16 @@ unsigned long pv_make_cr4(const struct vcpu *v)
if ( d->arch.vtsc || (v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_TSD) )
cr4 |= X86_CR4_TSD;
+ /*
+ * The {RD,WR}{FS,GS}BASE are only useable in 64bit code segments. While
+ * we must not have CR4.FSGSBASE set behind the back of a 64bit PV kernel,
+ * we do leave it set in 32bit PV context to speed up Xen's context switch
+ * path.
+ */
+ if ( !is_pv_32bit_domain(d) &&
+ !(v->arch.pv_vcpu.ctrlreg[4] & X86_CR4_FSGSBASE) )
+ cr4 &= ~X86_CR4_FSGSBASE;
+
return cr4;
}
@@ -375,7 +385,8 @@ void toggle_guest_mode(struct vcpu *v)
{
ASSERT(!is_pv_32bit_vcpu(v));
- if ( cpu_has_fsgsbase )
+ /* %fs/%gs bases can only be stale if WR{FS,GS}BASE are usable. */
+ if ( read_cr4() & X86_CR4_FSGSBASE )
{
if ( v->arch.flags & TF_kernel_mode )
v->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
diff --git a/xen/arch/x86/pv/emul-priv-op.c b/xen/arch/x86/pv/emul-priv-op.c
index 4abbc14..312c1ee 100644
--- a/xen/arch/x86/pv/emul-priv-op.c
+++ b/xen/arch/x86/pv/emul-priv-op.c
@@ -786,6 +786,17 @@ static int write_cr(unsigned int reg, unsigned long val,
}
case 4: /* Write CR4 */
+ /*
+ * If this write will disable FSGSBASE, refresh Xen's idea of the
+ * guest bases now that they can no longer change.
+ */
+ if ( (curr->arch.pv_vcpu.ctrlreg[4] & X86_CR4_FSGSBASE) &&
+ !(val & X86_CR4_FSGSBASE) )
+ {
+ curr->arch.pv_vcpu.fs_base = __rdfsbase();
+ curr->arch.pv_vcpu.gs_base_kernel = __rdgsbase();
+ }
+
curr->arch.pv_vcpu.ctrlreg[4] = pv_fixup_guest_cr4(curr, val);
write_cr4(pv_make_cr4(curr));
ctxt_switch_levelling(curr);
@@ -835,14 +846,15 @@ static int read_msr(unsigned int reg, uint64_t *val,
case MSR_FS_BASE:
if ( is_pv_32bit_domain(currd) )
break;
- *val = cpu_has_fsgsbase ? __rdfsbase() : curr->arch.pv_vcpu.fs_base;
+ *val = (read_cr4() & X86_CR4_FSGSBASE) ? __rdfsbase()
+ : curr->arch.pv_vcpu.fs_base;
return X86EMUL_OKAY;
case MSR_GS_BASE:
if ( is_pv_32bit_domain(currd) )
break;
- *val = cpu_has_fsgsbase ? __rdgsbase()
- : curr->arch.pv_vcpu.gs_base_kernel;
+ *val = (read_cr4() & X86_CR4_FSGSBASE) ? __rdgsbase()
+ : curr->arch.pv_vcpu.gs_base_kernel;
return X86EMUL_OKAY;
case MSR_SHADOW_GS_BASE:
diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index ecb0149..a353d76 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -1567,7 +1567,7 @@ void __init noreturn __start_xen(unsigned long mbi_p)
cr4_pv32_mask = mmu_cr4_features & XEN_CR4_PV32_BITS;
- if ( cpu_has_fsgsbase )
+ if ( boot_cpu_has(X86_FEATURE_FSGSBASE) )
set_in_cr4(X86_CR4_FSGSBASE);
if ( opt_invpcid && cpu_has_invpcid )
diff --git a/xen/include/asm-x86/cpufeature.h b/xen/include/asm-x86/cpufeature.h
index b237da1..861cb0a 100644
--- a/xen/include/asm-x86/cpufeature.h
+++ b/xen/include/asm-x86/cpufeature.h
@@ -90,7 +90,6 @@
#define cpu_has_xsaves boot_cpu_has(X86_FEATURE_XSAVES)
/* CPUID level 0x00000007:0.ebx */
-#define cpu_has_fsgsbase boot_cpu_has(X86_FEATURE_FSGSBASE)
#define cpu_has_bmi1 boot_cpu_has(X86_FEATURE_BMI1)
#define cpu_has_hle boot_cpu_has(X86_FEATURE_HLE)
#define cpu_has_avx2 boot_cpu_has(X86_FEATURE_AVX2)
diff --git a/xen/include/asm-x86/msr.h b/xen/include/asm-x86/msr.h
index afbeb7f..1ba6ee3 100644
--- a/xen/include/asm-x86/msr.h
+++ b/xen/include/asm-x86/msr.h
@@ -120,6 +120,14 @@ static inline uint64_t rdtsc_ordered(void)
: "=a" (low), "=d" (high) \
: "c" (counter))
+/*
+ * On hardware supporting FSGSBASE, the value loaded into hardware is the
+ * guest kernel's choice for 64bit PV guests (Xen's choice for Idle, HVM and
+ * 32bit PV).
+ *
+ * Therefore, the {RD,WR}{FS,GS}BASE instructions are only safe to use if
+ * %cr4.fsgsbase is set.
+ */
static inline unsigned long __rdfsbase(void)
{
unsigned long base;
@@ -150,7 +158,7 @@ static inline unsigned long rdfsbase(void)
{
unsigned long base;
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
return __rdfsbase();
rdmsrl(MSR_FS_BASE, base);
@@ -162,7 +170,7 @@ static inline unsigned long rdgsbase(void)
{
unsigned long base;
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
return __rdgsbase();
rdmsrl(MSR_GS_BASE, base);
@@ -174,7 +182,7 @@ static inline unsigned long rdgsshadow(void)
{
unsigned long base;
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
{
asm volatile ( "swapgs" );
base = __rdgsbase();
@@ -188,7 +196,7 @@ static inline unsigned long rdgsshadow(void)
static inline void wrfsbase(unsigned long base)
{
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
#ifdef HAVE_AS_FSGSBASE
asm volatile ( "wrfsbase %0" :: "r" (base) );
#else
@@ -200,7 +208,7 @@ static inline void wrfsbase(unsigned long base)
static inline void wrgsbase(unsigned long base)
{
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
#ifdef HAVE_AS_FSGSBASE
asm volatile ( "wrgsbase %0" :: "r" (base) );
#else
@@ -212,7 +220,7 @@ static inline void wrgsbase(unsigned long base)
static inline void wrgsshadow(unsigned long base)
{
- if ( cpu_has_fsgsbase )
+ if ( read_cr4() & X86_CR4_FSGSBASE )
{
asm volatile ( "swapgs\n\t"
#ifdef HAVE_AS_FSGSBASE
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 2bd9e69..8e253dc 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -305,11 +305,31 @@ static inline unsigned long read_cr4(void)
static inline void write_cr4(unsigned long val)
{
+ struct cpu_info *info = get_cpu_info();
+
/* No global pages in case of PCIDs enabled! */
ASSERT(!(val & X86_CR4_PGE) || !(val & X86_CR4_PCIDE));
- get_cpu_info()->cr4 = val;
- asm volatile ( "mov %0,%%cr4" : : "r" (val) );
+ /*
+ * On hardware supporting FSGSBASE, the value in %cr4 is the kernel's
+ * choice for 64bit PV guests, which impacts whether Xen can use the
+ * instructions.
+ *
+ * The {rd,wr}{fs,gs}base() helpers use info->cr4 to work out whether it
+ * is safe to execute the {RD,WR}{FS,GS}BASE instruction, falling back to
+ * the MSR path if not. Some users require interrupt safety.
+ *
+ * If FSGSBASE is currently or about to become clear, reflect this in
+ * info->cr4 before updating %cr4, so an interrupt which hits in the
+ * middle won't observe FSGSBASE set in info->cr4 but clear in %cr4.
+ */
+ info->cr4 = val & (info->cr4 | ~X86_CR4_FSGSBASE);
+
+ asm volatile ( "mov %[val], %%cr4"
+ : "+m" (info->cr4) /* Force ordering without a barrier. */
+ : [val] "r" (val) );
+
+ info->cr4 = val;
}
/* Clear and set 'TS' bit respectively */

View file

@ -1,71 +0,0 @@
From: Jan Beulich <JBeulich@suse.com>
Subject: x86/pv: _toggle_guest_pt() may not skip TLB flush for shadow mode guests
For shadow mode guests (e.g. PV ones forced into that mode as L1TF
mitigation, or during migration) update_cr3() -> sh_update_cr3() may
result in a change to the (shadow) root page table (compared to the
previous one when running the same vCPU with the same PCID). This can,
first and foremost, be a result of memory pressure on the shadow memory
pool of the domain. Shadow code legitimately relies on the original
(prior to commit 5c81d260c2 ["xen/x86: use PCID feature"]) behavior of
the subsequent CR3 write to flush the TLB of entries still left from
walks with an earlier, different (shadow) root page table.
Restore the flushing behavior, also for the second CR3 write on the exit
path to guest context when XPTI is active. For the moment accept that
this will introduce more flushes than are strictly necessary - no flush
would be needed when the (shadow) root page table doesn't actually
change, but this information isn't readily (i.e. without introducing a
layering violation) available here.
This is XSA-294.
Reported-by: XXX PERSON <XXX EMAIL>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Tested-by: Juergen Gross <jgross@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff --git a/xen/arch/x86/pv/domain.c b/xen/arch/x86/pv/domain.c
index b75ff6b..528413a 100644
--- a/xen/arch/x86/pv/domain.c
+++ b/xen/arch/x86/pv/domain.c
@@ -296,21 +296,35 @@ int pv_domain_initialise(struct domain *d)
static void _toggle_guest_pt(struct vcpu *v)
{
const struct domain *d = v->domain;
+ struct cpu_info *cpu_info = get_cpu_info();
+ unsigned long cr3;
v->arch.flags ^= TF_kernel_mode;
update_cr3(v);
if ( d->arch.pv_domain.xpti )
{
- struct cpu_info *cpu_info = get_cpu_info();
-
cpu_info->root_pgt_changed = true;
cpu_info->pv_cr3 = __pa(this_cpu(root_pgt)) |
(d->arch.pv_domain.pcid
? get_pcid_bits(v, true) : 0);
}
- /* Don't flush user global mappings from the TLB. Don't tick TLB clock. */
- write_cr3(v->arch.cr3);
+ /*
+ * Don't flush user global mappings from the TLB. Don't tick TLB clock.
+ *
+ * In shadow mode, though, update_cr3() may need to be accompanied by a
+ * TLB flush (for just the incoming PCID), as the top level page table may
+ * have changed behind our backs. To be on the safe side, suppress the
+ * no-flush unconditionally in this case. The XPTI CR3 write, if enabled,
+ * will then need to be a flushing one too.
+ */
+ cr3 = v->arch.cr3;
+ if ( shadow_mode_enabled(d) )
+ {
+ cr3 &= ~X86_CR3_NOFLUSH;
+ cpu_info->pv_cr3 &= ~X86_CR3_NOFLUSH;
+ }
+ write_cr3(cr3);
if ( !(v->arch.flags & TF_kernel_mode) )
return;

195
xsa296.patch Normal file
View file

@ -0,0 +1,195 @@
From: Andrew Cooper <andrew.cooper3@citrix.com>
Subject: xen/hypercall: Don't use BUG() for parameter checking in hypercall_create_continuation()
Since c/s 1d429034 "hypercall: update vcpu_op to take an unsigned vcpuid",
which incorrectly swapped 'i' for 'u' in the parameter type list, guests have
been able to hit the BUG() in next_args()'s default case.
Correct these back to 'i'.
In addition, make adjustments to prevent this class of issue from occurring in
the future - crashing Xen is not an appropriate form of parameter checking.
Capitalise NEXT_ARG() to catch all uses, to highlight that it is a macro doing
non-function-like things behind the scenes, and undef it when appropriate.
Implement a bad_fmt: block which prints an error, asserts unreachable, and
crashes the guest.
On the ARM side, drop all parameter checking of p. It is asymmetric with the
x86 side, and akin to expecting memcpy() or sprintf() to check their src/fmt
parameter before use. A caller passing "" or something other than a string
literal will be obvious during code review.
This is XSA-296.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Acked-by: Julien Grall <julien.grall@arm.com>
diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index 941bbff4fe..a3da8e9c08 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -383,14 +383,15 @@ void sync_vcpu_execstate(struct vcpu *v)
/* Nothing to do -- no lazy switching */
}
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -405,9 +406,6 @@ unsigned long hypercall_create_continuation(
unsigned int i;
va_list args;
- /* All hypercalls take at least one argument */
- BUG_ON( !p || *p == '\0' );
-
current->hcall_preempted = true;
va_start(args, format);
@@ -415,7 +413,7 @@ unsigned long hypercall_create_continuation(
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
/* Return value gets written back to mcs->call.result */
rc = mcs->call.result;
@@ -431,7 +429,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -454,7 +452,7 @@ unsigned long hypercall_create_continuation(
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
@@ -475,8 +473,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return rc;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(current->domain);
+ return 0;
}
+#undef NEXT_ARG
+
void startup_cpu_idle_loop(void)
{
struct vcpu *v = current;
diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c
index d483dbaa6b..4643e5eb43 100644
--- a/xen/arch/x86/hypercall.c
+++ b/xen/arch/x86/hypercall.c
@@ -80,14 +80,15 @@ const hypercall_args_t hypercall_args_table[NR_hypercalls] =
#undef COMP
#undef ARGS
-#define next_arg(fmt, args) ({ \
+#define NEXT_ARG(fmt, args) \
+({ \
unsigned long __arg; \
switch ( *(fmt)++ ) \
{ \
case 'i': __arg = (unsigned long)va_arg(args, unsigned int); break; \
case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break; \
case 'h': __arg = (unsigned long)va_arg(args, void *); break; \
- default: __arg = 0; BUG(); \
+ default: goto bad_fmt; \
} \
__arg; \
})
@@ -109,7 +110,7 @@ unsigned long hypercall_create_continuation(
if ( mcs->flags & MCSF_in_multicall )
{
for ( i = 0; *p != '\0'; i++ )
- mcs->call.args[i] = next_arg(p, args);
+ mcs->call.args[i] = NEXT_ARG(p, args);
}
else
{
@@ -121,7 +122,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->rdi = arg; break;
@@ -137,7 +138,7 @@ unsigned long hypercall_create_continuation(
{
for ( i = 0; *p != '\0'; i++ )
{
- arg = next_arg(p, args);
+ arg = NEXT_ARG(p, args);
switch ( i )
{
case 0: regs->rbx = arg; break;
@@ -154,8 +155,16 @@ unsigned long hypercall_create_continuation(
va_end(args);
return op;
+
+ bad_fmt:
+ gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+ ASSERT_UNREACHABLE();
+ domain_crash(curr->domain);
+ return 0;
}
+#undef NEXT_ARG
+
int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
unsigned int mask, ...)
{
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 39877b3ab2..2531fa7421 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -81,7 +81,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) ar
}
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 2308588052..65bcd85e34 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1411,7 +1411,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, XEN_GUEST_HANDLE_PARAM(void) arg)
rc = arch_initialise_vcpu(v, arg);
if ( rc == -ERESTART )
- rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+ rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
cmd, vcpuid, arg);
break;

87
xsa298-4.11.patch Normal file
View file

@ -0,0 +1,87 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/PV: check GDT/LDT limits during emulation
Accesses beyond the LDT limit originating from emulation would trigger
the ASSERT() in pv_map_ldt_shadow_page(). On production builds such
accesses would cause an attempt to promote the touched page (offset from
the present LDT base address) to a segment descriptor one. If this
happens to succeed, guest user mode would be able to elevate its
privileges to that of the guest kernel. This is particularly easy when
there's no LDT at all, in which case the LDT base stored internally to
Xen is simply zero.
Also adjust the ASSERT() that was triggering: It was off by one to
begin with, and for production builds we also better use
ASSERT_UNREACHABLE() instead with suitable recovery code afterwards.
This is XSA-298.
Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/pv/emul-gate-op.c
+++ b/xen/arch/x86/pv/emul-gate-op.c
@@ -51,7 +51,13 @@ static int read_gate_descriptor(unsigned
const struct desc_struct *pdesc = gdt_ldt_desc_ptr(gate_sel);
if ( (gate_sel < 4) ||
- ((gate_sel >= FIRST_RESERVED_GDT_BYTE) && !(gate_sel & 4)) ||
+ /*
+ * We're interested in call gates only, which occupy a single
+ * seg_desc_t for 32-bit and a consecutive pair of them for 64-bit.
+ */
+ ((gate_sel >> 3) + !is_pv_32bit_vcpu(v) >=
+ (gate_sel & 4 ? v->arch.pv_vcpu.ldt_ents
+ : v->arch.pv_vcpu.gdt_ents)) ||
__get_user(desc, pdesc) )
return 0;
@@ -70,7 +76,7 @@ static int read_gate_descriptor(unsigned
if ( !is_pv_32bit_vcpu(v) )
{
if ( (*ar & 0x1f00) != 0x0c00 ||
- (gate_sel >= FIRST_RESERVED_GDT_BYTE - 8 && !(gate_sel & 4)) ||
+ /* Limit check done above already. */
__get_user(desc, pdesc + 1) ||
(desc.b & 0x1f00) )
return 0;
--- a/xen/arch/x86/pv/emulate.c
+++ b/xen/arch/x86/pv/emulate.c
@@ -31,7 +31,14 @@ int pv_emul_read_descriptor(unsigned int
{
struct desc_struct desc;
- if ( sel < 4)
+ if ( sel < 4 ||
+ /*
+ * Don't apply the GDT limit here, as the selector may be a Xen
+ * provided one. __get_user() will fail (without taking further
+ * action) for ones falling in the gap between guest populated
+ * and Xen ones.
+ */
+ ((sel & 4) && (sel >> 3) >= v->arch.pv_vcpu.ldt_ents) )
desc.b = desc.a = 0;
else if ( __get_user(desc, gdt_ldt_desc_ptr(sel)) )
return 0;
--- a/xen/arch/x86/pv/mm.c
+++ b/xen/arch/x86/pv/mm.c
@@ -92,12 +92,16 @@ bool pv_map_ldt_shadow_page(unsigned int
BUG_ON(unlikely(in_irq()));
/*
- * Hardware limit checking should guarantee this property. NB. This is
+ * Prior limit checking should guarantee this property. NB. This is
* safe as updates to the LDT can only be made by MMUEXT_SET_LDT to the
* current vcpu, and vcpu_reset() will block until this vcpu has been
* descheduled before continuing.
*/
- ASSERT((offset >> 3) <= curr->arch.pv_vcpu.ldt_ents);
+ if ( unlikely((offset >> 3) >= curr->arch.pv_vcpu.ldt_ents) )
+ {
+ ASSERT_UNREACHABLE();
+ return false;
+ }
if ( is_pv_32bit_domain(currd) )
linear = (uint32_t)linear;

View file

@ -0,0 +1,94 @@
From 852df269d247e177d5f2e9b8f3a4301a6fdd76bd Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 01/11] x86/mm: L1TF checks don't leave a partial entry
On detection of a potential L1TF issue, most validation code returns
-ERESTART to allow the switch to shadow mode to happen and cause the
original operation to be restarted.
However, in the validation code, the return value -ERESTART has been
repurposed to indicate 1) the function has partially completed
something which needs to be undone, and 2) calling put_page_type()
should cleanly undo it. This causes problems in several places.
For L1 tables, on receiving an -ERESTART return from alloc_l1_table(),
alloc_page_type() will set PGT_partial on the page. If for some
reason the original operation never restarts, then on domain
destruction, relinquish_memory() will call free_page_type() on the
page.
Unfortunately, alloc_ and free_l1_table() aren't set up to deal with
PGT_partial. When returning a failure, alloc_l1_table() always
de-validates whatever it's validated so far, and free_l1_table()
always devalidates the whole page. This means that if
relinquish_memory() calls free_page_type() on an L1 that didn't
complete due to an L1TF, it will call put_page_from_l1e() on "page
entries" that have never been validated.
For L2+ tables, setting rc to ERESTART causes the rest of the
alloc_lN_table() function to *think* that the entry in question will
have PGT_partial set. This will cause it to set partial_pte = 1. If
relinqush_memory() then calls free_page_type() on one of those pages,
then free_lN_table() will call put_page_from_lNe() on the entry when
it shouldn't.
Rather than indicating -ERESTART, indicate -EINTR. This is the code
to indicate that nothing has changed from when you started the call
(which is effectively how alloc_l1_table() handles errors).
mod_lN_entry() shouldn't have any of these types of problems, so leave
potential changes there for a clean-up patch later.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e6a4cb28f8..8ced185b49 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1110,7 +1110,7 @@ get_page_from_l2e(
int rc;
if ( !(l2e_get_flags(l2e) & _PAGE_PRESENT) )
- return pv_l1tf_check_l2e(d, l2e) ? -ERESTART : 1;
+ return pv_l1tf_check_l2e(d, l2e) ? -EINTR : 1;
if ( unlikely((l2e_get_flags(l2e) & L2_DISALLOW_MASK)) )
{
@@ -1142,7 +1142,7 @@ get_page_from_l3e(
int rc;
if ( !(l3e_get_flags(l3e) & _PAGE_PRESENT) )
- return pv_l1tf_check_l3e(d, l3e) ? -ERESTART : 1;
+ return pv_l1tf_check_l3e(d, l3e) ? -EINTR : 1;
if ( unlikely((l3e_get_flags(l3e) & l3_disallow_mask(d))) )
{
@@ -1175,7 +1175,7 @@ get_page_from_l4e(
int rc;
if ( !(l4e_get_flags(l4e) & _PAGE_PRESENT) )
- return pv_l1tf_check_l4e(d, l4e) ? -ERESTART : 1;
+ return pv_l1tf_check_l4e(d, l4e) ? -EINTR : 1;
if ( unlikely((l4e_get_flags(l4e) & L4_DISALLOW_MASK)) )
{
@@ -1404,7 +1404,7 @@ static int alloc_l1_table(struct page_info *page)
{
if ( !(l1e_get_flags(pl1e[i]) & _PAGE_PRESENT) )
{
- ret = pv_l1tf_check_l1e(d, pl1e[i]) ? -ERESTART : 0;
+ ret = pv_l1tf_check_l1e(d, pl1e[i]) ? -EINTR : 0;
if ( ret )
goto out;
}
--
2.23.0

View file

@ -0,0 +1,99 @@
From 6bdddd7980eac0cc883945d823986f24682ca47a Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 02/11] x86/mm: Don't re-set PGT_pinned on a partially
de-validated page
When unpinning pagetables, if an operation is interrupted,
relinquish_memory() re-sets PGT_pinned so that the un-pin will
pickedup again when the hypercall restarts.
This is appropriate when put_page_and_type_preemptible() returns
-EINTR, which indicates that the page is back in its initial state
(i.e., completely validated). However, for -ERESTART, this leads to a
state where a page has both PGT_pinned and PGT_partial set.
This happens to work at the moment, although it's not really a
"canonical" state; but in subsequent patches, where we need to make a
distinction in handling between PGT_validated and PGT_partial pages,
this causes issues.
Move to a "canonical" state by:
- Only re-setting PGT_pinned on -EINTR
- Re-dropping the refcount held by PGT_pinned on -ERESTART
In the latter case, the PGT_partial bit will be cleared further down
with the rest of the other PGT_partial pages.
While here, clean up some trainling whitespace.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/domain.c | 31 ++++++++++++++++++++++++++++---
1 file changed, 28 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 29f892c04c..8fbecbb169 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -112,7 +112,7 @@ static void play_dead(void)
* this case, heap corruption or #PF can occur (when heap debugging is
* enabled). For example, even printk() can involve tasklet scheduling,
* which touches per-cpu vars.
- *
+ *
* Consider very carefully when adding code to *dead_idle. Most hypervisor
* subsystems are unsafe to call.
*/
@@ -1838,9 +1838,34 @@ static int relinquish_memory(
break;
case -ERESTART:
case -EINTR:
+ /*
+ * -EINTR means PGT_validated has been re-set; re-set
+ * PGT_pinned again so that it gets picked up next time
+ * around.
+ *
+ * -ERESTART, OTOH, means PGT_partial is set instead. Put
+ * it back on the list, but don't set PGT_pinned; the
+ * section below will finish off de-validation. But we do
+ * need to drop the general ref associated with
+ * PGT_pinned, since put_page_and_type_preemptible()
+ * didn't do it.
+ *
+ * NB we can do an ASSERT for PGT_validated, since we
+ * "own" the type ref; but theoretically, the PGT_partial
+ * could be cleared by someone else.
+ */
+ if ( ret == -EINTR )
+ {
+ ASSERT(page->u.inuse.type_info & PGT_validated);
+ set_bit(_PGT_pinned, &page->u.inuse.type_info);
+ }
+ else
+ put_page(page);
+
ret = -ERESTART;
+
+ /* Put the page back on the list and drop the ref we grabbed above */
page_list_add(page, list);
- set_bit(_PGT_pinned, &page->u.inuse.type_info);
put_page(page);
goto out;
default:
@@ -2062,7 +2087,7 @@ void vcpu_kick(struct vcpu *v)
* pending flag. These values may fluctuate (after all, we hold no
* locks) but the key insight is that each change will cause
* evtchn_upcall_pending to be polled.
- *
+ *
* NB2. We save the running flag across the unblock to avoid a needless
* IPI for domains that we IPI'd to unblock.
*/
--
2.23.0

View file

@ -0,0 +1,609 @@
From 7c0a37005f52d10903ce22851b52ae9b6f4f0ee2 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 03/11] x86/mm: Separate out partial_pte tristate into
individual flags
At the moment, partial_pte is a tri-state that contains two distinct bits
of information:
1. If zero, the pte at index [nr_validated_ptes] is un-validated. If
non-zero, the pte was last seen with PGT_partial set.
2. If positive, the pte at index [nr_validated_ptes] does not hold a
general reference count. If negative, it does.
To make future patches more clear, separate out this functionality
into two distinct, named bits: PTF_partial_set (for #1) and
PTF_partial_general_ref (for #2).
Additionally, a number of functions which need this information also
take other flags to control behavior (such as `preemptible` and
`defer`). These are hard to read in the caller (since you only see
'true' or 'false'), and ugly when many are added together. In
preparation for adding yet another flag in a future patch, collapse
all of these into a single `flag` variable.
NB that this does mean checking for what was previously the '-1'
condition a bit more ugly in the put_page_from_lNe functions (since
you have to check for both partial_set and general ref); but this
clause will go away in a future patch.
Also note that the original comment had an off-by-one error:
partial_flags (like partial_pte before it) concerns
plNe[nr_validated_ptes], not plNe[nr_validated_ptes+1].
No functional change intended.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 164 +++++++++++++++++++++++----------------
xen/include/asm-x86/mm.h | 41 ++++++----
2 files changed, 127 insertions(+), 78 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 8ced185b49..1c4f54e328 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -610,20 +610,34 @@ static int alloc_segdesc_page(struct page_info *page)
static int _get_page_type(struct page_info *page, unsigned long type,
bool preemptible);
+/*
+ * The following flags are used to specify behavior of various get and
+ * put commands. The first two are also stored in page->partial_flags
+ * to indicate the state of the page pointed to by
+ * page->pte[page->nr_validated_entries]. See the comment in mm.h for
+ * more information.
+ */
+#define PTF_partial_set (1 << 0)
+#define PTF_partial_general_ref (1 << 1)
+#define PTF_preemptible (1 << 2)
+#define PTF_defer (1 << 3)
+
static int get_page_and_type_from_mfn(
mfn_t mfn, unsigned long type, struct domain *d,
- int partial, int preemptible)
+ unsigned int flags)
{
struct page_info *page = mfn_to_page(mfn);
int rc;
+ bool preemptible = flags & PTF_preemptible,
+ partial_ref = flags & PTF_partial_general_ref;
- if ( likely(partial >= 0) &&
+ if ( likely(!partial_ref) &&
unlikely(!get_page_from_mfn(mfn, d)) )
return -EINVAL;
rc = _get_page_type(page, type, preemptible);
- if ( unlikely(rc) && partial >= 0 &&
+ if ( unlikely(rc) && !partial_ref &&
(!preemptible || page != current->arch.old_guest_table) )
put_page(page);
@@ -1104,7 +1118,7 @@ get_page_from_l1e(
define_get_linear_pagetable(l2);
static int
get_page_from_l2e(
- l2_pgentry_t l2e, unsigned long pfn, struct domain *d, int partial)
+ l2_pgentry_t l2e, unsigned long pfn, struct domain *d, unsigned int flags)
{
unsigned long mfn = l2e_get_pfn(l2e);
int rc;
@@ -1119,8 +1133,9 @@ get_page_from_l2e(
return -EINVAL;
}
- rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d,
- partial, false);
+ ASSERT(!(flags & PTF_preemptible));
+
+ rc = get_page_and_type_from_mfn(_mfn(mfn), PGT_l1_page_table, d, flags);
if ( unlikely(rc == -EINVAL) && get_l2_linear_pagetable(l2e, pfn, d) )
rc = 0;
@@ -1137,7 +1152,7 @@ get_page_from_l2e(
define_get_linear_pagetable(l3);
static int
get_page_from_l3e(
- l3_pgentry_t l3e, unsigned long pfn, struct domain *d, int partial)
+ l3_pgentry_t l3e, unsigned long pfn, struct domain *d, unsigned int flags)
{
int rc;
@@ -1152,7 +1167,7 @@ get_page_from_l3e(
}
rc = get_page_and_type_from_mfn(
- l3e_get_mfn(l3e), PGT_l2_page_table, d, partial, 1);
+ l3e_get_mfn(l3e), PGT_l2_page_table, d, flags | PTF_preemptible);
if ( unlikely(rc == -EINVAL) &&
!is_pv_32bit_domain(d) &&
get_l3_linear_pagetable(l3e, pfn, d) )
@@ -1170,7 +1185,7 @@ get_page_from_l3e(
define_get_linear_pagetable(l4);
static int
get_page_from_l4e(
- l4_pgentry_t l4e, unsigned long pfn, struct domain *d, int partial)
+ l4_pgentry_t l4e, unsigned long pfn, struct domain *d, unsigned int flags)
{
int rc;
@@ -1185,7 +1200,7 @@ get_page_from_l4e(
}
rc = get_page_and_type_from_mfn(
- l4e_get_mfn(l4e), PGT_l3_page_table, d, partial, 1);
+ l4e_get_mfn(l4e), PGT_l3_page_table, d, flags | PTF_preemptible);
if ( unlikely(rc == -EINVAL) && get_l4_linear_pagetable(l4e, pfn, d) )
rc = 0;
@@ -1275,7 +1290,7 @@ void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
* Note also that this automatically deals correctly with linear p.t.'s.
*/
static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
- int partial, bool defer)
+ unsigned int flags)
{
int rc = 0;
@@ -1295,12 +1310,13 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
struct page_info *pg = l2e_get_page(l2e);
struct page_info *ptpg = mfn_to_page(_mfn(pfn));
- if ( unlikely(partial > 0) )
+ if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
+ PTF_partial_set )
{
- ASSERT(!defer);
+ ASSERT(!(flags & PTF_defer));
rc = _put_page_type(pg, true, ptpg);
}
- else if ( defer )
+ else if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = ptpg;
current->arch.old_guest_table = pg;
@@ -1317,7 +1333,7 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
}
static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
- int partial, bool defer)
+ unsigned int flags)
{
struct page_info *pg;
int rc;
@@ -1340,13 +1356,14 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
pg = l3e_get_page(l3e);
- if ( unlikely(partial > 0) )
+ if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
+ PTF_partial_set )
{
- ASSERT(!defer);
+ ASSERT(!(flags & PTF_defer));
return _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
}
- if ( defer )
+ if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
current->arch.old_guest_table = pg;
@@ -1361,7 +1378,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
}
static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
- int partial, bool defer)
+ unsigned int flags)
{
int rc = 1;
@@ -1370,13 +1387,14 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
{
struct page_info *pg = l4e_get_page(l4e);
- if ( unlikely(partial > 0) )
+ if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
+ PTF_partial_set )
{
- ASSERT(!defer);
+ ASSERT(!(flags & PTF_defer));
return _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
}
- if ( defer )
+ if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
current->arch.old_guest_table = pg;
@@ -1483,12 +1501,13 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
unsigned long pfn = mfn_x(page_to_mfn(page));
l2_pgentry_t *pl2e;
unsigned int i;
- int rc = 0, partial = page->partial_pte;
+ int rc = 0;
+ unsigned int partial_flags = page->partial_flags;
pl2e = map_domain_page(_mfn(pfn));
for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES;
- i++, partial = 0 )
+ i++, partial_flags = 0 )
{
if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
{
@@ -1498,18 +1517,19 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
}
if ( !is_guest_l2_slot(d, type, i) ||
- (rc = get_page_from_l2e(pl2e[i], pfn, d, partial)) > 0 )
+ (rc = get_page_from_l2e(pl2e[i], pfn, d, partial_flags)) > 0 )
continue;
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: 1;
+ /* Set 'set', retain 'general ref' */
+ page->partial_flags = partial_flags | PTF_partial_set;
}
else if ( rc == -EINTR && i )
{
page->nr_validated_ptes = i;
- page->partial_pte = 0;
+ page->partial_flags = 0;
rc = -ERESTART;
}
else if ( rc < 0 && rc != -EINTR )
@@ -1518,7 +1538,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_pte = 0;
+ page->partial_flags = 0;
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
@@ -1542,7 +1562,8 @@ static int alloc_l3_table(struct page_info *page)
unsigned long pfn = mfn_x(page_to_mfn(page));
l3_pgentry_t *pl3e;
unsigned int i;
- int rc = 0, partial = page->partial_pte;
+ int rc = 0;
+ unsigned int partial_flags = page->partial_flags;
pl3e = map_domain_page(_mfn(pfn));
@@ -1557,7 +1578,7 @@ static int alloc_l3_table(struct page_info *page)
memset(pl3e + 4, 0, (L3_PAGETABLE_ENTRIES - 4) * sizeof(*pl3e));
for ( i = page->nr_validated_ptes; i < L3_PAGETABLE_ENTRIES;
- i++, partial = 0 )
+ i++, partial_flags = 0 )
{
if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
{
@@ -1574,20 +1595,22 @@ static int alloc_l3_table(struct page_info *page)
else
rc = get_page_and_type_from_mfn(
l3e_get_mfn(pl3e[i]),
- PGT_l2_page_table | PGT_pae_xen_l2, d, partial, 1);
+ PGT_l2_page_table | PGT_pae_xen_l2, d,
+ partial_flags | PTF_preemptible);
}
- else if ( (rc = get_page_from_l3e(pl3e[i], pfn, d, partial)) > 0 )
+ else if ( (rc = get_page_from_l3e(pl3e[i], pfn, d, partial_flags)) > 0 )
continue;
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: 1;
+ /* Set 'set', leave 'general ref' set if this entry was set */
+ page->partial_flags = partial_flags | PTF_partial_set;
}
else if ( rc == -EINTR && i )
{
page->nr_validated_ptes = i;
- page->partial_pte = 0;
+ page->partial_flags = 0;
rc = -ERESTART;
}
if ( rc < 0 )
@@ -1604,7 +1627,7 @@ static int alloc_l3_table(struct page_info *page)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_pte = 0;
+ page->partial_flags = 0;
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
@@ -1736,19 +1759,21 @@ static int alloc_l4_table(struct page_info *page)
unsigned long pfn = mfn_x(page_to_mfn(page));
l4_pgentry_t *pl4e = map_domain_page(_mfn(pfn));
unsigned int i;
- int rc = 0, partial = page->partial_pte;
+ int rc = 0;
+ unsigned int partial_flags = page->partial_flags;
for ( i = page->nr_validated_ptes; i < L4_PAGETABLE_ENTRIES;
- i++, partial = 0 )
+ i++, partial_flags = 0 )
{
if ( !is_guest_l4_slot(d, i) ||
- (rc = get_page_from_l4e(pl4e[i], pfn, d, partial)) > 0 )
+ (rc = get_page_from_l4e(pl4e[i], pfn, d, partial_flags)) > 0 )
continue;
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: 1;
+ /* Set 'set', leave 'general ref' set if this entry was set */
+ page->partial_flags = partial_flags | PTF_partial_set;
}
else if ( rc < 0 )
{
@@ -1758,7 +1783,7 @@ static int alloc_l4_table(struct page_info *page)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_pte = 0;
+ page->partial_flags = 0;
if ( rc == -EINTR )
rc = -ERESTART;
else
@@ -1811,19 +1836,20 @@ static int free_l2_table(struct page_info *page)
struct domain *d = page_get_owner(page);
unsigned long pfn = mfn_x(page_to_mfn(page));
l2_pgentry_t *pl2e;
- int rc = 0, partial = page->partial_pte;
- unsigned int i = page->nr_validated_ptes - !partial;
+ int rc = 0;
+ unsigned int partial_flags = page->partial_flags,
+ i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
pl2e = map_domain_page(_mfn(pfn));
for ( ; ; )
{
if ( is_guest_l2_slot(d, page->u.inuse.type_info, i) )
- rc = put_page_from_l2e(pl2e[i], pfn, partial, false);
+ rc = put_page_from_l2e(pl2e[i], pfn, partial_flags);
if ( rc < 0 )
break;
- partial = 0;
+ partial_flags = 0;
if ( !i-- )
break;
@@ -1845,12 +1871,14 @@ static int free_l2_table(struct page_info *page)
else if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: -1;
+ page->partial_flags = (partial_flags & PTF_partial_set) ?
+ partial_flags :
+ (PTF_partial_set | PTF_partial_general_ref);
}
else if ( rc == -EINTR && i < L2_PAGETABLE_ENTRIES - 1 )
{
page->nr_validated_ptes = i + 1;
- page->partial_pte = 0;
+ page->partial_flags = 0;
rc = -ERESTART;
}
@@ -1862,18 +1890,19 @@ static int free_l3_table(struct page_info *page)
struct domain *d = page_get_owner(page);
unsigned long pfn = mfn_x(page_to_mfn(page));
l3_pgentry_t *pl3e;
- int rc = 0, partial = page->partial_pte;
- unsigned int i = page->nr_validated_ptes - !partial;
+ int rc = 0;
+ unsigned int partial_flags = page->partial_flags,
+ i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
pl3e = map_domain_page(_mfn(pfn));
for ( ; ; )
{
- rc = put_page_from_l3e(pl3e[i], pfn, partial, 0);
+ rc = put_page_from_l3e(pl3e[i], pfn, partial_flags);
if ( rc < 0 )
break;
- partial = 0;
+ partial_flags = 0;
if ( rc == 0 )
pl3e[i] = unadjust_guest_l3e(pl3e[i], d);
@@ -1892,12 +1921,14 @@ static int free_l3_table(struct page_info *page)
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: -1;
+ page->partial_flags = (partial_flags & PTF_partial_set) ?
+ partial_flags :
+ (PTF_partial_set | PTF_partial_general_ref);
}
else if ( rc == -EINTR && i < L3_PAGETABLE_ENTRIES - 1 )
{
page->nr_validated_ptes = i + 1;
- page->partial_pte = 0;
+ page->partial_flags = 0;
rc = -ERESTART;
}
return rc > 0 ? 0 : rc;
@@ -1908,26 +1939,29 @@ static int free_l4_table(struct page_info *page)
struct domain *d = page_get_owner(page);
unsigned long pfn = mfn_x(page_to_mfn(page));
l4_pgentry_t *pl4e = map_domain_page(_mfn(pfn));
- int rc = 0, partial = page->partial_pte;
- unsigned int i = page->nr_validated_ptes - !partial;
+ int rc = 0;
+ unsigned partial_flags = page->partial_flags,
+ i = page->nr_validated_ptes - !(partial_flags & PTF_partial_set);
do {
if ( is_guest_l4_slot(d, i) )
- rc = put_page_from_l4e(pl4e[i], pfn, partial, 0);
+ rc = put_page_from_l4e(pl4e[i], pfn, partial_flags);
if ( rc < 0 )
break;
- partial = 0;
+ partial_flags = 0;
} while ( i-- );
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_pte = partial ?: -1;
+ page->partial_flags = (partial_flags & PTF_partial_set) ?
+ partial_flags :
+ (PTF_partial_set | PTF_partial_general_ref);
}
else if ( rc == -EINTR && i < L4_PAGETABLE_ENTRIES - 1 )
{
page->nr_validated_ptes = i + 1;
- page->partial_pte = 0;
+ page->partial_flags = 0;
rc = -ERESTART;
}
@@ -2203,7 +2237,7 @@ static int mod_l2_entry(l2_pgentry_t *pl2e,
return -EBUSY;
}
- put_page_from_l2e(ol2e, pfn, 0, true);
+ put_page_from_l2e(ol2e, pfn, PTF_defer);
return rc;
}
@@ -2271,7 +2305,7 @@ static int mod_l3_entry(l3_pgentry_t *pl3e,
if ( !create_pae_xen_mappings(d, pl3e) )
BUG();
- put_page_from_l3e(ol3e, pfn, 0, 1);
+ put_page_from_l3e(ol3e, pfn, PTF_defer);
return rc;
}
@@ -2334,7 +2368,7 @@ static int mod_l4_entry(l4_pgentry_t *pl4e,
return -EFAULT;
}
- put_page_from_l4e(ol4e, pfn, 0, 1);
+ put_page_from_l4e(ol4e, pfn, PTF_defer);
return rc;
}
@@ -2598,7 +2632,7 @@ int free_page_type(struct page_info *page, unsigned long type,
if ( !(type & PGT_partial) )
{
page->nr_validated_ptes = 1U << PAGETABLE_ORDER;
- page->partial_pte = 0;
+ page->partial_flags = 0;
}
switch ( type & PGT_type_mask )
@@ -2889,7 +2923,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
if ( !(x & PGT_partial) )
{
page->nr_validated_ptes = 0;
- page->partial_pte = 0;
+ page->partial_flags = 0;
}
page->linear_pt_count = 0;
rc = alloc_page_type(page, type, preemptible);
@@ -3064,7 +3098,7 @@ int new_guest_cr3(mfn_t mfn)
return 0;
}
- rc = get_page_and_type_from_mfn(mfn, PGT_root_page_table, d, 0, 1);
+ rc = get_page_and_type_from_mfn(mfn, PGT_root_page_table, d, PTF_preemptible);
switch ( rc )
{
case 0:
@@ -3452,7 +3486,7 @@ long do_mmuext_op(
if ( op.arg1.mfn != 0 )
{
rc = get_page_and_type_from_mfn(
- _mfn(op.arg1.mfn), PGT_root_page_table, currd, 0, 1);
+ _mfn(op.arg1.mfn), PGT_root_page_table, currd, PTF_preemptible);
if ( unlikely(rc) )
{
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 1ea173c555..46cba52941 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -228,19 +228,34 @@ struct page_info
* setting the flag must not drop that reference, whereas the instance
* clearing it will have to.
*
- * If @partial_pte is positive then PTE at @nr_validated_ptes+1 has
- * been partially validated. This implies that the general reference
- * to the page (acquired from get_page_from_lNe()) would be dropped
- * (again due to the apparent failure) and hence must be re-acquired
- * when resuming the validation, but must not be dropped when picking
- * up the page for invalidation.
+ * If partial_flags & PTF_partial_set is set, then the page at
+ * at @nr_validated_ptes had PGT_partial set as a result of an
+ * operation on the current page. (That page may or may not
+ * still have PGT_partial set.)
*
- * If @partial_pte is negative then PTE at @nr_validated_ptes+1 has
- * been partially invalidated. This is basically the opposite case of
- * above, i.e. the general reference to the page was not dropped in
- * put_page_from_lNe() (due to the apparent failure), and hence it
- * must be dropped when the put operation is resumed (and completes),
- * but it must not be acquired if picking up the page for validation.
+ * If PTF_partial_general_ref is set, then the PTE at
+ * @nr_validated_ptef holds a general reference count for the
+ * page.
+ *
+ * This happens:
+ * - During de-validation, if de-validation of the page was
+ * interrupted
+ * - During validation, if an invalid entry is encountered and
+ * validation is preemptible
+ * - During validation, if PTF_partial_general_ref was set on
+ * this entry to begin with (perhaps because we're picking
+ * up from a partial de-validation).
+ *
+ * When resuming validation, if PTF_partial_general_ref is clear,
+ * then a general reference must be re-acquired; if it is set, no
+ * reference should be acquired.
+ *
+ * When resuming de-validation, if PTF_partial_general_ref is
+ * clear, no reference should be dropped; if it is set, a
+ * reference should be dropped.
+ *
+ * NB that PTF_partial_set and PTF_partial_general_ref are
+ * defined in mm.c, the only place where they are used.
*
* The 3rd field, @linear_pt_count, indicates
* - by a positive value, how many same-level page table entries a page
@@ -251,7 +266,7 @@ struct page_info
struct {
u16 nr_validated_ptes:PAGETABLE_ORDER + 1;
u16 :16 - PAGETABLE_ORDER - 1 - 2;
- s16 partial_pte:2;
+ u16 partial_flags:2;
s16 linear_pt_count;
};
--
2.23.0

View file

@ -0,0 +1,140 @@
From 20b8a6702c6839bafd252789396b443d4b5c5474 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 04/11] x86/mm: Use flags for _put_page_type rather than a
boolean
This is in mainly in preparation for _put_page_type taking the
partial_flags value in the future. It also makes it easier to read in
the caller (since you see a flag name rather than `true` or `false`).
No functional change intended.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 25 +++++++++++++------------
1 file changed, 13 insertions(+), 12 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 1c4f54e328..e2fba15d86 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1207,7 +1207,7 @@ get_page_from_l4e(
return rc;
}
-static int _put_page_type(struct page_info *page, bool preemptible,
+static int _put_page_type(struct page_info *page, unsigned int flags,
struct page_info *ptpg);
void put_page_from_l1e(l1_pgentry_t l1e, struct domain *l1e_owner)
@@ -1314,7 +1314,7 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
PTF_partial_set )
{
ASSERT(!(flags & PTF_defer));
- rc = _put_page_type(pg, true, ptpg);
+ rc = _put_page_type(pg, PTF_preemptible, ptpg);
}
else if ( flags & PTF_defer )
{
@@ -1323,7 +1323,7 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
}
else
{
- rc = _put_page_type(pg, true, ptpg);
+ rc = _put_page_type(pg, PTF_preemptible, ptpg);
if ( likely(!rc) )
put_page(pg);
}
@@ -1360,7 +1360,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
PTF_partial_set )
{
ASSERT(!(flags & PTF_defer));
- return _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
+ return _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
}
if ( flags & PTF_defer )
@@ -1370,7 +1370,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
return 0;
}
- rc = _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
+ rc = _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
if ( likely(!rc) )
put_page(pg);
@@ -1391,7 +1391,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
PTF_partial_set )
{
ASSERT(!(flags & PTF_defer));
- return _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
+ return _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
}
if ( flags & PTF_defer )
@@ -1401,7 +1401,7 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
return 0;
}
- rc = _put_page_type(pg, true, mfn_to_page(_mfn(pfn)));
+ rc = _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
if ( likely(!rc) )
put_page(pg);
}
@@ -2701,10 +2701,11 @@ static int _put_final_page_type(struct page_info *page, unsigned long type,
}
-static int _put_page_type(struct page_info *page, bool preemptible,
+static int _put_page_type(struct page_info *page, unsigned int flags,
struct page_info *ptpg)
{
unsigned long nx, x, y = page->u.inuse.type_info;
+ bool preemptible = flags & PTF_preemptible;
ASSERT(current_locked_page_ne_check(page));
@@ -2911,7 +2912,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
if ( unlikely(iommu_ret) )
{
- _put_page_type(page, false, NULL);
+ _put_page_type(page, 0, NULL);
rc = iommu_ret;
goto out;
}
@@ -2938,7 +2939,7 @@ static int _get_page_type(struct page_info *page, unsigned long type,
void put_page_type(struct page_info *page)
{
- int rc = _put_page_type(page, false, NULL);
+ int rc = _put_page_type(page, 0, NULL);
ASSERT(rc == 0);
(void)rc;
}
@@ -2955,7 +2956,7 @@ int get_page_type(struct page_info *page, unsigned long type)
int put_page_type_preemptible(struct page_info *page)
{
- return _put_page_type(page, true, NULL);
+ return _put_page_type(page, PTF_preemptible, NULL);
}
int get_page_type_preemptible(struct page_info *page, unsigned long type)
@@ -2972,7 +2973,7 @@ int put_old_guest_table(struct vcpu *v)
if ( !v->arch.old_guest_table )
return 0;
- switch ( rc = _put_page_type(v->arch.old_guest_table, true,
+ switch ( rc = _put_page_type(v->arch.old_guest_table, PTF_preemptible,
v->arch.old_guest_ptpg) )
{
case -EINTR:
--
2.23.0

View file

@ -0,0 +1,79 @@
From 7b3f9f9a797459902bebba962e31be5cbfe7b515 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 05/11] x86/mm: Rework get_page_and_type_from_mfn conditional
Make it easier to read by declaring the conditions in which we will
retain the ref, rather than the conditions under which we release it.
The only way (page == current->arch.old_guest_table) can be true is if
preemptible is true; so remove this from the query itself, and add an
ASSERT() to that effect on the opposite path.
No functional change intended.
NB that alloc_lN_table() mishandle the "linear pt failure" situation
described in the comment; this will be addressed in a future patch.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 39 +++++++++++++++++++++++++++++++++++++--
1 file changed, 37 insertions(+), 2 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e2fba15d86..eaf7b14245 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -637,8 +637,43 @@ static int get_page_and_type_from_mfn(
rc = _get_page_type(page, type, preemptible);
- if ( unlikely(rc) && !partial_ref &&
- (!preemptible || page != current->arch.old_guest_table) )
+ /*
+ * Retain the refcount if:
+ * - page is fully validated (rc == 0)
+ * - page is not validated (rc < 0) but:
+ * - We came in with a reference (partial_ref)
+ * - page is partially validated but there's been an error
+ * (page == current->arch.old_guest_table)
+ *
+ * The partial_ref-on-error clause is worth an explanation. There
+ * are two scenarios where partial_ref might be true coming in:
+ * - mfn has been partially demoted as type `type`; i.e. has
+ * PGT_partial set
+ * - mfn has been partially demoted as L(type+1) (i.e., a linear
+ * page; e.g. we're being called from get_page_from_l2e with
+ * type == PGT_l1_table, but the mfn is PGT_l2_table)
+ *
+ * If there's an error, in the first case, _get_page_type will
+ * either return -ERESTART, in which case we want to retain the
+ * ref (as the caller will consider it retained), or -EINVAL, in
+ * which case old_guest_table will be set; in both cases, we need
+ * to retain the ref.
+ *
+ * In the second case, if there's an error, _get_page_type() can
+ * *only* return -EINVAL, and *never* set old_guest_table. In
+ * that case we also want to retain the reference, to allow the
+ * page to continue to be torn down (i.e., PGT_partial cleared)
+ * safely.
+ *
+ * Also note that we shouldn't be able to leave with the reference
+ * count retained unless we succeeded, or the operation was
+ * preemptible.
+ */
+ if ( likely(!rc) || partial_ref )
+ /* nothing */;
+ else if ( page == current->arch.old_guest_table )
+ ASSERT(preemptible);
+ else
put_page(page);
return rc;
--
2.23.0

View file

@ -0,0 +1,101 @@
From d28893777be56ef51562ed32502377974f738fd3 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 06/11] x86/mm: Have alloc_l[23]_table clear partial_flags when
preempting
In order to allow recursive pagetable promotions and demotions to be
interrupted, Xen must keep track of the state of the sub-pages
promoted or demoted. This is stored in two elements in the page
struct: nr_entries_validated and partial_flags.
The rule is that entries [0, nr_entries_validated) should always be
validated and hold a general reference count. If partial_flags is
zero, then [nr_entries_validated] is not validated and no reference
count is held. If PTF_partial_set is set, then [nr_entries_validated]
is partially validated.
At the moment, a distinction is made between promotion and demotion
with regard to whether the entry itself "holds" a general reference
count: when entry promotion is interrupted (i.e., returns -ERESTART),
the entry is not considered to hold a reference; when entry demotion
is interrupted, the entry is still considered to hold a general
reference.
PTF_partial_general_ref is used to distinguish between these cases.
If clear, it's a partial promotion => no general reference count held
by the entry; if set, it's partial demotion, so a general reference
count held. Because promotions and demotions can be interleaved, this
value is passed to get_page_and_type_from_mfn and put_page_from_l*e,
to be able to properly handle reference counts.
Unfortunately, when alloc_l[23]_table check hypercall_preempt_check()
and return -ERESTART, they set nr_entries_validated, but don't clear
partial_flags.
If we were picking up from a previously-interrupted promotion, that
means that PTF_partial_set would be set even though
[nr_entries_validated] was not partially validated. This means that
if the page in this state were de-validated, put_page_type() would
erroneously be called on that entry.
Perhaps worse, if we were racing with a de-validation, then we might
leave both PTF_partial_set and PTF_partial_general_ref; and when
de-validation picked up again, both the type and the general ref would
be erroneously dropped from [nr_entries_validated].
In a sense, the real issue here is code duplication. Rather than
duplicate the interruption code, set rc to -EINTR and fall through to
the code which already handles that case correctly.
Given the logic at this point, it should be impossible for
partial_flags to be non-zero; add an ASSERT() to catch any changes.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 18 ++++--------------
1 file changed, 4 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index eaf7b14245..053465cb7c 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1545,13 +1545,8 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
i++, partial_flags = 0 )
{
if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
- {
- page->nr_validated_ptes = i;
- rc = -ERESTART;
- break;
- }
-
- if ( !is_guest_l2_slot(d, type, i) ||
+ rc = -EINTR;
+ else if ( !is_guest_l2_slot(d, type, i) ||
(rc = get_page_from_l2e(pl2e[i], pfn, d, partial_flags)) > 0 )
continue;
@@ -1616,13 +1611,8 @@ static int alloc_l3_table(struct page_info *page)
i++, partial_flags = 0 )
{
if ( i > page->nr_validated_ptes && hypercall_preempt_check() )
- {
- page->nr_validated_ptes = i;
- rc = -ERESTART;
- break;
- }
-
- if ( is_pv_32bit_domain(d) && (i == 3) )
+ rc = -EINTR;
+ else if ( is_pv_32bit_domain(d) && (i == 3) )
{
if ( !(l3e_get_flags(pl3e[i]) & _PAGE_PRESENT) ||
(l3e_get_flags(pl3e[i]) & l3_disallow_mask(d)) )
--
2.23.0

View file

@ -0,0 +1,374 @@
From f608a53c25806a7a4318cbe225bc5f5bbf154d69 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 07/11] x86/mm: Always retain a general ref on partial
In order to allow recursive pagetable promotions and demotions to be
interrupted, Xen must keep track of the state of the sub-pages
promoted or demoted. This is stored in two elements in the page struct:
nr_entries_validated and partial_flags.
The rule is that entries [0, nr_entries_validated) should always be
validated and hold a general reference count. If partial_flags is
zero, then [nr_entries_validated] is not validated and no reference
count is held. If PTF_partial_set is set, then [nr_entries_validated]
is partially validated.
At the moment, a distinction is made between promotion and demotion
with regard to whether the entry itself "holds" a general reference
count: when entry promotion is interrupted (i.e., returns -ERESTART),
the entry is not considered to hold a reference; when entry demotion
is interrupted, the entry is still considered to hold a general
reference.
PTF_partial_general_ref is used to distinguish between these cases.
If clear, it's a partial promotion => no general reference count held
by the entry; if set, it's partial demotion, so a general reference
count held. Because promotions and demotions can be interleaved, this
value is passed to get_page_and_type_from_mfn and put_page_from_l*e,
to be able to properly handle reference counts.
Unfortunately, because a refcount is not held, it is possible to
engineer a situation where PFT_partial_set is set but the page in
question has been assigned to another domain. A sketch is provided in
the appendix.
Fix this by having the parent page table entry hold a general
reference count whenever PFT_partial_set is set. (For clarity of
change, keep two separate flags. These will be collapsed in a
subsequent changeset.)
This has two basic implications. On the put_page_from_lNe() side,
this mean that the (partial_set && !partial_ref) case can never happen,
and no longer needs to be special-cased.
Secondly, because both flags are set together, there's no need to carry over
existing bits from partial_pte.
(NB there is still another issue with calling _put_page_type() on a
page which had PGT_partial set; that will be handled in a subsequent
patch.)
On the get_page_and_type_from_mfn() side, we need to distinguish
between callers which hold a reference on partial (i.e.,
alloc_lN_table()), and those which do not (new_cr3, PIN_LN_TABLE, and
so on): pass a flag if the type should be retained on interruption.
NB that since l1 promotion can't be preempted, that get_page_from_l2e
can't return -ERESTART.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
-----
* Appendix: Engineering PTF_partial_set while a page belongs to a
foreign domain
Suppose A is a page which can be promoted to an l3, and B is a page
which can be promoted to an l2, and A[x] points to B. B has
PGC_allocated set but no other general references.
V1: PIN_L3 A.
A is validated, B is validated.
A.type_count = 1 | PGT_validated | PGT_pinned
B.type_count = 1 | PGT_validated
B.count = 2 | PGC_allocated (A[x] holds a general ref)
V1: UNPIN A.
A begins de-validation.
Arrange to be interrupted when i < x
V1->old_guest_table = A
V1->old_guest_table_ref_held = false
A.type_count = 1 | PGT_partial
A.nr_validated_entries = i < x
B.type_count = 0
B.count = 1 | PGC_allocated
V2: MOD_L4_ENTRY to point some l4e to A.
Picks up re-validation of A.
Arrange to be interrupted halfway through B's validation
B.type_count = 1 | PGT_partial
B.count = 2 | PGC_allocated (PGT_partial holds a general ref)
A.type_count = 1 | PGT_partial
A.nr_validated_entries = x
A.partial_pte = PTF_partial_set
V3: MOD_L3_ENTRY to point some other l3e (not in A) to B.
Validates B.
B.type_count = 1 | PGT_validated
B.count = 2 | PGC_allocated ("other l3e" holds a general ref)
V3: MOD_L3_ENTRY to clear l3e pointing to B.
Devalidates B.
B.type_count = 0
B.count = 1 | PGC_allocated
V3: decrease_reservation(B)
Clears PGC_allocated
B.count = 0 => B is freed
B gets assigned to a different domain
V1: Restarts UNPIN of A
put_old_guest_table(A)
...
free_l3_table(A)
Now since A.partial_flags has PTF_partial_set, free_l3_table() will
call put_page_from_l3e() on A[x], which points to B, while B is owned
by another domain.
If A[x] held a general refcount for B on partial validation, as it does
for partial de-validation, then B would still have a reference count of
1 after PGC_allocated was freed; so B wouldn't be freed until after
put_page_from_l3e() had happend on A[x].
---
xen/arch/x86/mm.c | 84 +++++++++++++++++++++++-----------------
xen/include/asm-x86/mm.h | 15 ++++---
2 files changed, 58 insertions(+), 41 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 053465cb7c..68a9e74002 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -617,10 +617,11 @@ static int _get_page_type(struct page_info *page, unsigned long type,
* page->pte[page->nr_validated_entries]. See the comment in mm.h for
* more information.
*/
-#define PTF_partial_set (1 << 0)
-#define PTF_partial_general_ref (1 << 1)
-#define PTF_preemptible (1 << 2)
-#define PTF_defer (1 << 3)
+#define PTF_partial_set (1 << 0)
+#define PTF_partial_general_ref (1 << 1)
+#define PTF_preemptible (1 << 2)
+#define PTF_defer (1 << 3)
+#define PTF_retain_ref_on_restart (1 << 4)
static int get_page_and_type_from_mfn(
mfn_t mfn, unsigned long type, struct domain *d,
@@ -629,7 +630,11 @@ static int get_page_and_type_from_mfn(
struct page_info *page = mfn_to_page(mfn);
int rc;
bool preemptible = flags & PTF_preemptible,
- partial_ref = flags & PTF_partial_general_ref;
+ partial_ref = flags & PTF_partial_general_ref,
+ partial_set = flags & PTF_partial_set,
+ retain_ref = flags & PTF_retain_ref_on_restart;
+
+ ASSERT(partial_ref == partial_set);
if ( likely(!partial_ref) &&
unlikely(!get_page_from_mfn(mfn, d)) )
@@ -642,13 +647,15 @@ static int get_page_and_type_from_mfn(
* - page is fully validated (rc == 0)
* - page is not validated (rc < 0) but:
* - We came in with a reference (partial_ref)
+ * - page is partially validated (rc == -ERESTART), and the
+ * caller has asked the ref to be retained in that case
* - page is partially validated but there's been an error
* (page == current->arch.old_guest_table)
*
* The partial_ref-on-error clause is worth an explanation. There
* are two scenarios where partial_ref might be true coming in:
- * - mfn has been partially demoted as type `type`; i.e. has
- * PGT_partial set
+ * - mfn has been partially promoted / demoted as type `type`;
+ * i.e. has PGT_partial set
* - mfn has been partially demoted as L(type+1) (i.e., a linear
* page; e.g. we're being called from get_page_from_l2e with
* type == PGT_l1_table, but the mfn is PGT_l2_table)
@@ -671,7 +678,8 @@ static int get_page_and_type_from_mfn(
*/
if ( likely(!rc) || partial_ref )
/* nothing */;
- else if ( page == current->arch.old_guest_table )
+ else if ( page == current->arch.old_guest_table ||
+ (retain_ref && rc == -ERESTART) )
ASSERT(preemptible);
else
put_page(page);
@@ -1348,8 +1356,8 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
PTF_partial_set )
{
- ASSERT(!(flags & PTF_defer));
- rc = _put_page_type(pg, PTF_preemptible, ptpg);
+ /* partial_set should always imply partial_ref */
+ BUG();
}
else if ( flags & PTF_defer )
{
@@ -1394,8 +1402,8 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
PTF_partial_set )
{
- ASSERT(!(flags & PTF_defer));
- return _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
+ /* partial_set should always imply partial_ref */
+ BUG();
}
if ( flags & PTF_defer )
@@ -1425,8 +1433,8 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
PTF_partial_set )
{
- ASSERT(!(flags & PTF_defer));
- return _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
+ /* partial_set should always imply partial_ref */
+ BUG();
}
if ( flags & PTF_defer )
@@ -1550,13 +1558,22 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
(rc = get_page_from_l2e(pl2e[i], pfn, d, partial_flags)) > 0 )
continue;
- if ( rc == -ERESTART )
- {
- page->nr_validated_ptes = i;
- /* Set 'set', retain 'general ref' */
- page->partial_flags = partial_flags | PTF_partial_set;
- }
- else if ( rc == -EINTR && i )
+ /*
+ * It shouldn't be possible for get_page_from_l2e to return
+ * -ERESTART, since we never call this with PTF_preemptible.
+ * (alloc_l1_table may return -EINTR on an L1TF-vulnerable
+ * entry.)
+ *
+ * NB that while on a "clean" promotion, we can never get
+ * PGT_partial. It is possible to arrange for an l2e to
+ * contain a partially-devalidated l2; but in that case, both
+ * of the following functions will fail anyway (the first
+ * because the page in question is not an l1; the second
+ * because the page is not fully validated).
+ */
+ ASSERT(rc != -ERESTART);
+
+ if ( rc == -EINTR && i )
{
page->nr_validated_ptes = i;
page->partial_flags = 0;
@@ -1565,6 +1582,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
else if ( rc < 0 && rc != -EINTR )
{
gdprintk(XENLOG_WARNING, "Failure in alloc_l2_table: slot %#x\n", i);
+ ASSERT(current->arch.old_guest_table == NULL);
if ( i )
{
page->nr_validated_ptes = i;
@@ -1621,16 +1639,17 @@ static int alloc_l3_table(struct page_info *page)
rc = get_page_and_type_from_mfn(
l3e_get_mfn(pl3e[i]),
PGT_l2_page_table | PGT_pae_xen_l2, d,
- partial_flags | PTF_preemptible);
+ partial_flags | PTF_preemptible | PTF_retain_ref_on_restart);
}
- else if ( (rc = get_page_from_l3e(pl3e[i], pfn, d, partial_flags)) > 0 )
+ else if ( (rc = get_page_from_l3e(pl3e[i], pfn, d,
+ partial_flags | PTF_retain_ref_on_restart)) > 0 )
continue;
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
/* Set 'set', leave 'general ref' set if this entry was set */
- page->partial_flags = partial_flags | PTF_partial_set;
+ page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
}
else if ( rc == -EINTR && i )
{
@@ -1791,14 +1810,15 @@ static int alloc_l4_table(struct page_info *page)
i++, partial_flags = 0 )
{
if ( !is_guest_l4_slot(d, i) ||
- (rc = get_page_from_l4e(pl4e[i], pfn, d, partial_flags)) > 0 )
+ (rc = get_page_from_l4e(pl4e[i], pfn, d,
+ partial_flags | PTF_retain_ref_on_restart)) > 0 )
continue;
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
/* Set 'set', leave 'general ref' set if this entry was set */
- page->partial_flags = partial_flags | PTF_partial_set;
+ page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
}
else if ( rc < 0 )
{
@@ -1896,9 +1916,7 @@ static int free_l2_table(struct page_info *page)
else if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = (partial_flags & PTF_partial_set) ?
- partial_flags :
- (PTF_partial_set | PTF_partial_general_ref);
+ page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
}
else if ( rc == -EINTR && i < L2_PAGETABLE_ENTRIES - 1 )
{
@@ -1946,9 +1964,7 @@ static int free_l3_table(struct page_info *page)
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = (partial_flags & PTF_partial_set) ?
- partial_flags :
- (PTF_partial_set | PTF_partial_general_ref);
+ page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
}
else if ( rc == -EINTR && i < L3_PAGETABLE_ENTRIES - 1 )
{
@@ -1979,9 +1995,7 @@ static int free_l4_table(struct page_info *page)
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = (partial_flags & PTF_partial_set) ?
- partial_flags :
- (PTF_partial_set | PTF_partial_general_ref);
+ page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
}
else if ( rc == -EINTR && i < L4_PAGETABLE_ENTRIES - 1 )
{
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index 46cba52941..dc9cb869dd 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -238,22 +238,25 @@ struct page_info
* page.
*
* This happens:
- * - During de-validation, if de-validation of the page was
+ * - During validation or de-validation, if the operation was
* interrupted
* - During validation, if an invalid entry is encountered and
* validation is preemptible
* - During validation, if PTF_partial_general_ref was set on
- * this entry to begin with (perhaps because we're picking
- * up from a partial de-validation).
+ * this entry to begin with (perhaps because it picked up a
+ * previous operation)
*
- * When resuming validation, if PTF_partial_general_ref is clear,
- * then a general reference must be re-acquired; if it is set, no
- * reference should be acquired.
+ * When resuming validation, if PTF_partial_general_ref is
+ * clear, then a general reference must be re-acquired; if it
+ * is set, no reference should be acquired.
*
* When resuming de-validation, if PTF_partial_general_ref is
* clear, no reference should be dropped; if it is set, a
* reference should be dropped.
*
+ * NB at the moment, PTF_partial_set should be set if and only if
+ * PTF_partial_general_ref is set.
+ *
* NB that PTF_partial_set and PTF_partial_general_ref are
* defined in mm.c, the only place where they are used.
*
--
2.23.0

View file

@ -0,0 +1,227 @@
From 6811df7fb7a1d4bb5a75fec9cf41519b5c86c605 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 08/11] x86/mm: Collapse PTF_partial_set and
PTF_partial_general_ref into one
...now that they are equivalent. No functional change intended.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 50 +++++++++++-----------------------------
xen/include/asm-x86/mm.h | 29 +++++++++++------------
2 files changed, 26 insertions(+), 53 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 68a9e74002..4970b19aff 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -612,13 +612,12 @@ static int _get_page_type(struct page_info *page, unsigned long type,
/*
* The following flags are used to specify behavior of various get and
- * put commands. The first two are also stored in page->partial_flags
- * to indicate the state of the page pointed to by
+ * put commands. The first is also stored in page->partial_flags to
+ * indicate the state of the page pointed to by
* page->pte[page->nr_validated_entries]. See the comment in mm.h for
* more information.
*/
#define PTF_partial_set (1 << 0)
-#define PTF_partial_general_ref (1 << 1)
#define PTF_preemptible (1 << 2)
#define PTF_defer (1 << 3)
#define PTF_retain_ref_on_restart (1 << 4)
@@ -630,13 +629,10 @@ static int get_page_and_type_from_mfn(
struct page_info *page = mfn_to_page(mfn);
int rc;
bool preemptible = flags & PTF_preemptible,
- partial_ref = flags & PTF_partial_general_ref,
partial_set = flags & PTF_partial_set,
retain_ref = flags & PTF_retain_ref_on_restart;
- ASSERT(partial_ref == partial_set);
-
- if ( likely(!partial_ref) &&
+ if ( likely(!partial_set) &&
unlikely(!get_page_from_mfn(mfn, d)) )
return -EINVAL;
@@ -646,14 +642,14 @@ static int get_page_and_type_from_mfn(
* Retain the refcount if:
* - page is fully validated (rc == 0)
* - page is not validated (rc < 0) but:
- * - We came in with a reference (partial_ref)
+ * - We came in with a reference (partial_set)
* - page is partially validated (rc == -ERESTART), and the
* caller has asked the ref to be retained in that case
* - page is partially validated but there's been an error
* (page == current->arch.old_guest_table)
*
- * The partial_ref-on-error clause is worth an explanation. There
- * are two scenarios where partial_ref might be true coming in:
+ * The partial_set-on-error clause is worth an explanation. There
+ * are two scenarios where partial_set might be true coming in:
* - mfn has been partially promoted / demoted as type `type`;
* i.e. has PGT_partial set
* - mfn has been partially demoted as L(type+1) (i.e., a linear
@@ -676,7 +672,7 @@ static int get_page_and_type_from_mfn(
* count retained unless we succeeded, or the operation was
* preemptible.
*/
- if ( likely(!rc) || partial_ref )
+ if ( likely(!rc) || partial_set )
/* nothing */;
else if ( page == current->arch.old_guest_table ||
(retain_ref && rc == -ERESTART) )
@@ -1353,13 +1349,7 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
struct page_info *pg = l2e_get_page(l2e);
struct page_info *ptpg = mfn_to_page(_mfn(pfn));
- if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
- PTF_partial_set )
- {
- /* partial_set should always imply partial_ref */
- BUG();
- }
- else if ( flags & PTF_defer )
+ if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = ptpg;
current->arch.old_guest_table = pg;
@@ -1399,13 +1389,6 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
pg = l3e_get_page(l3e);
- if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
- PTF_partial_set )
- {
- /* partial_set should always imply partial_ref */
- BUG();
- }
-
if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
@@ -1430,13 +1413,6 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
{
struct page_info *pg = l4e_get_page(l4e);
- if ( (flags & (PTF_partial_set | PTF_partial_general_ref)) ==
- PTF_partial_set )
- {
- /* partial_set should always imply partial_ref */
- BUG();
- }
-
if ( flags & PTF_defer )
{
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
@@ -1649,7 +1625,7 @@ static int alloc_l3_table(struct page_info *page)
{
page->nr_validated_ptes = i;
/* Set 'set', leave 'general ref' set if this entry was set */
- page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
+ page->partial_flags = PTF_partial_set;
}
else if ( rc == -EINTR && i )
{
@@ -1818,7 +1794,7 @@ static int alloc_l4_table(struct page_info *page)
{
page->nr_validated_ptes = i;
/* Set 'set', leave 'general ref' set if this entry was set */
- page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
+ page->partial_flags = PTF_partial_set;
}
else if ( rc < 0 )
{
@@ -1916,7 +1892,7 @@ static int free_l2_table(struct page_info *page)
else if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
+ page->partial_flags = PTF_partial_set;
}
else if ( rc == -EINTR && i < L2_PAGETABLE_ENTRIES - 1 )
{
@@ -1964,7 +1940,7 @@ static int free_l3_table(struct page_info *page)
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
+ page->partial_flags = PTF_partial_set;
}
else if ( rc == -EINTR && i < L3_PAGETABLE_ENTRIES - 1 )
{
@@ -1995,7 +1971,7 @@ static int free_l4_table(struct page_info *page)
if ( rc == -ERESTART )
{
page->nr_validated_ptes = i;
- page->partial_flags = PTF_partial_set | PTF_partial_general_ref;
+ page->partial_flags = PTF_partial_set;
}
else if ( rc == -EINTR && i < L4_PAGETABLE_ENTRIES - 1 )
{
diff --git a/xen/include/asm-x86/mm.h b/xen/include/asm-x86/mm.h
index dc9cb869dd..c6ba9e4d73 100644
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -233,7 +233,7 @@ struct page_info
* operation on the current page. (That page may or may not
* still have PGT_partial set.)
*
- * If PTF_partial_general_ref is set, then the PTE at
+ * Additionally, if PTF_partial_set is set, then the PTE at
* @nr_validated_ptef holds a general reference count for the
* page.
*
@@ -242,23 +242,20 @@ struct page_info
* interrupted
* - During validation, if an invalid entry is encountered and
* validation is preemptible
- * - During validation, if PTF_partial_general_ref was set on
- * this entry to begin with (perhaps because it picked up a
+ * - During validation, if PTF_partial_set was set on this
+ * entry to begin with (perhaps because it picked up a
* previous operation)
*
- * When resuming validation, if PTF_partial_general_ref is
- * clear, then a general reference must be re-acquired; if it
- * is set, no reference should be acquired.
+ * When resuming validation, if PTF_partial_set is clear, then
+ * a general reference must be re-acquired; if it is set, no
+ * reference should be acquired.
*
- * When resuming de-validation, if PTF_partial_general_ref is
- * clear, no reference should be dropped; if it is set, a
- * reference should be dropped.
+ * When resuming de-validation, if PTF_partial_set is clear,
+ * no reference should be dropped; if it is set, a reference
+ * should be dropped.
*
- * NB at the moment, PTF_partial_set should be set if and only if
- * PTF_partial_general_ref is set.
- *
- * NB that PTF_partial_set and PTF_partial_general_ref are
- * defined in mm.c, the only place where they are used.
+ * NB that PTF_partial_set is defined in mm.c, the only place
+ * where it is used.
*
* The 3rd field, @linear_pt_count, indicates
* - by a positive value, how many same-level page table entries a page
@@ -268,8 +265,8 @@ struct page_info
*/
struct {
u16 nr_validated_ptes:PAGETABLE_ORDER + 1;
- u16 :16 - PAGETABLE_ORDER - 1 - 2;
- u16 partial_flags:2;
+ u16 :16 - PAGETABLE_ORDER - 1 - 1;
+ u16 partial_flags:1;
s16 linear_pt_count;
};
--
2.23.0

View file

@ -0,0 +1,106 @@
From a6098b8920b02149220641cb13358e9012b5fc4d Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 09/11] x86/mm: Properly handle linear pagetable promotion
failures
In order to allow recursive pagetable promotions and demotions to be
interrupted, Xen must keep track of the state of the sub-pages
promoted or demoted. This is stored in two elements in the page
struct: nr_entries_validated and partial_flags.
The rule is that entries [0, nr_entries_validated) should always be
validated and hold a general reference count. If partial_flags is
zero, then [nr_entries_validated] is not validated and no reference
count is held. If PTF_partial_set is set, then [nr_entries_validated]
is partially validated, and a general reference count is held.
Unfortunately, in cases where an entry began with PTF_partial_set set,
and get_page_from_lNe() returns -EINVAL, the PTF_partial_set bit is
erroneously dropped. (This scenario can be engineered mainly by the
use of interleaving of promoting and demoting a page which has "linear
pagetable" entries; see the appendix for a sketch.) This means that
we will "leak" a general reference count on the page in question,
preventing the page from being freed.
Fix this by setting page->partial_flags to the partial_flags local
variable.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
-----
Appendix
Suppose A and B can both be promoted to L2 pages, and A[x] points to B.
V1: PIN_L2 B.
B.type_count = 1 | PGT_validated
B.count = 2 | PGC_allocated
V1: MOD_L3_ENTRY pointing something to A.
In the process of validating A[x], grab an extra type / ref on B:
B.type_count = 2 | PGT_validated
B.count = 3 | PGC_allocated
A.type_count = 1 | PGT_validated
A.count = 2 | PGC_allocated
V1: UNPIN B.
B.type_count = 1 | PGT_validate
B.count = 2 | PGC_allocated
V1: MOD_L3_ENTRY removing the reference to A.
De-validate A, down to A[x], which points to B.
Drop the final type on B. Arrange to be interrupted.
B.type_count = 1 | PGT_partial
B.count = 2 | PGC_allocated
A.type_count = 1 | PGT_partial
A.nr_validated_entries = x
A.partial_pte = -1
V2: MOD_L3_ENTRY adds a reference to A.
At this point, get_page_from_l2e(A[x]) tries
get_page_and_type_from_mfn(), which fails because it's the wrong type;
and get_l2_linear_pagetable() also fails, because B isn't validated as
an l2 anymore.
---
xen/arch/x86/mm.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index 4970b19aff..cfb7538403 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1562,7 +1562,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_flags = 0;
+ page->partial_flags = partial_flags;
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
@@ -1647,7 +1647,7 @@ static int alloc_l3_table(struct page_info *page)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_flags = 0;
+ page->partial_flags = partial_flags;
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
@@ -1804,7 +1804,7 @@ static int alloc_l4_table(struct page_info *page)
if ( i )
{
page->nr_validated_ptes = i;
- page->partial_flags = 0;
+ page->partial_flags = partial_flags;
if ( rc == -EINTR )
rc = -ERESTART;
else
--
2.23.0

View file

@ -0,0 +1,169 @@
From eabd77b59f4006128501d6e15f9e620dfb349420 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:49 +0100
Subject: [PATCH 10/11] x86/mm: Fix nested de-validation on error
If an invalid entry is discovered when validating a page-table tree,
the entire tree which has so far been validated must be de-validated.
Since this may take a long time, alloc_l[2-4]_table() set current
vcpu's old_guest_table immediately; put_old_guest_table() will make
sure that put_page_type() will be called to finish off the
de-validation before any other MMU operations can happen on the vcpu.
The invariant for partial pages should be:
* Entries [0, nr_validated_ptes) should be completely validated;
put_page_type() will de-validate these.
* If [nr_validated_ptes] is partially validated, partial_flags should
set PTF_partiaL_set. put_page_type() will be called on this page to
finish off devalidation, and the appropriate refcount adjustments
will be done.
alloc_l[2-3]_table() indicates partial validation to its callers by
setting current->old_guest_table.
Unfortunately, this is mishandled.
Take the case where validating lNe[x] returns an error.
First, alloc_l3_table() doesn't check old_guest_table at all; as a
result, partial_flags is not set when it should be. nr_validated_ptes
is set to x; and since PFT_partial_set clear, de-validation resumes at
nr_validated_ptes-1. This means that the l2 page at pl3e[x] will not
have put_page_type() called on it when de-validating the rest of the
l3: it will be stuck in the PGT_partial state until the domain is
destroyed, or until it is re-used as an l2. (Any other page type will
fail.)
Worse, alloc_l4_table(), rather than setting PTF_partial_set as it
should, sets nr_validated_ptes to x+1. When de-validating, since
partial is 0, this will correctly resume calling put_page_type at [x];
but, if the put_page_type() is never called, but instead
get_page_type() is called, validation will pick up at [x+1],
neglecting to validate [x]. If the rest of the validation succeeds,
the l4 will be validated even though [x] is invalid.
Fix this in both cases by setting PTF_partial_set if old_guest_table
is set.
While here, add some safety catches:
- old_guest_table must point to the page contained in
[nr_validated_ptes].
- alloc_l1_page shouldn't set old_guest_table
If we experience one of these situations in production builds, it's
safer to avoid calling put_page_type for the pages in question. If
they have PGT_partial set, they will be cleaned up on domain
destruction; if not, we have no idea whether a type count is safe to
drop. Retaining an extra type ref that should have been dropped may
trigger a BUG() on the free_domain_page() path, but dropping a type
count that shouldn't be dropped may cause a privilege escalation.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
xen/arch/x86/mm.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index cfb7538403..aa03cb8b40 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1561,6 +1561,20 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
ASSERT(current->arch.old_guest_table == NULL);
if ( i )
{
+ /*
+ * alloc_l1_table() doesn't set old_guest_table; it does
+ * its own tear-down immediately on failure. If it
+ * did we'd need to check it and set partial_flags as we
+ * do in alloc_l[34]_table().
+ *
+ * Note on the use of ASSERT: if it's non-null and
+ * hasn't been cleaned up yet, it should have
+ * PGT_partial set; and so the type will be cleaned up
+ * on domain destruction. Unfortunately, we would
+ * leak the general ref held by old_guest_table; but
+ * leaking a page is less bad than a host crash.
+ */
+ ASSERT(current->arch.old_guest_table == NULL);
page->nr_validated_ptes = i;
page->partial_flags = partial_flags;
current->arch.old_guest_ptpg = NULL;
@@ -1588,6 +1602,7 @@ static int alloc_l3_table(struct page_info *page)
unsigned int i;
int rc = 0;
unsigned int partial_flags = page->partial_flags;
+ l3_pgentry_t l3e = l3e_empty();
pl3e = map_domain_page(_mfn(pfn));
@@ -1634,7 +1649,11 @@ static int alloc_l3_table(struct page_info *page)
rc = -ERESTART;
}
if ( rc < 0 )
+ {
+ /* XSA-299 Backport: Copy l3e for checking */
+ l3e = pl3e[i];
break;
+ }
pl3e[i] = adjust_guest_l3e(pl3e[i], d);
}
@@ -1648,6 +1667,24 @@ static int alloc_l3_table(struct page_info *page)
{
page->nr_validated_ptes = i;
page->partial_flags = partial_flags;
+ if ( current->arch.old_guest_table )
+ {
+ /*
+ * We've experienced a validation failure. If
+ * old_guest_table is set, "transfer" the general
+ * reference count to pl3e[nr_validated_ptes] by
+ * setting PTF_partial_set.
+ *
+ * As a precaution, check that old_guest_table is the
+ * page pointed to by pl3e[nr_validated_ptes]. If
+ * not, it's safer to leak a type ref on production
+ * builds.
+ */
+ if ( current->arch.old_guest_table == l3e_get_page(l3e) )
+ page->partial_flags = PTF_partial_set;
+ else
+ ASSERT_UNREACHABLE();
+ }
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
@@ -1810,7 +1847,23 @@ static int alloc_l4_table(struct page_info *page)
else
{
if ( current->arch.old_guest_table )
- page->nr_validated_ptes++;
+ {
+ /*
+ * We've experienced a validation failure. If
+ * old_guest_table is set, "transfer" the general
+ * reference count to pl3e[nr_validated_ptes] by
+ * setting PTF_partial_set.
+ *
+ * As a precaution, check that old_guest_table is the
+ * page pointed to by pl4e[nr_validated_ptes]. If
+ * not, it's safer to leak a type ref on production
+ * builds.
+ */
+ if ( current->arch.old_guest_table == l4e_get_page(pl4e[i]) )
+ page->partial_flags = PTF_partial_set;
+ else
+ ASSERT_UNREACHABLE();
+ }
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
}
--
2.23.0

View file

@ -0,0 +1,413 @@
From f0086e3ac65c8bcabb84c1c29ab00b0c8a187555 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Thu, 10 Oct 2019 17:57:50 +0100
Subject: [PATCH 11/11] x86/mm: Don't drop a type ref unless you held a ref to
begin with
Validation and de-validation of pagetable trees may take arbitrarily
large amounts of time, and so must be preemptible. This is indicated
by setting the PGT_partial bit in the type_info, and setting
nr_validated_entries and partial_flags appropriately. Specifically,
if the entry at [nr_validated_entries] is partially validated,
partial_flags should have the PGT_partial_set bit set, and the entry
should hold a general reference count. During de-validation,
put_page_type() is called on partially validated entries.
Unfortunately, there are a number of issues with the current algorithm.
First, doing a "normal" put_page_type() is not safe when no type ref
is held: there is nothing to stop another vcpu from coming along and
picking up validation again: at which point the put_page_type may drop
the only page ref on an in-use page. Some examples are listed in the
appendix.
The core issue is that put_page_type() is being called both to clean
up PGT_partial, and to drop a type count; and has no way of knowing
which is which; and so if in between, PGT_partial is cleared,
put_page_type() will drop the type ref erroneously.
What is needed is to distinguish between two states:
- Dropping a type ref which is held
- Cleaning up a page which has been partially de/validated
Fix this by telling put_page_type() which of the two activities you
intend.
When cleaning up a partial de/validation, take no action unless you
find a page partially validated.
If put_page_type() is called without PTF_partial_set, and finds the
page in a PGT_partial state anyway, then there's certainly been a
misaccounting somewhere, and carrying on would almost certainly cause
a security issue, so crash the host instead.
In put_page_from_lNe, pass partial_flags on to _put_page_type().
old_guest_table may be set either with a fully validated page (when
using the "deferred put" pattern), or with a partially validated page
(when a normal "de-validation" is interrupted, or when a validation
fails part-way through due to invalid entries). Add a flag,
old_guest_table_partial, to indicate which of these it is, and use
that to pass the appropriate flag to _put_page_type().
While here, delete stray trailing whitespace.
This is part of XSA-299.
Reported-by: George Dunlap <george.dunlap@citrix.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
-----
Appendix:
Suppose page A, when interpreted as an l3 pagetable, contains all
valid entries; and suppose A[x] points to page B, which when
interpreted as an l2 pagetable, contains all valid entries.
P1: PIN_L3_TABLE
A -> PGT_l3_table | 1 | valid
B -> PGT_l2_table | 1 | valid
P1: UNPIN_TABLE
> Arrange to interrupt after B has been de-validated
B:
type_info -> PGT_l2_table | 0
A:
type_info -> PGT_l3_table | 1 | partial
nr_validated_enties -> (less than x)
P2: mod_l4_entry to point to A
> Arrange for this to be interrupted while B is being validated
B:
type_info -> PGT_l2_table | 1 | partial
(nr_validated_entires &c set as appropriate)
A:
type_info -> PGT_l3_table | 1 | partial
nr_validated_entries -> x
partial_pte = 1
P3: mod_l3_entry some other unrelated l3 to point to B:
B:
type_info -> PGT_l2_table | 1
P1: Restart UNPIN_TABLE
At this point, since A.nr_validate_entries == x and A.partial_pte !=
0, free_l3_table() will call put_page_from_l3e() on pl3e[x], dropping
its type count to 0 while it's still being pointed to by some other l3
A similar issue arises with old_guest_table. Consider the following
scenario:
Suppose A is a page which, when interpreted as an l2, has valid entries
until entry x, which is invalid.
V1: PIN_L2_TABLE(A)
<Validate until we try to validate [x], get -EINVAL>
A -> PGT_l2_table | 1 | PGT_partial
V1 -> old_guest_table = A
<delayed>
V2: PIN_L2_TABLE(A)
<Pick up where V1 left off, try to re-validate [x], get -EINVAL>
A -> PGT_l2_table | 1 | PGT_partial
V2 -> old_guest_table = A
<restart>
put_old_guest_table()
_put_page_type(A)
A -> PGT_l2_table | 0
V1: <restart>
put_old_guest_table()
_put_page_type(A) # UNDERFLOW
Indeed, it is possible to engineer for old_guest_table for every vcpu
a guest has to point to the same page.
---
xen/arch/x86/domain.c | 6 +++
xen/arch/x86/mm.c | 99 +++++++++++++++++++++++++++++++-----
xen/include/asm-x86/domain.h | 4 +-
3 files changed, 95 insertions(+), 14 deletions(-)
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index 8fbecbb169..c880568dd4 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1074,9 +1074,15 @@ int arch_set_info_guest(
rc = -ERESTART;
/* Fallthrough */
case -ERESTART:
+ /*
+ * NB that we're putting the kernel-mode table
+ * here, which we've already successfully
+ * validated above; hence partial = false;
+ */
v->arch.old_guest_ptpg = NULL;
v->arch.old_guest_table =
pagetable_get_page(v->arch.guest_table);
+ v->arch.old_guest_table_partial = false;
v->arch.guest_table = pagetable_null();
break;
default:
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index aa03cb8b40..c701c7ef14 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -1353,10 +1353,11 @@ static int put_page_from_l2e(l2_pgentry_t l2e, unsigned long pfn,
{
current->arch.old_guest_ptpg = ptpg;
current->arch.old_guest_table = pg;
+ current->arch.old_guest_table_partial = false;
}
else
{
- rc = _put_page_type(pg, PTF_preemptible, ptpg);
+ rc = _put_page_type(pg, flags | PTF_preemptible, ptpg);
if ( likely(!rc) )
put_page(pg);
}
@@ -1379,6 +1380,7 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
unsigned long mfn = l3e_get_pfn(l3e);
int writeable = l3e_get_flags(l3e) & _PAGE_RW;
+ ASSERT(!(flags & PTF_partial_set));
ASSERT(!(mfn & ((1UL << (L3_PAGETABLE_SHIFT - PAGE_SHIFT)) - 1)));
do {
put_data_page(mfn_to_page(_mfn(mfn)), writeable);
@@ -1391,12 +1393,14 @@ static int put_page_from_l3e(l3_pgentry_t l3e, unsigned long pfn,
if ( flags & PTF_defer )
{
+ ASSERT(!(flags & PTF_partial_set));
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
current->arch.old_guest_table = pg;
+ current->arch.old_guest_table_partial = false;
return 0;
}
- rc = _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
+ rc = _put_page_type(pg, flags | PTF_preemptible, mfn_to_page(_mfn(pfn)));
if ( likely(!rc) )
put_page(pg);
@@ -1415,12 +1419,15 @@ static int put_page_from_l4e(l4_pgentry_t l4e, unsigned long pfn,
if ( flags & PTF_defer )
{
+ ASSERT(!(flags & PTF_partial_set));
current->arch.old_guest_ptpg = mfn_to_page(_mfn(pfn));
current->arch.old_guest_table = pg;
+ current->arch.old_guest_table_partial = false;
return 0;
}
- rc = _put_page_type(pg, PTF_preemptible, mfn_to_page(_mfn(pfn)));
+ rc = _put_page_type(pg, flags | PTF_preemptible,
+ mfn_to_page(_mfn(pfn)));
if ( likely(!rc) )
put_page(pg);
}
@@ -1525,6 +1532,14 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
pl2e = map_domain_page(_mfn(pfn));
+ /*
+ * NB that alloc_l2_table will never set partial_pte on an l2; but
+ * free_l2_table might if a linear_pagetable entry is interrupted
+ * partway through de-validation. In that circumstance,
+ * get_page_from_l2e() will always return -EINVAL; and we must
+ * retain the type ref by doing the normal partial_flags tracking.
+ */
+
for ( i = page->nr_validated_ptes; i < L2_PAGETABLE_ENTRIES;
i++, partial_flags = 0 )
{
@@ -1579,6 +1594,7 @@ static int alloc_l2_table(struct page_info *page, unsigned long type)
page->partial_flags = partial_flags;
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
+ current->arch.old_guest_table_partial = true;
}
}
if ( rc < 0 )
@@ -1681,12 +1697,16 @@ static int alloc_l3_table(struct page_info *page)
* builds.
*/
if ( current->arch.old_guest_table == l3e_get_page(l3e) )
+ {
+ ASSERT(current->arch.old_guest_table_partial);
page->partial_flags = PTF_partial_set;
+ }
else
ASSERT_UNREACHABLE();
}
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
+ current->arch.old_guest_table_partial = true;
}
while ( i-- > 0 )
pl3e[i] = unadjust_guest_l3e(pl3e[i], d);
@@ -1860,12 +1880,16 @@ static int alloc_l4_table(struct page_info *page)
* builds.
*/
if ( current->arch.old_guest_table == l4e_get_page(pl4e[i]) )
+ {
+ ASSERT(current->arch.old_guest_table_partial);
page->partial_flags = PTF_partial_set;
+ }
else
ASSERT_UNREACHABLE();
}
current->arch.old_guest_ptpg = NULL;
current->arch.old_guest_table = page;
+ current->arch.old_guest_table_partial = true;
}
}
}
@@ -2782,6 +2806,28 @@ static int _put_page_type(struct page_info *page, unsigned int flags,
x = y;
nx = x - 1;
+ /*
+ * Is this expected to do a full reference drop, or only
+ * cleanup partial validation / devalidation?
+ *
+ * If the former, the caller must hold a "full" type ref;
+ * which means the page must be validated. If the page is
+ * *not* fully validated, continuing would almost certainly
+ * open up a security hole. An exception to this is during
+ * domain destruction, where PGT_validated can be dropped
+ * without dropping a type ref.
+ *
+ * If the latter, do nothing unless type PGT_partial is set.
+ * If it is set, the type count must be 1.
+ */
+ if ( !(flags & PTF_partial_set) )
+ BUG_ON((x & PGT_partial) ||
+ !((x & PGT_validated) || page_get_owner(page)->is_dying));
+ else if ( !(x & PGT_partial) )
+ return 0;
+ else
+ BUG_ON((x & PGT_count_mask) != 1);
+
ASSERT((x & PGT_count_mask) != 0);
switch ( nx & (PGT_locked | PGT_count_mask) )
@@ -3041,17 +3087,34 @@ int put_old_guest_table(struct vcpu *v)
if ( !v->arch.old_guest_table )
return 0;
- switch ( rc = _put_page_type(v->arch.old_guest_table, PTF_preemptible,
- v->arch.old_guest_ptpg) )
+ rc = _put_page_type(v->arch.old_guest_table,
+ PTF_preemptible |
+ ( v->arch.old_guest_table_partial ?
+ PTF_partial_set : 0 ),
+ v->arch.old_guest_ptpg);
+
+ if ( rc == -ERESTART || rc == -EINTR )
{
- case -EINTR:
- case -ERESTART:
+ v->arch.old_guest_table_partial = (rc == -ERESTART);
return -ERESTART;
- case 0:
- put_page(v->arch.old_guest_table);
}
+ /*
+ * It shouldn't be possible for _put_page_type() to return
+ * anything else at the moment; but if it does happen in
+ * production, leaking the type ref is probably the best thing to
+ * do. Either way, drop the general ref held by old_guest_table.
+ */
+ ASSERT(rc == 0);
+
+ put_page(v->arch.old_guest_table);
v->arch.old_guest_table = NULL;
+ v->arch.old_guest_ptpg = NULL;
+ /*
+ * Safest default if someone sets old_guest_table without
+ * explicitly setting old_guest_table_partial.
+ */
+ v->arch.old_guest_table_partial = true;
return rc;
}
@@ -3201,11 +3264,11 @@ int new_guest_cr3(mfn_t mfn)
switch ( rc = put_page_and_type_preemptible(page) )
{
case -EINTR:
- rc = -ERESTART;
- /* fallthrough */
case -ERESTART:
curr->arch.old_guest_ptpg = NULL;
curr->arch.old_guest_table = page;
+ curr->arch.old_guest_table_partial = (rc == -ERESTART);
+ rc = -ERESTART;
break;
default:
BUG_ON(rc);
@@ -3479,6 +3542,7 @@ long do_mmuext_op(
{
curr->arch.old_guest_ptpg = NULL;
curr->arch.old_guest_table = page;
+ curr->arch.old_guest_table_partial = false;
}
}
}
@@ -3513,6 +3577,11 @@ long do_mmuext_op(
case -ERESTART:
curr->arch.old_guest_ptpg = NULL;
curr->arch.old_guest_table = page;
+ /*
+ * EINTR means we still hold the type ref; ERESTART
+ * means PGT_partial holds the type ref
+ */
+ curr->arch.old_guest_table_partial = (rc == -ERESTART);
rc = 0;
break;
default:
@@ -3581,11 +3650,15 @@ long do_mmuext_op(
switch ( rc = put_page_and_type_preemptible(page) )
{
case -EINTR:
- rc = -ERESTART;
- /* fallthrough */
case -ERESTART:
curr->arch.old_guest_ptpg = NULL;
curr->arch.old_guest_table = page;
+ /*
+ * EINTR means we still hold the type ref;
+ * ERESTART means PGT_partial holds the ref
+ */
+ curr->arch.old_guest_table_partial = (rc == -ERESTART);
+ rc = -ERESTART;
break;
default:
BUG_ON(rc);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 1ac5a96c08..360c38bd83 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -309,7 +309,7 @@ struct arch_domain
struct paging_domain paging;
struct p2m_domain *p2m;
- /* To enforce lock ordering in the pod code wrt the
+ /* To enforce lock ordering in the pod code wrt the
* page_alloc lock */
int page_alloc_unlock_level;
@@ -542,6 +542,8 @@ struct arch_vcpu
struct page_info *old_guest_table; /* partially destructed pagetable */
struct page_info *old_guest_ptpg; /* containing page table of the */
/* former, if any */
+ bool old_guest_table_partial; /* Are we dropping a type ref, or just
+ * finishing up a partial de-validation? */
/* guest_table holds a ref to the page, and also a type-count unless
* shadow refcounts are in use */
pagetable_t shadow_table[4]; /* (MFN) shadow(s) of guest */
--
2.23.0

80
xsa301-4.11-1.patch Normal file
View file

@ -0,0 +1,80 @@
From 21dfe8f707febd62869d4ebbaa155736870bebec Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Wed, 2 Oct 2019 12:06:50 +0100
Subject: [PATCH 1/3] xen/arm: p2m: Avoid aliasing guest physical frame
The P2M helpers implementation is quite lax and will end up to ignore
the unused top bits of a guest physical frame.
This effectively means that p2m_set_entry() will create a mapping for a
different frame (it is always equal to gfn & (mask unused bits)). Yet
p2m->max_mapped_gfn will be updated using the original frame.
At the moment, p2m_get_entry() and p2m_resolve_translation_fault()
assume that p2m_get_root_pointer() will always return a non-NULL pointer
when the GFN is smaller than p2m->max_mapped_gfn.
Unfortunately, because of the aliasing described above, it would be
possible to set p2m->max_mapped_gfn high enough so it covers frame that
would lead p2m_get_root_pointer() to return NULL.
As we don't sanity check the guest physical frame provided by a guest, a
malicious guest could craft a series of hypercalls that will hit the
BUG_ON() and therefore DoS Xen.
To prevent aliasing, the function p2m_get_root_pointer() is now reworked
to return NULL If any of the unused top bits are not zero. The caller
can then decide what's the appropriate action to do. Since the two paths
(i.e. P2M_ROOT_PAGES == 1 and P2M_ROOT_PAGES != 1) are now very
similarly, take the opportunity to consolidate them making the code a
bit simpler.
With this change, p2m_get_entry() will not try to insert a mapping as
the root pointer is invalid.
Note that root_table is now switch to unsigned long as unsigned int is
not enough to hold part of a GFN.
This is part of XSA-301.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/p2m.c | 17 +++++------------
1 file changed, 5 insertions(+), 12 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index d43c3aa896..3967ee7306 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -177,21 +177,14 @@ void p2m_tlb_flush_sync(struct p2m_domain *p2m)
static lpae_t *p2m_get_root_pointer(struct p2m_domain *p2m,
gfn_t gfn)
{
- unsigned int root_table;
-
- if ( P2M_ROOT_PAGES == 1 )
- return __map_domain_page(p2m->root);
+ unsigned long root_table;
/*
- * Concatenated root-level tables. The table number will be the
- * offset at the previous level. It is not possible to
- * concatenate a level-0 root.
+ * While the root table index is the offset from the previous level,
+ * we can't use (P2M_ROOT_LEVEL - 1) because the root level might be
+ * 0. Yet we still want to check if all the unused bits are zeroed.
*/
- ASSERT(P2M_ROOT_LEVEL > 0);
-
- root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL - 1]);
- root_table &= LPAE_ENTRY_MASK;
-
+ root_table = gfn_x(gfn) >> (level_orders[P2M_ROOT_LEVEL] + LPAE_SHIFT);
if ( root_table >= P2M_ROOT_PAGES )
return NULL;
--
2.11.0

92
xsa301-4.11-2.patch Normal file
View file

@ -0,0 +1,92 @@
From 4426d993b7ee0966fb39531dc5a269ce8493ca97 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Wed, 2 Oct 2019 12:35:59 +0100
Subject: [PATCH 2/3] xen/arm: p2m: Avoid off-by-one check on
p2m->max_mapped_gfn
The code base is using inconsistently the field p2m->max_mapped_gfn.
Some of the useres expect that p2m->max_guest_gfn contain the highest
mapped GFN while others expect highest + 1.
p2m->max_guest_gfn is set as highest + 1, because of that the sanity
check on the GFN in p2m_resolved_translation_fault() and
p2m_get_entry() can be bypassed when GFN == p2m->max_guest_gfn.
p2m_get_root_pointer(p2m->max_guest_gfn) may return NULL if it is
outside of address range supported and therefore the BUG_ON() could be
hit.
The current value hold in p2m->max_mapped_gfn is inconsistent with the
expectation of the common code (see domain_get_maximum_gpfn()) and also
the documentation of the field.
Rather than changing the check in p2m_translation_fault() and
p2m_get_entry(), p2m->max_mapped_gfn is now containing the highest
mapped GFN and the callers assuming "highest + 1" are now adjusted.
Take the opportunity to use 1UL rather than 1 as page_order could
theoritically big enough to overflow a 32-bit integer.
Lastly, the documentation of the field max_guest_gfn to reflect how it
is computed.
This is part of XSA-301.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
---
xen/arch/arm/p2m.c | 6 +++---
xen/include/asm-arm/p2m.h | 5 +----
2 files changed, 4 insertions(+), 7 deletions(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index 3967ee7306..c7e049901d 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -931,7 +931,7 @@ static int __p2m_set_entry(struct p2m_domain *p2m,
p2m_write_pte(entry, pte, p2m->clean_pte);
p2m->max_mapped_gfn = gfn_max(p2m->max_mapped_gfn,
- gfn_add(sgfn, 1 << page_order));
+ gfn_add(sgfn, (1UL << page_order) - 1));
p2m->lowest_mapped_gfn = gfn_min(p2m->lowest_mapped_gfn, sgfn);
}
@@ -1291,7 +1291,7 @@ int relinquish_p2m_mapping(struct domain *d)
p2m_write_lock(p2m);
start = p2m->lowest_mapped_gfn;
- end = p2m->max_mapped_gfn;
+ end = gfn_add(p2m->max_mapped_gfn, 1);
for ( ; gfn_x(start) < gfn_x(end);
start = gfn_next_boundary(start, order) )
@@ -1356,7 +1356,7 @@ int p2m_cache_flush(struct domain *d, gfn_t start, unsigned long nr)
p2m_read_lock(p2m);
start = gfn_max(start, p2m->lowest_mapped_gfn);
- end = gfn_min(end, p2m->max_mapped_gfn);
+ end = gfn_min(end, gfn_add(p2m->max_mapped_gfn, 1));
for ( ; gfn_x(start) < gfn_x(end); start = next_gfn )
{
diff --git a/xen/include/asm-arm/p2m.h b/xen/include/asm-arm/p2m.h
index 8823707c17..7f1f7e9109 100644
--- a/xen/include/asm-arm/p2m.h
+++ b/xen/include/asm-arm/p2m.h
@@ -38,10 +38,7 @@ struct p2m_domain {
/* Current Translation Table Base Register for the p2m */
uint64_t vttbr;
- /*
- * Highest guest frame that's ever been mapped in the p2m
- * Only takes into account ram and foreign mapping
- */
+ /* Highest guest frame that's ever been mapped in the p2m */
gfn_t max_mapped_gfn;
/*
--
2.11.0

49
xsa301-4.11-3.patch Normal file
View file

@ -0,0 +1,49 @@
From 61c73af08b4ede1fc8cfd2cf72661e6c7cfdbeaa Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Wed, 2 Oct 2019 10:55:07 +0100
Subject: [PATCH 3/3] xen/arm: p2m: Don't check the return of
p2m_get_root_pointer() with BUG_ON()
It turns out that the BUG_ON() was actually reachable with well-crafted
hypercalls. The BUG_ON() is here to prevent catch logical error, so
crashing Xen is a bit over the top.
While all the holes should now be fixed, it would be better to downgrade
the BUG_ON() to something less fatal to prevent any more DoS.
The BUG_ON() in p2m_get_entry() is now replaced by ASSERT_UNREACHABLE()
to catch mistake in debug build and return INVALID_MFN for production
build. The interface also requires to set page_order to give an idea of
the size of "hole". So 'level' is now set so we report a hole of size of
the an entry of the root page-table. This stays inline with what happen
when the GFN is higher than p2m->max_mapped_gfn.
This is part of XSA-301.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
---
xen/arch/arm/p2m.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/xen/arch/arm/p2m.c b/xen/arch/arm/p2m.c
index c7e049901d..af3515df42 100644
--- a/xen/arch/arm/p2m.c
+++ b/xen/arch/arm/p2m.c
@@ -318,7 +318,12 @@ mfn_t p2m_get_entry(struct p2m_domain *p2m, gfn_t gfn,
* the table should always be non-NULL because the gfn is below
* p2m->max_mapped_gfn and the root table pages are always present.
*/
- BUG_ON(table == NULL);
+ if ( !table )
+ {
+ ASSERT_UNREACHABLE();
+ level = P2M_ROOT_LEVEL;
+ goto out;
+ }
for ( level = P2M_ROOT_LEVEL; level < 3; level++ )
{
--
2.11.0

View file

@ -0,0 +1,37 @@
From bbca29f88d9ad9c7e91125a3b5d5f13a23e5801f Mon Sep 17 00:00:00 2001
From: Jan Beulich <jbeulich@suse.com>
Date: Wed, 2 Oct 2019 13:36:59 +0200
Subject: [PATCH 1/2] IOMMU: add missing HVM check
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Fix an unguarded d->arch.hvm access in assign_device().
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>
(cherry picked from commit 41fd1009cd7416b73d745a77c24b4e8d1a296fe6)
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
xen/drivers/passthrough/pci.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index f51cae7f4e..037aba7c94 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1416,7 +1416,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
/* Prevent device assign if mem paging or mem sharing have been
* enabled for this domain */
if ( unlikely(!need_iommu(d) &&
- (d->arch.hvm_domain.mem_sharing_enabled ||
+ ((is_hvm_domain(d) &&
+ d->arch.hvm_domain.mem_sharing_enabled) ||
vm_event_check_ring(d->vm_event_paging) ||
p2m_get_hostp2m(d)->global_logdirty)) )
return -EXDEV;
--
2.11.0

View file

@ -0,0 +1,498 @@
From ec99857f59f7f06236f11ca8b0b2303e5e745cc4 Mon Sep 17 00:00:00 2001
From: Paul Durrant <paul.durrant@citrix.com>
Date: Mon, 14 Oct 2019 17:52:59 +0100
Subject: [PATCH 2/2] passthrough: quarantine PCI devices
When a PCI device is assigned to an untrusted domain, it is possible for
that domain to program the device to DMA to an arbitrary address. The
IOMMU is used to protect the host from malicious DMA by making sure that
the device addresses can only target memory assigned to the guest. However,
when the guest domain is torn down the device is assigned back to dom0,
thus allowing any in-flight DMA to potentially target critical host data.
This patch introduces a 'quarantine' for PCI devices using dom_io. When
the toolstack makes a device assignable (by binding it to pciback), it
will now also assign it to DOMID_IO and the device will only be assigned
back to dom0 when the device is made unassignable again. Whilst device is
assignable it will only ever transfer between dom_io and guest domains.
dom_io is actually only used as a sentinel domain for quarantining purposes;
it is not configured with any IOMMU mappings. Assignment to dom_io simply
means that the device's initiator (requestor) identifier is not present in
the IOMMU's device table and thus any DMA transactions issued will be
terminated with a fault condition.
In addition, a fix to assignment handling is made for VT-d. Failure
during the assignment step should not lead to a device still being
associated with its prior owner. Hand the device to DomIO temporarily,
until the assignment step has completed successfully. Remove the PI
hooks from the source domain then earlier as well.
Failure of the recovery reassign_device_ownership() may not go silent:
There e.g. may still be left over RMRR mappings in the domain assignment
to which has failed, and hence we can't allow that domain to continue
executing.
NOTE: This patch also includes one printk() cleanup; the
"XEN_DOMCTL_assign_device: " tag is dropped in iommu_do_pci_domctl(),
since similar printk()-s elsewhere also don't log such a tag.
This is XSA-302.
Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Ian Jackson <ian.jackson@eu.citrix.com>
---
tools/libxl/libxl_pci.c | 25 +++++++++++-
xen/arch/x86/mm.c | 2 +
xen/common/domctl.c | 14 ++++++-
xen/drivers/passthrough/amd/pci_amd_iommu.c | 10 ++++-
xen/drivers/passthrough/iommu.c | 9 +++++
xen/drivers/passthrough/pci.c | 59 ++++++++++++++++++++++-------
xen/drivers/passthrough/vtd/iommu.c | 40 ++++++++++++++++---
xen/include/xen/pci.h | 3 ++
8 files changed, 138 insertions(+), 24 deletions(-)
diff --git a/tools/libxl/libxl_pci.c b/tools/libxl/libxl_pci.c
index 4755a0c93c..81890a91ac 100644
--- a/tools/libxl/libxl_pci.c
+++ b/tools/libxl/libxl_pci.c
@@ -754,6 +754,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
libxl_device_pci *pcidev,
int rebind)
{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
unsigned dom, bus, dev, func;
char *spath, *driver_path = NULL;
int rc;
@@ -779,7 +780,7 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
}
if ( rc ) {
LOG(WARN, PCI_BDF" already assigned to pciback", dom, bus, dev, func);
- return 0;
+ goto quarantine;
}
/* Check to see if there's already a driver that we need to unbind from */
@@ -810,6 +811,19 @@ static int libxl__device_pci_assignable_add(libxl__gc *gc,
return ERROR_FAIL;
}
+quarantine:
+ /*
+ * DOMID_IO is just a sentinel domain, without any actual mappings,
+ * so always pass XEN_DOMCTL_DEV_RDM_RELAXED to avoid assignment being
+ * unnecessarily denied.
+ */
+ rc = xc_assign_device(ctx->xch, DOMID_IO, pcidev_encode_bdf(pcidev),
+ XEN_DOMCTL_DEV_RDM_RELAXED);
+ if ( rc < 0 ) {
+ LOG(ERROR, "failed to quarantine "PCI_BDF, dom, bus, dev, func);
+ return ERROR_FAIL;
+ }
+
return 0;
}
@@ -817,9 +831,18 @@ static int libxl__device_pci_assignable_remove(libxl__gc *gc,
libxl_device_pci *pcidev,
int rebind)
{
+ libxl_ctx *ctx = libxl__gc_owner(gc);
int rc;
char *driver_path;
+ /* De-quarantine */
+ rc = xc_deassign_device(ctx->xch, DOMID_IO, pcidev_encode_bdf(pcidev));
+ if ( rc < 0 ) {
+ LOG(ERROR, "failed to de-quarantine "PCI_BDF, pcidev->domain, pcidev->bus,
+ pcidev->dev, pcidev->func);
+ return ERROR_FAIL;
+ }
+
/* Unbind from pciback */
if ( (rc=pciback_dev_is_assigned(gc, pcidev)) < 0 ) {
return ERROR_FAIL;
diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c
index e6a4cb28f8..c1ab57f9a5 100644
--- a/xen/arch/x86/mm.c
+++ b/xen/arch/x86/mm.c
@@ -295,9 +295,11 @@ void __init arch_init_memory(void)
* Initialise our DOMID_IO domain.
* This domain owns I/O pages that are within the range of the page_info
* array. Mappings occur at the priv of the caller.
+ * Quarantined PCI devices will be associated with this domain.
*/
dom_io = domain_create(DOMID_IO, NULL);
BUG_ON(IS_ERR(dom_io));
+ INIT_LIST_HEAD(&dom_io->arch.pdev_list);
/*
* Initialise our COW domain.
diff --git a/xen/common/domctl.c b/xen/common/domctl.c
index 9b7bc083ee..741d774cd1 100644
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -392,6 +392,16 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
switch ( op->cmd )
{
+ case XEN_DOMCTL_assign_device:
+ case XEN_DOMCTL_deassign_device:
+ if ( op->domain == DOMID_IO )
+ {
+ d = dom_io;
+ break;
+ }
+ else if ( op->domain == DOMID_INVALID )
+ return -ESRCH;
+ /* fall through */
case XEN_DOMCTL_test_assign_device:
if ( op->domain == DOMID_INVALID )
{
@@ -413,7 +423,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
if ( !domctl_lock_acquire() )
{
- if ( d )
+ if ( d && d != dom_io )
rcu_unlock_domain(d);
return hypercall_create_continuation(
__HYPERVISOR_domctl, "h", u_domctl);
@@ -1148,7 +1158,7 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xen_domctl_t) u_domctl)
domctl_lock_release();
domctl_out_unlock_domonly:
- if ( d )
+ if ( d && d != dom_io )
rcu_unlock_domain(d);
if ( copyback && __copy_to_guest(u_domctl, op, 1) )
diff --git a/xen/drivers/passthrough/amd/pci_amd_iommu.c b/xen/drivers/passthrough/amd/pci_amd_iommu.c
index 12d2695b89..ec8baae717 100644
--- a/xen/drivers/passthrough/amd/pci_amd_iommu.c
+++ b/xen/drivers/passthrough/amd/pci_amd_iommu.c
@@ -118,6 +118,10 @@ static void amd_iommu_setup_domain_device(
u8 bus = pdev->bus;
const struct domain_iommu *hd = dom_iommu(domain);
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( domain == dom_io )
+ return;
+
BUG_ON( !hd->arch.root_table || !hd->arch.paging_mode ||
!iommu->dev_table.buffer );
@@ -305,6 +309,10 @@ void amd_iommu_disable_domain_device(struct domain *domain,
int req_id;
u8 bus = pdev->bus;
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( domain == dom_io )
+ return;
+
BUG_ON ( iommu->dev_table.buffer == NULL );
req_id = get_dma_requestor_id(iommu->seg, PCI_BDF2(bus, devfn));
dte = iommu->dev_table.buffer + (req_id * IOMMU_DEV_TABLE_ENTRY_SIZE);
@@ -391,7 +399,7 @@ static int amd_iommu_assign_device(struct domain *d, u8 devfn,
ivrs_mappings[req_id].read_permission);
}
- return reassign_device(hardware_domain, d, devfn, pdev);
+ return reassign_device(pdev->domain, d, devfn, pdev);
}
static void deallocate_next_page_table(struct page_info *pg, int level)
diff --git a/xen/drivers/passthrough/iommu.c b/xen/drivers/passthrough/iommu.c
index 04b0be37d3..8027d96f1c 100644
--- a/xen/drivers/passthrough/iommu.c
+++ b/xen/drivers/passthrough/iommu.c
@@ -219,6 +219,9 @@ void iommu_teardown(struct domain *d)
{
const struct domain_iommu *hd = dom_iommu(d);
+ if ( d == dom_io )
+ return;
+
d->need_iommu = 0;
hd->platform_ops->teardown(d);
tasklet_schedule(&iommu_pt_cleanup_tasklet);
@@ -229,6 +232,9 @@ int iommu_construct(struct domain *d)
if ( need_iommu(d) > 0 )
return 0;
+ if ( d == dom_io )
+ return 0;
+
if ( !iommu_use_hap_pt(d) )
{
int rc;
@@ -404,6 +410,9 @@ int __init iommu_setup(void)
printk("I/O virtualisation %sabled\n", iommu_enabled ? "en" : "dis");
if ( iommu_enabled )
{
+ if ( iommu_domain_init(dom_io) )
+ panic("Could not set up quarantine\n");
+
printk(" - Dom0 mode: %s\n",
iommu_passthrough ? "Passthrough" :
iommu_dom0_strict ? "Strict" : "Relaxed");
diff --git a/xen/drivers/passthrough/pci.c b/xen/drivers/passthrough/pci.c
index 037aba7c94..fb010a547b 100644
--- a/xen/drivers/passthrough/pci.c
+++ b/xen/drivers/passthrough/pci.c
@@ -1389,19 +1389,29 @@ static int iommu_remove_device(struct pci_dev *pdev)
return hd->platform_ops->remove_device(pdev->devfn, pci_to_dev(pdev));
}
-/*
- * If the device isn't owned by the hardware domain, it means it already
- * has been assigned to other domain, or it doesn't exist.
- */
static int device_assigned(u16 seg, u8 bus, u8 devfn)
{
struct pci_dev *pdev;
+ int rc = 0;
pcidevs_lock();
- pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+
+ pdev = pci_get_pdev(seg, bus, devfn);
+
+ if ( !pdev )
+ rc = -ENODEV;
+ /*
+ * If the device exists and it is not owned by either the hardware
+ * domain or dom_io then it must be assigned to a guest, or be
+ * hidden (owned by dom_xen).
+ */
+ else if ( pdev->domain != hardware_domain &&
+ pdev->domain != dom_io )
+ rc = -EBUSY;
+
pcidevs_unlock();
- return pdev ? 0 : -EBUSY;
+ return rc;
}
static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
@@ -1415,7 +1425,8 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
/* Prevent device assign if mem paging or mem sharing have been
* enabled for this domain */
- if ( unlikely(!need_iommu(d) &&
+ if ( d != dom_io &&
+ unlikely(!need_iommu(d) &&
((is_hvm_domain(d) &&
d->arch.hvm_domain.mem_sharing_enabled) ||
vm_event_check_ring(d->vm_event_paging) ||
@@ -1432,12 +1443,20 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
return rc;
}
- pdev = pci_get_pdev_by_domain(hardware_domain, seg, bus, devfn);
+ pdev = pci_get_pdev(seg, bus, devfn);
+
+ rc = -ENODEV;
if ( !pdev )
- {
- rc = pci_get_pdev(seg, bus, devfn) ? -EBUSY : -ENODEV;
goto done;
- }
+
+ rc = 0;
+ if ( d == pdev->domain )
+ goto done;
+
+ rc = -EBUSY;
+ if ( pdev->domain != hardware_domain &&
+ pdev->domain != dom_io )
+ goto done;
if ( pdev->msix )
msixtbl_init(d);
@@ -1460,6 +1479,10 @@ static int assign_device(struct domain *d, u16 seg, u8 bus, u8 devfn, u32 flag)
}
done:
+ /* The device is assigned to dom_io so mark it as quarantined */
+ if ( !rc && d == dom_io )
+ pdev->quarantine = true;
+
if ( !has_arch_pdevs(d) && need_iommu(d) )
iommu_teardown(d);
pcidevs_unlock();
@@ -1472,6 +1495,7 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
{
const struct domain_iommu *hd = dom_iommu(d);
struct pci_dev *pdev = NULL;
+ struct domain *target;
int ret = 0;
if ( !iommu_enabled || !hd->platform_ops )
@@ -1482,12 +1506,16 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
if ( !pdev )
return -ENODEV;
+ /* De-assignment from dom_io should de-quarantine the device */
+ target = (pdev->quarantine && pdev->domain != dom_io) ?
+ dom_io : hardware_domain;
+
while ( pdev->phantom_stride )
{
devfn += pdev->phantom_stride;
if ( PCI_SLOT(devfn) != PCI_SLOT(pdev->devfn) )
break;
- ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+ ret = hd->platform_ops->reassign_device(d, target, devfn,
pci_to_dev(pdev));
if ( !ret )
continue;
@@ -1498,7 +1526,7 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
}
devfn = pdev->devfn;
- ret = hd->platform_ops->reassign_device(d, hardware_domain, devfn,
+ ret = hd->platform_ops->reassign_device(d, target, devfn,
pci_to_dev(pdev));
if ( ret )
{
@@ -1508,6 +1536,9 @@ int deassign_device(struct domain *d, u16 seg, u8 bus, u8 devfn)
return ret;
}
+ if ( pdev->domain == hardware_domain )
+ pdev->quarantine = false;
+
pdev->fault.count = 0;
if ( !has_arch_pdevs(d) && need_iommu(d) )
@@ -1686,7 +1717,7 @@ int iommu_do_pci_domctl(
ret = hypercall_create_continuation(__HYPERVISOR_domctl,
"h", u_domctl);
else if ( ret )
- printk(XENLOG_G_ERR "XEN_DOMCTL_assign_device: "
+ printk(XENLOG_G_ERR
"assign %04x:%02x:%02x.%u to dom%d failed (%d)\n",
seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn),
d->domain_id, ret);
diff --git a/xen/drivers/passthrough/vtd/iommu.c b/xen/drivers/passthrough/vtd/iommu.c
index 4c719d4ee7..19f7d13013 100644
--- a/xen/drivers/passthrough/vtd/iommu.c
+++ b/xen/drivers/passthrough/vtd/iommu.c
@@ -1338,6 +1338,10 @@ int domain_context_mapping_one(
int agaw, rc, ret;
bool_t flush_dev_iotlb;
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( domain == dom_io )
+ return 0;
+
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
maddr = bus_to_context_maddr(iommu, bus);
@@ -1573,6 +1577,10 @@ int domain_context_unmap_one(
int iommu_domid, rc, ret;
bool_t flush_dev_iotlb;
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( domain == dom_io )
+ return 0;
+
ASSERT(pcidevs_locked());
spin_lock(&iommu->lock);
@@ -1705,6 +1713,10 @@ static int domain_context_unmap(struct domain *domain, u8 devfn,
goto out;
}
+ /* dom_io is used as a sentinel for quarantined devices */
+ if ( domain == dom_io )
+ goto out;
+
/*
* if no other devices under the same iommu owned by this domain,
* clear iommu in iommu_bitmap and clear domain_id in domid_bitmp
@@ -2389,6 +2401,15 @@ static int reassign_device_ownership(
if ( ret )
return ret;
+ if ( devfn == pdev->devfn )
+ {
+ list_move(&pdev->domain_list, &dom_io->arch.pdev_list);
+ pdev->domain = dom_io;
+ }
+
+ if ( !has_arch_pdevs(source) )
+ vmx_pi_hooks_deassign(source);
+
if ( !has_arch_pdevs(target) )
vmx_pi_hooks_assign(target);
@@ -2407,15 +2428,13 @@ static int reassign_device_ownership(
pdev->domain = target;
}
- if ( !has_arch_pdevs(source) )
- vmx_pi_hooks_deassign(source);
-
return ret;
}
static int intel_iommu_assign_device(
struct domain *d, u8 devfn, struct pci_dev *pdev, u32 flag)
{
+ struct domain *s = pdev->domain;
struct acpi_rmrr_unit *rmrr;
int ret = 0, i;
u16 bdf, seg;
@@ -2458,8 +2477,8 @@ static int intel_iommu_assign_device(
}
}
- ret = reassign_device_ownership(hardware_domain, d, devfn, pdev);
- if ( ret )
+ ret = reassign_device_ownership(s, d, devfn, pdev);
+ if ( ret || d == dom_io )
return ret;
/* Setup rmrr identity mapping */
@@ -2472,11 +2491,20 @@ static int intel_iommu_assign_device(
ret = rmrr_identity_mapping(d, 1, rmrr, flag);
if ( ret )
{
- reassign_device_ownership(d, hardware_domain, devfn, pdev);
+ int rc;
+
+ rc = reassign_device_ownership(d, s, devfn, pdev);
printk(XENLOG_G_ERR VTDPREFIX
" cannot map reserved region (%"PRIx64",%"PRIx64"] for Dom%d (%d)\n",
rmrr->base_address, rmrr->end_address,
d->domain_id, ret);
+ if ( rc )
+ {
+ printk(XENLOG_ERR VTDPREFIX
+ " failed to reclaim %04x:%02x:%02x.%u from %pd (%d)\n",
+ seg, bus, PCI_SLOT(devfn), PCI_FUNC(devfn), d, rc);
+ domain_crash(d);
+ }
break;
}
}
diff --git a/xen/include/xen/pci.h b/xen/include/xen/pci.h
index 4cfa774615..066364bdef 100644
--- a/xen/include/xen/pci.h
+++ b/xen/include/xen/pci.h
@@ -88,6 +88,9 @@ struct pci_dev {
nodeid_t node; /* NUMA node */
+ /* Device to be quarantined, don't automatically re-assign to dom0 */
+ bool quarantine;
+
enum pdev_type {
DEV_TYPE_PCI_UNKNOWN,
DEV_TYPE_PCIe_ENDPOINT,
--
2.11.0

View file

@ -0,0 +1,74 @@
From c8cb33fa64c9ccbfa2a494a9dad2e0a763c09176 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:07:53 +0100
Subject: [PATCH 1/4] xen/arm32: entry: Split __DEFINE_ENTRY_TRAP in two
The preprocessing macro __DEFINE_ENTRY_TRAP is used to generate trap
entry function. While the macro is fairly small today, follow-up patches
will increase the size signicantly.
In general, assembly macros are more readable as they allow you to name
parameters and avoid '\'. So the actual implementation of the trap is
now switched to an assembly macro.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/arm32/entry.S | 34 +++++++++++++++++++---------------
1 file changed, 19 insertions(+), 15 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 0b4cd19abd..4a762e04f1 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -126,24 +126,28 @@ abort_guest_exit_end:
skip_check:
mov pc, lr
-/*
- * Macro to define trap entry. The iflags corresponds to the list of
- * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
- */
+ /*
+ * Macro to define trap entry. The iflags corresponds to the list of
+ * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
+ */
+ .macro vector trap, iflags
+ SAVE_ALL
+ cpsie \iflags
+ adr lr, return_from_trap
+ mov r0, sp
+ /*
+ * Save the stack pointer in r11. It will be restored after the
+ * trap has been handled (see return_from_trap).
+ */
+ mov r11, sp
+ bic sp, #7 /* Align the stack pointer (noop on guest trap) */
+ b do_trap_\trap
+ .endm
+
#define __DEFINE_TRAP_ENTRY(trap, iflags) \
ALIGN; \
trap_##trap: \
- SAVE_ALL; \
- cpsie iflags; \
- adr lr, return_from_trap; \
- mov r0, sp; \
- /* \
- * Save the stack pointer in r11. It will be restored after the \
- * trap has been handled (see return_from_trap). \
- */ \
- mov r11, sp; \
- bic sp, #7; /* Align the stack pointer (noop on guest trap) */ \
- b do_trap_##trap
+ vector trap, iflags
/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
--
2.11.0

View file

@ -0,0 +1,97 @@
From be7379207c83fa74f8a6c22a8ea213f02714776f Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Tue, 1 Oct 2019 13:15:48 +0100
Subject: [PATCH 2/4] xen/arm32: entry: Fold the macro SAVE_ALL in the macro
vector
Follow-up rework will require the macro vector to distinguish between
a trap from a guest vs while in the hypervisor.
The macro SAVE_ALL already has code to distinguish between the two and
it is only called by the vector macro. So fold the former into the
latter. This will help to avoid duplicating the check.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/arm32/entry.S | 46 +++++++++++++++++++++++-----------------------
1 file changed, 23 insertions(+), 23 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 4a762e04f1..150cbc0b4b 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -13,27 +13,6 @@
#define RESTORE_BANKED(mode) \
RESTORE_ONE_BANKED(SP_##mode) ; RESTORE_ONE_BANKED(LR_##mode) ; RESTORE_ONE_BANKED(SPSR_##mode)
-#define SAVE_ALL \
- sub sp, #(UREGS_SP_usr - UREGS_sp); /* SP, LR, SPSR, PC */ \
- push {r0-r12}; /* Save R0-R12 */ \
- \
- mrs r11, ELR_hyp; /* ELR_hyp is return address. */\
- str r11, [sp, #UREGS_pc]; \
- \
- str lr, [sp, #UREGS_lr]; \
- \
- add r11, sp, #UREGS_kernel_sizeof+4; \
- str r11, [sp, #UREGS_sp]; \
- \
- mrc CP32(r11, HSR); /* Save exception syndrome */ \
- str r11, [sp, #UREGS_hsr]; \
- \
- mrs r11, SPSR_hyp; \
- str r11, [sp, #UREGS_cpsr]; \
- and r11, #PSR_MODE_MASK; \
- cmp r11, #PSR_MODE_HYP; \
- blne save_guest_regs
-
save_guest_regs:
#ifdef CONFIG_ARM32_HARDEN_BRANCH_PREDICTOR
/*
@@ -52,7 +31,7 @@ save_guest_regs:
ldr r11, =0xffffffff /* Clobber SP which is only valid for hypervisor frames. */
str r11, [sp, #UREGS_sp]
SAVE_ONE_BANKED(SP_usr)
- /* LR_usr is the same physical register as lr and is saved in SAVE_ALL */
+ /* LR_usr is the same physical register as lr and is saved by the caller */
SAVE_BANKED(svc)
SAVE_BANKED(abt)
SAVE_BANKED(und)
@@ -131,7 +110,28 @@ skip_check:
* interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
*/
.macro vector trap, iflags
- SAVE_ALL
+ /* Save registers in the stack */
+ sub sp, #(UREGS_SP_usr - UREGS_sp) /* SP, LR, SPSR, PC */
+ push {r0-r12} /* Save R0-R12 */
+ mrs r11, ELR_hyp /* ELR_hyp is return address */
+ str r11, [sp, #UREGS_pc]
+
+ str lr, [sp, #UREGS_lr]
+
+ add r11, sp, #(UREGS_kernel_sizeof + 4)
+
+ str r11, [sp, #UREGS_sp]
+
+ mrc CP32(r11, HSR) /* Save exception syndrome */
+ str r11, [sp, #UREGS_hsr]
+
+ mrs r11, SPSR_hyp
+ str r11, [sp, #UREGS_cpsr]
+ and r11, #PSR_MODE_MASK
+ cmp r11, #PSR_MODE_HYP
+ blne save_guest_regs
+
+ /* We are ready to handle the trap, setup the registers and jump. */
cpsie \iflags
adr lr, return_from_trap
mov r0, sp
--
2.11.0

View file

@ -0,0 +1,226 @@
From 098fe877967870ffda2dfd9629a5fd272f6aacdc Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Fri, 11 Oct 2019 17:49:28 +0100
Subject: [PATCH 3/4] xen/arm32: Don't blindly unmask interrupts on trap
without a change of level
Exception vectors will unmask interrupts regardless the state of them in
the interrupted context.
One of the consequences is IRQ will be unmasked when receiving an
undefined instruction exception (used by WARN*) from the hypervisor.
This could result to unexpected behavior such as deadlock (if a lock was
shared with interrupts).
In a nutshell, interrupts should only be unmasked when it is safe to do.
Xen only unmask IRQ and Abort interrupts, so the logic can stay simple.
As vectors exceptions may be shared between guest and hypervisor, we now
need to have a different policy for the interrupts.
On exception from hypervisor, each vector will select the list of
interrupts to inherit from the interrupted context. Any interrupts not
listed will be kept masked.
On exception from the guest, the Abort and IRQ will be unmasked
depending on the exact vector.
The interrupts will be kept unmasked when the vector cannot used by
either guest or hypervisor.
Note that each vector is not anymore preceded by ALIGN. This is fine
because the alignment is already bigger than what we need.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/arm32/entry.S | 138 +++++++++++++++++++++++++++++++++++----------
1 file changed, 109 insertions(+), 29 deletions(-)
diff --git a/xen/arch/arm/arm32/entry.S b/xen/arch/arm/arm32/entry.S
index 150cbc0b4b..ec90cca093 100644
--- a/xen/arch/arm/arm32/entry.S
+++ b/xen/arch/arm/arm32/entry.S
@@ -4,6 +4,17 @@
#include <asm/alternative.h>
#include <public/xen.h>
+/*
+ * Short-hands to defined the interrupts (A, I, F)
+ *
+ * _ means the interrupt state will not change
+ * X means the state of interrupt X will change
+ *
+ * To be used with msr cpsr_* only
+ */
+#define IFLAGS_AIF PSR_ABT_MASK | PSR_IRQ_MASK | PSR_FIQ_MASK
+#define IFLAGS_A_F PSR_ABT_MASK | PSR_FIQ_MASK
+
#define SAVE_ONE_BANKED(reg) mrs r11, reg; str r11, [sp, #UREGS_##reg]
#define RESTORE_ONE_BANKED(reg) ldr r11, [sp, #UREGS_##reg]; msr reg, r11
@@ -106,10 +117,18 @@ skip_check:
mov pc, lr
/*
- * Macro to define trap entry. The iflags corresponds to the list of
- * interrupts (Asynchronous Abort, IRQ, FIQ) to unmask.
+ * Macro to define a trap entry.
+ *
+ * @guest_iflags: Optional list of interrupts to unmask when
+ * entering from guest context. As this is used with cpsie,
+ * the letter (a, i, f) should be used.
+ *
+ * @hyp_iflags: Optional list of interrupts to inherit when
+ * entering from hypervisor context. Any interrupts not
+ * listed will be kept unchanged. As this is used with cpsr_*,
+ * IFLAGS_* short-hands should be used.
*/
- .macro vector trap, iflags
+ .macro vector trap, guest_iflags=n, hyp_iflags=0
/* Save registers in the stack */
sub sp, #(UREGS_SP_usr - UREGS_sp) /* SP, LR, SPSR, PC */
push {r0-r12} /* Save R0-R12 */
@@ -127,12 +146,39 @@ skip_check:
mrs r11, SPSR_hyp
str r11, [sp, #UREGS_cpsr]
- and r11, #PSR_MODE_MASK
- cmp r11, #PSR_MODE_HYP
- blne save_guest_regs
+ /*
+ * We need to distinguish whether we came from guest or
+ * hypervisor context.
+ */
+ and r0, r11, #PSR_MODE_MASK
+ cmp r0, #PSR_MODE_HYP
+
+ bne 1f
+ /*
+ * Trap from the hypervisor
+ *
+ * Inherit the state of the interrupts from the hypervisor
+ * context. For that we need to use SPSR (stored in r11) and
+ * modify CPSR accordingly.
+ *
+ * CPSR = (CPSR & ~hyp_iflags) | (SPSR & hyp_iflags)
+ */
+ mrs r10, cpsr
+ bic r10, r10, #\hyp_iflags
+ and r11, r11, #\hyp_iflags
+ orr r10, r10, r11
+ msr cpsr_cx, r10
+ b 2f
+
+1:
+ /* Trap from the guest */
+ bl save_guest_regs
+ .if \guest_iflags != n
+ cpsie \guest_iflags
+ .endif
+2:
/* We are ready to handle the trap, setup the registers and jump. */
- cpsie \iflags
adr lr, return_from_trap
mov r0, sp
/*
@@ -144,20 +190,6 @@ skip_check:
b do_trap_\trap
.endm
-#define __DEFINE_TRAP_ENTRY(trap, iflags) \
- ALIGN; \
-trap_##trap: \
- vector trap, iflags
-
-/* Trap handler which unmask IRQ/Abort, keep FIQ masked */
-#define DEFINE_TRAP_ENTRY(trap) __DEFINE_TRAP_ENTRY(trap, ai)
-
-/* Trap handler which unmask Abort, keep IRQ/FIQ masked */
-#define DEFINE_TRAP_ENTRY_NOIRQ(trap) __DEFINE_TRAP_ENTRY(trap, a)
-
-/* Trap handler which unmask IRQ, keep Abort/FIQ masked */
-#define DEFINE_TRAP_ENTRY_NOABORT(trap) __DEFINE_TRAP_ENTRY(trap, i)
-
.align 5
GLOBAL(hyp_traps_vector)
b trap_reset /* 0x00 - Reset */
@@ -228,14 +260,62 @@ decode_vectors:
#endif /* CONFIG_HARDEN_BRANCH_PREDICTOR */
-DEFINE_TRAP_ENTRY(reset)
-DEFINE_TRAP_ENTRY(undefined_instruction)
-DEFINE_TRAP_ENTRY(hypervisor_call)
-DEFINE_TRAP_ENTRY(prefetch_abort)
-DEFINE_TRAP_ENTRY(guest_sync)
-DEFINE_TRAP_ENTRY_NOIRQ(irq)
-DEFINE_TRAP_ENTRY_NOIRQ(fiq)
-DEFINE_TRAP_ENTRY_NOABORT(data_abort)
+/* Vector not used by the Hypervisor. */
+trap_reset:
+ vector reset
+
+/*
+ * Vector only used by the Hypervisor.
+ *
+ * While the exception can be executed with all the interrupts (e.g.
+ * IRQ) unmasked, the interrupted context may have purposefully masked
+ * some of them. So we want to inherit the state from the interrupted
+ * context.
+ */
+trap_undefined_instruction:
+ vector undefined_instruction, hyp_iflags=IFLAGS_AIF
+
+/* We should never reach this trap */
+trap_hypervisor_call:
+ vector hypervisor_call
+
+/*
+ * Vector only used by the hypervisor.
+ *
+ * While the exception can be executed with all the interrupts (e.g.
+ * IRQ) unmasked, the interrupted context may have purposefully masked
+ * some of them. So we want to inherit the state from the interrupted
+ * context.
+ */
+trap_prefetch_abort:
+ vector prefetch_abort, hyp_iflags=IFLAGS_AIF
+
+/*
+ * Vector only used by the hypervisor.
+ *
+ * Data Abort should be rare and most likely fatal. It is best to not
+ * unmask any interrupts to limit the amount of code that can run before
+ * the Data Abort is treated.
+ */
+trap_data_abort:
+ vector data_abort
+
+/* Vector only used by the guest. We can unmask Abort/IRQ. */
+trap_guest_sync:
+ vector guest_sync, guest_iflags=ai
+
+
+/* Vector used by the hypervisor and the guest. */
+trap_irq:
+ vector irq, guest_iflags=a, hyp_iflags=IFLAGS_A_F
+
+/*
+ * Vector used by the hypervisor and the guest.
+ *
+ * FIQ are not meant to happen, so we don't unmask any interrupts.
+ */
+trap_fiq:
+ vector fiq
return_from_trap:
/*
--
2.11.0

View file

@ -0,0 +1,114 @@
From c6d290ce157a044dec417fdda8db71e41a37d744 Mon Sep 17 00:00:00 2001
From: Julien Grall <julien.grall@arm.com>
Date: Mon, 7 Oct 2019 18:10:56 +0100
Subject: [PATCH 4/4] xen/arm64: Don't blindly unmask interrupts on trap
without a change of level
Some of the traps without a change of the level (i.e. hypervisor ->
hypervisor) will unmask interrupts regardless the state of them in the
interrupted context.
One of the consequences is IRQ will be unmasked when receiving a
synchronous exception (used by WARN*()). This could result to unexpected
behavior such as deadlock (if a lock was shared with interrupts).
In a nutshell, interrupts should only be unmasked when it is safe to
do. Xen only unmask IRQ and Abort interrupts, so the logic can stay
simple:
- hyp_error: All the interrupts are now kept masked. SError should
be pretty rare and if ever happen then we most likely want to
avoid any other interrupts to be generated. The potential main
"caller" is during virtual SError synchronization on the exit
path from the guest (see check_pending_vserror).
- hyp_sync: The interrupts state is inherited from the interrupted
context.
- hyp_irq: All the interrupts but IRQ state are inherited from the
interrupted context. IRQ is kept masked.
This is part of XSA-303.
Reported-by: Julien Grall <Julien.Grall@arm.com>
Signed-off-by: Julien Grall <julien.grall@arm.com>
Reviewed-by: Stefano Stabellini <sstabellini@kernel.org>
Reviewed-by: Andre Przywara <andre.przywara@arm.com>
---
xen/arch/arm/arm64/entry.S | 47 ++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 43 insertions(+), 4 deletions(-)
diff --git a/xen/arch/arm/arm64/entry.S b/xen/arch/arm/arm64/entry.S
index 2d9a2713a1..3e41ba65b6 100644
--- a/xen/arch/arm/arm64/entry.S
+++ b/xen/arch/arm/arm64/entry.S
@@ -188,24 +188,63 @@ hyp_error_invalid:
entry hyp=1
invalid BAD_ERROR
+/*
+ * SError received while running in the hypervisor mode.
+ *
+ * Technically, we could unmask the IRQ if it were unmasked in the
+ * interrupted context. However, this require to check the PSTATE. For
+ * simplicity, as SError should be rare and potentially fatal,
+ * all interrupts are kept masked.
+ */
hyp_error:
entry hyp=1
- msr daifclr, #2
mov x0, sp
bl do_trap_hyp_serror
exit hyp=1
-/* Traps taken in Current EL with SP_ELx */
+/*
+ * Synchronous exception received while running in the hypervisor mode.
+ *
+ * While the exception could be executed with all the interrupts (e.g.
+ * IRQ) unmasked, the interrupted context may have purposefully masked
+ * some of them. So we want to inherit the state from the interrupted
+ * context.
+ */
hyp_sync:
entry hyp=1
- msr daifclr, #6
+
+ /* Inherit interrupts */
+ mrs x0, SPSR_el2
+ and x0, x0, #(PSR_DBG_MASK | PSR_ABT_MASK | PSR_IRQ_MASK | PSR_FIQ_MASK)
+ msr daif, x0
+
mov x0, sp
bl do_trap_hyp_sync
exit hyp=1
+/*
+ * IRQ received while running in the hypervisor mode.
+ *
+ * While the exception could be executed with all the interrupts but IRQ
+ * unmasked, the interrupted context may have purposefully masked some
+ * of them. So we want to inherit the state from the interrupt context
+ * and keep IRQ masked.
+ *
+ * XXX: We may want to consider an ordering between interrupts (e.g. if
+ * SError are masked, then IRQ should be masked too). However, this
+ * would require some rework in some paths (e.g. panic, livepatch) to
+ * ensure the ordering is enforced everywhere.
+ */
hyp_irq:
entry hyp=1
- msr daifclr, #4
+
+ /* Inherit D, A, F interrupts and keep I masked */
+ mrs x0, SPSR_el2
+ mov x1, #(PSR_DBG_MASK | PSR_ABT_MASK | PSR_FIQ_MASK)
+ and x0, x0, x1
+ orr x0, x0, #PSR_IRQ_MASK
+ msr daif, x0
+
mov x0, sp
bl do_trap_irq
exit hyp=1
--
2.11.0