Page MenuHomeFreeBSD
Differential D16416 Diff 46059 head/emulators/xen-kernel47/files/0002-p2m-Check-return-value-of-p2m_set_entry-when-decreas.patch

Changeset View

Standalone View

View Options

head/emulators/xen-kernel47/files/0002-p2m-Check-return-value-of-p2m_set_entry-when-decreas.patch

PropertyOld ValueNew Value
fbsd:nokeywordsnullyes
\ No newline at end of property
svn:eol-stylenullnative
\ No newline at end of property
svn:mime-typenulltext/plain
\ No newline at end of property
From 01feeda5363dd8d2fea8395c2c435203751c8ba5 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 10 Nov 2017 16:53:55 +0000
Subject: [PATCH 2/2] p2m: Check return value of p2m_set_entry() when
decreasing reservation
If the entire range specified to p2m_pod_decrease_reservation() is marked
populate-on-demand, then it will make a single p2m_set_entry() call,
reducing its PoD entry count.
Unfortunately, in the right circumstances, this p2m_set_entry() call
may fail. It that case, repeated calls to decrease_reservation() may
cause p2m->pod.entry_count to fall below zero, potentially tripping
over BUG_ON()s to the contrary.
Instead, check to see if the entry succeeded, and return false if not.
The caller will then call guest_remove_page() on the gfns, which will
return -EINVAL upon finding no valid memory there to return.
Unfortunately if the order > 0, the entry may have partially changed.
A domain_crash() is probably the safest thing in that case.
Other p2m_set_entry() calls in the same function should be fine,
because they are writing the entry at its current order. Nonetheless,
check the return value and crash if our assumption turns otu to be
wrong.
This is part of XSA-247.
Reported-by: George Dunlap <george.dunlap.com>
Signed-off-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>
---
v2: Crash the domain if we're not sure it's safe (or if we think it
can't happen)
---
xen/arch/x86/mm/p2m-pod.c | 42 +++++++++++++++++++++++++++++++++---------
1 file changed, 33 insertions(+), 9 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 5ec8a37949..91d309647e 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -557,11 +557,23 @@ p2m_pod_decrease_reservation(struct domain *d,
if ( !nonpod )
{
- /* All PoD: Mark the whole region invalid and tell caller
- * we're done. */
- p2m_set_entry(p2m, gpfn, _mfn(INVALID_MFN), order, p2m_invalid,
- p2m->default_access);
- p2m->pod.entry_count-=(1<<order);
+ /*
+ * All PoD: Mark the whole region invalid and tell caller
+ * we're done.
+ */
+ if ( p2m_set_entry(p2m, gpfn, _mfn(INVALID_MFN), order, p2m_invalid,
+ p2m->default_access) )
+ {
+ /*
+ * If this fails, we can't tell how much of the range was changed.
+ * Best to crash the domain unless we're sure a partial change is
+ * impossible.
+ */
+ if ( order != 0 )
+ domain_crash(d);
+ goto out_unlock;
+ }
+ p2m->pod.entry_count -= 1UL << order;
BUG_ON(p2m->pod.entry_count < 0);
ret = 1;
goto out_entry_check;
@@ -602,8 +614,14 @@ p2m_pod_decrease_reservation(struct domain *d,
n = 1UL << cur_order;
if ( t == p2m_populate_on_demand )
{
- p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
- p2m_invalid, p2m->default_access);
+ /* This shouldn't be able to fail */
+ if ( p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
+ p2m_invalid, p2m->default_access) )
+ {
+ ASSERT_UNREACHABLE();
+ domain_crash(d);
+ goto out_unlock;
+ }
p2m->pod.entry_count -= n;
BUG_ON(p2m->pod.entry_count < 0);
pod -= n;
@@ -624,8 +642,14 @@ p2m_pod_decrease_reservation(struct domain *d,
page = mfn_to_page(mfn);
- p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
- p2m_invalid, p2m->default_access);
+ /* This shouldn't be able to fail */
+ if ( p2m_set_entry(p2m, gpfn + i, _mfn(INVALID_MFN), cur_order,
+ p2m_invalid, p2m->default_access) )
+ {
+ ASSERT_UNREACHABLE();
+ domain_crash(d);
+ goto out_unlock;
+ }
p2m_tlb_flush_sync(p2m);
for ( j = 0; j < n; ++j )
set_gpfn_from_mfn(mfn_x(mfn), INVALID_M2P_ENTRY);
--
2.15.0