diff mbox series

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

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

Commit Message

Alexandru Stefan ISAILA Jan. 17, 2020, 1:31 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 V7:
	- Make use of array_access_nospec() over
array_index_nospec(altp2m_idx, ARRAY_SIZE(d->arch.altp2m_p2m).
---
 xen/arch/x86/mm/mem_access.c | 21 ++++++++++---------
 xen/arch/x86/mm/p2m-ept.c    |  4 ++--
 xen/arch/x86/mm/p2m.c        | 39 +++++++++++++++++++++---------------
 3 files changed, 37 insertions(+), 27 deletions(-)

Comments

Jan Beulich Jan. 17, 2020, 4:13 p.m. UTC | #1 
On 17.01.2020 14:31, 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>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
Petre Ovidiu PIRCALABU Jan. 21, 2020, 12:53 p.m. UTC | #2 
On Fri, 2020-01-17 at 15:31 +0200, 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>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
> 
Reviewed-by: Petre Pircalabu <ppircalabu@bitdefender.com>
George Dunlap Jan. 23, 2020, 12:24 p.m. UTC | #3 
On 1/17/20 1:31 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>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Acked-by: George Dunlap <george.dunlap@citrix.com>
Andrew Cooper Jan. 23, 2020, 6:45 p.m. UTC | #4 
On 17/01/2020 13:31, 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>
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Something in this series broke the ARM build.  Sorry, but I don't have
any further time to investigate.

gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
-Wdeclaration-after-statement -Wno-unused-but-set-variable
-Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc
-fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith
-Wvla -pipe -D__XEN__ -include
/builds/xen-project/xen/xen/include/xen/config.h
'-D__OBJECT_FILE__="asm-offsets.s"' -Wa,--strip-local-absolute -g -MMD
-MF ./.asm-offsets.s.d -mcpu=generic -mgeneral-regs-only
-I/builds/xen-project/xen/xen/include -fno-stack-protector
-fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -S -o
asm-offsets.s arm64/asm-offsets.c
In file included from /builds/xen-project/xen/xen/include/asm/p2m.h:7,
                 from /builds/xen-project/xen/xen/include/asm/domain.h:7,
                 from /builds/xen-project/xen/xen/include/xen/domain.h:8,
                 from /builds/xen-project/xen/xen/include/xen/sched.h:11,
                 from arm64/asm-offsets.c:9:
/builds/xen-project/xen/xen/include/xen/mem_access.h:61:47: error:
'struct p2m_domain' declared inside parameter list will not be visible
outside of this definition or declaration [-Werror]
 bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
                                               ^~~~~~~~~~
/builds/xen-project/xen/xen/include/xen/mem_access.h:83:38: error:
'struct xen_hvm_altp2m_suppress_ve_multi' declared inside parameter list
will not be visible outside of this definition or declaration [-Werror]
                               struct xen_hvm_altp2m_suppress_ve_multi
*suppress_ve);
                                      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
cc1: all warnings being treated as errors
make[3]: *** [Makefile:124: asm-offsets.s] Error 1
make[3]: Leaving directory '/builds/xen-project/xen/xen/arch/arm'
make[2]: *** [Makefile:146: /builds/xen-project/xen/xen/xen] Error 2
make[2]: Leaving directory '/builds/xen-project/xen/xen'
make[1]: *** [Makefile:50: install] Error 2
make[1]: Leaving directory '/builds/xen-project/xen/xen'
make: *** [Makefile:130: install-xen] Error 2
make: *** Waiting for unfinished jobs....

Full logs: https://gitlab.com/xen-project/xen/-/jobs/412893448

~Andrew
Tamas K Lengyel Jan. 23, 2020, 6:57 p.m. UTC | #5 
On Thu, Jan 23, 2020 at 11:45 AM Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
>
> On 17/01/2020 13:31, 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>
> > Acked-by: Tamas K Lengyel <tamas@tklengyel.com>
>
> Something in this series broke the ARM build.  Sorry, but I don't have
> any further time to investigate.
>
> gcc -DBUILD_ID -fno-strict-aliasing -std=gnu99 -Wall -Wstrict-prototypes
> -Wdeclaration-after-statement -Wno-unused-but-set-variable
> -Wno-unused-local-typedefs -O1 -fno-omit-frame-pointer -nostdinc
> -fno-builtin -fno-common -Werror -Wredundant-decls -Wno-pointer-arith
> -Wvla -pipe -D__XEN__ -include
> /builds/xen-project/xen/xen/include/xen/config.h
> '-D__OBJECT_FILE__="asm-offsets.s"' -Wa,--strip-local-absolute -g -MMD
> -MF ./.asm-offsets.s.d -mcpu=generic -mgeneral-regs-only
> -I/builds/xen-project/xen/xen/include -fno-stack-protector
> -fno-exceptions -Wnested-externs -DGCC_HAS_VISIBILITY_ATTRIBUTE -S -o
> asm-offsets.s arm64/asm-offsets.c
> In file included from /builds/xen-project/xen/xen/include/asm/p2m.h:7,
>                  from /builds/xen-project/xen/xen/include/asm/domain.h:7,
>                  from /builds/xen-project/xen/xen/include/xen/domain.h:8,
>                  from /builds/xen-project/xen/xen/include/xen/sched.h:11,
>                  from arm64/asm-offsets.c:9:
> /builds/xen-project/xen/xen/include/xen/mem_access.h:61:47: error:
> 'struct p2m_domain' declared inside parameter list will not be visible
> outside of this definition or declaration [-Werror]
>  bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
>                                                ^~~~~~~~~~
> /builds/xen-project/xen/xen/include/xen/mem_access.h:83:38: error:
> 'struct xen_hvm_altp2m_suppress_ve_multi' declared inside parameter list

Looks like we need an explicit include for asm/p2m.h and
public/hvm/hvm_op.h in the mem_access.h header (both of which end up
being included prior to mem_access.h on an x86 build). Although from
the looks of it wrapping the _ve functions in #ifdef CONFIG_X86 ..
#endif would be even better.

Tamas
diff mbox series

Patch

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..31ff826393 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;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
     }
 #else
     ASSERT(!altp2m_idx);
@@ -425,11 +426,12 @@  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 = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
     }
 #else
     ASSERT(!altp2m_idx);
@@ -491,11 +493,12 @@  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 = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
     }
 #else
     ASSERT(!altp2m_idx);
diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b5517769c9..b078a9a59e 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1353,7 +1353,7 @@  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 = array_access_nospec(d->arch.altp2m_p2m, i);
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
@@ -1366,7 +1366,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..00b24342fc 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2502,7 +2502,7 @@  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 = array_access_nospec(d->arch.altp2m_p2m, idx);
 
     p2m_lock(p2m);
 
@@ -2543,7 +2543,7 @@  static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
 
     ASSERT(idx < MAX_ALTP2M);
 
-    p2m = d->arch.altp2m_p2m[idx];
+    p2m = array_access_nospec(d->arch.altp2m_p2m, idx);
     hostp2m = p2m_get_hostp2m(d);
 
     p2m_lock(p2m);
@@ -2574,12 +2574,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 +2616,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 +2626,16 @@  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 = array_access_nospec(d->arch.altp2m_p2m, idx);
 
         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 +2692,13 @@  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 = array_access_nospec(d->arch.altp2m_p2m, idx);
 
     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -3032,11 +3037,12 @@  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 = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
     }
     else
         p2m = host_p2m;
@@ -3075,11 +3081,12 @@  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 = array_access_nospec(d->arch.altp2m_p2m, altp2m_idx);
     }
     else
         p2m = host_p2m;