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

[Xen-devel] [PATCH V6 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 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;
 
-        ap2m = d->arch.altp2m_p2m[altp2m_idx];
+        ap2m = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
     }
 #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 = 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 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);
+
     altp2m_list_lock(d);
 
     if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
@@ -2618,6 +2620,8 @@ 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;
@@ -2689,11 +2693,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 = d->arch.altp2m_p2m[array_index_nospec(idx, MAX_ALTP2M)];
 
     p2m_lock(hp2m);
     p2m_lock(ap2m);
@@ -3032,11 +3038,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,
+                                                           MAX_ALTP2M)];
     }
     else
         p2m = host_p2m;
@@ -3075,11 +3083,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,
+                                                           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 V6 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>
Acked-by: Jan Beulich <jbeulich@suse.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 V5:
	- Change first_error_code to first_error and first_error to first_error_gfn
	- Update the requested comments.
---
 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 75f191ae3a..cc4eb1e3d3 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..46fb725806 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_gfn;
+        *error_code = arg->u.suppress_ve_multi.first_error;
+    }
+
+    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 4dfaf35566..4db15768d4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4526,6 +4526,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:
@@ -4684,6 +4685,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 4fc919a9c5..de832dcc6d 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -3070,6 +3070,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)
+        {
+            sve->first_error_gfn = start; /* Save the gfn of the first error */
+            sve->first_error = 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..1f049cfa2e 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; /* Should be set to 0 . */
+    uint64_t first_gfn; /* Value may be updated */
+    uint64_t last_gfn;
+    uint64_t first_error_gfn; /* Gfn of the first error. */
+};
+
 #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 V6 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 de832dcc6d..5b99d1eb97 100644
--- a/xen/arch/x86/mm/p2m.c
+++ b/xen/arch/x86/mm/p2m.c
@@ -2562,6 +2562,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 V6 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 4db15768d4..678faa4b14 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4660,7 +4660,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 a95a50bcae..80de6c2c65 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 5b99d1eb97..926438ed64 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>
@@ -2536,7 +2537,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;
@@ -2562,7 +2564,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);
@@ -2579,6 +2581,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;
@@ -2588,16 +2591,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);
 
@@ -2606,7 +2616,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 1f049cfa2e..bb98abec88 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 V6 2/4] x86/altp2m: Add hypercall to set a range of sve bits

[Tamas K Lengyel]

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 4fc919a9c5..de832dcc6d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3070,6 +3070,70 @@ out:
>      return rc;
>  }
>
> +/*
> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
> + */

I have to say I find it a bit odd why this function is in p2m.c but
it's declaration...

> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve_multi *sve)
> +{

...

> 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);
>

.. in mem_access.h?

> +int p2m_set_suppress_ve_multi(struct domain *d,
> +                              struct xen_hvm_altp2m_suppress_ve_multi *suppress_ve);
> +

I mean, even altp2m.h would make sore sense for this. So what's the
rational behind that?

Tamas

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

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

[Tamas K Lengyel]

On Mon, Dec 23, 2019 at 7:04 AM Alexandru Stefan ISAILA
<aisaila@bitdefender.com> 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>

For the mem_access bits:
Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

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

[George Dunlap]

[-- 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

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

[George Dunlap]

On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:

Why?

 -George

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

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

[George Dunlap]

On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
> 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

*break

> 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.

Weird English quirk: this should be "large".  ("Big" and "large" are
both adjectives, and "ranges" is a noun, so theoretically it should be
OK; but if you ask almost any native English speaker they'll say that
"big" sounds wrong in this case.  No real idea why.)

Both of these could be fixed on check-in.

> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
> index 4fc919a9c5..de832dcc6d 100644
> --- a/xen/arch/x86/mm/p2m.c
> +++ b/xen/arch/x86/mm/p2m.c
> @@ -3070,6 +3070,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;

So in the single-entry version, if altp2m_get_effective_entry() returns
an error, you pass that error up the stack; but in the multiple-entry
version, you ignore the error and simply set the access to
default_access?  I don't think that can be right.  If it is right, then
it definitely needs a comment.

This points out another issue: implementing this functionality twice
risks having this sort of drift between the single-entry version and the
multiple-entry version.  Would it make sense instead to implement the
single-entry version hypercall using p2m_set_suppress_ve_multi?

 -George

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

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

[George Dunlap]

On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
> 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.

That's certainly not what it looks like.  It looks like you're changing
it from...

> @@ -2562,7 +2564,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);

...hostp2m->default_access to...

> @@ -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;

...p2m_access_rwx (by passing NULL in to this function in
p2m_init_next_altp2m).

Why don't you...

> -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);
>

...pass in hostp2m here?

Also...

> @@ -2606,7 +2616,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];

What's this about?

 -George

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

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

[Alexandru Stefan ISAILA]

On 24.12.2019 10:30, George Dunlap wrote:
> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>> 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
> 
> *break

Sorry for the typo.

> 
>> 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.
> 
> Weird English quirk: this should be "large".  ("Big" and "large" are
> both adjectives, and "ranges" is a noun, so theoretically it should be
> OK; but if you ask almost any native English speaker they'll say that
> "big" sounds wrong in this case.  No real idea why.)
> 
> Both of these could be fixed on check-in.
> 
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 4fc919a9c5..de832dcc6d 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3070,6 +3070,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;
> 
> So in the single-entry version, if altp2m_get_effective_entry() returns
> an error, you pass that error up the stack; but in the multiple-entry
> version, you ignore the error and simply set the access to
> default_access?  I don't think that can be right.  If it is right, then
> it definitely needs a comment.
> 

The idea behind this was to have a best effort try and signal the first 
error. If the get_entry fails then the best way to go is with 
default_access but this is open for debate.

Another way to solve this is to update the first_error_gfn/first_error 
and then continue. I think this ca be used to make p2m_set_suppress_ve() 
call p2m_set_suppress_ve_multi.

> This points out another issue: implementing this functionality twice
> risks having this sort of drift between the single-entry version and the
> multiple-entry version.  Would it make sense instead to implement the
> single-entry version hypercall using p2m_set_suppress_ve_multi?
> 

In the single version there is no best-effort idea because the user can 
make use of every single error.

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

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

[George Dunlap]

On 12/24/19 8:48 AM, Alexandru Stefan ISAILA wrote:
> 
> 
> On 24.12.2019 10:30, George Dunlap wrote:
>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>> 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
>>
>> *break
> 
> Sorry for the typo.
> 
>>
>>> 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.
>>
>> Weird English quirk: this should be "large".  ("Big" and "large" are
>> both adjectives, and "ranges" is a noun, so theoretically it should be
>> OK; but if you ask almost any native English speaker they'll say that
>> "big" sounds wrong in this case.  No real idea why.)
>>
>> Both of these could be fixed on check-in.
>>
>>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>>> index 4fc919a9c5..de832dcc6d 100644
>>> --- a/xen/arch/x86/mm/p2m.c
>>> +++ b/xen/arch/x86/mm/p2m.c
>>> @@ -3070,6 +3070,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;
>>
>> So in the single-entry version, if altp2m_get_effective_entry() returns
>> an error, you pass that error up the stack; but in the multiple-entry
>> version, you ignore the error and simply set the access to
>> default_access?  I don't think that can be right.  If it is right, then
>> it definitely needs a comment.
>>
> 
> The idea behind this was to have a best effort try and signal the first 
> error. If the get_entry fails then the best way to go is with 
> default_access but this is open for debate.

I don't see how it's a good idea at all. If get_effective_entry fails,
then mfn and t may both be uninitialized.  If an attacker can arrange
for those to have the values she wants, she could use this to take over
the system.

> Another way to solve this is to update the first_error_gfn/first_error 
> and then continue. I think this ca be used to make p2m_set_suppress_ve() 
> call p2m_set_suppress_ve_multi.

Isn't that exactly the semantics you want -- try gfn N, if that fails,
record it and move on to the next one?  Why would "write an entry with
random values for mfn and type, but with the default access" be a better
response?

 -George

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

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

[Alexandru Stefan ISAILA]

>>>> +/*
>>>> + * 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;
>>>
>>> So in the single-entry version, if altp2m_get_effective_entry() returns
>>> an error, you pass that error up the stack; but in the multiple-entry
>>> version, you ignore the error and simply set the access to
>>> default_access?  I don't think that can be right.  If it is right, then
>>> it definitely needs a comment.
>>>
>>
>> The idea behind this was to have a best effort try and signal the first
>> error. If the get_entry fails then the best way to go is with
>> default_access but this is open for debate.
> 
> I don't see how it's a good idea at all. If get_effective_entry fails,
> then mfn and t may both be uninitialized.  If an attacker can arrange
> for those to have the values she wants, she could use this to take over
> the system.
> 
>> Another way to solve this is to update the first_error_gfn/first_error
>> and then continue. I think this ca be used to make p2m_set_suppress_ve()
>> call p2m_set_suppress_ve_multi.
> 
> Isn't that exactly the semantics you want -- try gfn N, if that fails,
> record it and move on to the next one?  Why would "write an entry with
> random values for mfn and type, but with the default access" be a better
> response?
> 

That is right, I'll go with this for the next version. Should I have the 
single version call the _multi version after this change?

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

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

[George Dunlap]

On 12/24/19 9:04 AM, Alexandru Stefan ISAILA wrote:
>>>>> +/*
>>>>> + * 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;
>>>>
>>>> So in the single-entry version, if altp2m_get_effective_entry() returns
>>>> an error, you pass that error up the stack; but in the multiple-entry
>>>> version, you ignore the error and simply set the access to
>>>> default_access?  I don't think that can be right.  If it is right, then
>>>> it definitely needs a comment.
>>>>
>>>
>>> The idea behind this was to have a best effort try and signal the first
>>> error. If the get_entry fails then the best way to go is with
>>> default_access but this is open for debate.
>>
>> I don't see how it's a good idea at all. If get_effective_entry fails,
>> then mfn and t may both be uninitialized.  If an attacker can arrange
>> for those to have the values she wants, she could use this to take over
>> the system.
>>
>>> Another way to solve this is to update the first_error_gfn/first_error
>>> and then continue. I think this ca be used to make p2m_set_suppress_ve()
>>> call p2m_set_suppress_ve_multi.
>>
>> Isn't that exactly the semantics you want -- try gfn N, if that fails,
>> record it and move on to the next one?  Why would "write an entry with
>> random values for mfn and type, but with the default access" be a better
>> response?
>>
> 
> That is right, I'll go with this for the next version. 

So, one potential behavior you might want.  Consider the following case:

gfn 'A' isn't mapped in the hostp2m yet.
1. Create altp2m X
2. Tools set the sve gfn A
3. Host adds mapping for A
4. Guest accesses A, faulting the mapping over to the altp2m

At the moment, for the single-entry call, #2 will fail, and #4 will get
the default sve value.  It might be nice for #2 to succeed, and #4 to
copy over the mfn, type, &c, but use the sve value specified in #2.

But at the moment, altp2m_get_or_propagate() won't end up copying sve
over if the altp2m entry is invalid (AFAICT).  So I think for now,
skipping that entry and leaving it an error is the best thing to do.

> Should I have the 
> single version call the _multi version after this change?

That seems like a good thing to try.  Thanks.

 -George

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

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

[Alexandru Stefan ISAILA]

On 24.12.2019 10:01, George Dunlap wrote:
> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
> 
> Why?
> 

This was a request from Jan.

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

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

[George Dunlap]

On 12/24/19 10:08 AM, Alexandru Stefan ISAILA wrote:
> 
> 
> On 24.12.2019 10:01, George Dunlap wrote:
>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>
>> Why?
>>
> 
> This was a request from Jan.

Yes, I saw the Requested-by.  It still needs an explanation.

 -George

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

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

[Alexandru Stefan ISAILA]

On 24.12.2019 10:48, George Dunlap wrote:
> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>> 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.
> 
> That's certainly not what it looks like.  It looks like you're changing
> it from...
> 
>> @@ -2562,7 +2564,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);
> 
> ...hostp2m->default_access to...
> 
>> @@ -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;
> 
> ...p2m_access_rwx (by passing NULL in to this function in
> p2m_init_next_altp2m).
> 
> Why don't you...
> 
>> -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);
>>
> 
> ...pass in hostp2m here?

That sound better then the current version, thanks.

> 
> Also...
> 
>> @@ -2606,7 +2616,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];
> 
> What's this about?
> 

This is an artifact form v3 when xenmem_access_to_p2m_access() needed 
p2m. I will clean it.

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

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

[Jan Beulich]

On 23.12.2019 19:08, George Dunlap wrote:
> 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)?

I wouldn't mind this BUILD_BUG_ON() approach as an alternative,
but imo one such instance would then need attaching to every
site.

> Also, this bit where we check the array value and then re-mask the index
> later seems really redundant;

But that's the idea behind the *_nospec() additions: They are to
guard against speculation, i.e. both the bounds check and the
masking of the index have their (distinct) reason.

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

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

[Jan Beulich]

On 23.12.2019 19:08, George Dunlap wrote:
> What about the attached series of patches (compile-tested only)?

This ...

>+#define nospec_clip(index, size)                 \
>+    ({                                           \
>+        bool clipped = (index >= size);          \
>+        index = array_index_nospec(index, size); \
>+        clipped;                                 \
>+    })

... in particular may misguide people on its use: If the clipped
"index" gets stored in a register, all is going to be fine (afaict),
but if it ends up in memory, there's be new (mis-)speculation
opportunities. Some of the clipping done in the patches is already
not fully safe against this, but in some other cases (especially
once array_access_nospec() would be used where possible) would at
least make things as safe as they can be made without compiler aid.

(As an aside, the suggested macro, if we were to put it in, would
need proper parenthesization of the macro parameter uses.)

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

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

[Jan Beulich]

(re-sending, as I still don't see the mail having appeared on the list)

On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
> Changes since V5:
> 	- Add black lines

Luckily no color comes through in plain text mails ;-)

> --- 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) ||

Stray blank after >= .

> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==

I accept you can't (currently) use array_access_nospec() 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)];

... I don't see why you still effectively open-code it here.

> @@ -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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];

Same two remarks here then, and again further down.

> --- 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);
> +
>      altp2m_list_lock(d);
>  
>      if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )

What about this array access?

> @@ -2618,6 +2620,8 @@ 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);

There's a d->arch.altp2m_eptp[] access down from here too. I'm not
going to look further. Please get things into consistent shape while
you do this transformation.

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

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

[Jan Beulich]

(re-sending, as I still don't see the mail having appeared on the list)

On 23.12.2019 15:04, 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; /* Should be set to 0 . */

Stray blank before full stop.

> +    uint64_t first_gfn; /* Value may be updated */
> +    uint64_t last_gfn;
> +    uint64_t first_error_gfn; /* Gfn of the first error. */
> +};

Please be consistent about your comment style here: The full
stop isn't mandatory, but at least adjacent comments (all
added at the same time) should have identical style.

Please may I ask that you apply a little more care when doing
updates, rather than relying on others to spend their time on
catching issues?

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

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

[George Dunlap]

On 12/27/19 7:59 AM, Jan Beulich wrote:
> On 23.12.2019 19:08, George Dunlap wrote:
>> What about the attached series of patches (compile-tested only)?
> 
> This ...
> 
>> +#define nospec_clip(index, size)                 \
>> +    ({                                           \
>> +        bool clipped = (index >= size);          \
>> +        index = array_index_nospec(index, size); \
>> +        clipped;                                 \
>> +    })
> 
> ... in particular may misguide people on its use: If the clipped
> "index" gets stored in a register, all is going to be fine (afaict),
> but if it ends up in memory, there's be new (mis-)speculation
> opportunities.

That makes sense; but in that case code like this:

> +    idx = array_index_nospec(idx, MAX_ALTP2M);
> +

...could end up stored on the stack and re-read, couldn't it?  I mean
yes, it's *very likely* going to stay in a register, but there's no way
to actually guarantee it, is there?

 -George

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

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

[Jan Beulich]

On 27.12.2019 11:52, George Dunlap wrote:
> On 12/27/19 7:59 AM, Jan Beulich wrote:
>> On 23.12.2019 19:08, George Dunlap wrote:
>>> What about the attached series of patches (compile-tested only)?
>>
>> This ...
>>
>>> +#define nospec_clip(index, size)                 \
>>> +    ({                                           \
>>> +        bool clipped = (index >= size);          \
>>> +        index = array_index_nospec(index, size); \
>>> +        clipped;                                 \
>>> +    })
>>
>> ... in particular may misguide people on its use: If the clipped
>> "index" gets stored in a register, all is going to be fine (afaict),
>> but if it ends up in memory, there's be new (mis-)speculation
>> opportunities.
> 
> That makes sense; but in that case code like this:
> 
>> +    idx = array_index_nospec(idx, MAX_ALTP2M);
>> +
> 
> ...could end up stored on the stack and re-read, couldn't it?  I mean
> yes, it's *very likely* going to stay in a register, but there's no way
> to actually guarantee it, is there?

Indeed - hence my "Some of the clipping done in the patches is
already not fully safe against this" in the prior response ("the
patches" meaning Alexandru's, not yours, and it would probably
better have been singular).

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

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

[Alexandru Stefan ISAILA]

On 23.12.2019 18:31, Tamas K Lengyel wrote:
>> diff --git a/xen/arch/x86/mm/p2m.c b/xen/arch/x86/mm/p2m.c
>> index 4fc919a9c5..de832dcc6d 100644
>> --- a/xen/arch/x86/mm/p2m.c
>> +++ b/xen/arch/x86/mm/p2m.c
>> @@ -3070,6 +3070,70 @@ out:
>>       return rc;
>>   }
>>
>> +/*
>> + * Set/clear the #VE suppress bit for multiple pages.  Only available on VMX.
>> + */
> 
> I have to say I find it a bit odd why this function is in p2m.c but
> it's declaration...
> 
>> +int p2m_set_suppress_ve_multi(struct domain *d,
>> +                              struct xen_hvm_altp2m_suppress_ve_multi *sve)
>> +{
> 
> ...
> 
>> 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);
>>
> 
> .. in mem_access.h?
> 
>> +int p2m_set_suppress_ve_multi(struct domain *d,
>> +                              struct xen_hvm_altp2m_suppress_ve_multi *suppress_ve);
>> +
> 
> I mean, even altp2m.h would make sore sense for this. So what's the
> rational behind that?
> 

Indeed it's odd but p2m_set_suppress_ve() is declared above this. I 
don't now how it got there in the first place but I just followed that 
pattern.

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

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

[Alexandru Stefan ISAILA]

On 24.12.2019 12:15, George Dunlap wrote:
> On 12/24/19 10:08 AM, Alexandru Stefan ISAILA wrote:
>>
>>
>> On 24.12.2019 10:01, George Dunlap wrote:
>>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>>
>>> Why?
>>>
>>
>> This was a request from Jan.
> 
> Yes, I saw the Requested-by.  It still needs an explanation.
> 

This is what Jan said in V2:

"All of this is not EPT-specific. Before adding more infrastructure to
cover for this (here: another function parameter), how about moving
these parts into vendor-independent code?"

If there is a need for further explanation maybe Jan can help here.

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

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

[Jan Beulich]

On 06.01.2020 12:55, Alexandru Stefan ISAILA wrote:
> On 24.12.2019 12:15, George Dunlap wrote:
>> On 12/24/19 10:08 AM, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 24.12.2019 10:01, George Dunlap wrote:
>>>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>>>
>>>> Why?
>>>>
>>>
>>> This was a request from Jan.
>>
>> Yes, I saw the Requested-by.  It still needs an explanation.
>>
> 
> This is what Jan said in V2:
> 
> "All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?"
> 
> If there is a need for further explanation maybe Jan can help here.

No matter who it was that asked for something, there's no reason
to have a completely empty commit message, unless the title is
both change description and rationale at once. Perhaps a slightly
re-written sentence like "Some of what this EPT-specific function
does it not EPT-specific" would already do. You could go further
and also state there what I've said in the second sentence.

Jan

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

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

[George Dunlap]

On 1/6/20 11:55 AM, Alexandru Stefan ISAILA wrote:
> On 24.12.2019 12:15, George Dunlap wrote:
>> On 12/24/19 10:08 AM, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 24.12.2019 10:01, George Dunlap wrote:
>>>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>>>
>>>> Why?
>>>>
>>>
>>> This was a request from Jan.
>>
>> Yes, I saw the Requested-by.  It still needs an explanation.
>>
> 
> This is what Jan said in V2:
> 
> "All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?"
> 
> If there is a need for further explanation maybe Jan can help here.

Well "EPT-specific" and "vendor-independent" are enough to indicate the
reason for the motion, but the title doesn't say either of those two
things, and so the reader is left to guess.  A title / message like this
would have been fine:

---
x86/mm: Pull non-EPT-specific altp2m handling code into
vendor-independent code

No functional changes.
---

Or since that's a bit long, maybe:

---
x86/mm: Pull vendor-independent altp2m code out of p2m-ept.c

...and into p2m.c.  No functional changes.
---

 -George

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

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

[George Dunlap]

On 1/6/20 11:55 AM, Alexandru Stefan ISAILA wrote:
> On 24.12.2019 12:15, George Dunlap wrote:
>> On 12/24/19 10:08 AM, Alexandru Stefan ISAILA wrote:
>>>
>>>
>>> On 24.12.2019 10:01, George Dunlap wrote:
>>>> On 12/23/19 2:04 PM, Alexandru Stefan ISAILA wrote:
>>>>
>>>> Why?
>>>>
>>>
>>> This was a request from Jan.
>>
>> Yes, I saw the Requested-by.  It still needs an explanation.
>>
> 
> This is what Jan said in V2:
> 
> "All of this is not EPT-specific. Before adding more infrastructure to
> cover for this (here: another function parameter), how about moving
> these parts into vendor-independent code?"
> 
> If there is a need for further explanation maybe Jan can help here.

You don't have to make every clean-up patch that reviewers ask for; but
if you do post a clean-up patch, it's your responsibility to make sure
it's got a suitable description.  The audience is not only the reviewer
who asked for the patch, but also normal developers 5 years from now
(perhaps yourself) who are trying to figure out why the change was made.

 -George

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

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

[Alexandru Stefan ISAILA]

On 27.12.2019 10:01, Jan Beulich wrote:
> (re-sending, as I still don't see the mail having appeared on the list)
> 
> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>> Changes since V5:
>> 	- Add black lines
> 
> Luckily no color comes through in plain text mails ;-)
> 
>> --- 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) ||
> 
> Stray blank after >= .
> 
>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
> 
> I accept you can't (currently) use array_access_nospec() 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)];
> 
> ... I don't see why you still effectively open-code it here.

I am not sure I follow you here, that is what we agreed in v5 
(https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html). 
Did I miss something?


> 
>> @@ -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 = d->arch.altp2m_p2m[array_index_nospec(altp2m_idx, MAX_ALTP2M)];
> 
> Same two remarks here then, and again further down.
> 
>> --- 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);
>> +
>>       altp2m_list_lock(d);
>>   
>>       if ( d->arch.altp2m_eptp[idx] == mfn_x(INVALID_MFN) )
> 
> What about this array access?
> 
>> @@ -2618,6 +2620,8 @@ 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);
> 
> There's a d->arch.altp2m_eptp[] access down from here too. I'm not
> going to look further. Please get things into consistent shape while
> you do this transformation.
> 

I will change the idx part in p2m_init_altp2m_by_id() and 
p2m_destroy_altp2m_by_id() so they match the rest of the checks:
"if ( idx >=  min(ARRAY_SIZE(d->arch.altp2m_p2m), MAX_EPTP))...", drop 
the idx = array_index_nospec(idx, MAX_ALTP2M); and have 
array_index_nospec() into place.


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

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

[Jan Beulich]

On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
> On 27.12.2019 10:01, Jan Beulich wrote:
>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>> --- 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) ||
>>
>> Stray blank after >= .
>>
>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>
>> I accept you can't (currently) use array_access_nospec() 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)];
>>
>> ... I don't see why you still effectively open-code it here.
> 
> I am not sure I follow you here, that is what we agreed in v5 
> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html). 
> Did I miss something?

In context there (from an earlier reply of mine) you will find me
having mentioned array_access_nospec(). This wasn't invalidated or
overridden by my "Yes, that's how I think it ought to be." I didn't
say so explicitly (again) because to me it goes without saying that
open-coding _anything_ is, in the common case, bad practice.

Jan

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

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

[Alexandru Stefan ISAILA]

On 07.01.2020 15:55, Jan Beulich wrote:
> On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
>> On 27.12.2019 10:01, Jan Beulich wrote:
>>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>> --- 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) ||
>>>
>>> Stray blank after >= .
>>>
>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>>
>>> I accept you can't (currently) use array_access_nospec() 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)];
>>>
>>> ... I don't see why you still effectively open-code it here.
>>
>> I am not sure I follow you here, that is what we agreed in v5
>> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
>> Did I miss something?
> 
> In context there (from an earlier reply of mine) you will find me
> having mentioned array_access_nospec(). This wasn't invalidated or
> overridden by my "Yes, that's how I think it ought to be." I didn't
> say so explicitly (again) because to me it goes without saying that
> open-coding _anything_ is, in the common case, bad practice.
> 

So the way to go is to have:

altp2m_idx = array_index_nospec(altp2m_idx, MAX_ALTP2M);
ap2m = d->arch.altp2m_p2m[altp2m_idx];


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

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

[Jan Beulich]

On 07.01.2020 15:31, Alexandru Stefan ISAILA wrote:
> 
> 
> On 07.01.2020 15:55, Jan Beulich wrote:
>> On 07.01.2020 14:25, Alexandru Stefan ISAILA wrote:
>>> On 27.12.2019 10:01, Jan Beulich wrote:
>>>> On 23.12.2019 15:04, Alexandru Stefan ISAILA wrote:
>>>>> --- 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) ||
>>>>
>>>> Stray blank after >= .
>>>>
>>>>> +             d->arch.altp2m_eptp[array_index_nospec(altp2m_idx, MAX_EPTP)] ==
>>>>
>>>> I accept you can't (currently) use array_access_nospec() 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)];
>>>>
>>>> ... I don't see why you still effectively open-code it here.
>>>
>>> I am not sure I follow you here, that is what we agreed in v5
>>> (https://lists.xenproject.org/archives/html/xen-devel/2019-12/msg01704.html).
>>> Did I miss something?
>>
>> In context there (from an earlier reply of mine) you will find me
>> having mentioned array_access_nospec(). This wasn't invalidated or
>> overridden by my "Yes, that's how I think it ought to be." I didn't
>> say so explicitly (again) because to me it goes without saying that
>> open-coding _anything_ is, in the common case, bad practice.
>>
> 
> So the way to go is to have:
> 
> altp2m_idx = array_index_nospec(altp2m_idx, MAX_ALTP2M);
> ap2m = d->arch.altp2m_p2m[altp2m_idx];

No. The way to go is to use array_access_nospec() wherever possible.
Besides (as said) avoiding its open-coding, this is the construct
correctly matching your uses of ARRAY_SIZE(), avoiding the explicit
specification of the upper array bound (MAX_ALTP2M). (I really don't
see how my previous reply was not crystal clear in this regard.)

Jan

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