Re: [Xen-devel] [PATCH V6 1/4] x86/mm: Add array_index_nospec to guest provided index values

[George Dunlap <george.dunlap@citrix.com>]

[-- Attachment #1: Type: text/plain, Size: 2523 bytes --]

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 <jbeulich@suse.com>
> Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
> ---
> CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
> CC: Tamas K Lengyel <tamas@tklengyel.com>
> CC: Petre Pircalabu <ppircalabu@bitdefender.com>
> CC: George Dunlap <george.dunlap@eu.citrix.com>
> CC: Jan Beulich <jbeulich@suse.com>
> CC: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Wei Liu <wl@xen.org>
> CC: "Roger Pau Monné" <roger.pau@citrix.com>
> CC: Jun Nakajima <jun.nakajima@intel.com>
> CC: Kevin Tian <kevin.tian@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

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: 0001-x86-p2m-Remove-some-trailing-whitespace.patch --]
[-- Type: text/x-patch; name="0001-x86-p2m-Remove-some-trailing-whitespace.patch", Size: 3403 bytes --]

From 1de1bae235186c5878b35a27eaaba7abb97f4739 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@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.dunlap@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-x86-altp2m-Restrict-MAX_EPTP-to-hap.c.patch --]
[-- Type: text/x-patch; name="0002-x86-altp2m-Restrict-MAX_EPTP-to-hap.c.patch", Size: 2034 bytes --]

From 028ae70bb69992617582dcafbe06da0e176c92cd Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@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.dunlap@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #4: 0003-nospec-Introduce-nospec_clip-macro.patch --]
[-- Type: text/x-patch; name="0003-nospec-Introduce-nospec_clip-macro.patch", Size: 1899 bytes --]

From 22b8e64b951234f9e5a6250e2389564bd4101915 Mon Sep 17 00:00:00 2001
From: George Dunlap <george.dunlap@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.dunlap@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


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #5: 0004-x86-mm-Use-nospec_clip-to-check-guest-supplied-value.patch --]
[-- Type: text/x-patch; name="0004-x86-mm-Use-nospec_clip-to-check-guest-supplied-value.patch", Size: 3623 bytes --]

From 1ee8a8048fe8ea7ba5b3240f12f11986af26f452 Mon Sep 17 00:00:00 2001
From: Alexandru Stefan ISAILA <aisaila@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 <aisaila@bitdefender.com>.

Signed-off-by: George Dunlap <george.dunlap@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


[-- Attachment #6: Type: text/plain, Size: 157 bytes --]

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel