diff mbox series

[V7,1/4] x86/mm: Add array_index_nospec to guest provided index values

Message ID 20200108140810.6528-1-aisaila@bitdefender.com (mailing list archive)
State Superseded
Headers show
Series [V7,1/4] x86/mm: Add array_index_nospec to guest provided index values | expand

Commit Message

Alexandru Stefan ISAILA Jan. 8, 2020, 2:08 p.m. UTC 
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>
Acked-by: Tamas K Lengyel <tamas@tklengyel.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 V6:
	- Remove stray spaces
	- Use ARRAY_SIZE(d->arch.altp2m_p2m) insead of MAX_ALTP2M.
---
 xen/arch/x86/mm/mem_access.c | 24 +++++++++++--------
 xen/arch/x86/mm/p2m-ept.c    |  6 +++--
 xen/arch/x86/mm/p2m.c        | 45 +++++++++++++++++++++++-------------
 3 files changed, 48 insertions(+), 27 deletions(-)

Comments

Jan Beulich Jan. 10, 2020, 4:12 p.m. UTC | #1 
On 08.01.2020 15:08, Alexandru Stefan ISAILA wrote:
> Changes since V6:
> 	- Remove stray spaces
> 	- Use ARRAY_SIZE(d->arch.altp2m_p2m) insead of MAX_ALTP2M.

I'm not utterly confused:

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -366,11 +366,13 @@ 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;
>  
> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
> +                                  ARRAY_SIZE(d->arch.altp2m_p2m))];

Why is this still not

        ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);

? What am I missing?

Jan
Alexandru Stefan ISAILA Jan. 17, 2020, 8:48 a.m. UTC | #2 
On 10.01.2020 18:12, Jan Beulich wrote:
> On 08.01.2020 15:08, Alexandru Stefan ISAILA wrote:
>> Changes since V6:
>> 	- Remove stray spaces
>> 	- Use ARRAY_SIZE(d->arch.altp2m_p2m) insead of MAX_ALTP2M.
> 
> I'm not utterly confused:
> 
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -366,11 +366,13 @@ 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;
>>   
>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
>> +                                  ARRAY_SIZE(d->arch.altp2m_p2m))];
> 
> Why is this still not
> 
>          ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
> 
> ? What am I missing?
> 

Sorry for the misunderstanding here, I will use  array_access_nospec() 
where the ARRAY_SIZE(d->arch.altp2m_p2m) is used.


Alex
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..f323d885b1 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -366,11 +366,13 @@  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;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                  ARRAY_SIZE(d->arch.altp2m_p2m))];
     }
 #else
     ASSERT(!altp2m_idx);
@@ -425,11 +427,13 @@  long p2m_set_mem_access_multi(struct domain *d,
 #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;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                  ARRAY_SIZE(d->arch.altp2m_p2m))];
     }
 #else
     ASSERT(!altp2m_idx);
@@ -491,11 +495,13 @@  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 ||
-             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;
 
-        p2m = d->arch.altp2m_p2m[altp2m_idx];
+        p2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                 ARRAY_SIZE(d->arch.altp2m_p2m))];
     }
 #else
     ASSERT(!altp2m_idx);
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b5517769c9..1c23ea6169 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1353,7 +1353,9 @@  void setup_ept_dump(void)
 
 void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
 {
-    struct p2m_domain *p2m = d->arch.altp2m_p2m[i];
+    struct p2m_domain *p2m = d->arch.
+                             altp2m_p2m[array_index_nospec(i,
+                                        ARRAY_SIZE(d->arch.altp2m_p2m))];
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
@@ -1366,7 +1368,7 @@  void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0;
     ept = &p2m->ept;
     ept->mfn = pagetable_get_pfn(p2m_get_pagetable(p2m));
-    d->arch.altp2m_eptp[i] = ept->eptp;
+    d->arch.altp2m_eptp[array_index_nospec(i, MAX_EPTP)] = ept->eptp;
 }
 
 unsigned int p2m_find_altp2m_by_eptp(struct domain *d, uint64_t eptp)
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 3119269073..5f046960a9 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2502,7 +2502,8 @@  static void p2m_reset_altp2m(struct domain *d, unsigned int idx,
     struct p2m_domain *p2m;
 
     ASSERT(idx < MAX_ALTP2M);
-    p2m = d->arch.altp2m_p2m[idx];
+    p2m = d->arch.altp2m_p2m[array_index_nospec(idx,
+                             ARRAY_SIZE(d->arch.altp2m_p2m))];
 
     p2m_lock(p2m);
 
@@ -2543,7 +2544,8 @@  static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
 
     ASSERT(idx < MAX_ALTP2M);
 
-    p2m = d->arch.altp2m_p2m[idx];
+    p2m = d->arch.altp2m_p2m[array_index_nospec(idx,
+                             ARRAY_SIZE(d->arch.altp2m_p2m))];
     hostp2m = p2m_get_hostp2m(d);
 
     p2m_lock(p2m);
@@ -2574,12 +2576,13 @@  int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
 
-    if ( idx >= MAX_ALTP2M )
+    if ( idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) )
         return rc;
 
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
+    if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
+         mfn_x(INVALID_MFN) )
         rc = p2m_activate_altp2m(d, idx);
 
     altp2m_list_unlock(d);
@@ -2615,7 +2618,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 || idx >= min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) )
         return rc;
 
     rc = domain_pause_except_self(d);
@@ -2625,14 +2628,17 @@  int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
     rc = -EBUSY;
     altp2m_list_lock(d);
 
-    if ( d->arch.altp2m_eptp[idx] != mfn_x(INVALID_MFN) )
+    if ( d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] !=
+         mfn_x(INVALID_MFN) )
     {
-        p2m = d->arch.altp2m_p2m[idx];
+        p2m = d->arch.altp2m_p2m[array_index_nospec(idx,
+                                 ARRAY_SIZE(d->arch.altp2m_p2m))];
 
         if ( !_atomic_read(p2m->active_vcpus) )
         {
             p2m_reset_altp2m(d, idx, ALTP2M_DEACTIVATE);
-            d->arch.altp2m_eptp[idx] = mfn_x(INVALID_MFN);
+            d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] =
+            mfn_x(INVALID_MFN);
             rc = 0;
         }
     }
@@ -2689,11 +2695,14 @@  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 ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP) ||
+         d->arch.altp2m_eptp[array_index_nospec(idx, MAX_EPTP)] ==
+         mfn_x(INVALID_MFN) )
         return rc;
 
     hp2m = p2m_get_hostp2m(d);
-    ap2m = d->arch.altp2m_p2m[idx];
+    ap2m = d->arch.altp2m_p2m[array_index_nospec(idx,
+                              ARRAY_SIZE(d->arch.altp2m_p2m))];
 
     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -3032,11 +3041,13 @@  int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
 
     if ( altp2m_idx > 0 )
     {
-        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;
 
-        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                        ARRAY_SIZE(d->arch.altp2m_p2m))];
     }
     else
         p2m = host_p2m;
@@ -3075,11 +3086,13 @@  int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
 
     if ( altp2m_idx > 0 )
     {
-        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;
 
-        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx,
+                                        ARRAY_SIZE(d->arch.altp2m_p2m))];
     }
     else
         p2m = host_p2m;