On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
> This patch aims to sanitize indexes, potentially guest provided
> values, for altp2m_eptp[] and altp2m_p2m[] arrays.
>
> Requested-by: Jan Beulich <jbeul...@suse.com>
> Signed-off-by: Alexandru Isaila <aisa...@bitdefender.com>
> ---
> CC: Razvan Cojocaru <rcojoc...@bitdefender.com>
> CC: Tamas K Lengyel <ta...@tklengyel.com>
> CC: Petre Pircalabu <ppircal...@bitdefender.com>
> CC: George Dunlap <george.dun...@eu.citrix.com>
> CC: Jan Beulich <jbeul...@suse.com>
> CC: Andrew Cooper <andrew.coop...@citrix.com>
> CC: Wei Liu <w...@xen.org>
> CC: "Roger Pau Monné" <roger....@citrix.com>
> CC: Jun Nakajima <jun.nakaj...@intel.com>
> CC: Kevin Tian <kevin.t...@intel.com>
> ---
> Changes since V5:
> - Add black lines
> - Check altp2m_idx against min(ARRAY_SIZE(d->arch.altp2m_p2m),
> MAX_EPTP).
> ---
> xen/arch/x86/mm/mem_access.c | 21 ++++++++++++---------
> xen/arch/x86/mm/p2m.c | 26 ++++++++++++++++++--------
> 2 files changed, 30 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 320b9fe621..a95a50bcae 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -366,11 +366,12 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn,
> uint32_t nr,
> #ifdef CONFIG_HVM
> if ( altp2m_idx )
> {
> - if ( altp2m_idx >= MAX_ALTP2M ||
> - d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> + if ( altp2m_idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
> + d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> + mfn_x(INVALID_MFN) )
> return -EINVAL;
I realize Jan asked for something like this, and I'm sorry I didn't have
time to bring it up then, but this seems really silly. If we're worried
about this, wouldn't it be better to have a BUILD_BUG_ON(MAX_ALTP2M >
MAX_EPTP)?
Also, this bit where we check the array value and then re-mask the index
later seems really redundant; both here, but especially...
> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 3119269073..4fc919a9c5 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2577,6 +2577,8 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned
> int idx)
> if ( idx >= MAX_ALTP2M )
> return rc;
>
> + idx = array_index_nospec(idx, MAX_ALTP2M);
> +
...here. What about the attached series of patches (compile-tested only)?
-George
From 1de1bae235186c5878b35a27eaaba7abb97f4739 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@citrix.com>
Date: Mon, 23 Dec 2019 17:54:53 +0000
Subject: [PATCH 1/4] x86/p2m: Remove some trailing whitespace
No functional changes.
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/arch/x86/mm/p2m.c | 16 ++++++++--------
1 file changed, 8 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ba126f790a..b9f8948130 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -892,7 +892,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
&a, 0, NULL, NULL);
if ( p2m_is_shared(ot) )
{
- /* Do an unshare to cleanly take care of all corner
+ /* Do an unshare to cleanly take care of all corner
* cases. */
int rc;
rc = mem_sharing_unshare_page(p2m->domain,
@@ -909,7 +909,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
* However, all current (changeset 3432abcf9380) code
* paths avoid this unsavoury situation. For now.
*
- * Foreign domains are okay to place an event as they
+ * Foreign domains are okay to place an event as they
* won't go to sleep. */
(void)mem_sharing_notify_enomem(p2m->domain,
gfn_x(gfn_add(gfn, i)), false);
@@ -924,7 +924,7 @@ guest_physmap_add_entry(struct domain *d, gfn_t gfn, mfn_t mfn,
/* Really shouldn't be unmapping grant/foreign maps this way */
domain_crash(d);
p2m_unlock(p2m);
-
+
return -EINVAL;
}
else if ( p2m_is_ram(ot) && !p2m_is_paged(ot) )
@@ -1787,7 +1787,7 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t buffer)
if ( user_ptr )
/* Sanity check the buffer and bail out early if trouble */
- if ( (buffer & (PAGE_SIZE - 1)) ||
+ if ( (buffer & (PAGE_SIZE - 1)) ||
(!access_ok(user_ptr, PAGE_SIZE)) )
return -EINVAL;
@@ -1832,7 +1832,7 @@ int p2m_mem_paging_prep(struct domain *d, unsigned long gfn_l, uint64_t buffer)
"bytes left %d\n", gfn_l, d->domain_id, rc);
ret = -EFAULT;
put_page(page); /* Don't leak pages */
- goto out;
+ goto out;
}
}
@@ -1904,7 +1904,7 @@ static struct p2m_domain *
p2m_getlru_nestedp2m(struct domain *d, struct p2m_domain *p2m)
{
struct list_head *lru_list = &p2m_get_hostp2m(d)->np2m_list;
-
+
ASSERT(!list_empty(lru_list));
if ( p2m == NULL )
@@ -2050,7 +2050,7 @@ p2m_get_nestedp2m_locked(struct vcpu *v)
nestedp2m_lock(d);
p2m = nv->nv_p2m;
- if ( p2m )
+ if ( p2m )
{
p2m_lock(p2m);
if ( p2m->np2m_base == np2m_base )
@@ -2889,7 +2889,7 @@ void audit_p2m(struct domain *d,
pod_unlock(p2m);
p2m_unlock(p2m);
-
+
P2M_PRINTK("p2m audit complete\n");
if ( orphans_count | mpbad | pmbad )
P2M_PRINTK("p2m audit found %lu orphans\n", orphans_count);
--
2.24.0
From 028ae70bb69992617582dcafbe06da0e176c92cd Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@citrix.com>
Date: Mon, 23 Dec 2019 17:21:33 +0000
Subject: [PATCH 2/4] x86/altp2m: Restrict MAX_EPTP to hap.c
Right now we have two altp2m structures hanging off arch_domain:
altp2m_eptp, which is hardware-based and points to a page with 512 ept
pointers, and altp2m_p2m, which is currently limited to 10 as a fairly
arbitary way of balancing performance, space, and usability. altp2m
indexes are used as index values to both, meaning the only safe option
is to check guest-supplied indexes against both. This is a bit
redundant, however, as MAX_ALTP2M must always be <= MAX_EPTP.
Move MAX_EPTP to hap.c, where the array is initialized; and add
BUILD_BUG_ON() asserting that MAX_ALTP2M < MAX_EPTP. Then, elsewhere,
it will always be safe to check guest-supplied indexes against
MAX_ALTP2M.
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/arch/x86/mm/hap/hap.c | 3 +++
xen/include/asm-x86/domain.h | 1 -
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/xen/arch/x86/mm/hap/hap.c b/xen/arch/x86/mm/hap/hap.c
index 3d93f3451c..69159c689e 100644
--- a/xen/arch/x86/mm/hap/hap.c
+++ b/xen/arch/x86/mm/hap/hap.c
@@ -488,6 +488,9 @@ int hap_enable(struct domain *d, u32 mode)
goto out;
}
+#define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t))
+ BUILD_BUG_ON(MAX_ALTP2M > MAX_EPTP);
+
for ( i = 0; i < MAX_EPTP; i++ )
d->arch.altp2m_eptp[i] = mfn_x(INVALID_MFN);
diff --git a/xen/include/asm-x86/domain.h b/xen/include/asm-x86/domain.h
index 3780287e7e..c46fb54d7e 100644
--- a/xen/include/asm-x86/domain.h
+++ b/xen/include/asm-x86/domain.h
@@ -240,7 +240,6 @@ struct paging_vcpu {
#define MAX_ALTP2M 10 /* arbitrary */
#define INVALID_ALTP2M 0xffff
-#define MAX_EPTP (PAGE_SIZE / sizeof(uint64_t))
struct p2m_domain;
struct time_scale {
int shift;
--
2.24.0
From 22b8e64b951234f9e5a6250e2389564bd4101915 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dun...@citrix.com>
Date: Mon, 23 Dec 2019 18:00:55 +0000
Subject: [PATCH 3/4] nospec: Introduce nospec_clip macro
There are lots of places in the code where we might want to:
1. Do a bounds check and return an error
2. Use the array_index_nospec() macro to prevent Spectre-style attacks
during speculation.
Create a simple macro to clip an index and return true if it was
clipped. This allows us to "fully" sanitize an index passed from
userspace in a single check, thus:
if ( nospec_clip(index, INDEX_MAX) )
return -EINVAL;
Afterwards, `index` wil be safe against speculation, having been
clipped via array_index_nospec().
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/include/xen/nospec.h | 15 +++++++++++++++
1 file changed, 15 insertions(+)
diff --git a/xen/include/xen/nospec.h b/xen/include/xen/nospec.h
index 76255bc46e..1cc0301848 100644
--- a/xen/include/xen/nospec.h
+++ b/xen/include/xen/nospec.h
@@ -64,6 +64,21 @@ static inline unsigned long array_index_mask_nospec(unsigned long index,
#define array_index_nospec(index, size) ((void)(size), (index))
#endif /* CONFIG_SPECULATIVE_HARDEN_ARRAY */
+/*
+ * nospec_clip - Do a bounds check and make an index speculation safe
+ *
+ * Use to simultaneously check the size and clip it appropriately, thus:
+ *
+ * if ( nospec_clip(index, size) )
+ * return -EINVAL;
+ */
+#define nospec_clip(index, size) \
+ ({ \
+ bool clipped = (index >= size); \
+ index = array_index_nospec(index, size); \
+ clipped; \
+ })
+
/*
* array_access_nospec - allow nospec access for static size arrays
*/
--
2.24.0
From 1ee8a8048fe8ea7ba5b3240f12f11986af26f452 Mon Sep 17 00:00:00 2001
From: Alexandru Stefan ISAILA <aisa...@bitdefender.com>
Date: Mon, 23 Dec 2019 14:04:31 +0000
Subject: [PATCH 4/4] x86/mm: Use nospec_clip() to check guest-supplied values.
This patch aims to sanitize indexes, potentially guest provided
values, for altp2m_eptp[] and altp2m_p2m[] arrays.
Based on a patch by Alexandru Isaila <aisa...@bitdefender.com>.
Signed-off-by: George Dunlap <george.dun...@citrix.com>
---
xen/arch/x86/mm/mem_access.c | 6 +++---
xen/arch/x86/mm/p2m.c | 11 ++++++-----
2 files changed, 9 insertions(+), 8 deletions(-)
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..5b4a4f43ef 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -366,7 +366,7 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
#ifdef CONFIG_HVM
if ( altp2m_idx )
{
- if ( altp2m_idx >= MAX_ALTP2M ||
+ if ( nospec_clip(altp2m_idx, MAX_ALTP2M) ||
d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
return -EINVAL;
@@ -425,7 +425,7 @@ long p2m_set_mem_access_multi(struct domain *d,
#ifdef CONFIG_HVM
if ( altp2m_idx )
{
- if ( altp2m_idx >= MAX_ALTP2M ||
+ if ( nospec_clip(altp2m_idx, MAX_ALTP2M) ||
d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
return -EINVAL;
@@ -491,7 +491,7 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
}
else if ( altp2m_idx ) /* altp2m view 0 is treated as the hostp2m */
{
- if ( altp2m_idx >= MAX_ALTP2M ||
+ if ( nospec_clip(altp2m_idx, MAX_ALTP2M) ||
d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
return -EINVAL;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index b9f8948130..4f93f410c8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2571,7 +2571,7 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
{
int rc = -EINVAL;
- if ( idx >= MAX_ALTP2M )
+ if ( nospec_clip(idx, MAX_ALTP2M) )
return rc;
altp2m_list_lock(d);
@@ -2612,7 +2612,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
struct p2m_domain *p2m;
int rc = -EBUSY;
- if ( !idx || idx >= MAX_ALTP2M )
+ if ( !idx || nospec_clip(idx, MAX_ALTP2M) )
return rc;
rc = domain_pause_except_self(d);
@@ -2686,7 +2686,8 @@ int p2m_change_altp2m_gfn(struct domain *d, unsigned int idx,
mfn_t mfn;
int rc = -EINVAL;
- if ( idx >= MAX_ALTP2M || d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
+ if ( nospec_clip(idx, MAX_ALTP2M) ||
+ d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
return rc;
hp2m = p2m_get_hostp2m(d);
@@ -3029,7 +3030,7 @@ int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
if ( altp2m_idx > 0 )
{
- if ( altp2m_idx >= MAX_ALTP2M ||
+ if ( nospec_clip(altp2m_idx, MAX_ALTP2M) ||
d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
return -EINVAL;
@@ -3072,7 +3073,7 @@ int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
if ( altp2m_idx > 0 )
{
- if ( altp2m_idx >= MAX_ALTP2M ||
+ if ( nospec_clip(altp2m_idx, MAX_ALTP2M) ||
d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
return -EINVAL;
--
2.24.0
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
