[PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

For the default EPT view we have xc_set_mem_access_multi(), which
is able to set an array of pages to an array of access rights with
a single hypercall. However, this functionality was lacking for the
altp2m subsystem, which could only set page restrictions for one
page at a time. This patch addresses the gap.

Signed-off-by: Razvan Cojocaru <rcojocaru@bitdefender.com>

---
Changes since V1:
 - Replaced the mask-based hypercall continuation code with code
   using an opaque member to store the index.
---
 tools/libxc/include/xenctrl.h   |  3 +++
 tools/libxc/xc_altp2m.c         | 41 +++++++++++++++++++++++++++++++++++++++++
 xen/arch/x86/hvm/hvm.c          | 25 +++++++++++++++++++++++++
 xen/include/public/hvm/hvm_op.h | 30 +++++++++++++++++++++++++-----
 4 files changed, 94 insertions(+), 5 deletions(-)

diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
index a48981a..645b5bd 100644
--- a/tools/libxc/include/xenctrl.h
+++ b/tools/libxc/include/xenctrl.h
@@ -1903,6 +1903,9 @@ int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
 int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
                              uint16_t view_id, xen_pfn_t gfn,
                              xenmem_access_t access);
+int xc_altp2m_set_mem_access_multi(xc_interface *handle, domid_t domid,
+                                   uint16_t view_id, uint8_t *access,
+                                   uint64_t *pages, uint32_t nr);
 int xc_altp2m_change_gfn(xc_interface *handle, domid_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 0639632..f202ca1 100644
--- a/tools/libxc/xc_altp2m.c
+++ b/tools/libxc/xc_altp2m.c
@@ -188,6 +188,47 @@ int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
     return rc;
 }
 
+int xc_altp2m_set_mem_access_multi(xc_interface *xch, domid_t domid,
+                                   uint16_t view_id, uint8_t *access,
+                                   uint64_t *pages, uint32_t nr)
+{
+    int rc;
+
+    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
+    DECLARE_HYPERCALL_BOUNCE(access, nr, XC_HYPERCALL_BUFFER_BOUNCE_IN);
+    DECLARE_HYPERCALL_BOUNCE(pages, nr * sizeof(uint64_t),
+                             XC_HYPERCALL_BUFFER_BOUNCE_IN);
+
+    arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
+    if ( arg == NULL )
+        return -1;
+
+    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
+    arg->cmd = HVMOP_altp2m_set_mem_access_multi;
+    arg->domain = domid;
+    arg->u.set_mem_access_multi.view = view_id;
+    arg->u.set_mem_access_multi.nr = nr;
+
+    if ( xc_hypercall_bounce_pre(xch, pages) ||
+         xc_hypercall_bounce_pre(xch, access) )
+    {
+        PERROR("Could not bounce memory for HVMOP_altp2m_set_mem_access_multi");
+        return -1;
+    }
+
+    set_xen_guest_handle(arg->u.set_mem_access_multi.pfn_list, pages);
+    set_xen_guest_handle(arg->u.set_mem_access_multi.access_list, access);
+
+    rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
+		  HYPERCALL_BUFFER_AS_ARG(arg));
+
+    xc_hypercall_buffer_free(xch, arg);
+    xc_hypercall_bounce_post(xch, access);
+    xc_hypercall_bounce_post(xch, pages);
+
+    return rc;
+}
+
 int xc_altp2m_change_gfn(xc_interface *handle, domid_t domid,
                          uint16_t view_id, xen_pfn_t old_gfn,
                          xen_pfn_t new_gfn)
diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index ccfae4f..12b34da 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4419,6 +4419,7 @@ static int do_altp2m_op(
     case HVMOP_altp2m_destroy_p2m:
     case HVMOP_altp2m_switch_p2m:
     case HVMOP_altp2m_set_mem_access:
+    case HVMOP_altp2m_set_mem_access_multi:
     case HVMOP_altp2m_change_gfn:
         break;
     default:
@@ -4535,6 +4536,30 @@ static int do_altp2m_op(
                                     a.u.set_mem_access.view);
         break;
 
+    case HVMOP_altp2m_set_mem_access_multi:
+        if ( a.u.set_mem_access_multi.pad ||
+             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
+                                      a.u.set_mem_access_multi.access_list,
+                                      a.u.set_mem_access_multi.nr,
+                                      a.u.set_mem_access_multi.opaque,
+                                      MEMOP_CMD_MASK,
+                                      a.u.set_mem_access_multi.view);
+        if ( rc > 0 )
+        {
+            a.u.set_mem_access_multi.opaque = rc;
+            if ( __copy_to_guest(arg, &a, 1) )
+                rc = -EFAULT;
+            else
+                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
+                                                   HVMOP_altp2m, arg);
+        }
+        break;
+
     case HVMOP_altp2m_change_gfn:
         if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
             rc = -EINVAL;
diff --git a/xen/include/public/hvm/hvm_op.h b/xen/include/public/hvm/hvm_op.h
index bc00ef0..f2429f8 100644
--- a/xen/include/public/hvm/hvm_op.h
+++ b/xen/include/public/hvm/hvm_op.h
@@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
 typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
 DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
 
+struct xen_hvm_altp2m_set_mem_access_multi {
+    /* view */
+    uint16_t view;
+    uint16_t pad;
+    /* Number of pages */
+    uint32_t nr;
+    /* Used for continuation purposes */
+    uint64_t opaque;
+    /* List of pfns to set access for */
+    XEN_GUEST_HANDLE(const_uint64) pfn_list;
+    /* Corresponding list of access settings for pfn_list */
+    XEN_GUEST_HANDLE(const_uint8) access_list;
+};
+typedef struct xen_hvm_altp2m_set_mem_access_multi
+    xen_hvm_altp2m_set_mem_access_multi_t;
+DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_multi_t);
+
 struct xen_hvm_altp2m_change_gfn {
     /* view */
     uint16_t view;
@@ -262,15 +279,18 @@ struct xen_hvm_altp2m_op {
 #define HVMOP_altp2m_set_mem_access       7
 /* Change a p2m entry to have a different gfn->mfn mapping */
 #define HVMOP_altp2m_change_gfn           8
+/* Set access for an array of pages */
+#define HVMOP_altp2m_set_mem_access_multi 9
     domid_t domain;
     uint16_t pad1;
     uint32_t pad2;
     union {
-        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_change_gfn         change_gfn;
+        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_change_gfn           change_gfn;
+        struct xen_hvm_altp2m_set_mem_access_multi set_mem_access_multi;
         uint8_t pad[64];
     } u;
 };
-- 
1.9.1


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>                                      a.u.set_mem_access.view);
>          break;
>  
> +    case HVMOP_altp2m_set_mem_access_multi:
> +        if ( a.u.set_mem_access_multi.pad ||
> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
> +        {
> +            rc = -EINVAL;
> +            break;
> +        }
> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
> +                                      a.u.set_mem_access_multi.access_list,
> +                                      a.u.set_mem_access_multi.nr,
> +                                      a.u.set_mem_access_multi.opaque,
> +                                      MEMOP_CMD_MASK,
> +                                      a.u.set_mem_access_multi.view);
> +        if ( rc > 0 )
> +        {
> +            a.u.set_mem_access_multi.opaque = rc;
> +            if ( __copy_to_guest(arg, &a, 1) )
> +                rc = -EFAULT;
> +            else
> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
> +                                                   HVMOP_altp2m, arg);
> +        }
> +        break;

Okay, so this is a hvmop, in which case I'm fine with the continuation
model used.

However - is this interface supposed to be usable by a guest on itself?
Arguably the same question would apply to some of the other sub-ops
too, but anyway.

> --- a/xen/include/public/hvm/hvm_op.h
> +++ b/xen/include/public/hvm/hvm_op.h
> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>  
> +struct xen_hvm_altp2m_set_mem_access_multi {
> +    /* view */
> +    uint16_t view;
> +    uint16_t pad;
> +    /* Number of pages */
> +    uint32_t nr;
> +    /* Used for continuation purposes */
> +    uint64_t opaque;
> +    /* List of pfns to set access for */
> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
> +    /* Corresponding list of access settings for pfn_list */
> +    XEN_GUEST_HANDLE(const_uint8) access_list;

I'm afraid these handles aren't going to work for a 32-bit guest. Note
how no other hvmop uses handles.

Jan


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>  
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> +                                      a.u.set_mem_access_multi.access_list,
>> +                                      a.u.set_mem_access_multi.nr,
>> +                                      a.u.set_mem_access_multi.opaque,
>> +                                      MEMOP_CMD_MASK,
>> +                                      a.u.set_mem_access_multi.view);
>> +        if ( rc > 0 )
>> +        {
>> +            a.u.set_mem_access_multi.opaque = rc;
>> +            if ( __copy_to_guest(arg, &a, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>> +                                                   HVMOP_altp2m, arg);
>> +        }
>> +        break;
> 
> Okay, so this is a hvmop, in which case I'm fine with the continuation
> model used.
> 
> However - is this interface supposed to be usable by a guest on itself?
> Arguably the same question would apply to some of the other sub-op
> too, but anyway.

Not for any of our use cases. The whole point is for dom0 (or another
suitably privileged domain) to monitor another guest that consequently
can't, by design, evade detection of bad behaviour by acting at a higher
privilege level than the protection software. It wouldn't make sense for
a domain to be doing this on itself.

Maybe Tamas has something in mind, but the short answer is no, it's not.

>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.ht
>> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>  
>> +struct xen_hvm_altp2m_set_mem_access_multi {
>> +    /* view */
>> +    uint16_t view;
>> +    uint16_t pad;
>> +    /* Number of pages */
>> +    uint32_t nr;
>> +    /* Used for continuation purposes */
>> +    uint64_t opaque;
>> +    /* List of pfns to set access for */
>> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
>> +    /* Corresponding list of access settings for pfn_list */
>> +    XEN_GUEST_HANDLE(const_uint8) access_list;
> 
> I'm afraid these handles aren't going to work for a 32-bit guest. Note
> how no other hvmop uses handles.

Right, I guess I'll have to pass these through the compat code then,
like the xc_set_mem_access_multi() code does. I'll take a look at what
that entails.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 09.03.17 at 18:15, <rcojocaru@bitdefender.com> wrote:
> On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>>                                      a.u.set_mem_access.view);
>>>          break;
>>>  
>>> +    case HVMOP_altp2m_set_mem_access_multi:
>>> +        if ( a.u.set_mem_access_multi.pad ||
>>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr 
> )
>>> +        {
>>> +            rc = -EINVAL;
>>> +            break;
>>> +        }
>>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>>> +                                      a.u.set_mem_access_multi.access_list,
>>> +                                      a.u.set_mem_access_multi.nr,
>>> +                                      a.u.set_mem_access_multi.opaque,
>>> +                                      MEMOP_CMD_MASK,
>>> +                                      a.u.set_mem_access_multi.view);
>>> +        if ( rc > 0 )
>>> +        {
>>> +            a.u.set_mem_access_multi.opaque = rc;
>>> +            if ( __copy_to_guest(arg, &a, 1) )
>>> +                rc = -EFAULT;
>>> +            else
>>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, 
> "lh",
>>> +                                                   HVMOP_altp2m, arg);
>>> +        }
>>> +        break;
>> 
>> Okay, so this is a hvmop, in which case I'm fine with the continuation
>> model used.
>> 
>> However - is this interface supposed to be usable by a guest on itself?
>> Arguably the same question would apply to some of the other sub-op
>> too, but anyway.
> 
> Not for any of our use cases. The whole point is for dom0 (or another
> suitably privileged domain) to monitor another guest that consequently
> can't, by design, evade detection of bad behaviour by acting at a higher
> privilege level than the protection software. It wouldn't make sense for
> a domain to be doing this on itself.

In which case this should be a domctl.

>>> --- a/xen/include/public/hvm/hvm_op.h
>>> +++ b/xen/include/public/hvm/hvm_op.ht
>>> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>>>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>>  
>>> +struct xen_hvm_altp2m_set_mem_access_multi {
>>> +    /* view */
>>> +    uint16_t view;
>>> +    uint16_t pad;
>>> +    /* Number of pages */
>>> +    uint32_t nr;
>>> +    /* Used for continuation purposes */
>>> +    uint64_t opaque;
>>> +    /* List of pfns to set access for */
>>> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
>>> +    /* Corresponding list of access settings for pfn_list */
>>> +    XEN_GUEST_HANDLE(const_uint8) access_list;
>> 
>> I'm afraid these handles aren't going to work for a 32-bit guest. Note
>> how no other hvmop uses handles.
> 
> Right, I guess I'll have to pass these through the compat code then,
> like the xc_set_mem_access_multi() code does. I'll take a look at what
> that entails.

Which in turn means you don't need to go through the hassles of
this.

Jan


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/10/2017 09:31 AM, Jan Beulich wrote:
>>>> On 09.03.17 at 18:15, <rcojocaru@bitdefender.com> wrote:
>> On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>>>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>>>                                      a.u.set_mem_access.view);
>>>>          break;
>>>>  
>>>> +    case HVMOP_altp2m_set_mem_access_multi:
>>>> +        if ( a.u.set_mem_access_multi.pad ||
>>>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr 
>> )
>>>> +        {
>>>> +            rc = -EINVAL;
>>>> +            break;
>>>> +        }
>>>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>>>> +                                      a.u.set_mem_access_multi.access_list,
>>>> +                                      a.u.set_mem_access_multi.nr,
>>>> +                                      a.u.set_mem_access_multi.opaque,
>>>> +                                      MEMOP_CMD_MASK,
>>>> +                                      a.u.set_mem_access_multi.view);
>>>> +        if ( rc > 0 )
>>>> +        {
>>>> +            a.u.set_mem_access_multi.opaque = rc;
>>>> +            if ( __copy_to_guest(arg, &a, 1) )
>>>> +                rc = -EFAULT;
>>>> +            else
>>>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, 
>> "lh",
>>>> +                                                   HVMOP_altp2m, arg);
>>>> +        }
>>>> +        break;
>>>
>>> Okay, so this is a hvmop, in which case I'm fine with the continuation
>>> model used.
>>>
>>> However - is this interface supposed to be usable by a guest on itself?
>>> Arguably the same question would apply to some of the other sub-op
>>> too, but anyway.
>>
>> Not for any of our use cases. The whole point is for dom0 (or another
>> suitably privileged domain) to monitor another guest that consequently
>> can't, by design, evade detection of bad behaviour by acting at a higher
>> privilege level than the protection software. It wouldn't make sense for
>> a domain to be doing this on itself.
> 
> In which case this should be a domctl.

Fair enough, if nobody objects I'll then just modify
XENMEM_access_op_set_access_multi to take a view_id as well an just
piggyback on that. It already does the right thing underneath.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Tamas K Lengyel]

On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> +                                      a.u.set_mem_access_multi.access_list,
>> +                                      a.u.set_mem_access_multi.nr,
>> +                                      a.u.set_mem_access_multi.opaque,
>> +                                      MEMOP_CMD_MASK,
>> +                                      a.u.set_mem_access_multi.view);
>> +        if ( rc > 0 )
>> +        {
>> +            a.u.set_mem_access_multi.opaque = rc;
>> +            if ( __copy_to_guest(arg, &a, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>> +                                                   HVMOP_altp2m, arg);
>> +        }
>> +        break;
>
> Okay, so this is a hvmop, in which case I'm fine with the continuation
> model used.
>
> However - is this interface supposed to be usable by a guest on itself?
> Arguably the same question would apply to some of the other sub-ops
> too, but anyway.
>

AFAIK the only op the guest would use on itself is
HVMOP_altp2m_vcpu_enable_notify.

Tamas

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>> However - is this interface supposed to be usable by a guest on itself?
>> Arguably the same question would apply to some of the other sub-ops
>> too, but anyway.
> 
> AFAIK the only op the guest would use on itself is
> HVMOP_altp2m_vcpu_enable_notify.

Which then means we should move all of them out of here, into a
suitable domctl. That will in turn reduce the scope of the bogus
interface versioning, which Andrew did point out, quite a bit.

Jan


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Andrew Cooper]

On 10/03/17 07:34, Jan Beulich wrote:
>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>> However - is this interface supposed to be usable by a guest on itself?
>>> Arguably the same question would apply to some of the other sub-ops
>>> too, but anyway.
>> AFAIK the only op the guest would use on itself is
>> HVMOP_altp2m_vcpu_enable_notify.
> Which then means we should move all of them out of here, into a
> suitable domctl. That will in turn reduce the scope of the bogus
> interface versioning, which Andrew did point out, quite a bit.

The original usecase for altp2m was for an entirely in-guest agent,
which is why they got in as hvmops to start with.  I don't think it is
wise to break that.

I think there needs to be slightly finer grain control, identifying
whether a domain may use altp2m, and whether it may configure altp2m
permissions itself.

The nature of altp2m means that using EPTP switching/etc necessarily can
only happen from inside guest context, but whether you trust the domain
to make adjustments to the permissions itself depends on your usecase
and threat model.

~Andrew

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Tamas K Lengyel]

On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> On 10/03/17 07:34, Jan Beulich wrote:
>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>> However - is this interface supposed to be usable by a guest on itself?
>>>> Arguably the same question would apply to some of the other sub-ops
>>>> too, but anyway.
>>> AFAIK the only op the guest would use on itself is
>>> HVMOP_altp2m_vcpu_enable_notify.
>> Which then means we should move all of them out of here, into a
>> suitable domctl. That will in turn reduce the scope of the bogus
>> interface versioning, which Andrew did point out, quite a bit.
>
> The original usecase for altp2m was for an entirely in-guest agent,
> which is why they got in as hvmops to start with.  I don't think it is
> wise to break that.
>
> I think there needs to be slightly finer grain control, identifying
> whether a domain may use altp2m, and whether it may configure altp2m
> permissions itself.
>
> The nature of altp2m means that using EPTP switching/etc necessarily can
> only happen from inside guest context, but whether you trust the domain
> to make adjustments to the permissions itself depends on your usecase
> and threat model.
>

So I'm actively using EPT switching and gfn remapping from a
privileged monitor domain (not with VMFUNC). My entire usecase for
altp2m is purely external without any in-guest agents. In fact, I have
to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
from the guest.

The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
only one I believe that is only accessible from within the guest is
this distinction in arch/x86/hvm/hvm.c:

    d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
        rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();

For the other ops I'm not sure if they were really required to be
accessible from within the guest or not. I'm not even sure using them
would work from the guest with the above check in place. However, if
they do work from the guest then I have no idea how it was supposed to
work for security purposes as any application in that guest could just
issue a hypercall to manipulate it or even turn it off.

Tamas

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
> <andrew.cooper3@citrix.com> wrote:
>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>> too, but anyway.
>>>> AFAIK the only op the guest would use on itself is
>>>> HVMOP_altp2m_vcpu_enable_notify.
>>> Which then means we should move all of them out of here, into a
>>> suitable domctl. That will in turn reduce the scope of the bogus
>>> interface versioning, which Andrew did point out, quite a bit.
>>
>> The original usecase for altp2m was for an entirely in-guest agent,
>> which is why they got in as hvmops to start with.  I don't think it is
>> wise to break that.
>>
>> I think there needs to be slightly finer grain control, identifying
>> whether a domain may use altp2m, and whether it may configure altp2m
>> permissions itself.
>>
>> The nature of altp2m means that using EPTP switching/etc necessarily can
>> only happen from inside guest context, but whether you trust the domain
>> to make adjustments to the permissions itself depends on your usecase
>> and threat model.
>>
> 
> So I'm actively using EPT switching and gfn remapping from a
> privileged monitor domain (not with VMFUNC). My entire usecase for
> altp2m is purely external without any in-guest agents. In fact, I have
> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
> from the guest.
> 
> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
> only one I believe that is only accessible from within the guest is
> this distinction in arch/x86/hvm/hvm.c:
> 
>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
> 
> For the other ops I'm not sure if they were really required to be
> accessible from within the guest or not. I'm not even sure using them
> would work from the guest with the above check in place. However, if
> they do work from the guest then I have no idea how it was supposed to
> work for security purposes as any application in that guest could just
> issue a hypercall to manipulate it or even turn it off.

Thanks to all for the replies! What I'm taking away from this is:

1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.

2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
HVMOP_altp2m_vcpu_enable_notify).

3. If we keep them as HVMOPs, the code for handling the set_mem_access()
part needs to be duplicated, both for the hypercall continuation / HVMOP
hypercall structure part, and for the compat part (since the _multi()
function sends arrays / handles to the hypervisor).

So an agreement on point 2 is required before being able to proceed.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 13.03.17 at 13:01, <rcojocaru@bitdefender.com> wrote:
> On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
>> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
>> <andrew.cooper3@citrix.com> wrote:
>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>>> too, but anyway.
>>>>> AFAIK the only op the guest would use on itself is
>>>>> HVMOP_altp2m_vcpu_enable_notify.
>>>> Which then means we should move all of them out of here, into a
>>>> suitable domctl. That will in turn reduce the scope of the bogus
>>>> interface versioning, which Andrew did point out, quite a bit.
>>>
>>> The original usecase for altp2m was for an entirely in-guest agent,
>>> which is why they got in as hvmops to start with.  I don't think it is
>>> wise to break that.
>>>
>>> I think there needs to be slightly finer grain control, identifying
>>> whether a domain may use altp2m, and whether it may configure altp2m
>>> permissions itself.
>>>
>>> The nature of altp2m means that using EPTP switching/etc necessarily can
>>> only happen from inside guest context, but whether you trust the domain
>>> to make adjustments to the permissions itself depends on your usecase
>>> and threat model.
>>>
>> 
>> So I'm actively using EPT switching and gfn remapping from a
>> privileged monitor domain (not with VMFUNC). My entire usecase for
>> altp2m is purely external without any in-guest agents. In fact, I have
>> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
>> from the guest.
>> 
>> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
>> only one I believe that is only accessible from within the guest is
>> this distinction in arch/x86/hvm/hvm.c:
>> 
>>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>> 
>> For the other ops I'm not sure if they were really required to be
>> accessible from within the guest or not. I'm not even sure using them
>> would work from the guest with the above check in place. However, if
>> they do work from the guest then I have no idea how it was supposed to
>> work for security purposes as any application in that guest could just
>> issue a hypercall to manipulate it or even turn it off.
> 
> Thanks to all for the replies! What I'm taking away from this is:
> 
> 1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.
> 
> 2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
> HVMOP_altp2m_vcpu_enable_notify).
> 
> 3. If we keep them as HVMOPs, the code for handling the set_mem_access()
> part needs to be duplicated, both for the hypercall continuation / HVMOP
> hypercall structure part, and for the compat part (since the _multi()
> function sends arrays / handles to the hypervisor).
> 
> So an agreement on point 2 is required before being able to proceed.

I think as long as there's no need for the guest to use an operation
on itself, it should not be a hvmop. After all, if you make it a domctl
now and later find a need for it to be called by the guest, we can
always replace the domctl by a hvmop. If, however, you start out
with a hvmop, we'll be bound to be supporting it virtually forever.

Jan


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>>> On 13.03.17 at 13:01, <rcojocaru@bitdefender.com> wrote:
>> On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
>>> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
>>> <andrew.cooper3@citrix.com> wrote:
>>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>>>> too, but anyway.
>>>>>> AFAIK the only op the guest would use on itself is
>>>>>> HVMOP_altp2m_vcpu_enable_notify.
>>>>> Which then means we should move all of them out of here, into a
>>>>> suitable domctl. That will in turn reduce the scope of the bogus
>>>>> interface versioning, which Andrew did point out, quite a bit.
>>>>
>>>> The original usecase for altp2m was for an entirely in-guest agent,
>>>> which is why they got in as hvmops to start with.  I don't think it is
>>>> wise to break that.
>>>>
>>>> I think there needs to be slightly finer grain control, identifying
>>>> whether a domain may use altp2m, and whether it may configure altp2m
>>>> permissions itself.
>>>>
>>>> The nature of altp2m means that using EPTP switching/etc necessarily can
>>>> only happen from inside guest context, but whether you trust the domain
>>>> to make adjustments to the permissions itself depends on your usecase
>>>> and threat model.
>>>>
>>>
>>> So I'm actively using EPT switching and gfn remapping from a
>>> privileged monitor domain (not with VMFUNC). My entire usecase for
>>> altp2m is purely external without any in-guest agents. In fact, I have
>>> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
>>> from the guest.
>>>
>>> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
>>> only one I believe that is only accessible from within the guest is
>>> this distinction in arch/x86/hvm/hvm.c:
>>>
>>>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>>>
>>> For the other ops I'm not sure if they were really required to be
>>> accessible from within the guest or not. I'm not even sure using them
>>> would work from the guest with the above check in place. However, if
>>> they do work from the guest then I have no idea how it was supposed to
>>> work for security purposes as any application in that guest could just
>>> issue a hypercall to manipulate it or even turn it off.
>>
>> Thanks to all for the replies! What I'm taking away from this is:
>>
>> 1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.
>>
>> 2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
>> HVMOP_altp2m_vcpu_enable_notify).
>>
>> 3. If we keep them as HVMOPs, the code for handling the set_mem_access()
>> part needs to be duplicated, both for the hypercall continuation / HVMOP
>> hypercall structure part, and for the compat part (since the _multi()
>> function sends arrays / handles to the hypervisor).
>>
>> So an agreement on point 2 is required before being able to proceed.
> 
> I think as long as there's no need for the guest to use an operation
> on itself, it should not be a hvmop. After all, if you make it a domctl
> now and later find a need for it to be called by the guest, we can
> always replace the domctl by a hvmop. If, however, you start out
> with a hvmop, we'll be bound to be supporting it virtually forever.

Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
set_mem_access() and set_mem_access_multi() shouldn't exist at all.
Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
should be enough, as long as we also add the view_id as an
extra-parameter, where view ID 0 is (already) the default EPT view.

As it stands now, xc_set_mem_access() can do less than
xc_altp2m_set_mem_access() in that its view ID is always 0, but more
than xc_altp2m_set_mem_access() in that it is able to set more than one
page with a single hypercall, while the underlying hypervisor code is
the same.

Maybe I'm missing something design-wise (obviously if these really do
need to be HVMOPs a separate libxc function is required). Maybe the
altp2m maintainers have a different view of the matter.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Jan Beulich]

>>> On 13.03.17 at 13:29, <rcojocaru@bitdefender.com> wrote:
> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>> I think as long as there's no need for the guest to use an operation
>> on itself, it should not be a hvmop. After all, if you make it a domctl
>> now and later find a need for it to be called by the guest, we can
>> always replace the domctl by a hvmop. If, however, you start out
>> with a hvmop, we'll be bound to be supporting it virtually forever.
> 
> Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
> set_mem_access() and set_mem_access_multi() shouldn't exist at all.
> Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
> should be enough, as long as we also add the view_id as an
> extra-parameter, where view ID 0 is (already) the default EPT view.
> 
> As it stands now, xc_set_mem_access() can do less than
> xc_altp2m_set_mem_access() in that its view ID is always 0, but more
> than xc_altp2m_set_mem_access() in that it is able to set more than one
> page with a single hypercall, while the underlying hypervisor code is
> the same.
> 
> Maybe I'm missing something design-wise (obviously if these really do
> need to be HVMOPs a separate libxc function is required). Maybe the
> altp2m maintainers have a different view of the matter.

"altp2m maintainers"? Maintainership is covered by the x86/mm/
entry afaict, so perhaps you mean the original altp2m authors. In
any event - you didn't Cc anyone of them.

Jan


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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/13/2017 02:40 PM, Jan Beulich wrote:
>>>> On 13.03.17 at 13:29, <rcojocaru@bitdefender.com> wrote:
>> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>> I think as long as there's no need for the guest to use an operation
>>> on itself, it should not be a hvmop. After all, if you make it a domctl
>>> now and later find a need for it to be called by the guest, we can
>>> always replace the domctl by a hvmop. If, however, you start out
>>> with a hvmop, we'll be bound to be supporting it virtually forever.
>>
>> Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
>> set_mem_access() and set_mem_access_multi() shouldn't exist at all.
>> Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
>> should be enough, as long as we also add the view_id as an
>> extra-parameter, where view ID 0 is (already) the default EPT view.
>>
>> As it stands now, xc_set_mem_access() can do less than
>> xc_altp2m_set_mem_access() in that its view ID is always 0, but more
>> than xc_altp2m_set_mem_access() in that it is able to set more than one
>> page with a single hypercall, while the underlying hypervisor code is
>> the same.
>>
>> Maybe I'm missing something design-wise (obviously if these really do
>> need to be HVMOPs a separate libxc function is required). Maybe the
>> altp2m maintainers have a different view of the matter.
> 
> "altp2m maintainers"? Maintainership is covered by the x86/mm/
> entry afaict, so perhaps you mean the original altp2m authors. In
> any event - you didn't Cc anyone of them.

I've used scripts/get_maintainer.pl on the patch, and CCd everyone in
the output. Consulting the original authors is a very good idea, I've
CCd Ravi Sahita here, perhaps he can help us out.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Tamas K Lengyel]

On Mon, Mar 13, 2017 at 6:29 AM, Razvan Cojocaru
<rcojocaru@bitdefender.com> wrote:
> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>>>> On 13.03.17 at 13:01, <rcojocaru@bitdefender.com> wrote:
>>> On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
>>>> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
>>>> <andrew.cooper3@citrix.com> wrote:
>>>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>>>>> too, but anyway.
>>>>>>> AFAIK the only op the guest would use on itself is
>>>>>>> HVMOP_altp2m_vcpu_enable_notify.
>>>>>> Which then means we should move all of them out of here, into a
>>>>>> suitable domctl. That will in turn reduce the scope of the bogus
>>>>>> interface versioning, which Andrew did point out, quite a bit.
>>>>>
>>>>> The original usecase for altp2m was for an entirely in-guest agent,
>>>>> which is why they got in as hvmops to start with.  I don't think it is
>>>>> wise to break that.
>>>>>
>>>>> I think there needs to be slightly finer grain control, identifying
>>>>> whether a domain may use altp2m, and whether it may configure altp2m
>>>>> permissions itself.
>>>>>
>>>>> The nature of altp2m means that using EPTP switching/etc necessarily can
>>>>> only happen from inside guest context, but whether you trust the domain
>>>>> to make adjustments to the permissions itself depends on your usecase
>>>>> and threat model.
>>>>>
>>>>
>>>> So I'm actively using EPT switching and gfn remapping from a
>>>> privileged monitor domain (not with VMFUNC). My entire usecase for
>>>> altp2m is purely external without any in-guest agents. In fact, I have
>>>> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
>>>> from the guest.
>>>>
>>>> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
>>>> only one I believe that is only accessible from within the guest is
>>>> this distinction in arch/x86/hvm/hvm.c:
>>>>
>>>>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>>>>
>>>> For the other ops I'm not sure if they were really required to be
>>>> accessible from within the guest or not. I'm not even sure using them
>>>> would work from the guest with the above check in place. However, if
>>>> they do work from the guest then I have no idea how it was supposed to
>>>> work for security purposes as any application in that guest could just
>>>> issue a hypercall to manipulate it or even turn it off.
>>>
>>> Thanks to all for the replies! What I'm taking away from this is:
>>>
>>> 1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.
>>>
>>> 2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
>>> HVMOP_altp2m_vcpu_enable_notify).
>>>
>>> 3. If we keep them as HVMOPs, the code for handling the set_mem_access()
>>> part needs to be duplicated, both for the hypercall continuation / HVMOP
>>> hypercall structure part, and for the compat part (since the _multi()
>>> function sends arrays / handles to the hypervisor).
>>>
>>> So an agreement on point 2 is required before being able to proceed.
>>
>> I think as long as there's no need for the guest to use an operation
>> on itself, it should not be a hvmop. After all, if you make it a domctl
>> now and later find a need for it to be called by the guest, we can
>> always replace the domctl by a hvmop. If, however, you start out
>> with a hvmop, we'll be bound to be supporting it virtually forever.
>
> Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
> set_mem_access() and set_mem_access_multi() shouldn't exist at all.
> Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
> should be enough, as long as we also add the view_id as an
> extra-parameter, where view ID 0 is (already) the default EPT view.
>
> As it stands now, xc_set_mem_access() can do less than
> xc_altp2m_set_mem_access() in that its view ID is always 0, but more
> than xc_altp2m_set_mem_access() in that it is able to set more than one
> page with a single hypercall, while the underlying hypervisor code is
> the same.

Yeap, I remember suggesting that the two set_mem_access interfaces
should be merged when altp2m was being contributed. Unfortunately we
were not yet maintainers to make that suggestion a requirement so it
was let in without that change..

>
> Maybe I'm missing something design-wise (obviously if these really do
> need to be HVMOPs a separate libxc function is required). Maybe the
> altp2m maintainers have a different view of the matter.
>

I think altp2m is still considered experimental at this point.. With
that said I'm not sure if the altp2m HVMOPs need to be considered as
set-in-stone as other HVMOPs might be. I would also like to see the
mem_access setting interfaces merged.

Tamas

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/13/2017 07:17 PM, Tamas K Lengyel wrote:
> On Mon, Mar 13, 2017 at 6:29 AM, Razvan Cojocaru
> <rcojocaru@bitdefender.com> wrote:
>> On 03/13/2017 02:19 PM, Jan Beulich wrote:
>>>>>> On 13.03.17 at 13:01, <rcojocaru@bitdefender.com> wrote:
>>>> On 03/10/2017 09:01 PM, Tamas K Lengyel wrote:
>>>>> On Fri, Mar 10, 2017 at 4:21 AM, Andrew Cooper
>>>>> <andrew.cooper3@citrix.com> wrote:
>>>>>> On 10/03/17 07:34, Jan Beulich wrote:
>>>>>>>>>> On 09.03.17 at 18:29, <tamas@tklengyel.com> wrote:
>>>>>>>> On Thu, Mar 9, 2017 at 9:56 AM, Jan Beulich <JBeulich@suse.com> wrote:
>>>>>>>>> However - is this interface supposed to be usable by a guest on itself?
>>>>>>>>> Arguably the same question would apply to some of the other sub-ops
>>>>>>>>> too, but anyway.
>>>>>>>> AFAIK the only op the guest would use on itself is
>>>>>>>> HVMOP_altp2m_vcpu_enable_notify.
>>>>>>> Which then means we should move all of them out of here, into a
>>>>>>> suitable domctl. That will in turn reduce the scope of the bogus
>>>>>>> interface versioning, which Andrew did point out, quite a bit.
>>>>>>
>>>>>> The original usecase for altp2m was for an entirely in-guest agent,
>>>>>> which is why they got in as hvmops to start with.  I don't think it is
>>>>>> wise to break that.
>>>>>>
>>>>>> I think there needs to be slightly finer grain control, identifying
>>>>>> whether a domain may use altp2m, and whether it may configure altp2m
>>>>>> permissions itself.
>>>>>>
>>>>>> The nature of altp2m means that using EPTP switching/etc necessarily can
>>>>>> only happen from inside guest context, but whether you trust the domain
>>>>>> to make adjustments to the permissions itself depends on your usecase
>>>>>> and threat model.
>>>>>>
>>>>>
>>>>> So I'm actively using EPT switching and gfn remapping from a
>>>>> privileged monitor domain (not with VMFUNC). My entire usecase for
>>>>> altp2m is purely external without any in-guest agents. In fact, I have
>>>>> to deploy a custom XSM policy to blacklist altp2mhvm_op being issued
>>>>> from the guest.
>>>>>
>>>>> The reason I mentioned HVMOP_altp2m_vcpu_enable_notify as being the
>>>>> only one I believe that is only accessible from within the guest is
>>>>> this distinction in arch/x86/hvm/hvm.c:
>>>>>
>>>>>     d = ( a.cmd != HVMOP_altp2m_vcpu_enable_notify ) ?
>>>>>         rcu_lock_domain_by_any_id(a.domain) : rcu_lock_current_domain();
>>>>>
>>>>> For the other ops I'm not sure if they were really required to be
>>>>> accessible from within the guest or not. I'm not even sure using them
>>>>> would work from the guest with the above check in place. However, if
>>>>> they do work from the guest then I have no idea how it was supposed to
>>>>> work for security purposes as any application in that guest could just
>>>>> issue a hypercall to manipulate it or even turn it off.
>>>>
>>>> Thanks to all for the replies! What I'm taking away from this is:
>>>>
>>>> 1. The hypercall continuation model proposed by Tamas is fine for HVMOPs.
>>>>
>>>> 2. But we're not sure if these should be DOMCTLs or HVMOPs (except for
>>>> HVMOP_altp2m_vcpu_enable_notify).
>>>>
>>>> 3. If we keep them as HVMOPs, the code for handling the set_mem_access()
>>>> part needs to be duplicated, both for the hypercall continuation / HVMOP
>>>> hypercall structure part, and for the compat part (since the _multi()
>>>> function sends arrays / handles to the hypervisor).
>>>>
>>>> So an agreement on point 2 is required before being able to proceed.
>>>
>>> I think as long as there's no need for the guest to use an operation
>>> on itself, it should not be a hvmop. After all, if you make it a domctl
>>> now and later find a need for it to be called by the guest, we can
>>> always replace the domctl by a hvmop. If, however, you start out
>>> with a hvmop, we'll be bound to be supporting it virtually forever.
>>
>> Since we're on this point, IMHO the xc_altp2m_ prefixed versions of
>> set_mem_access() and set_mem_access_multi() shouldn't exist at all.
>> Plain xc_set_mem_access() and xc_set_mem_access_multi() (as DOMCTLs)
>> should be enough, as long as we also add the view_id as an
>> extra-parameter, where view ID 0 is (already) the default EPT view.
>>
>> As it stands now, xc_set_mem_access() can do less than
>> xc_altp2m_set_mem_access() in that its view ID is always 0, but more
>> than xc_altp2m_set_mem_access() in that it is able to set more than one
>> page with a single hypercall, while the underlying hypervisor code is
>> the same.
> 
> Yeap, I remember suggesting that the two set_mem_access interfaces
> should be merged when altp2m was being contributed. Unfortunately we
> were not yet maintainers to make that suggestion a requirement so it
> was let in without that change..
> 
>>
>> Maybe I'm missing something design-wise (obviously if these really do
>> need to be HVMOPs a separate libxc function is required). Maybe the
>> altp2m maintainers have a different view of the matter.
>>
> 
> I think altp2m is still considered experimental at this point.. With
> that said I'm not sure if the altp2m HVMOPs need to be considered as
> set-in-stone as other HVMOPs might be. I would also like to see the
> mem_access setting interfaces merged.

Then I'll rework the patch to remove the special altp2m functions and
add an extra parameter to the regular xc_set_mem_access() functions
(while also increasing the DOMCTL version macro value). Unless somebody
objects, of course.


Thanks,
Razvan

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

Re: [PATCH V2] x86/altp2m: Added xc_altp2m_set_mem_access_multi()

[Razvan Cojocaru]

On 03/09/2017 06:56 PM, Jan Beulich wrote:
>>>> On 09.03.17 at 10:38, <rcojocaru@bitdefender.com> wrote:
>> @@ -4535,6 +4536,30 @@ static int do_altp2m_op(
>>                                      a.u.set_mem_access.view);
>>          break;
>>  
>> +    case HVMOP_altp2m_set_mem_access_multi:
>> +        if ( a.u.set_mem_access_multi.pad ||
>> +             a.u.set_mem_access_multi.opaque >= a.u.set_mem_access_multi.nr )
>> +        {
>> +            rc = -EINVAL;
>> +            break;
>> +        }
>> +        rc = p2m_set_mem_access_multi(d, a.u.set_mem_access_multi.pfn_list,
>> +                                      a.u.set_mem_access_multi.access_list,
>> +                                      a.u.set_mem_access_multi.nr,
>> +                                      a.u.set_mem_access_multi.opaque,
>> +                                      MEMOP_CMD_MASK,
>> +                                      a.u.set_mem_access_multi.view);
>> +        if ( rc > 0 )
>> +        {
>> +            a.u.set_mem_access_multi.opaque = rc;
>> +            if ( __copy_to_guest(arg, &a, 1) )
>> +                rc = -EFAULT;
>> +            else
>> +                rc = hypercall_create_continuation(__HYPERVISOR_hvm_op, "lh",
>> +                                                   HVMOP_altp2m, arg);
>> +        }
>> +        break;
> 
> Okay, so this is a hvmop, in which case I'm fine with the continuation
> model used.
> 
> However - is this interface supposed to be usable by a guest on itself?
> Arguably the same question would apply to some of the other sub-ops
> too, but anyway.
> 
>> --- a/xen/include/public/hvm/hvm_op.h
>> +++ b/xen/include/public/hvm/hvm_op.h
>> @@ -231,6 +231,23 @@ struct xen_hvm_altp2m_set_mem_access {
>>  typedef struct xen_hvm_altp2m_set_mem_access xen_hvm_altp2m_set_mem_access_t;
>>  DEFINE_XEN_GUEST_HANDLE(xen_hvm_altp2m_set_mem_access_t);
>>  
>> +struct xen_hvm_altp2m_set_mem_access_multi {
>> +    /* view */
>> +    uint16_t view;
>> +    uint16_t pad;
>> +    /* Number of pages */
>> +    uint32_t nr;
>> +    /* Used for continuation purposes */
>> +    uint64_t opaque;
>> +    /* List of pfns to set access for */
>> +    XEN_GUEST_HANDLE(const_uint64) pfn_list;
>> +    /* Corresponding list of access settings for pfn_list */
>> +    XEN_GUEST_HANDLE(const_uint8) access_list;
> 
> I'm afraid these handles aren't going to work for a 32-bit guest. Note
> how no other hvmop uses handles.

OK, so at this point, since Ravi has not expressed a preference, but
Andrew has said that these should be accessible from the guest as well,
I assume we should move forward with this as a HVMOP, addressing the
comment above regarding compatibility. (Just to help the discussion,
here is the original patch: https://patchwork.kernel.org/patch/9612799/).

I do prefer the DOMCTL version, but of course the operation will not be
available for the guest then and we may just as well make it a HVMOP
from the beginnig.

If there are any objections, please let me know.


Thanks,
Razvan

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