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

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

[Alexandru Stefan ISAILA]

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 V4:
	- Change bounds check from MAX_EPTP to MAX_ALTP2M
	- Move array_index_nospec() closer to the bounds check.
---
 xen/arch/x86/mm/mem_access.c | 15 +++++++++------
 xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
 2 files changed, 23 insertions(+), 12 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 320b9fe621..33e379db8f 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
     if ( altp2m_idx )
     {
         if ( altp2m_idx >= MAX_ALTP2M ||
-             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+             mfn_x(INVALID_MFN) )
             return -EINVAL;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #else
     ASSERT(!altp2m_idx);
@@ -426,10 +427,11 @@ long p2m_set_mem_access_multi(struct domain *d,
     if ( altp2m_idx )
     {
         if ( altp2m_idx >= MAX_ALTP2M ||
-             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+             mfn_x(INVALID_MFN) )
             return -EINVAL;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #else
     ASSERT(!altp2m_idx);
@@ -492,10 +494,11 @@ 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) )
+             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+             mfn_x(INVALID_MFN) )
             return -EINVAL;
 
-        p2m = d->arch.altp2m_p2m[altp2m_idx];
+        p2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #else
     ASSERT(!altp2m_idx);
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index ba126f790a..16039c7a57 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2574,6 +2574,7 @@ 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);
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2615,6 +2616,7 @@ int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx)
     if ( !idx || idx >= MAX_ALTP2M )
         return rc;
 
+    idx = array_index_nospec(idx, MAX_ALTP2M);
     rc = domain_pause_except_self(d);
     if ( rc )
         return rc;
@@ -2686,11 +2688,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 >= MAX_ALTP2M ||
+         d->arch.altp2m_eptp[array_index_nospec(idx, MAX_ALTP2M)] ==
+         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, MAX_ALTP2M)];
 
     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -3030,10 +3034,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) )
+             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+             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,
+                                                           MAX_ALTP2M)];
     }
     else
         p2m = host_p2m;
@@ -3073,10 +3079,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) )
+             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
+             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,
+                                                           MAX_ALTP2M)];
     }
     else
         p2m = host_p2m;
-- 
2.17.1

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

[Xen-devel] [PATCH V5 2/4] x86/altp2m: Add hypercall to set a range of sve bits

[Alexandru Stefan ISAILA]

By default the sve bits are not set.
This patch adds a new hypercall, xc_altp2m_set_supress_ve_multi(),
to set a range of sve bits.
The core function, p2m_set_suppress_ve_multi(), does not brake in case
of a error and it is doing a best effort for setting the bits in the
given range. A check for continuation is made in order to have
preemption on big ranges.
The gfn of the first error is stored in
xen_hvm_altp2m_suppress_ve_multi.first_error and the error code is
stored in xen_hvm_altp2m_suppress_ve_multi.first_error_code.
If no error occurred the values will be 0.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
---
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Andrew Cooper <andrew.cooper3@citrix.com>
CC: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Jan Beulich <jbeulich@suse.com>
CC: Julien Grall <julien@xen.org>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: "Roger Pau Monné" <roger.pau@citrix.com>
CC: George Dunlap <george.dunlap@eu.citrix.com>
CC: Razvan Cojocaru <rcojocaru@bitdefender.com>
CC: Tamas K Lengyel <tamas@tklengyel.com>
CC: Petre Pircalabu <ppircalabu@bitdefender.com>
---
Changes since V4:
	- Remove ->first_error and first_error_code from
HVMOP_altp2m_set_suppress_ve_multi check
	- Check ->first_error_code so that first_error on gfn 0 can be
saved
	- Chage type of first_error_code to int32_t
	- Clip ->last_gfn before sanity check.
---
 tools/libxc/include/xenctrl.h   |  4 +++
 tools/libxc/xc_altp2m.c         | 33 +++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 20 +++++++++++
 xen/arch/x86/mm/p2m.c           | 64 +++++++++++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h | 13 +++++++
 xen/include/xen/mem_access.h    |  3 ++
 6 files changed, 137 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index f4431687b3..21a333f2c4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,10 @@ int xc_altp2m_switch_to_view(xc_interface *handle, uint32_t domid,
                              uint16_t view_id);
 int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
                               uint16_t view_id, xen_pfn_t gfn, bool sve);
+int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
+                                   uint16_t view_id, xen_pfn_t first_gfn,
+                                   xen_pfn_t last_gfn, bool sve,
+                                   xen_pfn_t *error_gfn, int32_t *error_code);
 int xc_altp2m_get_suppress_ve(xc_interface *handle, uint32_t domid,
                               uint16_t view_id, xen_pfn_t gfn, bool *sve);
 int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 09dad0355e..4ba930666a 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -234,6 +234,39 @@ int xc_altp2m_set_suppress_ve(xc_interface *handle, uint32_t domid,
     return rc;
 }
 
+int xc_altp2m_set_supress_ve_multi(xc_interface *handle, uint32_t domid,
+                                   uint16_t view_id, xen_pfn_t first_gfn,
+                                   xen_pfn_t last_gfn, bool sve,
+                                   xen_pfn_t *error_gfn, int32_t *error_code)
+{
+    int rc;
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+
+    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_set_suppress_ve_multi;
+    arg->domain = domid;
+    arg->u.suppress_ve_multi.view = view_id;
+    arg->u.suppress_ve_multi.first_gfn = first_gfn;
+    arg->u.suppress_ve_multi.last_gfn = last_gfn;
+    arg->u.suppress_ve_multi.suppress_ve = sve;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+                  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    if ( arg->u.suppress_ve_multi.first_error )
+    {
+        *error_gfn = arg->u.suppress_ve_multi.first_error;
+        *error_code = arg->u.suppress_ve_multi.first_error_code;
+    }
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
+
 int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 47573f71b8..98d1d9788b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4553,6 +4553,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_destroy_p2m:
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_suppress_ve:
+    case HVMOP_altp2m_set_suppress_ve_multi:
     case HVMOP_altp2m_get_suppress_ve:
     case HVMOP_altp2m_set_mem_access:
     case HVMOP_altp2m_set_mem_access_multi:
@@ -4711,6 +4712,25 @@ static int do_altp2m_op(
         }
         break;
 
+    case HVMOP_altp2m_set_suppress_ve_multi:
+    {
+        uint64_t max_phys_addr = (1UL << d->arch.cpuid->extd.maxphysaddr) - 1;
+
+        a.u.suppress_ve_multi.last_gfn = min(a.u.suppress_ve_multi.last_gfn,
+                                             max_phys_addr);
+
+        if ( a.u.suppress_ve_multi.pad1 ||
+             a.u.suppress_ve_multi.first_gfn > a.u.suppress_ve_multi.last_gfn )
+            rc = -EINVAL;
+        else
+        {
+            rc = p2m_set_suppress_ve_multi(d, &a.u.suppress_ve_multi);
+            if ( (!rc || rc == -ERESTART) && __copy_to_guest(arg, &a, 1) )
+                rc = -EFAULT;
+        }
+        break;
+    }
+
     case HVMOP_altp2m_get_suppress_ve:
         if ( a.u.suppress_ve.pad1 || a.u.suppress_ve.pad2 )
             rc = -EINVAL;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index 16039c7a57..d92613ebe4 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -3065,6 +3065,70 @@ out:
     return rc;
 }
 
+/*
+ * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
+ */
+int p2m_set_suppress_ve_multi(struct domain *d,
+                              struct xen_hvm_altp2m_suppress_ve_multi *sve)
+{
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m = host_p2m;
+    uint64_t start = sve->first_gfn;
+    int rc = 0;
+
+    if ( sve->view > 0 )
+    {
+        if ( sve->view >= MAX_ALTP2M ||
+             d->arch.altp2m_eptp[array_index_nospec(sve->view, MAX_ALTP2M)] ==
+             mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        p2m = ap2m = d->arch.altp2m_p2m[array_index_nospec(sve->view,
+                                                           MAX_ALTP2M)];
+    }
+
+    p2m_lock(host_p2m);
+
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+    while ( sve->last_gfn >= start )
+    {
+        p2m_access_t a;
+        p2m_type_t t;
+        mfn_t mfn;
+        int err = 0;
+
+        if ( altp2m_get_effective_entry(p2m, _gfn(start), &mfn, &t, &a, AP2MGET_query) )
+            a = p2m->default_access;
+
+        if ( (err = p2m->set_entry(p2m, _gfn(start), mfn, PAGE_ORDER_4K, t, a,
+                                   sve->suppress_ve)) &&
+             !sve->first_error_code )
+        {
+            sve->first_error = start; /* Save the gfn of the first error */
+            sve->first_error_code = err; /* Save the first error code */
+        }
+
+        /* Check for continuation if it's not the last iteration. */
+        if ( sve->last_gfn >= ++start && hypercall_preempt_check() )
+        {
+            rc = -ERESTART;
+            break;
+        }
+    }
+
+    sve->first_gfn = start;
+
+    if ( ap2m )
+        p2m_unlock(ap2m);
+
+    p2m_unlock(host_p2m);
+
+    return rc;
+}
+
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
                         unsigned int altp2m_idx)
 {
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 353f8034d9..5446d634d8 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -46,6 +46,16 @@ struct xen_hvm_altp2m_suppress_ve {
     uint64_t gfn;
 };
 
+struct xen_hvm_altp2m_suppress_ve_multi {
+    uint16_t view;
+    uint8_t suppress_ve; /* Boolean type. */
+    uint8_t pad1;
+    int32_t first_error_code; /* Must be set to 0 . */
+    uint64_t first_gfn; /* Value will be updated */
+    uint64_t last_gfn;
+    uint64_t first_error; /* Gfn of the first error. Must be set to 0. */
+};
+
 #if __XEN_INTERFACE_VERSION__ < 0x00040900
 
 /* Set the logical level of one of a domain's PCI INTx wires. */
@@ -339,6 +349,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_vcpu_disable_notify  13
 /* Get the active vcpu p2m index */
 #define HVMOP_altp2m_get_p2m_idx          14
+/* Set the "Supress #VE" bit for a range of pages */
+#define HVMOP_altp2m_set_suppress_ve_multi 15
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -353,6 +365,7 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_change_gfn           change_gfn;
         struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
         struct xen_hvm_altp2m_suppress_ve          suppress_ve;
+        struct xen_hvm_altp2m_suppress_ve_multi    suppress_ve_multi;
         struct xen_hvm_altp2m_vcpu_disable_notify  disable_notify;
         struct xen_hvm_altp2m_get_vcpu_p2m_idx     get_vcpu_p2m_idx;
         uint8_t pad[64];
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index e4d24502e0..00e594a0ad 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -75,6 +75,9 @@ long p2m_set_mem_access_multi(struct domain *d,
 int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
                         unsigned int altp2m_idx);
 
+int p2m_set_suppress_ve_multi(struct domain *d,
+                              struct xen_hvm_altp2m_suppress_ve_multi *suppress_ve);
+
 int p2m_get_suppress_ve(struct domain *d, gfn_t gfn, bool *suppress_ve,
                         unsigned int altp2m_idx);
 
-- 
2.17.1

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

[Xen-devel] [PATCH V5 3/4] x86/mm: Pull out the p2m specifics from p2m_init_altp2m_ept

[Alexandru Stefan ISAILA]

Requested-by: Jan Beulich <jbeulich@suse.com>
Signed-off-by: Alexandru Isaila <aisaila@bitdefender.com>
Reviewed-by: Jan Beulich <jbeulich@suse.com>

---
CC: Jun Nakajima <jun.nakajima@intel.com>
CC: Kevin Tian <kevin.tian@intel.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>
---
 xen/arch/x86/mm/p2m-ept.c | 6 ------
 xen/arch/x86/mm/p2m.c     | 6 ++++++
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/xen/arch/x86/mm/p2m-ept.c b/xen/arch/x86/mm/p2m-ept.c
index b5517769c9..d861cd7b51 100644
--- a/xen/arch/x86/mm/p2m-ept.c
+++ b/xen/arch/x86/mm/p2m-ept.c
@@ -1357,13 +1357,7 @@ void p2m_init_altp2m_ept(struct domain *d, unsigned int i)
     struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
     struct ept_data *ept;
 
-    p2m->default_access = hostp2m->default_access;
-    p2m->domain = hostp2m->domain;
-
-    p2m->global_logdirty = hostp2m->global_logdirty;
     p2m->ept.ad = hostp2m->ept.ad;
-    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
-    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;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index d92613ebe4..cb5b8d67d1 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2559,6 +2559,12 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
         goto out;
     }
 
+    p2m->default_access = hostp2m->default_access;
+    p2m->domain = hostp2m->domain;
+    p2m->global_logdirty = hostp2m->global_logdirty;
+    p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
+    p2m->max_mapped_pfn = p2m->max_remapped_gfn = 0;
+
     p2m_init_altp2m_ept(d, idx);
 
  out:
-- 
2.17.1

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

[Xen-devel] [PATCH V5 4/4] x86/mm: Make use of the default access param from xc_altp2m_create_view

[Alexandru Stefan ISAILA]

At this moment the default_access param from xc_altp2m_create_view is
not used.

This patch assigns default_access to p2m->default_access at the time of
initializing a new altp2m view.

Signed-off-by: Alexandru Isaila <aisaila@bitdefender.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: George Dunlap <George.Dunlap@eu.citrix.com>
CC: Ian Jackson <ian.jackson@eu.citrix.com>
CC: Julien Grall <julien@xen.org>
CC: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
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>
---
Changes since V4:
	- Add const struct p2m_domain *p2m to
xenmem_access_to_p2m_access()
	- Pull xenmem_access_to_p2m_access() out of the locked area
	- Add a check for NULL p2m in xenmem_access_to_p2m_access().
---
 xen/arch/x86/hvm/hvm.c          |  3 ++-
 xen/arch/x86/mm/mem_access.c    | 11 +++++++----
 xen/arch/x86/mm/p2m.c           | 21 ++++++++++++++++-----
 xen/include/asm-x86/p2m.h       |  3 ++-
 xen/include/public/hvm/hvm_op.h |  2 --
 xen/include/xen/mem_access.h    |  4 ++++
 6 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 98d1d9788b..d7a55568c9 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4687,7 +4687,8 @@ static int do_altp2m_op(
     }
 
     case HVMOP_altp2m_create_p2m:
-        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view)) )
+        if ( !(rc = p2m_init_next_altp2m(d, &a.u.view.view,
+                                         a.u.view.hvmmem_default_access)) )
             rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         break;
 
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 33e379db8f..5b74a6898b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -314,9 +314,9 @@ static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
     return rc;
 }
 
-static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
-                                        xenmem_access_t xaccess,
-                                        p2m_access_t *paccess)
+bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
+                                 xenmem_access_t xaccess,
+                                 p2m_access_t *paccess)
 {
     static const p2m_access_t memaccess[] = {
 #define ACCESS(ac) [XENMEM_access_##ac] = p2m_access_##ac
@@ -340,7 +340,10 @@ static bool xenmem_access_to_p2m_access(struct p2m_domain *p2m,
         *paccess = memaccess[xaccess];
         break;
     case XENMEM_access_default:
-        *paccess = p2m->default_access;
+        if ( !p2m )
+            *paccess = p2m_access_rwx;
+        else
+            *paccess = p2m->default_access;
         break;
     default:
         return false;
diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
index cb5b8d67d1..2774811bb8 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -25,6 +25,7 @@
 
 #include <xen/guest_access.h> /* copy_from_guest() */
 #include <xen/iommu.h>
+#include <xen/mem_access.h>
 #include <xen/vm_event.h>
 #include <xen/event.h>
 #include <public/vm_event.h>
@@ -2533,7 +2534,8 @@ void p2m_flush_altp2m(struct domain *d)
     altp2m_list_unlock(d);
 }
 
-static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
+static int p2m_activate_altp2m(struct domain *d, unsigned int idx,
+                               p2m_access_t hvmmem_default_access)
 {
     struct p2m_domain *hostp2m, *p2m;
     int rc;
@@ -2559,7 +2561,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
         goto out;
     }
 
-    p2m->default_access = hostp2m->default_access;
+    p2m->default_access = hvmmem_default_access;
     p2m->domain = hostp2m->domain;
     p2m->global_logdirty = hostp2m->global_logdirty;
     p2m->min_remapped_gfn = gfn_x(INVALID_GFN);
@@ -2576,6 +2578,7 @@ static int p2m_activate_altp2m(struct domain *d, unsigned int idx)
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
 {
     int rc = -EINVAL;
+    struct p2m_domain *hostp2m = p2m_get_hostp2m(d);
 
     if ( idx >= MAX_ALTP2M )
         return rc;
@@ -2584,16 +2587,23 @@ int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx)
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
-        rc = p2m_activate_altp2m(d, idx);
+        rc = p2m_activate_altp2m(d, idx, hostp2m->default_access);
 
     altp2m_list_unlock(d);
     return rc;
 }
 
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+                         xenmem_access_t hvmmem_default_access)
 {
     int rc = -EINVAL;
     unsigned int i;
+    p2m_access_t a;
+    struct p2m_domain *p2m;
+
+    if ( hvmmem_default_access > XENMEM_access_default ||
+         !xenmem_access_to_p2m_access(NULL, hvmmem_default_access, &a) )
+        return rc;
 
     altp2m_list_lock(d);
 
@@ -2602,7 +2612,8 @@ int p2m_init_next_altp2m(struct domain *d, uint16_t *idx)
         if ( d->arch.altp2m_eptp[i] != mfn_x(INVALID_MFN) )
             continue;
 
-        rc = p2m_activate_altp2m(d, i);
+        p2m = d->arch.altp2m_p2m[i];
+        rc = p2m_activate_altp2m(d, i, a);
 
         if ( !rc )
             *idx = i;
diff --git a/xen/include/asm-x86/p2m.h b/xen/include/asm-x86/p2m.h
index 94285db1b4..ac2d2787f4 100644
--- a/xen/include/asm-x86/p2m.h
+++ b/xen/include/asm-x86/p2m.h
@@ -884,7 +884,8 @@ bool p2m_altp2m_get_or_propagate(struct p2m_domain *ap2m, unsigned long gfn_l,
 int p2m_init_altp2m_by_id(struct domain *d, unsigned int idx);
 
 /* Find an available alternate p2m and make it valid */
-int p2m_init_next_altp2m(struct domain *d, uint16_t *idx);
+int p2m_init_next_altp2m(struct domain *d, uint16_t *idx,
+                         xenmem_access_t hvmmem_default_access);
 
 /* Make a specific alternate p2m invalid */
 int p2m_destroy_altp2m_by_id(struct domain *d, unsigned int idx);
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index 5446d634d8..49816d9312 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -251,8 +251,6 @@ DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_vcpu_disable_notify_t);
 struct xen_hvm_altp2m_view {
     /* IN/OUT variable */
     uint16_t view;
-    /* Create view only: default access type
-     * NOTE: currently ignored */
     uint16_t hvmmem_default_access; /* xenmem_access_t */
 };
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 00e594a0ad..5d53fb8ce4 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -58,6 +58,10 @@ typedef enum {
     /* NOTE: Assumed to be only 4 bits right now on x86. */
 } p2m_access_t;
 
+bool xenmem_access_to_p2m_access(const struct p2m_domain *p2m,
+                                 xenmem_access_t xaccess,
+                                 p2m_access_t *paccess);
+
 /*
  * Set access type for a region of gfns.
  * If gfn == INVALID_GFN, sets the default access type.
-- 
2.17.1

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

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

[Jan Beulich]

On 19.12.2019 10:42, 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 V4:
> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
> 	- Move array_index_nospec() closer to the bounds check.
> ---
>  xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>  xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>  2 files changed, 23 insertions(+), 12 deletions(-)
> 
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index 320b9fe621..33e379db8f 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>      if ( altp2m_idx )
>      {
>          if ( altp2m_idx >= MAX_ALTP2M ||
> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==

As implied by a reply to v4, this is still latently buggy: There's
no guarantee anyone will notice the issue here when bumping
MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
the actual bounds check. Then each array access itself can be made
use the correct bound. In fact you'd probably have noticed this if
you had made use of array_access_nospec() where possible (which
also would help readability) - apparently not here, but ...

> +             mfn_x(INVALID_MFN) )
>              return -EINVAL;
>  
> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];

... here. The min() suggested above would then better become
min(ARRAY_SIZE(d->arch.altp2m_eptp), MAX_EPTP), which I think
would then even compile cleanly (the apparently simpler form
above wouldn't as is afaict).

> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -2574,6 +2574,7 @@ 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);
>      altp2m_list_lock(d);

I wouldn't object to there being no blank line between the if()
and the line you add, but you surely want a blank line ahead of
the unrelated lock acquire (similarly at least once more below).

Jan

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

Re: [Xen-devel] [PATCH V5 2/4] x86/altp2m: Add hypercall to set a range of sve bits

[Jan Beulich]

On 19.12.2019 10:42, Alexandru Stefan ISAILA wrote:
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -46,6 +46,16 @@ struct xen_hvm_altp2m_suppress_ve {
>      uint64_t gfn;
>  };
>  
> +struct xen_hvm_altp2m_suppress_ve_multi {
> +    uint16_t view;
> +    uint8_t suppress_ve; /* Boolean type. */
> +    uint8_t pad1;
> +    int32_t first_error_code; /* Must be set to 0 . */
> +    uint64_t first_gfn; /* Value will be updated */

s/will/may/

> +    uint64_t last_gfn;
> +    uint64_t first_error; /* Gfn of the first error. Must be set to 0. */

There's no real "must" here. Please at most say "should", but I'd
even consider dropping that part of the comment altogether. The
consumer logic needs to key off of the error code anyway. Even
for the error code field I'd suggest saying just "should": You
don't check it (because you can't), and the caller only shoots
itself in the foot if it doesn't do so.

Also looking at this yet again - I think field names would better
be "first_error" for the error code and "first_error_gfn" for the
GFN.

Anyway, preferably with the suggested adjustments, applicable
hypervisor parts
Acked-by: Jan Beulich <jbeulich@suse.com>


Jan

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

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

[Alexandru Stefan ISAILA]

On 19.12.2019 12:43, Jan Beulich wrote:
> On 19.12.2019 10:42, 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 V4:
>> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
>> 	- Move array_index_nospec() closer to the bounds check.
>> ---
>>   xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>>   xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>>   2 files changed, 23 insertions(+), 12 deletions(-)
>>
>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>> index 320b9fe621..33e379db8f 100644
>> --- a/xen/arch/x86/mm/mem_access.c
>> +++ b/xen/arch/x86/mm/mem_access.c
>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>       if ( altp2m_idx )
>>       {
>>           if ( altp2m_idx >= MAX_ALTP2M ||

Ok, so have if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_eptp), 
MAX_EPTP) ||
here and then...

>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==

have MAX_EPTP here and ...

> 
> As implied by a reply to v4, this is still latently buggy: There's
> no guarantee anyone will notice the issue here when bumping
> MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
> is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
> the actual bounds check. Then each array access itself can be made
> use the correct bound. In fact you'd probably have noticed this if
> you had made use of array_access_nospec() where possible (which
> also would help readability) - apparently not here, but ... >
>> +             mfn_x(INVALID_MFN) )
>>               return -EINVAL;
>>   
>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];

MAX_ALTP2M here ?


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

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

[Jan Beulich]

On 20.12.2019 10:09, Alexandru Stefan ISAILA wrote:
> 
> 
> On 19.12.2019 12:43, Jan Beulich wrote:
>> On 19.12.2019 10:42, 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 V4:
>>> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
>>> 	- Move array_index_nospec() closer to the bounds check.
>>> ---
>>>   xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>>>   xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>>>   2 files changed, 23 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index 320b9fe621..33e379db8f 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>>       if ( altp2m_idx )
>>>       {
>>>           if ( altp2m_idx >= MAX_ALTP2M ||
> 
> Ok, so have if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_eptp), 
> MAX_EPTP) ||
> here and then...
> 
>>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
> 
> have MAX_EPTP here and ...
> 
>>
>> As implied by a reply to v4, this is still latently buggy: There's
>> no guarantee anyone will notice the issue here when bumping
>> MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
>> is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
>> the actual bounds check. Then each array access itself can be made
>> use the correct bound. In fact you'd probably have noticed this if
>> you had made use of array_access_nospec() where possible (which
>> also would help readability) - apparently not here, but ... >
>>> +             mfn_x(INVALID_MFN) )
>>>               return -EINVAL;
>>>   
>>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
> 
> MAX_ALTP2M here ?

Yes, that's how I think it ought to be. Give others a chance to
disagree with me, though.

Jan

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

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

[Alexandru Stefan ISAILA]

On 20.12.2019 11:39, Jan Beulich wrote:
> On 20.12.2019 10:09, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 19.12.2019 12:43, Jan Beulich wrote:
>>> On 19.12.2019 10:42, 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 V4:
>>>> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
>>>> 	- Move array_index_nospec() closer to the bounds check.
>>>> ---
>>>>    xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>>>>    xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>>>>    2 files changed, 23 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>> index 320b9fe621..33e379db8f 100644
>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>>>        if ( altp2m_idx )
>>>>        {
>>>>            if ( altp2m_idx >= MAX_ALTP2M ||
>>
>> Ok, so have if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_eptp),
>> MAX_EPTP) ||
>> here and then...
>>
>>>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
>>
>> have MAX_EPTP here and ...
>>
>>>
>>> As implied by a reply to v4, this is still latently buggy: There's
>>> no guarantee anyone will notice the issue here when bumping
>>> MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
>>> is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
>>> the actual bounds check. Then each array access itself can be made
>>> use the correct bound. In fact you'd probably have noticed this if
>>> you had made use of array_access_nospec() where possible (which
>>> also would help readability) - apparently not here, but ... >
>>>> +             mfn_x(INVALID_MFN) )
>>>>                return -EINVAL;
>>>>    
>>>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
>>
>> MAX_ALTP2M here ?
> 
> Yes, that's how I think it ought to be. Give others a chance to
> disagree with me, though.
> 

There is a slight problem with using (ARRAY_SIZE(..)) it will give 
"error: static assertion failed:" on  __must_be_array(x) because 
d->arch.altp2m_eptp is not static.

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

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

[Jan Beulich]

On 20.12.2019 12:49, Alexandru Stefan ISAILA wrote:
> 
> 
> On 20.12.2019 11:39, Jan Beulich wrote:
>> On 20.12.2019 10:09, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 19.12.2019 12:43, Jan Beulich wrote:
>>>> On 19.12.2019 10:42, 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 V4:
>>>>> 	- Change bounds check from MAX_EPTP to MAX_ALTP2M
>>>>> 	- Move array_index_nospec() closer to the bounds check.
>>>>> ---
>>>>>    xen/arch/x86/mm/mem_access.c | 15 +++++++++------
>>>>>    xen/arch/x86/mm/p2m.c        | 20 ++++++++++++++------
>>>>>    2 files changed, 23 insertions(+), 12 deletions(-)
>>>>>
>>>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>>>> index 320b9fe621..33e379db8f 100644
>>>>> --- a/xen/arch/x86/mm/mem_access.c
>>>>> +++ b/xen/arch/x86/mm/mem_access.c
>>>>> @@ -367,10 +367,11 @@ long p2m_set_mem_access(struct domain *d, gfn_t gfn, uint32_t nr,
>>>>>        if ( altp2m_idx )
>>>>>        {
>>>>>            if ( altp2m_idx >= MAX_ALTP2M ||
>>>
>>> Ok, so have if ( altp2m_idx >=  min(ARRAY_SIZE(d->arch.altp2m_eptp),
>>> MAX_EPTP) ||
>>> here and then...

The 1st arg to min() equals the 2nd, which is ...

>>>>> -             d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
>>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_ALTP2M)] ==
>>>
>>> have MAX_EPTP here and ...
>>>
>>>>
>>>> As implied by a reply to v4, this is still latently buggy: There's
>>>> no guarantee anyone will notice the issue here when bumping
>>>> MAX_ALTP2M past MAX_EPTP. The only future proof thing to do here
>>>> is, as suggested, using some form of min(MAX_ALTP2M, MAX_EPTP) in
>>>> the actual bounds check. Then each array access itself can be made
>>>> use the correct bound. In fact you'd probably have noticed this if
>>>> you had made use of array_access_nospec() where possible (which
>>>> also would help readability) - apparently not here, but ... >
>>>>> +             mfn_x(INVALID_MFN) )
>>>>>                return -EINVAL;
>>>>>    
>>>>> -        ap2m = d->arch.altp2m_p2m[altp2m_idx];
>>>>> +        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
>>>
>>> MAX_ALTP2M here ?
>>
>> Yes, that's how I think it ought to be. Give others a chance to
>> disagree with me, though.
>>
> 
> There is a slight problem with using (ARRAY_SIZE(..)) it will give 
> "error: static assertion failed:" on  __must_be_array(x) because 
> d->arch.altp2m_eptp is not static.

... causing this. Once you use the correct array above, I think
things will work.

Jan

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