[PATCH 0/3] x86: Add a hvmop for setting the #VE suppress bit

[PATCH 0/3] x86: Add a hvmop for setting the #VE suppress bit

[Adrian Pop]

As the code stands right now, after DomU has enabled #VE using
HVMOP_altp2m_vcpu_enable_notify, all its pages have the #VE suppress bit
cleared, generating #VEs for any EPT violation.  There is currently no
way to change the value of the #VE suppress bit for a page from a
domain; it can only be done in Xen internally using ept_set_entry().

Following the discussion from
https://lists.xen.org/archives/html/xen-devel/2017-03/msg01312.html this
patch introduces a new hvmop to set this bit and thus have control over
which pages generate #VE and which VM-Exit.

I'm not sure whether it's best to define p2m_set_suppress_ve() in
mem_access.c since this file contains common functions for x86 (vmx &
svm) and the function is Intel-specific.

Adrian Pop (2):
  x86/altp2m: Add a hvmop for setting the suppress #VE bit
  libxc: Add support for the altp2m suppress #VE hvmop

Vlad Ioan Topan (1):
  x86/mm: Change default value for suppress #VE in set_mem_access()

 tools/libxc/include/xenctrl.h   |  2 ++
 tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
 xen/arch/x86/mm/mem_access.c    | 51 +++++++++++++++++++++++++++++++++++++++--
 xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
 xen/include/xen/mem_access.h    |  3 +++
 6 files changed, 107 insertions(+), 2 deletions(-)

-- 
2.12.1


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

[PATCH 1/3] x86/mm: Change default value for suppress #VE in set_mem_access()

[Adrian Pop]

From: Vlad Ioan Topan <itopan@bitdefender.com>

The default value for the "suppress #VE" bit set by set_mem_access()
currently depends on whether the call is made from the same domain (the
bit is set when called from another domain and cleared if called from
the same domain). This patch changes that behavior to inherit the old
suppress #VE bit value if it is already set and to set it to 1
otherwise, which is safer and more reliable.

Signed-off-by: Vlad Ioan Topan <itopan@bitdefender.com>
Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
 xen/arch/x86/mm/mem_access.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index 5adaf6df90..d0b0767855 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -273,8 +273,7 @@ int p2m_set_altp2m_mem_access(struct domain *d, struct p2m_domain *hp2m,
         }
     }
 
-    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
-                         (current->domain != d));
+    return ap2m->set_entry(ap2m, gfn_l, mfn, PAGE_ORDER_4K, t, a, -1);
 }
 
 static int set_mem_access(struct domain *d, struct p2m_domain *p2m,
-- 
2.12.1


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

[PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
domain to change the value of the #VE suppress bit for a page.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
 xen/arch/x86/hvm/hvm.c          | 14 ++++++++++++
 xen/arch/x86/mm/mem_access.c    | 48 +++++++++++++++++++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h | 15 +++++++++++++
 xen/include/xen/mem_access.h    |  3 +++
 4 files changed, 80 insertions(+)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 2e76c2345b..eb01527c5b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4356,6 +4356,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_destroy_p2m:
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
+    case HVMOP_altp2m_set_suppress_ve:
     case HVMOP_altp2m_change_gfn:
         break;
     default:
@@ -4472,6 +4473,19 @@ static int do_altp2m_op(
                                     a.u.set_mem_access.view);
         break;
 
+    case HVMOP_altp2m_set_suppress_ve:
+        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
+            rc = -EINVAL;
+        else
+        {
+            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
+            unsigned int altp2m_idx = a.u.set_mem_access.view;
+            uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve;
+
+            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
+        }
+        break;
+
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
index d0b0767855..b9e611d3db 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
 }
 
 /*
+ * Set/clear the #VE suppress bit for a page.  Only available on VMX.
+ */
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
+                        unsigned int altp2m_idx)
+{
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m = NULL;
+    mfn_t mfn;
+    p2m_access_t a;
+    p2m_type_t t;
+    unsigned long gfn_l;
+    int rc = 0;
+
+    if ( !cpu_has_vmx )
+        return -EOPNOTSUPP;
+
+    if ( altp2m_idx > 0 )
+    {
+        if ( altp2m_idx >= MAX_ALTP2M ||
+                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
+            return -EINVAL;
+
+        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
+    }
+    else
+    {
+        p2m = host_p2m;
+    }
+
+    p2m_lock(host_p2m);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+    gfn_l = gfn_x(gfn);
+    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
+    if ( !mfn_valid(mfn) )
+        return -ESRCH;
+    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
+                        suppress_ve);
+    if ( ap2m )
+        p2m_unlock(ap2m);
+    p2m_unlock(host_p2m);
+
+    return rc;
+}
+
+/*
  * Local variables:
  * mode: C
  * c-file-style: "BSD"
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bc00ef0e65..9736092f58 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access {
 typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
 
+struct xen_hvm_altp2m_set_suppress_ve {
+    /* view */
+    uint16_t view;
+    uint8_t suppress_ve;
+    uint8_t pad1;
+    uint32_t pad2;
+    /* gfn */
+    uint64_t gfn;
+};
+typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
+
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
+/* Set the "Suppress #VE" bit on a page */
+#define HVMOP_altp2m_set_suppress_ve      9
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
         struct xen_hvm_altp2m_view               view;
         struct xen_hvm_altp2m_set_mem_access     set_mem_access;
+        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
         struct xen_hvm_altp2m_change_gfn         change_gfn;
         uint8_t pad[64];
     } u;
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 5ab34c1553..b6e6a7650a 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d,
  */
 int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
 
+int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
+                        unsigned int altp2m_idx);
+
 #ifdef CONFIG_HAS_MEM_ACCESS
 int mem_access_memop(unsigned long cmd,
                      XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
-- 
2.12.1


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

[PATCH 3/3] libxc: Add support for the altp2m suppress #VE hvmop

[Adrian Pop]

This adds a wrapper for issuing HVMOP_altp2m_set_suppress_ve from a
domain.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
---
 tools/libxc/include/xenctrl.h |  2 ++
 tools/libxc/xc_altp2m.c       | 24 ++++++++++++++++++++++++
 2 files changed, 26 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 2d97d36c38..5e1e4cfa81 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1923,6 +1923,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, domid_t domid,
 /* Switch all vCPUs of the domain to the specified altp2m view */
 int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
                              uint16_t view_id);
+int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
+                              uint16_t view_id, xen_pfn_t gfn, uint8_t sve);
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index 0639632477..b0f3e344af 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
     return rc;
 }
 
+int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
+                              uint16_t view_id, xen_pfn_t gfn, uint8_t sve)
+{
+    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;
+    arg->domain = domid;
+    arg->u.set_suppress_ve.view = view_id;
+    arg->u.set_suppress_ve.gfn = gfn;
+    arg->u.set_suppress_ve.suppress_ve = sve;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+		  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
+
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access)
-- 
2.12.1


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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Tamas K Lengyel]

On Thu, May 18, 2017 at 9:07 AM, Adrian Pop <apop@bitdefender.com> wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> domain to change the value of the #VE suppress bit for a page.
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> ---
>  xen/arch/x86/hvm/hvm.c          | 14 ++++++++++++
>  xen/arch/x86/mm/mem_access.c    | 48 +++++++++++++++++++++++++++++++++++++++++
>  xen/include/public/hvm/hvm_op.h | 15 +++++++++++++
>  xen/include/xen/mem_access.h    |  3 +++
>  4 files changed, 80 insertions(+)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 2e76c2345b..eb01527c5b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4356,6 +4356,7 @@ static int do_altp2m_op(
>      case HVMOP_altp2m_destroy_p2m:
>      case HVMOP_altp2m_switch_p2m:
>      case HVMOP_altp2m_set_mem_access:
> +    case HVMOP_altp2m_set_suppress_ve:
>      case HVMOP_altp2m_change_gfn:
>          break;
>      default:
> @@ -4472,6 +4473,19 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>
> +    case HVMOP_altp2m_set_suppress_ve:
> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> +            rc = -EINVAL;
> +        else
> +        {
> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
> +            uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve;
> +
> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> +        }
> +        break;
> +
>      case HVMOP_altp2m_change_gfn:
>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>              rc = -EINVAL;
> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> index d0b0767855..b9e611d3db 100644
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>  }
>
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m = NULL;
> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    unsigned long gfn_l;
> +    int rc = 0;
> +
> +    if ( !cpu_has_vmx )
> +        return -EOPNOTSUPP;
> +
> +    if ( altp2m_idx > 0 )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +    {
> +        p2m = host_p2m;
> +    }


IMHO there should be some further checks here to verify that the
domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it
was allowed (ie. this hypercall should not be able to enable the
suppress_bit if there is no veinfo_gfn). That said, is there anything
that would prevent a malicious application issuing rouge altp2m HVMOPs
from messing with this if it is activated (which I guess stands for
the rest of the altp2m ops too)?

> +
> +    p2m_lock(host_p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    gfn_l = gfn_x(gfn);
> +    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> +    if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> +                        suppress_ve);
> +    if ( ap2m )
> +        p2m_unlock(ap2m);
> +    p2m_unlock(host_p2m);
> +
> +    return rc;
> +}
> +
> +/*
>   * Local variables:
>   * mode: C
>   * c-file-style: "BSD"
> diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> index bc00ef0e65..9736092f58 100644
> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>
> +struct xen_hvm_altp2m_set_suppress_ve {
> +    /* view */
> +    uint16_t view;
> +    uint8_t suppress_ve;
> +    uint8_t pad1;
> +    uint32_t pad2;
> +    /* gfn */
> +    uint64_t gfn;
> +};
> +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
> +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
> +
>  struct xen_hvm_altp2m_change_gfn {
>      /* view */
>      uint16_t view;
> @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op {
>  #define HVMOP_altp2m_set_mem_access       7
>  /* Change a p2m entry to have a different gfn->mfn mapping */
>  #define HVMOP_altp2m_change_gfn           8
> +/* Set the "Suppress #VE" bit on a page */
> +#define HVMOP_altp2m_set_suppress_ve      9
>      domid_t domain;
>      uint16_t pad1;
>      uint32_t pad2;
> @@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op {
>          struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
>          struct xen_hvm_altp2m_view               view;
>          struct xen_hvm_altp2m_set_mem_access     set_mem_access;
> +        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
>          struct xen_hvm_altp2m_change_gfn         change_gfn;
>          uint8_t pad[64];
>      } u;
> diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
> index 5ab34c1553..b6e6a7650a 100644
> --- a/xen/include/xen/mem_access.h
> +++ b/xen/include/xen/mem_access.h
> @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d,
>   */
>  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
>
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> +                        unsigned int altp2m_idx);
> +
>  #ifdef CONFIG_HAS_MEM_ACCESS
>  int mem_access_memop(unsigned long cmd,
>                       XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
> --
> 2.12.1
>
>
> _______________________________________________
> Xen-devel mailing list
> Xen-devel@lists.xen.org
> https://lists.xen.org/xen-devel

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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

On Thu, May 18, 2017 at 11:27:44AM -0600, Tamas K Lengyel wrote:
> On Thu, May 18, 2017 at 9:07 AM, Adrian Pop <apop@bitdefender.com> wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > domain to change the value of the #VE suppress bit for a page.
> >
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > ---
> >  xen/arch/x86/hvm/hvm.c          | 14 ++++++++++++
> >  xen/arch/x86/mm/mem_access.c    | 48 +++++++++++++++++++++++++++++++++++++++++
> >  xen/include/public/hvm/hvm_op.h | 15 +++++++++++++
> >  xen/include/xen/mem_access.h    |  3 +++
> >  4 files changed, 80 insertions(+)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 2e76c2345b..eb01527c5b 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -4356,6 +4356,7 @@ static int do_altp2m_op(
> >      case HVMOP_altp2m_destroy_p2m:
> >      case HVMOP_altp2m_switch_p2m:
> >      case HVMOP_altp2m_set_mem_access:
> > +    case HVMOP_altp2m_set_suppress_ve:
> >      case HVMOP_altp2m_change_gfn:
> >          break;
> >      default:
> > @@ -4472,6 +4473,19 @@ static int do_altp2m_op(
> >                                      a.u.set_mem_access.view);
> >          break;
> >
> > +    case HVMOP_altp2m_set_suppress_ve:
> > +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
> > +            rc = -EINVAL;
> > +        else
> > +        {
> > +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
> > +            unsigned int altp2m_idx = a.u.set_mem_access.view;
> > +            uint8_t suppress_ve = a.u.set_suppress_ve.suppress_ve;
> > +
> > +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
> > +        }
> > +        break;
> > +
> >      case HVMOP_altp2m_change_gfn:
> >          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
> >              rc = -EINVAL;
> > diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
> > index d0b0767855..b9e611d3db 100644
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >  }
> >
> >  /*
> > + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> > + */
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> > +                        unsigned int altp2m_idx)
> > +{
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m = NULL;
> > +    mfn_t mfn;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    unsigned long gfn_l;
> > +    int rc = 0;
> > +
> > +    if ( !cpu_has_vmx )
> > +        return -EOPNOTSUPP;
> > +
> > +    if ( altp2m_idx > 0 )
> > +    {
> > +        if ( altp2m_idx >= MAX_ALTP2M ||
> > +                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> > +            return -EINVAL;
> > +
> > +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> > +    }
> > +    else
> > +    {
> > +        p2m = host_p2m;
> > +    }
> 
> 
> IMHO there should be some further checks here to verify that the
> domain has issued HVMOP_altp2m_vcpu_enable_notify before and that it
> was allowed (ie. this hypercall should not be able to enable the
> suppress_bit if there is no veinfo_gfn). That said, is there anything

Ok, a check can be added easily.  The reasoning behind not adding it in
the first place was that should the #VE be disabled in the VM's VMCS,
setting/clearing the suppress #VE bit would do nothing (it is ignored).

> that would prevent a malicious application issuing rouge altp2m HVMOPs
> from messing with this if it is activated (which I guess stands for
> the rest of the altp2m ops too)?

AFAIK there isn't any safeguard of this sort.  I might just be
excessively ignorant, though.  On the other hand the current default
behavior is to enable #VE for all the pages.  The default with these
patches would be to issue a VM-Exit, and either a SVA, the Dom0 or the
target DomU itself could modify this behavior to generate #VE instead of
VM-Exit.  In any case I'll investigate some more.

Thanks!

> > +
> > +    p2m_lock(host_p2m);
> > +    if ( ap2m )
> > +        p2m_lock(ap2m);
> > +
> > +    gfn_l = gfn_x(gfn);
> > +    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> > +    if ( !mfn_valid(mfn) )
> > +        return -ESRCH;
> > +    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> > +                        suppress_ve);
> > +    if ( ap2m )
> > +        p2m_unlock(ap2m);
> > +    p2m_unlock(host_p2m);
> > +
> > +    return rc;
> > +}
> > +
> > +/*
> >   * Local variables:
> >   * mode: C
> >   * c-file-style: "BSD"
> > diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
> > index bc00ef0e65..9736092f58 100644
> > --- a/xen/include/public/hvm/hvm_op.h
> > +++ b/xen/include/public/hvm/hvm_op.h
> > @@ -231,6 +231,18 @@ struct xen_hvm_altp2m_set_mem_access {
> >  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
> >  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
> >
> > +struct xen_hvm_altp2m_set_suppress_ve {
> > +    /* view */
> > +    uint16_t view;
> > +    uint8_t suppress_ve;
> > +    uint8_t pad1;
> > +    uint32_t pad2;
> > +    /* gfn */
> > +    uint64_t gfn;
> > +};
> > +typedef struct xen_hvm_altp2m_set_suppress_ve xen_hvm_altp2m_set_suppress_ve_t;
> > +DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_suppress_ve_t);
> > +
> >  struct xen_hvm_altp2m_change_gfn {
> >      /* view */
> >      uint16_t view;
> > @@ -262,6 +274,8 @@ struct xen_hvm_altp2m_op {
> >  #define HVMOP_altp2m_set_mem_access       7
> >  /* Change a p2m entry to have a different gfn->mfn mapping */
> >  #define HVMOP_altp2m_change_gfn           8
> > +/* Set the "Suppress #VE" bit on a page */
> > +#define HVMOP_altp2m_set_suppress_ve      9
> >      domid_t domain;
> >      uint16_t pad1;
> >      uint32_t pad2;
> > @@ -270,6 +284,7 @@ struct xen_hvm_altp2m_op {
> >          struct xen_hvm_altp2m_vcpu_enable_notify enable_notify;
> >          struct xen_hvm_altp2m_view               view;
> >          struct xen_hvm_altp2m_set_mem_access     set_mem_access;
> > +        struct xen_hvm_altp2m_set_suppress_ve    set_suppress_ve;
> >          struct xen_hvm_altp2m_change_gfn         change_gfn;
> >          uint8_t pad[64];
> >      } u;
> > diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
> > index 5ab34c1553..b6e6a7650a 100644
> > --- a/xen/include/xen/mem_access.h
> > +++ b/xen/include/xen/mem_access.h
> > @@ -78,6 +78,9 @@ long p2m_set_mem_access_multi(struct domain *d,
> >   */
> >  int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
> >
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> > +                        unsigned int altp2m_idx);
> > +
> >  #ifdef CONFIG_HAS_MEM_ACCESS
> >  int mem_access_memop(unsigned long cmd,
> >                       XEN_GUEST_HANDLE_PARAM(xen_mem_access_op_t) arg);
> > --
> > 2.12.1
> >
> >
> > _______________________________________________
> > Xen-devel mailing list
> > Xen-devel@lists.xen.org
> > https://lists.xen.org/xen-devel

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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Jan Beulich]

>>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
>  }
>  
>  /*
> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> + */
> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,

suppress_ve presumably is meant to be boolean.

> +                        unsigned int altp2m_idx)
> +{
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m = NULL;

Pointless initializer.

> +    mfn_t mfn;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    unsigned long gfn_l;

Please avoid this local variable and use gfn_x() in the two places
you need to.

> +    int rc = 0;

Pointless initializer again.

> +
> +    if ( !cpu_has_vmx )
> +        return -EOPNOTSUPP;

Is this enough? Wouldn't it be better to signal the caller whenever
hardware (or even software) isn't going to honor the request?

> +    if ( altp2m_idx > 0 )
> +    {
> +        if ( altp2m_idx >= MAX_ALTP2M ||
> +                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )

Indentation.

> +            return -EINVAL;
> +
> +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> +    }
> +    else
> +    {
> +        p2m = host_p2m;
> +    }

Unnecessary braces.

> +    p2m_lock(host_p2m);
> +    if ( ap2m )
> +        p2m_lock(ap2m);
> +
> +    gfn_l = gfn_x(gfn);
> +    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> +    if ( !mfn_valid(mfn) )
> +        return -ESRCH;
> +    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> +                        suppress_ve);
> +    if ( ap2m )
> +        p2m_unlock(ap2m);
> +    p2m_unlock(host_p2m);

To fiddle with a single gfn, this looks to be very heavy locking.
While for now gfn_lock() is the same as p2m_lock(), from an
abstract perspective I'd expect gfn_lock() to suffice here at 
least in the non-altp2m case.

And then there are two general questions: Without a libxc layer
function, how is one supposed to use this new sub-op? Is it
really intended to permit a guest to call this for itself?

Jan


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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

Hello,

On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -466,6 +466,54 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> >  }
> >  
> >  /*
> > + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
> > + */
> > +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, uint8_t suppress_ve,
> 
> suppress_ve presumably is meant to be boolean.

Yes.  It can be changed to bool.

> > +                        unsigned int altp2m_idx)
> > +{
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m = NULL;
> 
> Pointless initializer.

Ok.

> > +    mfn_t mfn;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    unsigned long gfn_l;
> 
> Please avoid this local variable and use gfn_x() in the two places
> you need to.

Sure.

> > +    int rc = 0;
> 
> Pointless initializer again.
 
Right.

> > +
> > +    if ( !cpu_has_vmx )
> > +        return -EOPNOTSUPP;
> 
> Is this enough? Wouldn't it be better to signal the caller whenever
> hardware (or even software) isn't going to honor the request?

Well, the caller checks the return value.  The libxc function
xc_altp2m_set_suppress_ve for instance will return a negative in this
case.


> > +    if ( altp2m_idx > 0 )
> > +    {
> > +        if ( altp2m_idx >= MAX_ALTP2M ||
> > +                d->arch.altp2m_eptp[altp2m_idx] == mfn_x(INVALID_MFN) )
> 
> Indentation.

Ok.

> > +            return -EINVAL;
> > +
> > +        p2m = ap2m = d->arch.altp2m_p2m[altp2m_idx];
> > +    }
> > +    else
> > +    {
> > +        p2m = host_p2m;
> > +    }
> 
> Unnecessary braces.
 
Ok.

> > +    p2m_lock(host_p2m);
> > +    if ( ap2m )
> > +        p2m_lock(ap2m);
> > +
> > +    gfn_l = gfn_x(gfn);
> > +    mfn = p2m->get_entry(p2m, gfn_l, &t, &a, 0, NULL, NULL);
> > +    if ( !mfn_valid(mfn) )
> > +        return -ESRCH;
> > +    rc = p2m->set_entry(p2m, gfn_l, mfn, PAGE_ORDER_4K, t, a,
> > +                        suppress_ve);
> > +    if ( ap2m )
> > +        p2m_unlock(ap2m);
> > +    p2m_unlock(host_p2m);
> 
> To fiddle with a single gfn, this looks to be very heavy locking.
> While for now gfn_lock() is the same as p2m_lock(), from an
> abstract perspective I'd expect gfn_lock() to suffice here at 
> least in the non-altp2m case.
 
Ok.

> And then there are two general questions: Without a libxc layer
> function, how is one supposed to use this new sub-op? Is it
> really intended to permit a guest to call this for itself?
 
Well, the sub-op could be used from a Linux kernel module if libxc is
not available if struct xen_hvm_altp2m_op and struct
xen_hvm_altp2m_set_suppress_ve are defined.

Our use case, though, involves either Dom0 or a "privileged" DomU
altering the suppress #VE bit for the target guest.

> Jan
> 

Thanks!

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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Jan Beulich]

>>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote:
> On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
>> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
>> > +
>> > +    if ( !cpu_has_vmx )
>> > +        return -EOPNOTSUPP;
>> 
>> Is this enough? Wouldn't it be better to signal the caller whenever
>> hardware (or even software) isn't going to honor the request?
> 
> Well, the caller checks the return value.  The libxc function
> xc_altp2m_set_suppress_ve for instance will return a negative in this
> case.

The question wasn't what the caller does but whether checking just
cpu_has_vmx is enough. What if you're running on a machine with
VMX but no #VE support?

>> And then there are two general questions: Without a libxc layer
>> function, how is one supposed to use this new sub-op? Is it
>> really intended to permit a guest to call this for itself?
>  
> Well, the sub-op could be used from a Linux kernel module if libxc is
> not available if struct xen_hvm_altp2m_op and struct
> xen_hvm_altp2m_set_suppress_ve are defined.
> 
> Our use case, though, involves either Dom0 or a "privileged" DomU
> altering the suppress #VE bit for the target guest.

This doesn't really answer the question: What are the security
implications if a guest can invoke this on itself?

(FTR I think my first question was kind of pointless, as patch 3
looks like it does introduce a libxc function; I simply didn't realize
that back then, because I'd generally have expected the
consumer of the hypercall to be introduce together with the
producer.)

Jan


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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote:
> >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote:
> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
> >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
> >> > +
> >> > +    if ( !cpu_has_vmx )
> >> > +        return -EOPNOTSUPP;
> >> 
> >> Is this enough? Wouldn't it be better to signal the caller whenever
> >> hardware (or even software) isn't going to honor the request?
> > 
> > Well, the caller checks the return value.  The libxc function
> > xc_altp2m_set_suppress_ve for instance will return a negative in this
> > case.
> 
> The question wasn't what the caller does but whether checking just
> cpu_has_vmx is enough. What if you're running on a machine with
> VMX but no #VE support?

Oh, all right.  I misinterpreted it.  Yes, at least using something like
cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be
more appropriate in this case.  Do you think there should be a more
thorough check?

> >> And then there are two general questions: Without a libxc layer
> >> function, how is one supposed to use this new sub-op? Is it
> >> really intended to permit a guest to call this for itself?
> >  
> > Well, the sub-op could be used from a Linux kernel module if libxc is
> > not available if struct xen_hvm_altp2m_op and struct
> > xen_hvm_altp2m_set_suppress_ve are defined.
> > 
> > Our use case, though, involves either Dom0 or a "privileged" DomU
> > altering the suppress #VE bit for the target guest.
> 
> This doesn't really answer the question: What are the security
> implications if a guest can invoke this on itself?

Indeed it would be desirable that the guest doesn't use this hvmop on
itself.  It's also less than ideal that a DomU can call this on other
DomUs.

After some talks it turns out that restricting this hvmop to a
privileged domain solves this issue and still works for our use case.
What do you think?

> (FTR I think my first question was kind of pointless, as patch 3
> looks like it does introduce a libxc function; I simply didn't realize
> that back then, because I'd generally have expected the
> consumer of the hypercall to be introduce together with the
> producer.)

I can merge these two patches for v2 if that's what you want.

> Jan

Thank you!

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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Jan Beulich]

>>> On 08.06.17 at 15:49, <apop@bitdefender.com> wrote:
> On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote:
>> >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote:
>> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
>> >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
>> >> > +
>> >> > +    if ( !cpu_has_vmx )
>> >> > +        return -EOPNOTSUPP;
>> >> 
>> >> Is this enough? Wouldn't it be better to signal the caller whenever
>> >> hardware (or even software) isn't going to honor the request?
>> > 
>> > Well, the caller checks the return value.  The libxc function
>> > xc_altp2m_set_suppress_ve for instance will return a negative in this
>> > case.
>> 
>> The question wasn't what the caller does but whether checking just
>> cpu_has_vmx is enough. What if you're running on a machine with
>> VMX but no #VE support?
> 
> Oh, all right.  I misinterpreted it.  Yes, at least using something like
> cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be
> more appropriate in this case.  Do you think there should be a more
> thorough check?

Depends on what "more thorough" means: You'll want to check all
features the code requires; I'm not certain if virt_exceptions is all
it needs.

>> >> And then there are two general questions: Without a libxc layer
>> >> function, how is one supposed to use this new sub-op? Is it
>> >> really intended to permit a guest to call this for itself?
>> >  
>> > Well, the sub-op could be used from a Linux kernel module if libxc is
>> > not available if struct xen_hvm_altp2m_op and struct
>> > xen_hvm_altp2m_set_suppress_ve are defined.
>> > 
>> > Our use case, though, involves either Dom0 or a "privileged" DomU
>> > altering the suppress #VE bit for the target guest.
>> 
>> This doesn't really answer the question: What are the security
>> implications if a guest can invoke this on itself?
> 
> Indeed it would be desirable that the guest doesn't use this hvmop on
> itself.  It's also less than ideal that a DomU can call this on other
> DomUs.

The latter is an absolute no-go.

> After some talks it turns out that restricting this hvmop to a
> privileged domain solves this issue and still works for our use case.
> What do you think?

Restrictions should generally be put in place because of
abstract considerations, not because of them not harming
one's particular use case.

>> (FTR I think my first question was kind of pointless, as patch 3
>> looks like it does introduce a libxc function; I simply didn't realize
>> that back then, because I'd generally have expected the
>> consumer of the hypercall to be introduce together with the
>> producer.)
> 
> I can merge these two patches for v2 if that's what you want.

I'd prefer that, but others may have differing opinions. And
there are certainly benefits in keeping hypervisor and tools
changes separate.

Jan


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

Re: [PATCH 2/3] x86/altp2m: Add a hvmop for setting the suppress #VE bit

[Adrian Pop]

On Thu, Jun 08, 2017 at 08:08:56AM -0600, Jan Beulich wrote:
> >>> On 08.06.17 at 15:49, <apop@bitdefender.com> wrote:
> > On Tue, Jun 06, 2017 at 07:08:43AM -0600, Jan Beulich wrote:
> >> >>> On 06.06.17 at 15:00, <apop@bitdefender.com> wrote:
> >> > On Mon, May 29, 2017 at 08:38:33AM -0600, Jan Beulich wrote:
> >> >> >>> On 18.05.17 at 17:07, <apop@bitdefender.com> wrote:
> >> >> > +
> >> >> > +    if ( !cpu_has_vmx )
> >> >> > +        return -EOPNOTSUPP;
> >> >> 
> >> >> Is this enough? Wouldn't it be better to signal the caller whenever
> >> >> hardware (or even software) isn't going to honor the request?
> >> > 
> >> > Well, the caller checks the return value.  The libxc function
> >> > xc_altp2m_set_suppress_ve for instance will return a negative in this
> >> > case.
> >> 
> >> The question wasn't what the caller does but whether checking just
> >> cpu_has_vmx is enough. What if you're running on a machine with
> >> VMX but no #VE support?
> > 
> > Oh, all right.  I misinterpreted it.  Yes, at least using something like
> > cpu_has_vmx_virt_exceptions instead of cpu_has_vmx would definitely be
> > more appropriate in this case.  Do you think there should be a more
> > thorough check?
> 
> Depends on what "more thorough" means: You'll want to check all
> features the code requires; I'm not certain if virt_exceptions is all
> it needs.
 
The checks so far would be:
- is the domain invoking this hvmop privileged?
- does the cpu have the #VE feature?
- is #VE enabled on this vcpu?

> >> >> And then there are two general questions: Without a libxc layer
> >> >> function, how is one supposed to use this new sub-op? Is it
> >> >> really intended to permit a guest to call this for itself?
> >> >  
> >> > Well, the sub-op could be used from a Linux kernel module if libxc is
> >> > not available if struct xen_hvm_altp2m_op and struct
> >> > xen_hvm_altp2m_set_suppress_ve are defined.
> >> > 
> >> > Our use case, though, involves either Dom0 or a "privileged" DomU
> >> > altering the suppress #VE bit for the target guest.
> >> 
> >> This doesn't really answer the question: What are the security
> >> implications if a guest can invoke this on itself?
> > 
> > Indeed it would be desirable that the guest doesn't use this hvmop on
> > itself.  It's also less than ideal that a DomU can call this on other
> > DomUs.
> 
> The latter is an absolute no-go.

Indeed.

> > After some talks it turns out that restricting this hvmop to a
> > privileged domain solves this issue and still works for our use case.
> > What do you think?
> 
> Restrictions should generally be put in place because of
> abstract considerations, not because of them not harming
> one's particular use case.

Of course.

> >> (FTR I think my first question was kind of pointless, as patch 3
> >> looks like it does introduce a libxc function; I simply didn't realize
> >> that back then, because I'd generally have expected the
> >> consumer of the hypercall to be introduce together with the
> >> producer.)
> > 
> > I can merge these two patches for v2 if that's what you want.
> 
> I'd prefer that, but others may have differing opinions. And
> there are certainly benefits in keeping hypervisor and tools
> changes separate.
 
Ok then.

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