system/xen: XSA 246-251 update.

Signed-off-by: Mario Preksavec <mario@slackware.hr>
This commit is contained in:
Mario Preksavec 2017-12-14 02:37:37 +01:00 committed by Willy Sudiarto Raharjo
parent 2acfdbef50
commit 83300a98e0
9 changed files with 655 additions and 2 deletions

View file

@ -46,7 +46,7 @@ Xen EFI binary.
To make things a bit easier, a copy of Xen EFI binary can be found here:
http://slackware.hr/~mario/xen/xen-4.9.0.efi.gz
http://slackware.hr/~mario/xen/xen-4.9.1.efi.gz
If an automatic boot to Xen kernel is desired, the binary should be renamed and
copied to the following location: /boot/efi/EFI/BOOT/bootx64.efi

View file

@ -24,7 +24,7 @@
PRGNAM=xen
VERSION=${VERSION:-4.9.1}
BUILD=${BUILD:-1}
BUILD=${BUILD:-2}
TAG=${TAG:-_SBo}
SEABIOS=${SEABIOS:-1.10.0}

View file

@ -0,0 +1,74 @@
From: Julien Grall <julien.grall@linaro.org>
Subject: x86/pod: prevent infinite loop when shattering large pages
When populating pages, the PoD may need to split large ones using
p2m_set_entry and request the caller to retry (see ept_get_entry for
instance).
p2m_set_entry may fail to shatter if it is not possible to allocate
memory for the new page table. However, the error is not propagated
resulting to the callers to retry infinitely the PoD.
Prevent the infinite loop by return false when it is not possible to
shatter the large mapping.
This is XSA-246.
Signed-off-by: Julien Grall <julien.grall@linaro.org>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -1071,9 +1071,8 @@ p2m_pod_demand_populate(struct p2m_domai
* NOTE: In a fine-grained p2m locking scenario this operation
* may need to promote its locking from gfn->1g superpage
*/
- p2m_set_entry(p2m, gfn_aligned, INVALID_MFN, PAGE_ORDER_2M,
- p2m_populate_on_demand, p2m->default_access);
- return 0;
+ return p2m_set_entry(p2m, gfn_aligned, INVALID_MFN, PAGE_ORDER_2M,
+ p2m_populate_on_demand, p2m->default_access);
}
/* Only reclaim if we're in actual need of more cache. */
@@ -1104,8 +1103,12 @@ p2m_pod_demand_populate(struct p2m_domai
gfn_aligned = (gfn >> order) << order;
- p2m_set_entry(p2m, gfn_aligned, mfn, order, p2m_ram_rw,
- p2m->default_access);
+ if ( p2m_set_entry(p2m, gfn_aligned, mfn, order, p2m_ram_rw,
+ p2m->default_access) )
+ {
+ p2m_pod_cache_add(p2m, p, order);
+ goto out_fail;
+ }
for( i = 0; i < (1UL << order); i++ )
{
@@ -1150,13 +1153,18 @@ remap_and_retry:
BUG_ON(order != PAGE_ORDER_2M);
pod_unlock(p2m);
- /* Remap this 2-meg region in singleton chunks */
- /* NOTE: In a p2m fine-grained lock scenario this might
- * need promoting the gfn lock from gfn->2M superpage */
+ /*
+ * Remap this 2-meg region in singleton chunks. See the comment on the
+ * 1G page splitting path above for why a single call suffices.
+ *
+ * NOTE: In a p2m fine-grained lock scenario this might
+ * need promoting the gfn lock from gfn->2M superpage.
+ */
gfn_aligned = (gfn>>order)<<order;
- for(i=0; i<(1<<order); i++)
- p2m_set_entry(p2m, gfn_aligned + i, INVALID_MFN, PAGE_ORDER_4K,
- p2m_populate_on_demand, p2m->default_access);
+ if ( p2m_set_entry(p2m, gfn_aligned, INVALID_MFN, PAGE_ORDER_4K,
+ p2m_populate_on_demand, p2m->default_access) )
+ return -1;
+
if ( tb_init_done )
{
struct {

View file

@ -0,0 +1,176 @@
From ad208b8b7e45fb2b7c572b86c61c26412609e82d Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@citrix.com>
Date: Fri, 10 Nov 2017 16:53:54 +0000
Subject: [PATCH 1/2] p2m: Always check to see if removing a p2m entry actually
worked
The PoD zero-check functions speculatively remove memory from the p2m,
then check to see if it's completely zeroed, before putting it in the
cache.
Unfortunately, the p2m_set_entry() calls may fail if the underlying
pagetable structure needs to change and the domain has exhausted its
p2m memory pool: for instance, if we're removing a 2MiB region out of
a 1GiB entry (in the p2m_pod_zero_check_superpage() case), or a 4k
region out of a 2MiB or larger entry (in the p2m_pod_zero_check()
case); and the return value is not checked.
The underlying mfn will then be added into the PoD cache, and at some
point mapped into another location in the p2m. If the guest
afterwards ballons out this memory, it will be freed to the hypervisor
and potentially reused by another domain, in spite of the fact that
the original domain still has writable mappings to it.
There are several places where p2m_set_entry() shouldn't be able to
fail, as it is guaranteed to write an entry of the same order that
succeeded before. Add a backstop of crashing the domain just in case,
and an ASSERT_UNREACHABLE() to flag up the broken assumption on debug
builds.
While we're here, use PAGE_ORDER_2M rather than a magic constant.
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>
---
v4:
- Removed some training whitespace
v3:
- Reformat reset clause to be more compact
- Make sure to set map[i] = NULL when unmapping in case we need to bail
v2:
- Crash a domain if a p2m_set_entry we think cannot fail fails anyway.
---
xen/arch/x86/mm/p2m-pod.c | 77 +++++++++++++++++++++++++++++++++++++----------
1 file changed, 61 insertions(+), 16 deletions(-)
diff --git a/xen/arch/x86/mm/p2m-pod.c b/xen/arch/x86/mm/p2m-pod.c
index 730a48f928..f2ed751892 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -752,8 +752,10 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, unsigned long gfn)
}
/* Try to remove the page, restoring old mapping if it fails. */
- p2m_set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_2M,
- p2m_populate_on_demand, p2m->default_access);
+ if ( p2m_set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_2M,
+ p2m_populate_on_demand, p2m->default_access) )
+ goto out;
+
p2m_tlb_flush_sync(p2m);
/* Make none of the MFNs are used elsewhere... for example, mapped
@@ -810,9 +812,18 @@ p2m_pod_zero_check_superpage(struct p2m_domain *p2m, unsigned long gfn)
ret = SUPERPAGE_PAGES;
out_reset:
- if ( reset )
- p2m_set_entry(p2m, gfn, mfn0, 9, type0, p2m->default_access);
-
+ /*
+ * This p2m_set_entry() call shouldn't be able to fail, since the same order
+ * on the same gfn succeeded above. If that turns out to be false, crashing
+ * the domain should be the safest way of making sure we don't leak memory.
+ */
+ if ( reset && p2m_set_entry(p2m, gfn, mfn0, PAGE_ORDER_2M,
+ type0, p2m->default_access) )
+ {
+ ASSERT_UNREACHABLE();
+ domain_crash(d);
+ }
+
out:
gfn_unlock(p2m, gfn, SUPERPAGE_ORDER);
return ret;
@@ -869,19 +880,30 @@ p2m_pod_zero_check(struct p2m_domain *p2m, unsigned long *gfns, int count)
}
/* Try to remove the page, restoring old mapping if it fails. */
- p2m_set_entry(p2m, gfns[i], INVALID_MFN, PAGE_ORDER_4K,
- p2m_populate_on_demand, p2m->default_access);
+ if ( p2m_set_entry(p2m, gfns[i], INVALID_MFN, PAGE_ORDER_4K,
+ p2m_populate_on_demand, p2m->default_access) )
+ goto skip;
/* See if the page was successfully unmapped. (Allow one refcount
* for being allocated to a domain.) */
if ( (mfn_to_page(mfns[i])->count_info & PGC_count_mask) > 1 )
{
+ /*
+ * If the previous p2m_set_entry call succeeded, this one shouldn't
+ * be able to fail. If it does, crashing the domain should be safe.
+ */
+ if ( p2m_set_entry(p2m, gfns[i], mfns[i], PAGE_ORDER_4K,
+ types[i], p2m->default_access) )
+ {
+ ASSERT_UNREACHABLE();
+ domain_crash(d);
+ goto out_unmap;
+ }
+
+ skip:
unmap_domain_page(map[i]);
map[i] = NULL;
- p2m_set_entry(p2m, gfns[i], mfns[i], PAGE_ORDER_4K,
- types[i], p2m->default_access);
-
continue;
}
}
@@ -900,12 +922,25 @@ p2m_pod_zero_check(struct p2m_domain *p2m, unsigned long *gfns, int count)
unmap_domain_page(map[i]);
- /* See comment in p2m_pod_zero_check_superpage() re gnttab
- * check timing. */
- if ( j < PAGE_SIZE/sizeof(*map[i]) )
+ map[i] = NULL;
+
+ /*
+ * See comment in p2m_pod_zero_check_superpage() re gnttab
+ * check timing.
+ */
+ if ( j < (PAGE_SIZE / sizeof(*map[i])) )
{
- p2m_set_entry(p2m, gfns[i], mfns[i], PAGE_ORDER_4K,
- types[i], p2m->default_access);
+ /*
+ * If the previous p2m_set_entry call succeeded, this one shouldn't
+ * be able to fail. If it does, crashing the domain should be safe.
+ */
+ if ( p2m_set_entry(p2m, gfns[i], mfns[i], PAGE_ORDER_4K,
+ types[i], p2m->default_access) )
+ {
+ ASSERT_UNREACHABLE();
+ domain_crash(d);
+ goto out_unmap;
+ }
}
else
{
@@ -929,7 +964,17 @@ p2m_pod_zero_check(struct p2m_domain *p2m, unsigned long *gfns, int count)
p2m->pod.entry_count++;
}
}
-
+
+ return;
+
+out_unmap:
+ /*
+ * Something went wrong, probably crashing the domain. Unmap
+ * everything and return.
+ */
+ for ( i = 0; i < count; i++ )
+ if ( map[i] )
+ unmap_domain_page(map[i]);
}
#define POD_SWEEP_LIMIT 1024
--
2.15.0

View file

@ -0,0 +1,109 @@
From d4bc7833707351a5341a6bdf04c752a028d9560d 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 f2ed751892..473d6a6dbf 100644
--- a/xen/arch/x86/mm/p2m-pod.c
+++ b/xen/arch/x86/mm/p2m-pod.c
@@ -555,11 +555,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, 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, 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;
@@ -600,8 +612,14 @@ p2m_pod_decrease_reservation(struct domain *d,
n = 1UL << cur_order;
if ( t == p2m_populate_on_demand )
{
- p2m_set_entry(p2m, gpfn + i, INVALID_MFN, cur_order,
- p2m_invalid, p2m->default_access);
+ /* This shouldn't be able to fail */
+ if ( p2m_set_entry(p2m, gpfn + i, 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;
@@ -622,8 +640,14 @@ p2m_pod_decrease_reservation(struct domain *d,
page = mfn_to_page(mfn);
- p2m_set_entry(p2m, gpfn + i, INVALID_MFN, cur_order,
- p2m_invalid, p2m->default_access);
+ /* This shouldn't be able to fail */
+ if ( p2m_set_entry(p2m, gpfn + i, 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

164
system/xen/xsa/xsa248.patch Normal file
View file

@ -0,0 +1,164 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/mm: don't wrongly set page ownership
PV domains can obtain mappings of any pages owned by the correct domain,
including ones that aren't actually assigned as "normal" RAM, but used
by Xen internally. At the moment such "internal" pages marked as owned
by a guest include pages used to track logdirty bits, as well as p2m
pages and the "unpaged pagetable" for HVM guests. Since the PV memory
management and shadow code conflict in their use of struct page_info
fields, and since shadow code is being used for log-dirty handling for
PV domains, pages coming from the shadow pool must, for PV domains, not
have the domain set as their owner.
While the change could be done conditionally for just the PV case in
shadow code, do it unconditionally (and for consistency also for HAP),
just to be on the safe side.
There's one special case though for shadow code: The page table used for
running a HVM guest in unpaged mode is subject to get_page() (in
set_shadow_status()) and hence must have its owner set.
This is XSA-248.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
---
v2: Drop PGC_page_table related pieces.
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -286,8 +286,7 @@ static struct page_info *hap_alloc_p2m_p
{
d->arch.paging.hap.total_pages--;
d->arch.paging.hap.p2m_pages++;
- page_set_owner(pg, d);
- pg->count_info |= 1;
+ ASSERT(!page_get_owner(pg) && !(pg->count_info & PGC_count_mask));
}
else if ( !d->arch.paging.p2m_alloc_failed )
{
@@ -302,21 +301,23 @@ static struct page_info *hap_alloc_p2m_p
static void hap_free_p2m_page(struct domain *d, struct page_info *pg)
{
+ struct domain *owner = page_get_owner(pg);
+
/* This is called both from the p2m code (which never holds the
* paging lock) and the log-dirty code (which always does). */
paging_lock_recursive(d);
- ASSERT(page_get_owner(pg) == d);
- /* Should have just the one ref we gave it in alloc_p2m_page() */
- if ( (pg->count_info & PGC_count_mask) != 1 ) {
- HAP_ERROR("Odd p2m page %p count c=%#lx t=%"PRtype_info"\n",
- pg, pg->count_info, pg->u.inuse.type_info);
+ /* Should still have no owner and count zero. */
+ if ( owner || (pg->count_info & PGC_count_mask) )
+ {
+ HAP_ERROR("d%d: Odd p2m page %"PRI_mfn" d=%d c=%lx t=%"PRtype_info"\n",
+ d->domain_id, mfn_x(page_to_mfn(pg)),
+ owner ? owner->domain_id : DOMID_INVALID,
+ pg->count_info, pg->u.inuse.type_info);
WARN();
+ pg->count_info &= ~PGC_count_mask;
+ page_set_owner(pg, NULL);
}
- pg->count_info &= ~PGC_count_mask;
- /* Free should not decrement domain's total allocation, since
- * these pages were allocated without an owner. */
- page_set_owner(pg, NULL);
d->arch.paging.hap.p2m_pages--;
d->arch.paging.hap.total_pages++;
hap_free(d, page_to_mfn(pg));
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -1503,32 +1503,29 @@ shadow_alloc_p2m_page(struct domain *d)
pg = mfn_to_page(shadow_alloc(d, SH_type_p2m_table, 0));
d->arch.paging.shadow.p2m_pages++;
d->arch.paging.shadow.total_pages--;
+ ASSERT(!page_get_owner(pg) && !(pg->count_info & PGC_count_mask));
paging_unlock(d);
- /* Unlike shadow pages, mark p2m pages as owned by the domain.
- * Marking the domain as the owner would normally allow the guest to
- * create mappings of these pages, but these p2m pages will never be
- * in the domain's guest-physical address space, and so that is not
- * believed to be a concern. */
- page_set_owner(pg, d);
- pg->count_info |= 1;
return pg;
}
static void
shadow_free_p2m_page(struct domain *d, struct page_info *pg)
{
- ASSERT(page_get_owner(pg) == d);
- /* Should have just the one ref we gave it in alloc_p2m_page() */
- if ( (pg->count_info & PGC_count_mask) != 1 )
+ struct domain *owner = page_get_owner(pg);
+
+ /* Should still have no owner and count zero. */
+ if ( owner || (pg->count_info & PGC_count_mask) )
{
- SHADOW_ERROR("Odd p2m page count c=%#lx t=%"PRtype_info"\n",
+ SHADOW_ERROR("d%d: Odd p2m page %"PRI_mfn" d=%d c=%lx t=%"PRtype_info"\n",
+ d->domain_id, mfn_x(page_to_mfn(pg)),
+ owner ? owner->domain_id : DOMID_INVALID,
pg->count_info, pg->u.inuse.type_info);
+ pg->count_info &= ~PGC_count_mask;
+ page_set_owner(pg, NULL);
}
- pg->count_info &= ~PGC_count_mask;
pg->u.sh.type = SH_type_p2m_table; /* p2m code reuses type-info */
- page_set_owner(pg, NULL);
/* This is called both from the p2m code (which never holds the
* paging lock) and the log-dirty code (which always does). */
@@ -3132,7 +3129,9 @@ int shadow_enable(struct domain *d, u32
e = __map_domain_page(pg);
write_32bit_pse_identmap(e);
unmap_domain_page(e);
+ pg->count_info = 1;
pg->u.inuse.type_info = PGT_l2_page_table | 1 | PGT_validated;
+ page_set_owner(pg, d);
}
paging_lock(d);
@@ -3170,7 +3169,11 @@ int shadow_enable(struct domain *d, u32
if ( rv != 0 && !pagetable_is_null(p2m_get_pagetable(p2m)) )
p2m_teardown(p2m);
if ( rv != 0 && pg != NULL )
+ {
+ pg->count_info &= ~PGC_count_mask;
+ page_set_owner(pg, NULL);
shadow_free_p2m_page(d, pg);
+ }
domain_unpause(d);
return rv;
}
@@ -3279,7 +3282,22 @@ out:
/* Must be called outside the lock */
if ( unpaged_pagetable )
+ {
+ if ( page_get_owner(unpaged_pagetable) == d &&
+ (unpaged_pagetable->count_info & PGC_count_mask) == 1 )
+ {
+ unpaged_pagetable->count_info &= ~PGC_count_mask;
+ page_set_owner(unpaged_pagetable, NULL);
+ }
+ /* Complain here in cases where shadow_free_p2m_page() won't. */
+ else if ( !page_get_owner(unpaged_pagetable) &&
+ !(unpaged_pagetable->count_info & PGC_count_mask) )
+ SHADOW_ERROR("d%d: Odd unpaged pt %"PRI_mfn" c=%lx t=%"PRtype_info"\n",
+ d->domain_id, mfn_x(page_to_mfn(unpaged_pagetable)),
+ unpaged_pagetable->count_info,
+ unpaged_pagetable->u.inuse.type_info);
shadow_free_p2m_page(d, unpaged_pagetable);
+ }
}
void shadow_final_teardown(struct domain *d)

View file

@ -0,0 +1,42 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/shadow: fix refcount overflow check
Commit c385d27079 ("x86 shadow: for multi-page shadows, explicitly track
the first page") reduced the refcount width to 25, without adjusting the
overflow check. Eliminate the disconnect by using a manifest constant.
Interestingly, up to commit 047782fa01 ("Out-of-sync L1 shadows: OOS
snapshot") the refcount was 27 bits wide, yet the check was already
using 26.
This is XSA-249.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: George Dunlap <george.dunlap@citrix.com>
Reviewed-by: Tim Deegan <tim@xen.org>
---
v2: Simplify expression back to the style it was.
--- a/xen/arch/x86/mm/shadow/private.h
+++ b/xen/arch/x86/mm/shadow/private.h
@@ -529,7 +529,7 @@ static inline int sh_get_ref(struct doma
x = sp->u.sh.count;
nx = x + 1;
- if ( unlikely(nx >= 1U<<26) )
+ if ( unlikely(nx >= (1U << PAGE_SH_REFCOUNT_WIDTH)) )
{
SHADOW_PRINTK("shadow ref overflow, gmfn=%lx smfn=%lx\n",
__backpointer(sp), mfn_x(smfn));
--- a/xen/include/asm-x86/mm.h
+++ b/xen/include/asm-x86/mm.h
@@ -82,7 +82,8 @@ struct page_info
unsigned long type:5; /* What kind of shadow is this? */
unsigned long pinned:1; /* Is the shadow pinned? */
unsigned long head:1; /* Is this the first page of the shadow? */
- unsigned long count:25; /* Reference count */
+#define PAGE_SH_REFCOUNT_WIDTH 25
+ unsigned long count:PAGE_SH_REFCOUNT_WIDTH; /* Reference count */
} sh;
/* Page is on a free list: ((count_info & PGC_count_mask) == 0). */

View file

@ -0,0 +1,67 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/shadow: fix ref-counting error handling
The old-Linux handling in shadow_set_l4e() mistakenly ORed together the
results of sh_get_ref() and sh_pin(). As the latter failing is not a
correctness problem, simply ignore its return value.
In sh_set_toplevel_shadow() a failing sh_get_ref() must not be
accompanied by installing the entry, despite the domain being crashed.
This is XSA-250.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Tim Deegan <tim@xen.org>
--- a/xen/arch/x86/mm/shadow/multi.c
+++ b/xen/arch/x86/mm/shadow/multi.c
@@ -923,7 +923,7 @@ static int shadow_set_l4e(struct domain
shadow_l4e_t new_sl4e,
mfn_t sl4mfn)
{
- int flags = 0, ok;
+ int flags = 0;
shadow_l4e_t old_sl4e;
paddr_t paddr;
ASSERT(sl4e != NULL);
@@ -938,15 +938,16 @@ static int shadow_set_l4e(struct domain
{
/* About to install a new reference */
mfn_t sl3mfn = shadow_l4e_get_mfn(new_sl4e);
- ok = sh_get_ref(d, sl3mfn, paddr);
- /* Are we pinning l3 shadows to handle wierd linux behaviour? */
- if ( sh_type_is_pinnable(d, SH_type_l3_64_shadow) )
- ok |= sh_pin(d, sl3mfn);
- if ( !ok )
+
+ if ( !sh_get_ref(d, sl3mfn, paddr) )
{
domain_crash(d);
return SHADOW_SET_ERROR;
}
+
+ /* Are we pinning l3 shadows to handle weird Linux behaviour? */
+ if ( sh_type_is_pinnable(d, SH_type_l3_64_shadow) )
+ sh_pin(d, sl3mfn);
}
/* Write the new entry */
@@ -3965,14 +3966,15 @@ sh_set_toplevel_shadow(struct vcpu *v,
/* Take a ref to this page: it will be released in sh_detach_old_tables()
* or the next call to set_toplevel_shadow() */
- if ( !sh_get_ref(d, smfn, 0) )
+ if ( sh_get_ref(d, smfn, 0) )
+ new_entry = pagetable_from_mfn(smfn);
+ else
{
SHADOW_ERROR("can't install %#lx as toplevel shadow\n", mfn_x(smfn));
domain_crash(d);
+ new_entry = pagetable_null();
}
- new_entry = pagetable_from_mfn(smfn);
-
install_new_entry:
/* Done. Install it */
SHADOW_PRINTK("%u/%u [%u] gmfn %#"PRI_mfn" smfn %#"PRI_mfn"\n",

View file

@ -0,0 +1,21 @@
From: Jan Beulich <jbeulich@suse.com>
Subject: x86/paging: don't unconditionally BUG() on finding SHARED_M2P_ENTRY
PV guests can fully control the values written into the P2M.
This is XSA-251.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
--- a/xen/arch/x86/mm/paging.c
+++ b/xen/arch/x86/mm/paging.c
@@ -274,7 +274,7 @@ void paging_mark_pfn_dirty(struct domain
return;
/* Shared MFNs should NEVER be marked dirty */
- BUG_ON(SHARED_M2P(pfn_x(pfn)));
+ BUG_ON(paging_mode_translate(d) && SHARED_M2P(pfn_x(pfn)));
/*
* Values with the MSB set denote MFNs that aren't really part of the