[PATCH v3 0/2] Add a hvmop for setting the #VE suppress bit

[PATCH v3 0/2] 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.

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

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    | 56 +++++++++++++++++++++++++++++++++++++++--
 xen/include/public/hvm/hvm_op.h | 11 ++++++++
 xen/include/xen/mem_access.h    |  3 +++
 6 files changed, 108 insertions(+), 2 deletions(-)

-- 
2.13.0


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

[PATCH v3 1/2] 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.13.0


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

[PATCH v3 2/2] 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
privileged domain to change the value of the #VE suppress bit for a
page.

Add a libxc wrapper for invoking this hvmop.

Signed-off-by: Adrian Pop <apop@bitdefender.com>
Acked-by: Wei Liu <wei.liu2@citrix.com>
---
changes in v3:
- fix indentation (Wei Liu)
- use return values other than EINVAL where appropriate (Ian Beulich)
- remove the irrelevant comments from the
  xen_hvm_altp2m_set_suppress_ve struct (Ian Beulich)
- add comment for the suppress_ve field in the struct above (Ian
  Beulich)
- remove the typedef and DEFINE_XEN_GUEST_HANDLE for
  xen_hvm_altp2m_set_suppress_ve (Ian Beulich)
- use XSM_DM_PRIV check instead of domain->is_privileged (Ian Beulich)

changes in v2:
- check if #VE has been enabled on the target domain (Tamas K Lengyel)
- check if the cpu has the #VE feature
- make the suppress_ve argument boolean (Jan Beulich)
- initialize only local variables that need initializing (Jan Beulich)
- use fewer local variables (Jan Beulich)
- fix indentation (Jan Beulich)
- remove unnecessary braces (Jan Beulich)
- use gfn_lock() instead of p2m_lock() in the non-altp2m case (Jan
  Beulich)
- allow only privileged domains to use this hvmop
- merge patch #2 and patch #3 (Jan Beulich)
---
 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    | 53 +++++++++++++++++++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h | 11 +++++++++
 xen/include/xen/mem_access.h    |  3 +++
 6 files changed, 107 insertions(+)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 552a4fd47d..ea985696e4 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1938,6 +1938,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, bool 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..26e3a56fb1 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, bool 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)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 8145385747..b7ea945f78 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4413,6 +4413,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:
@@ -4530,6 +4531,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;
+            bool 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..87a92a632b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -29,6 +29,7 @@
 #include <asm/p2m.h>
 #include <asm/altp2m.h>
 #include <asm/vm_event.h>
+#include <xsm/xsm.h>
 
 #include "mm-locks.h"
 
@@ -466,6 +467,58 @@ 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, bool suppress_ve,
+                        unsigned int altp2m_idx)
+{
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m;
+    mfn_t mfn;
+    p2m_access_t a;
+    p2m_type_t t;
+    int rc;
+
+    if ( !cpu_has_vmx_virt_exceptions )
+        return -EOPNOTSUPP;
+
+    /* This subop should only be used from a privileged domain. */
+    if ( xsm_dm_op(XSM_DM_PRIV, d) )
+        return -EPERM;
+
+    /* #VE should be enabled for this vcpu. */
+    if ( gfn_eq(vcpu_altp2m(current).veinfo_gfn, INVALID_GFN) )
+        return -ENXIO;
+
+    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;
+
+    gfn_lock(host_p2m, gfn, 0);
+    if ( ap2m )
+        p2m_lock(ap2m);
+
+    mfn = p2m->get_entry(p2m, gfn_x(gfn), &t, &a, 0, NULL, NULL);
+    if ( !mfn_valid(mfn) )
+        return -ESRCH;
+    rc = p2m->set_entry(p2m, gfn_x(gfn), mfn, PAGE_ORDER_4K, t, a,
+                        suppress_ve);
+    if ( ap2m )
+        p2m_unlock(ap2m);
+    gfn_unlock(host_p2m, gfn, 0);
+
+    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 0bdafdf59a..16983a8f4e 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -237,6 +237,14 @@ 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 {
+    uint16_t view;
+    uint8_t suppress_ve; /* Boolean type. */
+    uint8_t pad1;
+    uint32_t pad2;
+    uint64_t gfn;
+};
+
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -268,6 +276,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;
@@ -276,6 +286,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..0c6717d80f 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, bool 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.13.0


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

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

[Tamas K Lengyel]

On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote:
> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> privileged domain to change the value of the #VE suppress bit for a
> page.
>
> Add a libxc wrapper for invoking this hvmop.
>
> Signed-off-by: Adrian Pop <apop@bitdefender.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

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

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

[Tamas K Lengyel]

On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote:
> 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.

With the way things are currently if the in-guest tool calls
set_mem_access for an altp2m view, it implies it wants to receive #VE
for it. Wouldn't this change in this patch effectively make it
impossible for an in-guest tool to decide which pages it wants to
receive #VE for? The new HVMOP you are introducing is only accessible
from a privileged domain..

Tamas

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

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

[Adrian Pop]

Hello,

On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote:
> On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote:
> > 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.
> 
> With the way things are currently if the in-guest tool calls
> set_mem_access for an altp2m view, it implies it wants to receive #VE
> for it. Wouldn't this change in this patch effectively make it
> impossible for an in-guest tool to decide which pages it wants to
> receive #VE for? The new HVMOP you are introducing is only accessible
> from a privileged domain..

Yes, this change, along with the restrictions from the new HVMOP would
virtually prevent a guest from changing the suppress #VE bit for its
pages.  The current set_mem_access functionality, if I'm not mistaken,
is a bit odd since the guest can only clear the sve, but to set it,
another domain would have to call set_mem_access for it.

I think the issue would be whether to allow a domain to set/clear the
suppress #VE bit for its pages by calling the new HVMOP on itself.

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

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

[Adrian Pop]

On Tue, Jul 18, 2017 at 11:19:07AM -0600, Tamas K Lengyel wrote:
> On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote:
> > Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
> > privileged domain to change the value of the #VE suppress bit for a
> > page.
> >
> > Add a libxc wrapper for invoking this hvmop.
> >
> > Signed-off-by: Adrian Pop <apop@bitdefender.com>
> > Acked-by: Wei Liu <wei.liu2@citrix.com>
> 
> Acked-by: Tamas K Lengyel <tamas@tklengyel.com>

Thanks!

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

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

[Tamas K Lengyel]

On Wed, Jul 19, 2017 at 5:47 AM, Adrian Pop <apop@bitdefender.com> wrote:
> Hello,
>
> On Tue, Jul 18, 2017 at 11:26:45AM -0600, Tamas K Lengyel wrote:
>> On Tue, Jul 18, 2017 at 9:25 AM, Adrian Pop <apop@bitdefender.com> wrote:
>> > 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.
>>
>> With the way things are currently if the in-guest tool calls
>> set_mem_access for an altp2m view, it implies it wants to receive #VE
>> for it. Wouldn't this change in this patch effectively make it
>> impossible for an in-guest tool to decide which pages it wants to
>> receive #VE for? The new HVMOP you are introducing is only accessible
>> from a privileged domain..
>
> Yes, this change, along with the restrictions from the new HVMOP would
> virtually prevent a guest from changing the suppress #VE bit for its
> pages. The current set_mem_access functionality, if I'm not mistaken,
> is a bit odd since the guest can only clear the sve, but to set it,
> another domain would have to call set_mem_access for it.

Stating that change explicitly in the patch message would have been
something I would want to see.

Calling set_mem_access from the guest itself by design clears the SVE
bit, which makes sense. The in-guest tool doesn't know whether there
is an external mem_access listener, so the only thing it should be
allowed to do is to signal to the hypervisor that when it changes EPT
permissions, violations on those pages need to be injected into the
guest with #VE. If you don't want to allow a domain to make changes
like that, you need to restrict altp2m ops to be issued from the
domain completely.

>
> I think the issue would be whether to allow a domain to set/clear the
> suppress #VE bit for its pages by calling the new HVMOP on itself.

This problem is not limited to setting the SVE bit. It also applies to
swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
from a user-space program without any way to check whether that
process is allowed to do that or not. If you don't think it is safe
for a domain to set SVE, the none of the altp2m ops are safe for the
domain to issue on itself. If we could say ensure only the kernel can
issue the hvmops, that would be OK. But that's not possible at the
moment AFAICT.

Tamas

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

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

[George Dunlap]

On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>> I think the issue would be whether to allow a domain to set/clear the
>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>
> This problem is not limited to setting the SVE bit. It also applies to
> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
> from a user-space program without any way to check whether that
> process is allowed to do that or not. If you don't think it is safe
> for a domain to set SVE, the none of the altp2m ops are safe for the
> domain to issue on itself. If we could say ensure only the kernel can
> issue the hvmops, that would be OK. But that's not possible at the
> moment AFAICT.

Wait, is that right?  I think we normally restrict hypercalls to only
being made from the guest kernel, don't we?

 -George

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
<George.Dunlap@eu.citrix.com> wrote:
> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>> I think the issue would be whether to allow a domain to set/clear the
>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>
>> This problem is not limited to setting the SVE bit. It also applies to
>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>> from a user-space program without any way to check whether that
>> process is allowed to do that or not. If you don't think it is safe
>> for a domain to set SVE, the none of the altp2m ops are safe for the
>> domain to issue on itself. If we could say ensure only the kernel can
>> issue the hvmops, that would be OK. But that's not possible at the
>> moment AFAICT.
>
> Wait, is that right?  I think we normally restrict hypercalls to only
> being made from the guest kernel, don't we?
>

If that's the case then it's good to know (can you point me where that
restriction is done?) I was just referring to the fact that
technically a userspace program can issue VMCALL.

Tamas

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

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

[George Dunlap]

On 07/20/2017 05:46 PM, Tamas K Lengyel wrote:
> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>> I think the issue would be whether to allow a domain to set/clear the
>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>
>>> This problem is not limited to setting the SVE bit. It also applies to
>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>> from a user-space program without any way to check whether that
>>> process is allowed to do that or not. If you don't think it is safe
>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>> domain to issue on itself. If we could say ensure only the kernel can
>>> issue the hvmops, that would be OK. But that's not possible at the
>>> moment AFAICT.
>>
>> Wait, is that right?  I think we normally restrict hypercalls to only
>> being made from the guest kernel, don't we?
>>
> 
> If that's the case then it's good to know (can you point me where that
> restriction is done?) I was just referring to the fact that
> technically a userspace program can issue VMCALL.

Well for vmcall in particular, it's in
xen/arch/x86/hvm/hypercall/hvm_hypercall().  The check for PV guests is
in xen/arch/x86/x86_64/entry.S:lstar_enter.  Other checks are left as an
exercise for the reader. :-)

 -George

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap
<george.dunlap@citrix.com> wrote:
> On 07/20/2017 05:46 PM, Tamas K Lengyel wrote:
>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>> <George.Dunlap@eu.citrix.com> wrote:
>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>
>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>> from a user-space program without any way to check whether that
>>>> process is allowed to do that or not. If you don't think it is safe
>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>> moment AFAICT.
>>>
>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>> being made from the guest kernel, don't we?
>>>
>>
>> If that's the case then it's good to know (can you point me where that
>> restriction is done?) I was just referring to the fact that
>> technically a userspace program can issue VMCALL.
>
> Well for vmcall in particular, it's in
> xen/arch/x86/hvm/hypercall/hvm_hypercall().  The check for PV guests is
> in xen/arch/x86/x86_64/entry.S:lstar_enter.  Other checks are left as an
> exercise for the reader. :-)

Thanks ;) I'm looking through it.

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 11:03 AM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
> On Thu, Jul 20, 2017 at 10:57 AM, George Dunlap
> <george.dunlap@citrix.com> wrote:
>> On 07/20/2017 05:46 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>>> <George.Dunlap@eu.citrix.com> wrote:
>>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>>
>>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>>> from a user-space program without any way to check whether that
>>>>> process is allowed to do that or not. If you don't think it is safe
>>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>>> moment AFAICT.
>>>>
>>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>>> being made from the guest kernel, don't we?
>>>>
>>>
>>> If that's the case then it's good to know (can you point me where that
>>> restriction is done?) I was just referring to the fact that
>>> technically a userspace program can issue VMCALL.
>>
>> Well for vmcall in particular, it's in
>> xen/arch/x86/hvm/hypercall/hvm_hypercall().  The check for PV guests is
>> in xen/arch/x86/x86_64/entry.S:lstar_enter.  Other checks are left as an
>> exercise for the reader. :-)
>
> Thanks ;) I'm looking through it.

All checks out, we can ignore my concerns above. (Some comments around
vmx_get_cpl would have been helpful, took me a bit to find in the
manual why the bitshift is needed)

Tamas

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

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

[Razvan Cojocaru]

On 07/20/2017 07:46 PM, Tamas K Lengyel wrote:
> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
> <George.Dunlap@eu.citrix.com> wrote:
>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>> I think the issue would be whether to allow a domain to set/clear the
>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>
>>> This problem is not limited to setting the SVE bit. It also applies to
>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>> from a user-space program without any way to check whether that
>>> process is allowed to do that or not. If you don't think it is safe
>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>> domain to issue on itself. If we could say ensure only the kernel can
>>> issue the hvmops, that would be OK. But that's not possible at the
>>> moment AFAICT.
>>
>> Wait, is that right?  I think we normally restrict hypercalls to only
>> being made from the guest kernel, don't we?
>>
> 
> If that's the case then it's good to know (can you point me where that
> restriction is done?) I was just referring to the fact that
> technically a userspace program can issue VMCALL.

It is the case AFAIK. We had to do this trick to allow guest request
hypercalls coming from guest userspace:

https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch

By default they can only come from the guest kernel.


Thanks,
Razvan

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

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

[Tamas K Lengyel]

On Thu, Jul 20, 2017 at 12:25 PM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 07/20/2017 07:46 PM, Tamas K Lengyel wrote:
>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>> <George.Dunlap@eu.citrix.com> wrote:
>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>
>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>> from a user-space program without any way to check whether that
>>>> process is allowed to do that or not. If you don't think it is safe
>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>> moment AFAICT.
>>>
>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>> being made from the guest kernel, don't we?
>>>
>>
>> If that's the case then it's good to know (can you point me where that
>> restriction is done?) I was just referring to the fact that
>> technically a userspace program can issue VMCALL.
>
> It is the case AFAIK. We had to do this trick to allow guest request
> hypercalls coming from guest userspace:
>
> https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch
>
> By default they can only come from the guest kernel.

Makes sense. Any reason not to have that patch upstreamed?

Tamas

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

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

[Razvan Cojocaru]

On 07/20/2017 09:52 PM, Tamas K Lengyel wrote:
> On Thu, Jul 20, 2017 at 12:25 PM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 07/20/2017 07:46 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 20, 2017 at 10:43 AM, George Dunlap
>>> <George.Dunlap@eu.citrix.com> wrote:
>>>> On Wed, Jul 19, 2017 at 7:24 PM, Tamas K Lengyel <tamas@tklengyel.com> wrote:
>>>>>> I think the issue would be whether to allow a domain to set/clear the
>>>>>> suppress #VE bit for its pages by calling the new HVMOP on itself.
>>>>>
>>>>> This problem is not limited to setting the SVE bit. It also applies to
>>>>> swapping altp2m views. Pretty much all altp2m HVMOPs can be issued
>>>>> from a user-space program without any way to check whether that
>>>>> process is allowed to do that or not. If you don't think it is safe
>>>>> for a domain to set SVE, the none of the altp2m ops are safe for the
>>>>> domain to issue on itself. If we could say ensure only the kernel can
>>>>> issue the hvmops, that would be OK. But that's not possible at the
>>>>> moment AFAICT.
>>>>
>>>> Wait, is that right?  I think we normally restrict hypercalls to only
>>>> being made from the guest kernel, don't we?
>>>>
>>>
>>> If that's the case then it's good to know (can you point me where that
>>> restriction is done?) I was just referring to the fact that
>>> technically a userspace program can issue VMCALL.
>>
>> It is the case AFAIK. We had to do this trick to allow guest request
>> hypercalls coming from guest userspace:
>>
>> https://github.com/xenserver/xen-4.7.pg/blob/master/master/xen-x86-hvm-Allow-guest_request-vm_events-coming-from-us.patch
>>
>> By default they can only come from the guest kernel.
> 
> Makes sense. Any reason not to have that patch upstreamed?

It's been rejected: https://patchwork.kernel.org/patch/9264837/

We need that because we inject a cleanup tool in the guest once a threat
is detected, and we want to be able to know what it is doing. So every
once in a while, the in-guest tool will do a VMCALL that ends up being
an event letting the dom0 application know what it's been up to (a
communication protocol if you will).

We'd be more than happy to retry upstreaming it if I've managed to
explain the need for it better today. :)


Thanks,
Razvan

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