[PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Adrian Pop]

A new function, p2m_access_to_xenmem_access(), has been added to avoid
code duplication.  The existing _p2m_get_mem_access() now uses this
function as does p2m_get_mem_access().
---
 tools/libxc/include/xenctrl.h   |  3 ++
 tools/libxc/xc_altp2m.c         | 33 ++++++++++++--
 xen/arch/arm/mem_access.c       |  5 ++-
 xen/arch/x86/hvm/hvm.c          | 26 +++++++++--
 xen/arch/x86/mm/mem_access.c    | 80 +++++++++++++++++++++++----------
 xen/common/mem_access.c         |  2 +-
 xen/include/public/hvm/hvm_op.h | 10 +++--
 xen/include/xen/mem_access.h    |  3 +-
 8 files changed, 124 insertions(+), 38 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index 408fa1c6a4..f326bcbb4a 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1964,6 +1964,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
 int xc_altp2m_set_mem_access_multi(xc_interface *handle, uint32_t domid,
                                    uint16_t view_id, uint8_t *access,
                                    uint64_t *gfns, uint32_t nr);
+int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
+                             uint16_t view_id, xen_pfn_t gfn,
+                             xenmem_access_t *access);
 int xc_altp2m_change_gfn(xc_interface *handle, uint32_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn);
diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
index ce4a1e4d60..0ddb18fa2c 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -177,9 +177,9 @@ int xc_altp2m_set_mem_access(xc_interface *handle, uint32_t domid,
     arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
     arg->cmd = HVMOP_altp2m_set_mem_access;
     arg->domain = domid;
-    arg->u.set_mem_access.view = view_id;
-    arg->u.set_mem_access.hvmmem_access = access;
-    arg->u.set_mem_access.gfn = gfn;
+    arg->u.mem_access.view = view_id;
+    arg->u.mem_access.hvmmem_access = access;
+    arg->u.mem_access.gfn = gfn;
 
     rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
 		  HYPERCALL_BUFFER_AS_ARG(arg));
@@ -254,3 +254,30 @@ int xc_altp2m_set_mem_access_multi(xc_interface *xch, uint32_t domid,
 
     return rc;
 }
+
+int xc_altp2m_get_mem_access(xc_interface *handle, uint32_t domid,
+                             uint16_t view_id, xen_pfn_t gfn,
+                             xenmem_access_t *access)
+{
+    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_get_mem_access;
+    arg->domain = domid;
+    arg->u.mem_access.view = view_id;
+    arg->u.mem_access.gfn = gfn;
+
+    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+                 HYPERCALL_BUFFER_AS_ARG(arg));
+
+    if ( !rc )
+        *access = arg->u.mem_access.hvmmem_access;
+
+    xc_hypercall_buffer_free(handle, arg);
+    return rc;
+}
diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
index ae2686ffa2..ba9e50e7f6 100644
--- a/xen/arch/arm/mem_access.c
+++ b/xen/arch/arm/mem_access.c
@@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
     if ( !p2m->mem_access_enabled )
         return true;
 
-    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
+    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
     if ( rc )
         return true;
 
@@ -440,8 +440,9 @@ long p2m_set_mem_access_multi(struct domain *d,
     return -EOPNOTSUPP;
 }
 
+/* The altp2m_idx argument is not used on ARM. */
 int p2m_get_mem_access(struct domain *d, gfn_t gfn,
-                       xenmem_access_t *access)
+                       xenmem_access_t *access, unsigned int altp2m_idx)
 {
     int ret;
     struct p2m_domain *p2m = p2m_get_hostp2m(d);
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c23983cdff..28e1719751 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4509,6 +4509,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
     case HVMOP_altp2m_set_mem_access_multi:
+    case HVMOP_altp2m_get_mem_access:
     case HVMOP_altp2m_change_gfn:
         break;
 
@@ -4625,12 +4626,12 @@ static int do_altp2m_op(
         break;
 
     case HVMOP_altp2m_set_mem_access:
-        if ( a.u.set_mem_access.pad )
+        if ( a.u.mem_access.pad )
             rc = -EINVAL;
         else
-            rc = p2m_set_mem_access(d, _gfn(a.u.set_mem_access.gfn), 1, 0, 0,
-                                    a.u.set_mem_access.hvmmem_access,
-                                    a.u.set_mem_access.view);
+            rc = p2m_set_mem_access(d, _gfn(a.u.mem_access.gfn), 1, 0, 0,
+                                    a.u.mem_access.hvmmem_access,
+                                    a.u.mem_access.view);
         break;
 
     case HVMOP_altp2m_set_mem_access_multi:
@@ -4666,6 +4667,23 @@ static int do_altp2m_op(
         }
         break;
 
+    case HVMOP_altp2m_get_mem_access:
+        if ( a.u.mem_access.pad )
+            rc = -EINVAL;
+        else
+        {
+            xenmem_access_t access;
+
+            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
+                                    a.u.mem_access.view);
+            if ( !rc )
+            {
+                a.u.mem_access.hvmmem_access = access;
+                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            }
+        }
+        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 c0cd0174cf..b7b17acb4b 100644
--- a/xen/arch/x86/mm/mem_access.c
+++ b/xen/arch/x86/mm/mem_access.c
@@ -32,17 +32,10 @@
 
 #include "mm-locks.h"
 
-/*
- * Get access type for a gfn.
- * If gfn == INVALID_GFN, gets the default access type.
- */
-static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
-                               xenmem_access_t *access)
+static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,
+                                       p2m_access_t paccess,
+                                       xenmem_access_t *xaccess)
 {
-    p2m_type_t t;
-    p2m_access_t a;
-    mfn_t mfn;
-
     static const xenmem_access_t memaccess[] = {
 #define ACCESS(ac) [p2m_access_##ac] = XENMEM_access_##ac
             ACCESS(n),
@@ -58,12 +51,27 @@ static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
 #undef ACCESS
     };
 
+    if ( (unsigned int)paccess >= ARRAY_SIZE(memaccess) )
+        return -ERANGE;
+
+    *xaccess = memaccess[paccess];
+    return 0;
+}
+
+/*
+ * Get access type for a gfn.
+ * If gfn == INVALID_GFN, gets the default access type.
+ */
+static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
+                               xenmem_access_t *access, unsigned int altp2m_idx)
+{
+    p2m_type_t t;
+    p2m_access_t a;
+    mfn_t mfn;
+
     /* If request to get default access. */
     if ( gfn_eq(gfn, INVALID_GFN) )
-    {
-        *access = memaccess[p2m->default_access];
-        return 0;
-    }
+        return p2m_access_to_xenmem_access(p2m, p2m->default_access, access);
 
     gfn_lock(p2m, gfn, 0);
     mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
@@ -72,11 +80,7 @@ static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
     if ( mfn_eq(mfn, INVALID_MFN) )
         return -ESRCH;
 
-    if ( (unsigned int)a >= ARRAY_SIZE(memaccess) )
-        return -ERANGE;
-
-    *access =  memaccess[a];
-    return 0;
+    return p2m_access_to_xenmem_access(p2m, a, access);
 }
 
 bool p2m_mem_access_emulate_check(struct vcpu *v,
@@ -93,7 +97,7 @@ bool p2m_mem_access_emulate_check(struct vcpu *v,
     if ( !p2m )
         p2m = p2m_get_hostp2m(d);
 
-    if ( _p2m_get_mem_access(p2m, _gfn(data->gfn), &access) == 0 )
+    if ( _p2m_get_mem_access(p2m, _gfn(data->gfn), &access, 0) == 0 )
     {
         switch ( access )
         {
@@ -458,11 +462,41 @@ long p2m_set_mem_access_multi(struct domain *d,
     return rc;
 }
 
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
+                       unsigned int altp2m_idx)
 {
-    struct p2m_domain *p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
+    struct p2m_domain *ap2m = NULL;
+    struct p2m_domain *p2m;
+    p2m_access_t a;
+    p2m_type_t t;
+    mfn_t mfn;
+
+    if ( altp2m_idx )
+    {
+        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_read_lock(host_p2m);
+    if (ap2m)
+        p2m_read_lock(ap2m);
+
+    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
+
+    if (ap2m)
+        p2m_read_unlock(ap2m);
+    p2m_read_unlock(host_p2m);
+
+    if ( mfn_eq(mfn, INVALID_MFN) )
+        return -ESRCH;
 
-    return _p2m_get_mem_access(p2m, gfn, access);
+    return p2m_access_to_xenmem_access(p2m, a, access);
 }
 
 /*
diff --git a/xen/common/mem_access.c b/xen/common/mem_access.c
index 1bf6824442..010e6f8dbf 100644
--- a/xen/common/mem_access.c
+++ b/xen/common/mem_access.c
@@ -99,7 +99,7 @@ int mem_access_memop(unsigned long cmd,
         if ( (mao.pfn > domain_get_maximum_gpfn(d)) && mao.pfn != ~0ull )
             break;
 
-        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access);
+        rc = p2m_get_mem_access(d, _gfn(mao.pfn), &access, 0);
         if ( rc != 0 )
             break;
 
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bbba99e5f5..36fd97f329 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -234,7 +234,7 @@ struct xen_hvm_altp2m_view {
 typedef struct xen_hvm_altp2m_view xen_hvm_altp2m_view_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_view_t);
 
-struct xen_hvm_altp2m_set_mem_access {
+struct xen_hvm_altp2m_mem_access {
     /* view */
     uint16_t view;
     /* Memory type */
@@ -243,8 +243,8 @@ struct xen_hvm_altp2m_set_mem_access {
     /* gfn */
     uint64_t gfn;
 };
-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);
+typedef struct xen_hvm_altp2m_mem_access xen_hvm_altp2m_mem_access_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_mem_access_t);
 
 struct xen_hvm_altp2m_set_mem_access_multi {
     /* view */
@@ -296,6 +296,8 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_change_gfn           8
 /* Set access for an array of pages */
 #define HVMOP_altp2m_set_mem_access_multi 9
+/* Get the access of a page of memory from a certain view */
+#define HVMOP_altp2m_get_mem_access       10
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
@@ -303,7 +305,7 @@ struct xen_hvm_altp2m_op {
         struct xen_hvm_altp2m_domain_state         domain_state;
         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_mem_access           mem_access;
         struct xen_hvm_altp2m_change_gfn           change_gfn;
         struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
         uint8_t pad[64];
diff --git a/xen/include/xen/mem_access.h b/xen/include/xen/mem_access.h
index 5ab34c1553..99fe11f6bc 100644
--- a/xen/include/xen/mem_access.h
+++ b/xen/include/xen/mem_access.h
@@ -76,7 +76,8 @@ long p2m_set_mem_access_multi(struct domain *d,
  * Get access type for a gfn.
  * If gfn == INVALID_GFN, gets the default access type.
  */
-int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access);
+int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
+                       unsigned int altp2m_idx);
 
 #ifdef CONFIG_HAS_MEM_ACCESS
 int mem_access_memop(unsigned long cmd,
-- 
2.17.0


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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Jan Beulich]

>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>          }
>          break;
>  
> +    case HVMOP_altp2m_get_mem_access:
> +        if ( a.u.mem_access.pad )
> +            rc = -EINVAL;
> +        else
> +        {
> +            xenmem_access_t access;
> +
> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> +                                    a.u.mem_access.view);
> +            if ( !rc )
> +            {
> +                a.u.mem_access.hvmmem_access = access;
> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;

__copy_field_to_guest()? Or wait, no, the function argument is still a
handle of void.

And then - here we are again: Is it reasonable to permit a domain inquiring
for itself?

> --- a/xen/arch/x86/mm/mem_access.c
> +++ b/xen/arch/x86/mm/mem_access.c
> @@ -32,17 +32,10 @@
>  
>  #include "mm-locks.h"
>  
> -/*
> - * Get access type for a gfn.
> - * If gfn == INVALID_GFN, gets the default access type.
> - */
> -static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
> -                               xenmem_access_t *access)
> +static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,

This is not even p2m code - why the p2m_ prefix?

> @@ -458,11 +462,41 @@ long p2m_set_mem_access_multi(struct domain *d,
>      return rc;
>  }
>  
> -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> +                       unsigned int altp2m_idx)
>  {
> -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> +    struct p2m_domain *ap2m = NULL;
> +    struct p2m_domain *p2m;
> +    p2m_access_t a;
> +    p2m_type_t t;
> +    mfn_t mfn;
> +
> +    if ( altp2m_idx )
> +    {
> +        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_read_lock(host_p2m);
> +    if (ap2m)

Missing blanks (also below).

> +        p2m_read_lock(ap2m);
> +
> +    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> +
> +    if (ap2m)
> +        p2m_read_unlock(ap2m);
> +    p2m_read_unlock(host_p2m);
> +
> +    if ( mfn_eq(mfn, INVALID_MFN) )
> +        return -ESRCH;
>  
> -    return _p2m_get_mem_access(p2m, gfn, access);
> +    return p2m_access_to_xenmem_access(p2m, a, access);

I'm confused: Why does p2m_get_mem_access() not use its helper
function p2m_get_mem_access() (which you retain) anymore? I
guess the description is a little too terse. It might also have helped
if some of the mechanical preparation steps were broken out.

Jan



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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>          }
>>          break;
>>  
>> +    case HVMOP_altp2m_get_mem_access:
>> +        if ( a.u.mem_access.pad )
>> +            rc = -EINVAL;
>> +        else
>> +        {
>> +            xenmem_access_t access;
>> +
>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>> +                                    a.u.mem_access.view);
>> +            if ( !rc )
>> +            {
>> +                a.u.mem_access.hvmmem_access = access;
>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> 
> __copy_field_to_guest()? Or wait, no, the function argument is still a
> handle of void.
> 
> And then - here we are again: Is it reasonable to permit a domain inquiring
> for itself?

A good question. Perhaps the following are decision factors:

1. It is already possible for a domain to set mem_access restrictions
(via HVMOP_altp2m_set_mem_access) on itself.

2. Tamas' patch allows setting this externally:

https://patchwork.kernel.org/patch/9639779/

Specifically, we have altp2m = disabled, mixed, external and limited to
control who is allowed to do what:

https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html

That being said, we don't specifically need this to be a HVMOP - we
intend to use this from a privileged-domain based userspace agent only.


Thanks,
Razvan

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Jan Beulich]

>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>          }
>>>          break;
>>>  
>>> +    case HVMOP_altp2m_get_mem_access:
>>> +        if ( a.u.mem_access.pad )
>>> +            rc = -EINVAL;
>>> +        else
>>> +        {
>>> +            xenmem_access_t access;
>>> +
>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>> +                                    a.u.mem_access.view);
>>> +            if ( !rc )
>>> +            {
>>> +                a.u.mem_access.hvmmem_access = access;
>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>> 
>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>> handle of void.
>> 
>> And then - here we are again: Is it reasonable to permit a domain inquiring
>> for itself?
> 
> A good question. Perhaps the following are decision factors:
> 
> 1. It is already possible for a domain to set mem_access restrictions
> (via HVMOP_altp2m_set_mem_access) on itself.

Which, as before, I consider a flaw.

> 2. Tamas' patch allows setting this externally:
> 
> https://patchwork.kernel.org/patch/9639779/ 
> 
> Specifically, we have altp2m = disabled, mixed, external and limited to
> control who is allowed to do what:
> 
> https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html 

Indeed this has at least made the situation less bad.

Jan



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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Julien Grall]

Hi,

On 06/28/2018 02:00 PM, Adrian Pop wrote:
> diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> index ae2686ffa2..ba9e50e7f6 100644
> --- a/xen/arch/arm/mem_access.c
> +++ b/xen/arch/arm/mem_access.c
> @@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
>       if ( !p2m->mem_access_enabled )
>           return true;
>   
> -    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
> +    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
>       if ( rc )
>           return true;
>   
> @@ -440,8 +440,9 @@ long p2m_set_mem_access_multi(struct domain *d,
>       return -EOPNOTSUPP;
>   }
>   
> +/* The altp2m_idx argument is not used on ARM. */

s/ARM/Arm/ please.

Also, I guess altp2m_idx should always be 0 until altp2m is implemented 
for Arm. If so, I would like to see a check on the variable and return 
-EINVAL on error.

Chees,

-- 
Julien Grall

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 07/02/2018 09:34 AM, Jan Beulich wrote:
>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>          }
>>>>          break;
>>>>  
>>>> +    case HVMOP_altp2m_get_mem_access:
>>>> +        if ( a.u.mem_access.pad )
>>>> +            rc = -EINVAL;
>>>> +        else
>>>> +        {
>>>> +            xenmem_access_t access;
>>>> +
>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>>> +                                    a.u.mem_access.view);
>>>> +            if ( !rc )
>>>> +            {
>>>> +                a.u.mem_access.hvmmem_access = access;
>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>>
>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>> handle of void.
>>>
>>> And then - here we are again: Is it reasonable to permit a domain inquiring
>>> for itself?
>>
>> A good question. Perhaps the following are decision factors:
>>
>> 1. It is already possible for a domain to set mem_access restrictions
>> (via HVMOP_altp2m_set_mem_access) on itself.
> 
> Which, as before, I consider a flaw.
> 
>> 2. Tamas' patch allows setting this externally:
>>
>> https://patchwork.kernel.org/patch/9639779/ 
>>
>> Specifically, we have altp2m = disabled, mixed, external and limited to
>> control who is allowed to do what:
>>
>> https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html 
> 
> Indeed this has at least made the situation less bad.

Should we then switch this to a DOMCTL? Tamas, any thoughts on this?
For us at least a DOMCTL is sufficient.


Thanks,
Razvan

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Adrian Pop]

On Fri, Jun 29, 2018 at 09:38:58AM -0600, Jan Beulich wrote:
> >>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
> > @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
> >          }
> >          break;
> >  
> > +    case HVMOP_altp2m_get_mem_access:
> > +        if ( a.u.mem_access.pad )
> > +            rc = -EINVAL;
> > +        else
> > +        {
> > +            xenmem_access_t access;
> > +
> > +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> > +                                    a.u.mem_access.view);
> > +            if ( !rc )
> > +            {
> > +                a.u.mem_access.hvmmem_access = access;
> > +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> 
> __copy_field_to_guest()? Or wait, no, the function argument is still a
> handle of void.

I'll then leave the __copy_to_guest() in place as it is for now.

> And then - here we are again: Is it reasonable to permit a domain inquiring
> for itself?

Yes, this is a questionable aspect of altp2m that warrants further
discussion.  I'll resend a version with the other problems addressed to
have them out of the way.

> > --- a/xen/arch/x86/mm/mem_access.c
> > +++ b/xen/arch/x86/mm/mem_access.c
> > @@ -32,17 +32,10 @@
> >  
> >  #include "mm-locks.h"
> >  
> > -/*
> > - * Get access type for a gfn.
> > - * If gfn == INVALID_GFN, gets the default access type.
> > - */
> > -static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
> > -                               xenmem_access_t *access)
> > +static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,
> 
> This is not even p2m code - why the p2m_ prefix?

There's indeed no reason for this to have the p2m_ prefix.  Will remove
it.

> > @@ -458,11 +462,41 @@ long p2m_set_mem_access_multi(struct domain *d,
> >      return rc;
> >  }
> >  
> > -int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access)
> > +int p2m_get_mem_access(struct domain *d, gfn_t gfn, xenmem_access_t *access,
> > +                       unsigned int altp2m_idx)
> >  {
> > -    struct p2m_domain *p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
> > +    struct p2m_domain *ap2m = NULL;
> > +    struct p2m_domain *p2m;
> > +    p2m_access_t a;
> > +    p2m_type_t t;
> > +    mfn_t mfn;
> > +
> > +    if ( altp2m_idx )
> > +    {
> > +        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_read_lock(host_p2m);
> > +    if (ap2m)
> 
> Missing blanks (also below).

All right.

> > +        p2m_read_lock(ap2m);
> > +
> > +    mfn = p2m->get_entry(p2m, gfn, &t, &a, 0, NULL, NULL);
> > +
> > +    if (ap2m)
> > +        p2m_read_unlock(ap2m);
> > +    p2m_read_unlock(host_p2m);
> > +
> > +    if ( mfn_eq(mfn, INVALID_MFN) )
> > +        return -ESRCH;
> >  
> > -    return _p2m_get_mem_access(p2m, gfn, access);
> > +    return p2m_access_to_xenmem_access(p2m, a, access);
> 
> I'm confused: Why does p2m_get_mem_access() not use its helper
> function p2m_get_mem_access() (which you retain) anymore? I
> guess the description is a little too terse. It might also have helped
> if some of the mechanical preparation steps were broken out.

Ok, I'll fix this and attempt to provide more information with the
commit message.

Thank you!

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Adrian Pop]

Hello,

On Mon, Jul 02, 2018 at 12:01:01PM +0100, Julien Grall wrote:
> Hi,
> 
> On 06/28/2018 02:00 PM, Adrian Pop wrote:
> > diff --git a/xen/arch/arm/mem_access.c b/xen/arch/arm/mem_access.c
> > index ae2686ffa2..ba9e50e7f6 100644
> > --- a/xen/arch/arm/mem_access.c
> > +++ b/xen/arch/arm/mem_access.c
> > @@ -236,7 +236,7 @@ bool p2m_mem_access_check(paddr_t gpa, vaddr_t gla, const struct npfec npfec)
> >       if ( !p2m->mem_access_enabled )
> >           return true;
> > -    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma);
> > +    rc = p2m_get_mem_access(v->domain, gaddr_to_gfn(gpa), &xma, 0);
> >       if ( rc )
> >           return true;
> > @@ -440,8 +440,9 @@ long p2m_set_mem_access_multi(struct domain *d,
> >       return -EOPNOTSUPP;
> >   }
> > +/* The altp2m_idx argument is not used on ARM. */
> 
> s/ARM/Arm/ please.
> 
> Also, I guess altp2m_idx should always be 0 until altp2m is implemented for
> Arm. If so, I would like to see a check on the variable and return -EINVAL
> on error.

All right.  Thank you!

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Adrian Pop]

On Wed, Jul 04, 2018 at 03:20:45PM +0300, Adrian POP wrote:
> On Fri, Jun 29, 2018 at 09:38:58AM -0600, Jan Beulich wrote:
> > > --- a/xen/arch/x86/mm/mem_access.c
> > > +++ b/xen/arch/x86/mm/mem_access.c
> > > @@ -32,17 +32,10 @@
> > >  
> > >  #include "mm-locks.h"
> > >  
> > > -/*
> > > - * Get access type for a gfn.
> > > - * If gfn == INVALID_GFN, gets the default access type.
> > > - */
> > > -static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
> > > -                               xenmem_access_t *access)
> > > +static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,
> > 
> > This is not even p2m code - why the p2m_ prefix?
> 
> There's indeed no reason for this to have the p2m_ prefix.  Will remove
> it.

Oh I remember now.  The p2m_ prefix was added there because the helper
function converts a p2m_access_t to a xenmem_access_t, so
p2m_access_to_xenmem_access().  Took the naming from the complementary
function, xenmem_access_to_p2m_access().  I could add a comment to
clarify this.

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[George Dunlap]

> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>         }
>>>>         break;
>>>> 
>>>> +    case HVMOP_altp2m_get_mem_access:
>>>> +        if ( a.u.mem_access.pad )
>>>> +            rc = -EINVAL;
>>>> +        else
>>>> +        {
>>>> +            xenmem_access_t access;
>>>> +
>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>>> +                                    a.u.mem_access.view);
>>>> +            if ( !rc )
>>>> +            {
>>>> +                a.u.mem_access.hvmmem_access = access;
>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>> 
>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>> handle of void.
>>> 
>>> And then - here we are again: Is it reasonable to permit a domain inquiring
>>> for itself?
>> 
>> A good question. Perhaps the following are decision factors:
>> 
>> 1. It is already possible for a domain to set mem_access restrictions
>> (via HVMOP_altp2m_set_mem_access) on itself.
> 
> Which, as before, I consider a flaw.

How many times do we have to go over this?  Here is my recollection from the last time we had a discussion on this topic:

* The original authors of this code probably thought having guests set their own memaccess would be a potential use case
* The maintainers and main users of the code (Tamas and Razvan) think it’s a useful use case
* The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a useful use case.

(Correct me if I’ve misremembered anywhere.)

Do we need to have a formal vote by the committers for you to accept that this should be a supported use case, and stop making objections any time someone wants to improve it?

As long as not being able to call altp2m_set_mem_access  means not being able to call altp2m_get_mem_access, this should be fine.

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Jan Beulich]

>>> On 04.07.18 at 15:20, <apop@bitdefender.com> wrote:
> On Wed, Jul 04, 2018 at 03:20:45PM +0300, Adrian POP wrote:
>> On Fri, Jun 29, 2018 at 09:38:58AM -0600, Jan Beulich wrote:
>> > > --- a/xen/arch/x86/mm/mem_access.c
>> > > +++ b/xen/arch/x86/mm/mem_access.c
>> > > @@ -32,17 +32,10 @@
>> > >  
>> > >  #include "mm-locks.h"
>> > >  
>> > > -/*
>> > > - * Get access type for a gfn.
>> > > - * If gfn == INVALID_GFN, gets the default access type.
>> > > - */
>> > > -static int _p2m_get_mem_access(struct p2m_domain *p2m, gfn_t gfn,
>> > > -                               xenmem_access_t *access)
>> > > +static int p2m_access_to_xenmem_access(struct p2m_domain *p2m,
>> > 
>> > This is not even p2m code - why the p2m_ prefix?
>> 
>> There's indeed no reason for this to have the p2m_ prefix.  Will remove
>> it.
> 
> Oh I remember now.  The p2m_ prefix was added there because the helper
> function converts a p2m_access_t to a xenmem_access_t, so
> p2m_access_to_xenmem_access().  Took the naming from the complementary
> function, xenmem_access_to_p2m_access().  I could add a comment to
> clarify this.

Oh, of course - I simply wasn't paying attention to this aspect, sorry.

Jan



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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Jan Beulich]

>>> On 04.07.18 at 16:05, <George.Dunlap@citrix.com> wrote:
>> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
>>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>>         }
>>>>>         break;
>>>>> 
>>>>> +    case HVMOP_altp2m_get_mem_access:
>>>>> +        if ( a.u.mem_access.pad )
>>>>> +            rc = -EINVAL;
>>>>> +        else
>>>>> +        {
>>>>> +            xenmem_access_t access;
>>>>> +
>>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>>>> +                                    a.u.mem_access.view);
>>>>> +            if ( !rc )
>>>>> +            {
>>>>> +                a.u.mem_access.hvmmem_access = access;
>>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>>> 
>>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>>> handle of void.
>>>> 
>>>> And then - here we are again: Is it reasonable to permit a domain inquiring
>>>> for itself?
>>> 
>>> A good question. Perhaps the following are decision factors:
>>> 
>>> 1. It is already possible for a domain to set mem_access restrictions
>>> (via HVMOP_altp2m_set_mem_access) on itself.
>> 
>> Which, as before, I consider a flaw.
> 
> How many times do we have to go over this?  Here is my recollection from the 
> last time we had a discussion on this topic:
> 
> * The original authors of this code probably thought having guests set their 
> own memaccess would be a potential use case
> * The maintainers and main users of the code (Tamas and Razvan) think it’s a 
> useful use case
> * The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a 
> useful use case.
> 
> (Correct me if I’ve misremembered anywhere.)
> 
> Do we need to have a formal vote by the committers for you to accept that 
> this should be a supported use case, and stop making objections any time 
> someone wants to improve it?

There's no need for a vote, since - as before - I won't object to the
addition, but I consider it to widen the badness (once again). In all
the "think it's a valid use case" it was never really made clear to me
how this "valid" implies "still secure".

Jan


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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[George Dunlap]

> On Jul 4, 2018, at 4:38 PM, Jan Beulich <JBeulich@suse.com> wrote:
> 
>>>> On 04.07.18 at 16:05, <George.Dunlap@citrix.com> wrote:
>>> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
>>>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>>>        }
>>>>>>        break;
>>>>>> 
>>>>>> +    case HVMOP_altp2m_get_mem_access:
>>>>>> +        if ( a.u.mem_access.pad )
>>>>>> +            rc = -EINVAL;
>>>>>> +        else
>>>>>> +        {
>>>>>> +            xenmem_access_t access;
>>>>>> +
>>>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>>>>> +                                    a.u.mem_access.view);
>>>>>> +            if ( !rc )
>>>>>> +            {
>>>>>> +                a.u.mem_access.hvmmem_access = access;
>>>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>>>> 
>>>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>>>> handle of void.
>>>>> 
>>>>> And then - here we are again: Is it reasonable to permit a domain inquiring
>>>>> for itself?
>>>> 
>>>> A good question. Perhaps the following are decision factors:
>>>> 
>>>> 1. It is already possible for a domain to set mem_access restrictions
>>>> (via HVMOP_altp2m_set_mem_access) on itself.
>>> 
>>> Which, as before, I consider a flaw.
>> 
>> How many times do we have to go over this?  Here is my recollection from the 
>> last time we had a discussion on this topic:
>> 
>> * The original authors of this code probably thought having guests set their 
>> own memaccess would be a potential use case
>> * The maintainers and main users of the code (Tamas and Razvan) think it’s a 
>> useful use case
>> * The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a 
>> useful use case.
>> 
>> (Correct me if I’ve misremembered anywhere.)
>> 
>> Do we need to have a formal vote by the committers for you to accept that 
>> this should be a supported use case, and stop making objections any time 
>> someone wants to improve it?
> 
> There's no need for a vote, since - as before - I won't object to the
> addition, but I consider it to widen the badness (once again).

But you did object.  This whole thread is you re-objecting to the original decision that we’ve discussed twice before.  Either the altp2m functionality should be exposed to the guest, or it shouldn’t.  If we do expose functionality to the guest, then the interface exposed should be useful; and being able to read what you wrote, rather than keeping a separate copy of it, is part of a useful interface.

I mean, I understand objecting to the idea of building an extension to your house.  But what doesn’t make sense to me is, once the extension is built, then objecting to the idea of painting it; and then objecting to the idea of adding electrical sockets; and then objecting to the idea of adding heat.  Why not just accept that you have an extra room and make the best of it?  I understand that you’d prefer extra garden space to the utility room that’s there now, but given that you can’t have the garden, why is a utility room with no paint and no electricity and no heat better than a utility room with all those things?

> In all
> the "think it's a valid use case" it was never really made clear to me
> how this "valid" implies "still secure”.

That was never the argument.  The argument is that the behavior is off by default, and that host administrators should be treated as adults and allowed to judge for themselves whether it’s safe to turn it on or not — just like nested virt, PCI pass-through, COLO, or the host of other features that aren’t security supported.

I mean, I’d understand if supporting that use case this meant add tons of extra functionality that was likely to be fragile and introduce new bugs; but it’s not — all the complexity of memaccess would be there even if we only allowed dom0 access to this functionality.

Would you feel better if we had a line covering memaccess in SUPPORT.md?

 -George


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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Tamas K Lengyel]

On Mon, Jul 2, 2018 at 5:14 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 07/02/2018 09:34 AM, Jan Beulich wrote:
> >>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
> >> On 06/29/2018 06:38 PM, Jan Beulich wrote:
> >>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
> >>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
> >>>>          }
> >>>>          break;
> >>>>
> >>>> +    case HVMOP_altp2m_get_mem_access:
> >>>> +        if ( a.u.mem_access.pad )
> >>>> +            rc = -EINVAL;
> >>>> +        else
> >>>> +        {
> >>>> +            xenmem_access_t access;
> >>>> +
> >>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> >>>> +                                    a.u.mem_access.view);
> >>>> +            if ( !rc )
> >>>> +            {
> >>>> +                a.u.mem_access.hvmmem_access = access;
> >>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> >>>
> >>> __copy_field_to_guest()? Or wait, no, the function argument is still a
> >>> handle of void.
> >>>
> >>> And then - here we are again: Is it reasonable to permit a domain inquiring
> >>> for itself?
> >>
> >> A good question. Perhaps the following are decision factors:
> >>
> >> 1. It is already possible for a domain to set mem_access restrictions
> >> (via HVMOP_altp2m_set_mem_access) on itself.
> >
> > Which, as before, I consider a flaw.
> >
> >> 2. Tamas' patch allows setting this externally:
> >>
> >> https://patchwork.kernel.org/patch/9639779/
> >>
> >> Specifically, we have altp2m = disabled, mixed, external and limited to
> >> control who is allowed to do what:
> >>
> >> https://xenbits.xen.org/docs/unstable/man/xl.cfg.5.html
> >
> > Indeed this has at least made the situation less bad.
>
> Should we then switch this to a DOMCTL? Tamas, any thoughts on this?
> For us at least a DOMCTL is sufficient.

I pretty much agree with George in this thread. Being able to set the
memaccess permission from the guest is already an allowed setup with
the proper altp2m setting (and that is not on by default). So I don't
see why being able to get that permission should be any different.

Tamas

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Jan Beulich]

>>> On 04.07.18 at 18:44, <George.Dunlap@citrix.com> wrote:

> 
>> On Jul 4, 2018, at 4:38 PM, Jan Beulich <JBeulich@suse.com> wrote:
>> 
>>>>> On 04.07.18 at 16:05, <George.Dunlap@citrix.com> wrote:
>>>> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
>>>>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
>>>>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
>>>>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
>>>>>>>        }
>>>>>>>        break;
>>>>>>> 
>>>>>>> +    case HVMOP_altp2m_get_mem_access:
>>>>>>> +        if ( a.u.mem_access.pad )
>>>>>>> +            rc = -EINVAL;
>>>>>>> +        else
>>>>>>> +        {
>>>>>>> +            xenmem_access_t access;
>>>>>>> +
>>>>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
>>>>>>> +                                    a.u.mem_access.view);
>>>>>>> +            if ( !rc )
>>>>>>> +            {
>>>>>>> +                a.u.mem_access.hvmmem_access = access;
>>>>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>>>>>> 
>>>>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
>>>>>> handle of void.
>>>>>> 
>>>>>> And then - here we are again: Is it reasonable to permit a domain inquiring
>>>>>> for itself?
>>>>> 
>>>>> A good question. Perhaps the following are decision factors:
>>>>> 
>>>>> 1. It is already possible for a domain to set mem_access restrictions
>>>>> (via HVMOP_altp2m_set_mem_access) on itself.
>>>> 
>>>> Which, as before, I consider a flaw.
>>> 
>>> How many times do we have to go over this?  Here is my recollection from the 
> 
>>> last time we had a discussion on this topic:
>>> 
>>> * The original authors of this code probably thought having guests set their 
> 
>>> own memaccess would be a potential use case
>>> * The maintainers and main users of the code (Tamas and Razvan) think it’s a 
> 
>>> useful use case
>>> * The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a 
>>> useful use case.
>>> 
>>> (Correct me if I’ve misremembered anywhere.)
>>> 
>>> Do we need to have a formal vote by the committers for you to accept that 
>>> this should be a supported use case, and stop making objections any time 
>>> someone wants to improve it?
>> 
>> There's no need for a vote, since - as before - I won't object to the
>> addition, but I consider it to widen the badness (once again).
> 
> But you did object.  This whole thread is you re-objecting to the original 
> decision that we’ve discussed twice before.  Either the altp2m functionality 
> should be exposed to the guest, or it shouldn’t.  If we do expose 
> functionality to the guest, then the interface exposed should be useful; and 
> being able to read what you wrote, rather than keeping a separate copy of it, 
> is part of a useful interface.

No, that's not the right way to put it. If this was an addition to the
interface not having the potential to weaken security, I wouldn't have
re-raised the point of there being an original (apparent, i.e. at least to
me) weakness. 

> I mean, I understand objecting to the idea of building an extension to your 
> house.  But what doesn’t make sense to me is, once the extension is built, 
> then objecting to the idea of painting it; and then objecting to the idea of 
> adding electrical sockets; and then objecting to the idea of adding heat.  
> Why not just accept that you have an extra room and make the best of it?  I 
> understand that you’d prefer extra garden space to the utility room that’s 
> there now, but given that you can’t have the garden, why is a utility room 
> with no paint and no electricity and no heat better than a utility room with 
> all those things?

And this is not a proper analogy either: I'm questioning whether adding
something that increases the risk of the house to crash or burn is a good
idea.

>> In all
>> the "think it's a valid use case" it was never really made clear to me
>> how this "valid" implies "still secure”.
> 
> That was never the argument.  The argument is that the behavior is off by 
> default, and that host administrators should be treated as adults and allowed 
> to judge for themselves whether it’s safe to turn it on or not — just like 
> nested virt, PCI pass-through, COLO, or the host of other features that 
> aren’t security supported.

Even for other security unsupported pieces of code I would always at
least raise concerns if security was further weakened by a change.

> I mean, I’d understand if supporting that use case this meant add tons of 
> extra functionality that was likely to be fragile and introduce new bugs; but 
> it’s not — all the complexity of memaccess would be there even if we only 
> allowed dom0 access to this functionality.
> 
> Would you feel better if we had a line covering memaccess in SUPPORT.md?

That would imo make a difference only if altp2m itself was already
security supported. And anyway - why memaccess? The issue is with the
too broad exposure of altp2m ops in general, irrespective of this not
being the default mode.

Jan


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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Tamas K Lengyel]

On Thu, Jul 5, 2018 at 2:31 AM Jan Beulich <JBeulich@suse.com> wrote:
>
> >>> On 04.07.18 at 18:44, <George.Dunlap@citrix.com> wrote:
>
> >
> >> On Jul 4, 2018, at 4:38 PM, Jan Beulich <JBeulich@suse.com> wrote:
> >>
> >>>>> On 04.07.18 at 16:05, <George.Dunlap@citrix.com> wrote:
> >>>> On Jul 2, 2018, at 7:34 AM, Jan Beulich <JBeulich@suse.com> wrote:
> >>>>>>> On 29.06.18 at 18:39, <rcojocaru@bitdefender.com> wrote:
> >>>>> On 06/29/2018 06:38 PM, Jan Beulich wrote:
> >>>>>>>>> On 28.06.18 at 15:00, <apop@bitdefender.com> wrote:
> >>>>>>> @@ -4666,6 +4667,23 @@ static int do_altp2m_op(
> >>>>>>>        }
> >>>>>>>        break;
> >>>>>>>
> >>>>>>> +    case HVMOP_altp2m_get_mem_access:
> >>>>>>> +        if ( a.u.mem_access.pad )
> >>>>>>> +            rc = -EINVAL;
> >>>>>>> +        else
> >>>>>>> +        {
> >>>>>>> +            xenmem_access_t access;
> >>>>>>> +
> >>>>>>> +            rc = p2m_get_mem_access(d, _gfn(a.u.mem_access.gfn), &access,
> >>>>>>> +                                    a.u.mem_access.view);
> >>>>>>> +            if ( !rc )
> >>>>>>> +            {
> >>>>>>> +                a.u.mem_access.hvmmem_access = access;
> >>>>>>> +                rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
> >>>>>>
> >>>>>> __copy_field_to_guest()? Or wait, no, the function argument is still a
> >>>>>> handle of void.
> >>>>>>
> >>>>>> And then - here we are again: Is it reasonable to permit a domain inquiring
> >>>>>> for itself?
> >>>>>
> >>>>> A good question. Perhaps the following are decision factors:
> >>>>>
> >>>>> 1. It is already possible for a domain to set mem_access restrictions
> >>>>> (via HVMOP_altp2m_set_mem_access) on itself.
> >>>>
> >>>> Which, as before, I consider a flaw.
> >>>
> >>> How many times do we have to go over this?  Here is my recollection from the
> >
> >>> last time we had a discussion on this topic:
> >>>
> >>> * The original authors of this code probably thought having guests set their
> >
> >>> own memaccess would be a potential use case
> >>> * The maintainers and main users of the code (Tamas and Razvan) think it’s a
> >
> >>> useful use case
> >>> * The MM maintainer (me) and one of the x86 maintainers (Andy) think it’s a
> >>> useful use case.
> >>>
> >>> (Correct me if I’ve misremembered anywhere.)
> >>>
> >>> Do we need to have a formal vote by the committers for you to accept that
> >>> this should be a supported use case, and stop making objections any time
> >>> someone wants to improve it?
> >>
> >> There's no need for a vote, since - as before - I won't object to the
> >> addition, but I consider it to widen the badness (once again).
> >
> > But you did object.  This whole thread is you re-objecting to the original
> > decision that we’ve discussed twice before.  Either the altp2m functionality
> > should be exposed to the guest, or it shouldn’t.  If we do expose
> > functionality to the guest, then the interface exposed should be useful; and
> > being able to read what you wrote, rather than keeping a separate copy of it,
> > is part of a useful interface.
>
> No, that's not the right way to put it. If this was an addition to the
> interface not having the potential to weaken security, I wouldn't have
> re-raised the point of there being an original (apparent, i.e. at least to
> me) weakness.
>
> > I mean, I understand objecting to the idea of building an extension to your
> > house.  But what doesn’t make sense to me is, once the extension is built,
> > then objecting to the idea of painting it; and then objecting to the idea of
> > adding electrical sockets; and then objecting to the idea of adding heat.
> > Why not just accept that you have an extra room and make the best of it?  I
> > understand that you’d prefer extra garden space to the utility room that’s
> > there now, but given that you can’t have the garden, why is a utility room
> > with no paint and no electricity and no heat better than a utility room with
> > all those things?
>
> And this is not a proper analogy either: I'm questioning whether adding
> something that increases the risk of the house to crash or burn is a good
> idea.
>
> >> In all
> >> the "think it's a valid use case" it was never really made clear to me
> >> how this "valid" implies "still secure”.
> >
> > That was never the argument.  The argument is that the behavior is off by
> > default, and that host administrators should be treated as adults and allowed
> > to judge for themselves whether it’s safe to turn it on or not — just like
> > nested virt, PCI pass-through, COLO, or the host of other features that
> > aren’t security supported.
>
> Even for other security unsupported pieces of code I would always at
> least raise concerns if security was further weakened by a change.
>
> > I mean, I’d understand if supporting that use case this meant add tons of
> > extra functionality that was likely to be fragile and introduce new bugs; but
> > it’s not — all the complexity of memaccess would be there even if we only
> > allowed dom0 access to this functionality.
> >
> > Would you feel better if we had a line covering memaccess in SUPPORT.md?
>
> That would imo make a difference only if altp2m itself was already
> security supported. And anyway - why memaccess? The issue is with the
> too broad exposure of altp2m ops in general, irrespective of this not
> being the default mode.
>

Jan's comment here about the too broad exposure is not without a
point. For a security application to point in using altp2m and
memaccess is to have memory protections that the guest can't alter, so
even if the guest kernel gets compromised, some security checks remain
in place. Otherwise using the in-guest pagetables would be better in
every way - faster, less complexity, etc. #VE and VMFUNC blur this
picture a bit by allowing a guest agent to filter some of the events
that result from EPT violations. This may already be seen as
"weakening" the security of the approach but IMHO that's just the
tradeoff between security and performance. It can be configured on
which page can the in-guest agent act as a filter and which pages go
to the external agent, so at least that tradeoff can be fine-tuned (in
theory). So with that in mind, I would certainly consider an in-guest
application less of a security concern if it was only able to filter
events for which it was explicitly permitted to do so instead of being
able to both see all page permissions and also setting (ie removing
them) at will. The current altp2m interface is certainly not suitable
for making such a fine-grained distinction, and XSM doesn't help with
this either. But that's a separate problem, once we allow a guest
kernel to set these permissions, also allowing it get them makes very
little difference. Perhaps what we should be discussing is splitting
altp2m hvmop into two ops, one that's required for EPTP switching and
receiving #VE events, and one that adds the "rest" in case it's needed
during development/testing.

Tamas

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 07/05/2018 05:35 PM, Tamas K Lengyel wrote:
> Jan's comment here about the too broad exposure is not without a
> point. For a security application to point in using altp2m and
> memaccess is to have memory protections that the guest can't alter, so
> even if the guest kernel gets compromised, some security checks remain
> in place. Otherwise using the in-guest pagetables would be better in
> every way - faster, less complexity, etc. #VE and VMFUNC blur this
> picture a bit by allowing a guest agent to filter some of the events
> that result from EPT violations. This may already be seen as
> "weakening" the security of the approach but IMHO that's just the
> tradeoff between security and performance. It can be configured on
> which page can the in-guest agent act as a filter and which pages go
> to the external agent, so at least that tradeoff can be fine-tuned (in
> theory). So with that in mind, I would certainly consider an in-guest
> application less of a security concern if it was only able to filter
> events for which it was explicitly permitted to do so instead of being
> able to both see all page permissions and also setting (ie removing
> them) at will. The current altp2m interface is certainly not suitable
> for making such a fine-grained distinction, and XSM doesn't help with
> this either. But that's a separate problem, once we allow a guest
> kernel to set these permissions, also allowing it get them makes very
> little difference. Perhaps what we should be discussing is splitting
> altp2m hvmop into two ops, one that's required for EPTP switching and
> receiving #VE events, and one that adds the "rest" in case it's needed
> during development/testing.

That's true, the principle of least possible access is probably best.
However, I believe Andrew's initial objection in the
xc_altp2m_set_mem_access_multi() discussion was that altp2m seems to
have been designed with the specific goal of allowing several operations
to happen from the guest. From Intel's original design text [1]:

"- Hypercalls for altp2m

Altp2m mode introduces a new set of hypercalls for altp2m management
from software agents operating in Xen HVM guests.

The hypercalls are as follows:

Enable or Disable altp2m mode for domain
Create a new alternate p2m
Edit permissions for a specific GPA within an alternate p2m
Destroy an existing alternate p2m"

Since that has obviously been accepted upon upstreaming the altp2m
series, and since these new operations do respect that text exactly and,
as George has pointed out, neither increase the complexity of the code,
nor introduce a design inconsistency (with regard to what the in-guest
agent is allowed to do), and furthermore is even more restricted by your
later code, I would argue that simply extending the existing HVMOPs is
what looks the most consistent.

However, our particular application is only interested in setting (and
querying) page restrictions from userspace (from the dom0 agent). It
will also need to be able to set the convertible bit of guest pages from
the dom0 agent as well (patches pending). So we're also fine with a
"DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
(possibly temporarily) is an option.

We could also (at least between Tamas and us) come up with current /
likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
in all the scenarios to DOMCTLs.

The bigger problem is the uncertainty of the (by now quite venerable)
debate - I think we all agree that we need a final decision on this so
as to be able to constructively move forward. It looks like the
designers of the original code have moved on.


Thank you,
Razvan

[1]
https://lists.xenproject.org/archives/html/xen-devel/2015-06/msg01319.html

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Tamas K Lengyel]

On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 07/05/2018 05:35 PM, Tamas K Lengyel wrote:
> > Jan's comment here about the too broad exposure is not without a
> > point. For a security application to point in using altp2m and
> > memaccess is to have memory protections that the guest can't alter, so
> > even if the guest kernel gets compromised, some security checks remain
> > in place. Otherwise using the in-guest pagetables would be better in
> > every way - faster, less complexity, etc. #VE and VMFUNC blur this
> > picture a bit by allowing a guest agent to filter some of the events
> > that result from EPT violations. This may already be seen as
> > "weakening" the security of the approach but IMHO that's just the
> > tradeoff between security and performance. It can be configured on
> > which page can the in-guest agent act as a filter and which pages go
> > to the external agent, so at least that tradeoff can be fine-tuned (in
> > theory). So with that in mind, I would certainly consider an in-guest
> > application less of a security concern if it was only able to filter
> > events for which it was explicitly permitted to do so instead of being
> > able to both see all page permissions and also setting (ie removing
> > them) at will. The current altp2m interface is certainly not suitable
> > for making such a fine-grained distinction, and XSM doesn't help with
> > this either. But that's a separate problem, once we allow a guest
> > kernel to set these permissions, also allowing it get them makes very
> > little difference. Perhaps what we should be discussing is splitting
> > altp2m hvmop into two ops, one that's required for EPTP switching and
> > receiving #VE events, and one that adds the "rest" in case it's needed
> > during development/testing.
>
> That's true, the principle of least possible access is probably best.
> However, I believe Andrew's initial objection in the
> xc_altp2m_set_mem_access_multi() discussion was that altp2m seems to
> have been designed with the specific goal of allowing several operations
> to happen from the guest. From Intel's original design text [1]:
>
> "- Hypercalls for altp2m
>
> Altp2m mode introduces a new set of hypercalls for altp2m management
> from software agents operating in Xen HVM guests.
>
> The hypercalls are as follows:
>
> Enable or Disable altp2m mode for domain
> Create a new alternate p2m
> Edit permissions for a specific GPA within an alternate p2m
> Destroy an existing alternate p2m"
>
> Since that has obviously been accepted upon upstreaming the altp2m
> series, and since these new operations do respect that text exactly and,
> as George has pointed out, neither increase the complexity of the code,
> nor introduce a design inconsistency (with regard to what the in-guest
> agent is allowed to do), and furthermore is even more restricted by your
> later code, I would argue that simply extending the existing HVMOPs is
> what looks the most consistent.

Agree.

>
> However, our particular application is only interested in setting (and
> querying) page restrictions from userspace (from the dom0 agent). It
> will also need to be able to set the convertible bit of guest pages from
> the dom0 agent as well (patches pending). So we're also fine with a
> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
> (possibly temporarily) is an option.
>
> We could also (at least between Tamas and us) come up with current /
> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
> in all the scenarios to DOMCTLs.

Aye. There is really just one HVMOP that the guest absolutely needs
access to so that it can use #VE, and that's
HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
DOMCTL.

Tamas

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> However, our particular application is only interested in setting (and
>> querying) page restrictions from userspace (from the dom0 agent). It
>> will also need to be able to set the convertible bit of guest pages from
>> the dom0 agent as well (patches pending). So we're also fine with a
>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>> (possibly temporarily) is an option.
>>
>> We could also (at least between Tamas and us) come up with current /
>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>> in all the scenarios to DOMCTLs.
> 
> Aye. There is really just one HVMOP that the guest absolutely needs
> access to so that it can use #VE, and that's
> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
> DOMCTL.

We need even less than that - we want to modify
HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
and we don't call it from the in-guest agent ever. Because we agree that
the smallest attack surface is a requirement, all we ever call that's
#VE / altp2m related is actually from the privileged domain doing
introspection. The in-guest driver only needs to do VMFUNC and be able
to communicate with the dom0 introspection agent.

So at the moment we neither need nor use _any_ HVMOP from the guest (but
it does make sense that the guest be able to do
HVMOP_altp2m_vcpu_enable_notify indeed).

So it would appear that at least at this time, for both us and Tamas the
only operation that needs to be a HVMOP is HVMOP_altp2m_vcpu_enable_notify.

In that case, if everyone agrees, I propose that we make all the others
DOMCTLs. This would also have several maintenance benefits:

1. We can then get rid of the ugly compat code that was required for
upstreaming xc_altp2m_set_mem_access_multi() (and clean up the
hypervisor code corresponding to it).

2. We can probably remove Tamas' patch that controls if dom0, the guest,
or both can call altp2m operations (although maybe we should keep it for
the one remaining HVMOP? I'm not sure).

So to my mind, it's less, cleaner, safer code. I don't see how the
original designers of the code would object, since their goal I would
assume was helping introspection, and Tamas and us are the ones trying
to use it - furthermore these changes address the security objections of
the Xen community.

Does the plan sound reasonable?


Thanks,
Razvan

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Tamas K Lengyel]

On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
>
> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
> > On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
> > <rcojocaru@bitdefender.com> wrote:
> >> However, our particular application is only interested in setting (and
> >> querying) page restrictions from userspace (from the dom0 agent). It
> >> will also need to be able to set the convertible bit of guest pages from
> >> the dom0 agent as well (patches pending). So we're also fine with a
> >> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
> >> (possibly temporarily) is an option.
> >>
> >> We could also (at least between Tamas and us) come up with current /
> >> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
> >> in all the scenarios to DOMCTLs.
> >
> > Aye. There is really just one HVMOP that the guest absolutely needs
> > access to so that it can use #VE, and that's
> > HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
> > DOMCTL.
>
> We need even less than that - we want to modify
> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
> and we don't call it from the in-guest agent ever. Because we agree that
> the smallest attack surface is a requirement, all we ever call that's
> #VE / altp2m related is actually from the privileged domain doing
> introspection. The in-guest driver only needs to do VMFUNC and be able
> to communicate with the dom0 introspection agent.

Awesome, +1 for modifying it so that it can be called from
dom0/privileged domain!

>
> So at the moment we neither need nor use _any_ HVMOP from the guest (but
> it does make sense that the guest be able to do
> HVMOP_altp2m_vcpu_enable_notify indeed).
>
> So it would appear that at least at this time, for both us and Tamas the
> only operation that needs to be a HVMOP is HVMOP_altp2m_vcpu_enable_notify.

With the modification you are planning, even this could be just a
DOMCTL. There is no need for the guest to call that as long as the
external agent can figure out what page to use for #VE.

>
> In that case, if everyone agrees, I propose that we make all the others
> DOMCTLs. This would also have several maintenance benefits:
>
> 1. We can then get rid of the ugly compat code that was required for
> upstreaming xc_altp2m_set_mem_access_multi() (and clean up the
> hypervisor code corresponding to it).
>
> 2. We can probably remove Tamas' patch that controls if dom0, the guest,
> or both can call altp2m operations (although maybe we should keep it for
> the one remaining HVMOP? I'm not sure).
>
> So to my mind, it's less, cleaner, safer code. I don't see how the
> original designers of the code would object, since their goal I would
> assume was helping introspection, and Tamas and us are the ones trying
> to use it - furthermore these changes address the security objections of
> the Xen community.
>
> Does the plan sound reasonable?

+1 from me!

Thanks,
Tamas

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[George Dunlap]

On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>>
>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>> <rcojocaru@bitdefender.com> wrote:
>>>> However, our particular application is only interested in setting (and
>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>> will also need to be able to set the convertible bit of guest pages from
>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>> (possibly temporarily) is an option.
>>>>
>>>> We could also (at least between Tamas and us) come up with current /
>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>> in all the scenarios to DOMCTLs.
>>>
>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>> access to so that it can use #VE, and that's
>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>> DOMCTL.
>>
>> We need even less than that - we want to modify
>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>> and we don't call it from the in-guest agent ever. Because we agree that
>> the smallest attack surface is a requirement, all we ever call that's
>> #VE / altp2m related is actually from the privileged domain doing
>> introspection. The in-guest driver only needs to do VMFUNC and be able
>> to communicate with the dom0 introspection agent.

For some reason my impression was that Intel was hoping to be able to
enable a guest-only usage as well -- that basically a guest which had
been booted (say) with measured boot, and then wrote its own enclave
using #VE and altp2ms, should be able to allow an in-guest agent to be
reasonably secure and also keep tabs on the operating system.  Was this
not your impression?

>> In that case, if everyone agrees, I propose that we make all the others
>> DOMCTLs. This would also have several maintenance benefits:
>>
>> 1. We can then get rid of the ugly compat code that was required for
>> upstreaming xc_altp2m_set_mem_access_multi() (and clean up the
>> hypervisor code corresponding to it).
>>
>> 2. We can probably remove Tamas' patch that controls if dom0, the guest,
>> or both can call altp2m operations (although maybe we should keep it for
>> the one remaining HVMOP? I'm not sure).
>>
>> So to my mind, it's less, cleaner, safer code. I don't see how the
>> original designers of the code would object, since their goal I would
>> assume was helping introspection, and Tamas and us are the ones trying
>> to use it - furthermore these changes address the security objections of
>> the Xen community.
>>
>> Does the plan sound reasonable?
> 
> +1 from me!

I don't have much skin in this particular game -- my main goal in these
discussions (other than keeping Xen well-designed and reasonably secure)
has been to keep contributors happy.  If you guys are happy, and Intel
doesn't express any opinions, then I have no objections.

 -George

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 07/09/2018 02:04 PM, George Dunlap wrote:
> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
>> <rcojocaru@bitdefender.com> wrote:
>>>
>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>>> <rcojocaru@bitdefender.com> wrote:
>>>>> However, our particular application is only interested in setting (and
>>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>>> will also need to be able to set the convertible bit of guest pages from
>>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>>> (possibly temporarily) is an option.
>>>>>
>>>>> We could also (at least between Tamas and us) come up with current /
>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>>> in all the scenarios to DOMCTLs.
>>>>
>>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>>> access to so that it can use #VE, and that's
>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>>> DOMCTL.
>>>
>>> We need even less than that - we want to modify
>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>>> and we don't call it from the in-guest agent ever. Because we agree that
>>> the smallest attack surface is a requirement, all we ever call that's
>>> #VE / altp2m related is actually from the privileged domain doing
>>> introspection. The in-guest driver only needs to do VMFUNC and be able
>>> to communicate with the dom0 introspection agent.
> 
> For some reason my impression was that Intel was hoping to be able to
> enable a guest-only usage as well -- that basically a guest which had
> been booted (say) with measured boot, and then wrote its own enclave
> using #VE and altp2ms, should be able to allow an in-guest agent to be
> reasonably secure and also keep tabs on the operating system.  Was this
> not your impression?

The wording on their initial design document does say:

"- Hypercalls for altp2m

Altp2m mode introduces a new set of hypercalls for altp2m management
from software agents operating in Xen HVM guests.

The hypercalls are as follows:

Enable or Disable altp2m mode for domain
Create a new alternate p2m
Edit permissions for a specific GPA within an alternate p2m
Destroy an existing alternate p2m"

But I've always suspected that it might have been just what they have
thought would offer the most possibilities.

The problem with in-guest agents doing all these things is that it kind
of kills the whole "the introspection cannot be stopped or manipulated
at all from within the guest" assumption that gives hypervisor-level
introspection its edge - because then it's possible for in-guest code to
bypass dom0-based introspection by simply switching to a non-restricted
EPTP index, or editing the restricted pages permissions. And we're back
to in-guest protection.

Hence, Tamas has come up with the config files restrictions allowing
those HVMOPs to be called only from dom0, and our decision to basically
disable in-guest access and call them all only from dom0.

The in-guest agent should in our view be an optimization-only, not
something allowed to make its own decisions about the introspection process.


Thanks,
Razvan

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[George Dunlap]

On 07/09/2018 12:18 PM, Razvan Cojocaru wrote:
> On 07/09/2018 02:04 PM, George Dunlap wrote:
>> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
>>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
>>> <rcojocaru@bitdefender.com> wrote:
>>>>
>>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>> However, our particular application is only interested in setting (and
>>>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>>>> will also need to be able to set the convertible bit of guest pages from
>>>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>>>> (possibly temporarily) is an option.
>>>>>>
>>>>>> We could also (at least between Tamas and us) come up with current /
>>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>>>> in all the scenarios to DOMCTLs.
>>>>>
>>>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>>>> access to so that it can use #VE, and that's
>>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>>>> DOMCTL.
>>>>
>>>> We need even less than that - we want to modify
>>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>>>> and we don't call it from the in-guest agent ever. Because we agree that
>>>> the smallest attack surface is a requirement, all we ever call that's
>>>> #VE / altp2m related is actually from the privileged domain doing
>>>> introspection. The in-guest driver only needs to do VMFUNC and be able
>>>> to communicate with the dom0 introspection agent.
>>
>> For some reason my impression was that Intel was hoping to be able to
>> enable a guest-only usage as well -- that basically a guest which had
>> been booted (say) with measured boot, and then wrote its own enclave
>> using #VE and altp2ms, should be able to allow an in-guest agent to be
>> reasonably secure and also keep tabs on the operating system.  Was this
>> not your impression?
> 
> The wording on their initial design document does say:
> 
> "- Hypercalls for altp2m
> 
> Altp2m mode introduces a new set of hypercalls for altp2m management
> from software agents operating in Xen HVM guests.
> 
> The hypercalls are as follows:
> 
> Enable or Disable altp2m mode for domain
> Create a new alternate p2m
> Edit permissions for a specific GPA within an alternate p2m
> Destroy an existing alternate p2m"
> 
> But I've always suspected that it might have been just what they have
> thought would offer the most possibilities.
> 
> The problem with in-guest agents doing all these things is that it kind
> of kills the whole "the introspection cannot be stopped or manipulated
> at all from within the guest" assumption that gives hypervisor-level
> introspection its edge - because then it's possible for in-guest code to
> bypass dom0-based introspection by simply switching to a non-restricted
> EPTP index, or editing the restricted pages permissions. And we're back
> to in-guest protection.

I don't think you've quite gotten the design in mind here.  The idea is
that *all* of the "introspection" happens inside the guest.  The guest
agent is given all the tools it needs to protect itself even from the
guest operating system.  Just having a chat with Andy, apparently Intel
actually had just such an agent once upon a time, and used this
interface on Xen for real.

I think there are advantages to each model.  Obviously having something
run in the guest means that if someone manages to mess with your disk
and then cause you to reboot, it's pretty close to game-over.  But one
of the advantages is that it wouldn't rely on the host administrator to
install your introspection engine -- you could have a cloud provider
simply expose the altp2m functionality, and then each person could have
their own in-guest agent as they want.

And although arguably the in-guest agent is less secure from *being*
attacked, it does mean that the system as a whole is more secure.
Having an agent in dom0 means that if you compromise the dom0 agent, you
now have complete access to the entire system; whereas compromising your
in-guest agent gives you only control over that guest, no other guests
on the system.

> The in-guest agent should in our view be an optimization-only, not
> something allowed to make its own decisions about the introspection process.

I think having an interface which allows an only-in-guest-agent makes
sense -- particularly as one instance has already been made.  If it were
entirely up to me, I'd leave the interface, so that it's there for
someone later to pick up if they want to.

 -George

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Razvan Cojocaru]

On 07/09/2018 02:46 PM, George Dunlap wrote:
> On 07/09/2018 12:18 PM, Razvan Cojocaru wrote:
>> On 07/09/2018 02:04 PM, George Dunlap wrote:
>>> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
>>>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>
>>>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>>> However, our particular application is only interested in setting (and
>>>>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>>>>> will also need to be able to set the convertible bit of guest pages from
>>>>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>>>>> (possibly temporarily) is an option.
>>>>>>>
>>>>>>> We could also (at least between Tamas and us) come up with current /
>>>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>>>>> in all the scenarios to DOMCTLs.
>>>>>>
>>>>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>>>>> access to so that it can use #VE, and that's
>>>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>>>>> DOMCTL.
>>>>>
>>>>> We need even less than that - we want to modify
>>>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>>>>> and we don't call it from the in-guest agent ever. Because we agree that
>>>>> the smallest attack surface is a requirement, all we ever call that's
>>>>> #VE / altp2m related is actually from the privileged domain doing
>>>>> introspection. The in-guest driver only needs to do VMFUNC and be able
>>>>> to communicate with the dom0 introspection agent.
>>>
>>> For some reason my impression was that Intel was hoping to be able to
>>> enable a guest-only usage as well -- that basically a guest which had
>>> been booted (say) with measured boot, and then wrote its own enclave
>>> using #VE and altp2ms, should be able to allow an in-guest agent to be
>>> reasonably secure and also keep tabs on the operating system.  Was this
>>> not your impression?
>>
>> The wording on their initial design document does say:
>>
>> "- Hypercalls for altp2m
>>
>> Altp2m mode introduces a new set of hypercalls for altp2m management
>> from software agents operating in Xen HVM guests.
>>
>> The hypercalls are as follows:
>>
>> Enable or Disable altp2m mode for domain
>> Create a new alternate p2m
>> Edit permissions for a specific GPA within an alternate p2m
>> Destroy an existing alternate p2m"
>>
>> But I've always suspected that it might have been just what they have
>> thought would offer the most possibilities.
>>
>> The problem with in-guest agents doing all these things is that it kind
>> of kills the whole "the introspection cannot be stopped or manipulated
>> at all from within the guest" assumption that gives hypervisor-level
>> introspection its edge - because then it's possible for in-guest code to
>> bypass dom0-based introspection by simply switching to a non-restricted
>> EPTP index, or editing the restricted pages permissions. And we're back
>> to in-guest protection.
> 
> I don't think you've quite gotten the design in mind here.  The idea is
> that *all* of the "introspection" happens inside the guest.  The guest
> agent is given all the tools it needs to protect itself even from the
> guest operating system.  Just having a chat with Andy, apparently Intel
> actually had just such an agent once upon a time, and used this
> interface on Xen for real.
> 
> I think there are advantages to each model.  Obviously having something
> run in the guest means that if someone manages to mess with your disk
> and then cause you to reboot, it's pretty close to game-over.  But one
> of the advantages is that it wouldn't rely on the host administrator to
> install your introspection engine -- you could have a cloud provider
> simply expose the altp2m functionality, and then each person could have
> their own in-guest agent as they want.
> 
> And although arguably the in-guest agent is less secure from *being*
> attacked, it does mean that the system as a whole is more secure.
> Having an agent in dom0 means that if you compromise the dom0 agent, you
> now have complete access to the entire system; whereas compromising your
> in-guest agent gives you only control over that guest, no other guests
> on the system.
> 
>> The in-guest agent should in our view be an optimization-only, not
>> something allowed to make its own decisions about the introspection process.
> 
> I think having an interface which allows an only-in-guest-agent makes
> sense -- particularly as one instance has already been made.  If it were
> entirely up to me, I'd leave the interface, so that it's there for
> someone later to pick up if they want to.

That is all well argued, and is fair enough.

All I was really trying to point out was that we're happy with either
DOMCTL or HVMOPs for this (and it looks like Tamas as well) - the actual
point is to just try and settle this once for all so that altp2m
development can go forward (because really at this point its de-facto
blocked for unreasonably long ammounts of time by the design debate, on
every patch adding or modifying an operation).

Obviously actual users of a full-in-guest agent changes things - in
which case the HVMOPs likely need to remain HVMOPs if for no other
reason but to remain compatible with those users.


Thanks,
Razvan

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[George Dunlap]

On 07/09/2018 12:53 PM, Razvan Cojocaru wrote:
> On 07/09/2018 02:46 PM, George Dunlap wrote:
>> On 07/09/2018 12:18 PM, Razvan Cojocaru wrote:
>>> On 07/09/2018 02:04 PM, George Dunlap wrote:
>>>> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>>
>>>>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>>>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>>>> However, our particular application is only interested in setting (and
>>>>>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>>>>>> will also need to be able to set the convertible bit of guest pages from
>>>>>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>>>>>> (possibly temporarily) is an option.
>>>>>>>>
>>>>>>>> We could also (at least between Tamas and us) come up with current /
>>>>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>>>>>> in all the scenarios to DOMCTLs.
>>>>>>>
>>>>>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>>>>>> access to so that it can use #VE, and that's
>>>>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>>>>>> DOMCTL.
>>>>>>
>>>>>> We need even less than that - we want to modify
>>>>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>>>>>> and we don't call it from the in-guest agent ever. Because we agree that
>>>>>> the smallest attack surface is a requirement, all we ever call that's
>>>>>> #VE / altp2m related is actually from the privileged domain doing
>>>>>> introspection. The in-guest driver only needs to do VMFUNC and be able
>>>>>> to communicate with the dom0 introspection agent.
>>>>
>>>> For some reason my impression was that Intel was hoping to be able to
>>>> enable a guest-only usage as well -- that basically a guest which had
>>>> been booted (say) with measured boot, and then wrote its own enclave
>>>> using #VE and altp2ms, should be able to allow an in-guest agent to be
>>>> reasonably secure and also keep tabs on the operating system.  Was this
>>>> not your impression?
>>>
>>> The wording on their initial design document does say:
>>>
>>> "- Hypercalls for altp2m
>>>
>>> Altp2m mode introduces a new set of hypercalls for altp2m management
>>> from software agents operating in Xen HVM guests.
>>>
>>> The hypercalls are as follows:
>>>
>>> Enable or Disable altp2m mode for domain
>>> Create a new alternate p2m
>>> Edit permissions for a specific GPA within an alternate p2m
>>> Destroy an existing alternate p2m"
>>>
>>> But I've always suspected that it might have been just what they have
>>> thought would offer the most possibilities.
>>>
>>> The problem with in-guest agents doing all these things is that it kind
>>> of kills the whole "the introspection cannot be stopped or manipulated
>>> at all from within the guest" assumption that gives hypervisor-level
>>> introspection its edge - because then it's possible for in-guest code to
>>> bypass dom0-based introspection by simply switching to a non-restricted
>>> EPTP index, or editing the restricted pages permissions. And we're back
>>> to in-guest protection.
>>
>> I don't think you've quite gotten the design in mind here.  The idea is
>> that *all* of the "introspection" happens inside the guest.  The guest
>> agent is given all the tools it needs to protect itself even from the
>> guest operating system.  Just having a chat with Andy, apparently Intel
>> actually had just such an agent once upon a time, and used this
>> interface on Xen for real.
>>
>> I think there are advantages to each model.  Obviously having something
>> run in the guest means that if someone manages to mess with your disk
>> and then cause you to reboot, it's pretty close to game-over.  But one
>> of the advantages is that it wouldn't rely on the host administrator to
>> install your introspection engine -- you could have a cloud provider
>> simply expose the altp2m functionality, and then each person could have
>> their own in-guest agent as they want.
>>
>> And although arguably the in-guest agent is less secure from *being*
>> attacked, it does mean that the system as a whole is more secure.
>> Having an agent in dom0 means that if you compromise the dom0 agent, you
>> now have complete access to the entire system; whereas compromising your
>> in-guest agent gives you only control over that guest, no other guests
>> on the system.
>>
>>> The in-guest agent should in our view be an optimization-only, not
>>> something allowed to make its own decisions about the introspection process.
>>
>> I think having an interface which allows an only-in-guest-agent makes
>> sense -- particularly as one instance has already been made.  If it were
>> entirely up to me, I'd leave the interface, so that it's there for
>> someone later to pick up if they want to.
> 
> That is all well argued, and is fair enough.
> 
> All I was really trying to point out was that we're happy with either
> DOMCTL or HVMOPs for this (and it looks like Tamas as well) - the actual
> point is to just try and settle this once for all so that altp2m
> development can go forward (because really at this point its de-facto
> blocked for unreasonably long ammounts of time by the design debate, on
> every patch adding or modifying an operation).

Sure, it's certainly unpleasant, and unfair, every time you want to
extend the interface for your own use case, to get into a big argument
about what should happen with respect to some other use case, which not
only *you* aren't using, but it would seem, *nobody* at the moment is
using.

And I remember now, the last time this came up you wanted to add a new
subop, and Jan said it should be a domctl unless we knew we wanted to
expose it to the guest, and Andy said it should be with all the other
functionality (i.e., an hvmop).

This is just silly.  We already decided they should all be HVMOPs.  If
we want to disable ALTP2M_mixed mode until someone wants to audit it --
or whitelist only the curret functionality -- that's one thing.  But we
shouldn't make a second hypercall for non-guest-accessible
functionality, nor should we even consider changing the current HVMOP
into a DOMCTL.  Absolutely not.

We do need to stop having pointless arguments, however.  Let me see what
I can come up with.

 -George

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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Sergej Proskurin]

Hi all,

as I am currently working on a concept that uses the #VE functionality
from inside of the unprivileged guest domain myself, I would like to add
my opinion to the discussion.


On 07/09/2018 07:53 AM, Razvan Cojocaru wrote:
> On 07/09/2018 02:46 PM, George Dunlap wrote:
>> On 07/09/2018 12:18 PM, Razvan Cojocaru wrote:
>>> On 07/09/2018 02:04 PM, George Dunlap wrote:
>>>> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
>>>>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
>>>>>>> <rcojocaru@bitdefender.com> wrote:
>>>>>>>> However, our particular application is only interested in setting (and
>>>>>>>> querying) page restrictions from userspace (from the dom0 agent). It
>>>>>>>> will also need to be able to set the convertible bit of guest pages from
>>>>>>>> the dom0 agent as well (patches pending). So we're also fine with a
>>>>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
>>>>>>>> (possibly temporarily) is an option.
>>>>>>>>
>>>>>>>> We could also (at least between Tamas and us) come up with current /
>>>>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
>>>>>>>> in all the scenarios to DOMCTLs.
>>>>>>> Aye. There is really just one HVMOP that the guest absolutely needs
>>>>>>> access to so that it can use #VE, and that's
>>>>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
>>>>>>> DOMCTL.
>>>>>> We need even less than that - we want to modify
>>>>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
>>>>>> and we don't call it from the in-guest agent ever. Because we agree that
>>>>>> the smallest attack surface is a requirement, all we ever call that's
>>>>>> #VE / altp2m related is actually from the privileged domain doing
>>>>>> introspection. The in-guest driver only needs to do VMFUNC and be able
>>>>>> to communicate with the dom0 introspection agent.
>>>> For some reason my impression was that Intel was hoping to be able to
>>>> enable a guest-only usage as well -- that basically a guest which had
>>>> been booted (say) with measured boot, and then wrote its own enclave
>>>> using #VE and altp2ms, should be able to allow an in-guest agent to be
>>>> reasonably secure and also keep tabs on the operating system.  Was this
>>>> not your impression?

I absolutely agree upon that Intel was building a system that allows
guest domains to enable and control the #VE (including the funcitonality
to set up different altp2ms). Although this functionality has not been
widely adopted (yet?), I personally would prefer a hybrid solution that
does not completely prohibit this concept from inside of the
unprivileged guest domain. I agree with Tamas upon the fact that some
concepts can be equally implemented by using the guest's page tables
only. However, (I understand that I am biased, as I am working on a
concept that makes use of this functionality from inside of domu), I
also believe that we can apply the functionality given by #VE and VMFUNC
from inside the guest to harden certain system resources. As such, I
would be happy to see a hybrid solution that allows this feature to be
configured either for unlimited or for external use only.

Best,
~Sergej



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

Re: [PATCH] x86/altp2m: Add a subop for obtaining the mem access of a page

[Tamas K Lengyel]

On Mon, Jul 9, 2018 at 8:50 AM Sergej Proskurin <proskurin@sec.in.tum.de> wrote:
>
> Hi all,
>
> as I am currently working on a concept that uses the #VE functionality
> from inside of the unprivileged guest domain myself, I would like to add
> my opinion to the discussion.
>
>
> On 07/09/2018 07:53 AM, Razvan Cojocaru wrote:
> > On 07/09/2018 02:46 PM, George Dunlap wrote:
> >> On 07/09/2018 12:18 PM, Razvan Cojocaru wrote:
> >>> On 07/09/2018 02:04 PM, George Dunlap wrote:
> >>>> On 07/06/2018 05:52 PM, Tamas K Lengyel wrote:
> >>>>> On Fri, Jul 6, 2018 at 2:56 AM Razvan Cojocaru
> >>>>> <rcojocaru@bitdefender.com> wrote:
> >>>>>> On 07/05/2018 07:45 PM, Tamas K Lengyel wrote:
> >>>>>>> On Thu, Jul 5, 2018 at 9:22 AM Razvan Cojocaru
> >>>>>>> <rcojocaru@bitdefender.com> wrote:
> >>>>>>>> However, our particular application is only interested in setting (and
> >>>>>>>> querying) page restrictions from userspace (from the dom0 agent). It
> >>>>>>>> will also need to be able to set the convertible bit of guest pages from
> >>>>>>>> the dom0 agent as well (patches pending). So we're also fine with a
> >>>>>>>> "DOMCTL if nobody wants it as a HVMOP" policy, if polluting the DOMCTLs
> >>>>>>>> (possibly temporarily) is an option.
> >>>>>>>>
> >>>>>>>> We could also (at least between Tamas and us) come up with current /
> >>>>>>>> likely use-cases and downgrade all altp2m HVMOPs that could be DOMCTLs
> >>>>>>>> in all the scenarios to DOMCTLs.
> >>>>>>> Aye. There is really just one HVMOP that the guest absolutely needs
> >>>>>>> access to so that it can use #VE, and that's
> >>>>>>> HVMOP_altp2m_vcpu_enable_notify. AFAIU everything else could be just a
> >>>>>>> DOMCTL.
> >>>>>> We need even less than that - we want to modify
> >>>>>> HVMOP_altp2m_vcpu_enable_notify to be able to call it from dom0 as well,
> >>>>>> and we don't call it from the in-guest agent ever. Because we agree that
> >>>>>> the smallest attack surface is a requirement, all we ever call that's
> >>>>>> #VE / altp2m related is actually from the privileged domain doing
> >>>>>> introspection. The in-guest driver only needs to do VMFUNC and be able
> >>>>>> to communicate with the dom0 introspection agent.
> >>>> For some reason my impression was that Intel was hoping to be able to
> >>>> enable a guest-only usage as well -- that basically a guest which had
> >>>> been booted (say) with measured boot, and then wrote its own enclave
> >>>> using #VE and altp2ms, should be able to allow an in-guest agent to be
> >>>> reasonably secure and also keep tabs on the operating system.  Was this
> >>>> not your impression?
>
> I absolutely agree upon that Intel was building a system that allows
> guest domains to enable and control the #VE (including the funcitonality
> to set up different altp2ms). Although this functionality has not been
> widely adopted (yet?), I personally would prefer a hybrid solution that
> does not completely prohibit this concept from inside of the
> unprivileged guest domain. I agree with Tamas upon the fact that some
> concepts can be equally implemented by using the guest's page tables
> only. However, (I understand that I am biased, as I am working on a
> concept that makes use of this functionality from inside of domu), I
> also believe that we can apply the functionality given by #VE and VMFUNC
> from inside the guest to harden certain system resources. As such, I
> would be happy to see a hybrid solution that allows this feature to be
> configured either for unlimited or for external use only.

Thanks for the input Sergej. With that and George pointing out other
users of the in-guest use-case we can't just do the switch. Letting
people decide using the existing domain config option / XSM what way
they want to have the interface accessible is not the worst thing in
the world. Introducing further, more restricted in-guest accessible
modes could be done potentially in the future that only allows the #VE
page-setup op to go through - if there is a need for it.

Tamas

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