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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jul 20, 2017 at 9:11 AM, George Dunlap
<George.Dunlap@xxxxxxxxxxxxx> wrote:
> On Thu, Jun 15, 2017 at 8:01 PM, Tamas K Lengyel <tamas@xxxxxxxxxxxxx> wrote:
>> On Fri, Jun 9, 2017 at 10:51 AM, Adrian Pop <apop@xxxxxxxxxxxxxxx> wrote:
>>> Introduce a new hvmop, HVMOP_altp2m_set_suppress_ve, which allows a
>>> privileged domain to change the value of the #VE suppress bit for a
>>> page.
>>>
>>> Add a libxc wrapper for invoking this hvmop.
>>>
>>> Signed-off-by: Adrian Pop <apop@xxxxxxxxxxxxxxx>
>>> ---
>>>  tools/libxc/include/xenctrl.h   |  2 ++
>>>  tools/libxc/xc_altp2m.c         | 24 +++++++++++++++++++
>>>  xen/arch/x86/hvm/hvm.c          | 14 +++++++++++
>>>  xen/arch/x86/mm/mem_access.c    | 52 
>>> +++++++++++++++++++++++++++++++++++++++++
>>>  xen/include/public/hvm/hvm_op.h | 15 ++++++++++++
>>>  xen/include/xen/mem_access.h    |  3 +++
>>>  6 files changed, 110 insertions(+)
>>>
>>> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h
>>> index 1629f412dd..f6ba8635bf 100644
>>> --- a/tools/libxc/include/xenctrl.h
>>> +++ b/tools/libxc/include/xenctrl.h
>>> @@ -1926,6 +1926,8 @@ int xc_altp2m_destroy_view(xc_interface *handle, 
>>> domid_t domid,
>>>  /* Switch all vCPUs of the domain to the specified altp2m view */
>>>  int xc_altp2m_switch_to_view(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id);
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve);
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id, xen_pfn_t gfn,
>>>                               xenmem_access_t access);
>>> diff --git a/tools/libxc/xc_altp2m.c b/tools/libxc/xc_altp2m.c
>>> index 0639632477..4710133918 100644
>>> --- a/tools/libxc/xc_altp2m.c
>>> +++ b/tools/libxc/xc_altp2m.c
>>> @@ -163,6 +163,30 @@ int xc_altp2m_switch_to_view(xc_interface *handle, 
>>> domid_t domid,
>>>      return rc;
>>>  }
>>>
>>> +int xc_altp2m_set_suppress_ve(xc_interface *handle, domid_t domid,
>>> +                              uint16_t view_id, xen_pfn_t gfn, bool sve)
>>> +{
>>> +    int rc;
>>> +    DECLARE_HYPERCALL_BUFFER(xen_hvm_altp2m_op_t, arg);
>>> +
>>> +    arg = xc_hypercall_buffer_alloc(handle, arg, sizeof(*arg));
>>> +    if ( arg == NULL )
>>> +        return -1;
>>> +
>>> +    arg->version = HVMOP_ALTP2M_INTERFACE_VERSION;
>>> +    arg->cmd = HVMOP_altp2m_set_suppress_ve;
>>> +    arg->domain = domid;
>>> +    arg->u.set_suppress_ve.view = view_id;
>>> +    arg->u.set_suppress_ve.gfn = gfn;
>>> +    arg->u.set_suppress_ve.suppress_ve = sve;
>>> +
>>> +    rc = xencall2(handle->xcall, __HYPERVISOR_hvm_op, HVMOP_altp2m,
>>> +                 HYPERCALL_BUFFER_AS_ARG(arg));
>>> +
>>> +    xc_hypercall_buffer_free(handle, arg);
>>> +    return rc;
>>> +}
>>> +
>>>  int xc_altp2m_set_mem_access(xc_interface *handle, domid_t domid,
>>>                               uint16_t view_id, xen_pfn_t gfn,
>>>                               xenmem_access_t access)
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 70ddc81d44..dd8e205551 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4358,6 +4358,7 @@ static int do_altp2m_op(
>>>      case HVMOP_altp2m_destroy_p2m:
>>>      case HVMOP_altp2m_switch_p2m:
>>>      case HVMOP_altp2m_set_mem_access:
>>> +    case HVMOP_altp2m_set_suppress_ve:
>>>      case HVMOP_altp2m_change_gfn:
>>>          break;
>>>      default:
>>> @@ -4475,6 +4476,19 @@ static int do_altp2m_op(
>>>                                      a.u.set_mem_access.view);
>>>          break;
>>>
>>> +    case HVMOP_altp2m_set_suppress_ve:
>>> +        if ( a.u.set_suppress_ve.pad1 || a.u.set_suppress_ve.pad2 )
>>> +            rc = -EINVAL;
>>> +        else
>>> +        {
>>> +            gfn_t gfn = _gfn(a.u.set_mem_access.gfn);
>>> +            unsigned int altp2m_idx = a.u.set_mem_access.view;
>>> +            bool suppress_ve = a.u.set_suppress_ve.suppress_ve;
>>> +
>>> +            rc = p2m_set_suppress_ve(d, gfn, suppress_ve, altp2m_idx);
>>> +        }
>>> +        break;
>>> +
>>>      case HVMOP_altp2m_change_gfn:
>>>          if ( a.u.change_gfn.pad1 || a.u.change_gfn.pad2 )
>>>              rc = -EINVAL;
>>> diff --git a/xen/arch/x86/mm/mem_access.c b/xen/arch/x86/mm/mem_access.c
>>> index d0b0767855..8c39db13e3 100644
>>> --- a/xen/arch/x86/mm/mem_access.c
>>> +++ b/xen/arch/x86/mm/mem_access.c
>>> @@ -466,6 +466,58 @@ int p2m_get_mem_access(struct domain *d, gfn_t gfn, 
>>> xenmem_access_t *access)
>>>  }
>>>
>>>  /*
>>> + * Set/clear the #VE suppress bit for a page.  Only available on VMX.
>>> + */
>>> +int p2m_set_suppress_ve(struct domain *d, gfn_t gfn, bool suppress_ve,
>>> +                        unsigned int altp2m_idx)
>>> +{
>>> +    struct p2m_domain *host_p2m = p2m_get_hostp2m(d);
>>> +    struct p2m_domain *ap2m = NULL;
>>> +    struct p2m_domain *p2m;
>>> +    mfn_t mfn;
>>> +    p2m_access_t a;
>>> +    p2m_type_t t;
>>> +    int rc;
>>> +
>>> +    if ( !cpu_has_vmx_virt_exceptions )
>>> +        return -EOPNOTSUPP;
>>> +
>>> +    /* This subop should only be used from a privileged domain. */
>>> +    if ( !current->domain->is_privileged )
>>> +        return -EINVAL;
>>
>> This check looks wrong to me. If this subop should only be used by an
>> external (privileged) domain then I don't think this should be
>> implemented as an HVMOP, looks more like a domctl to me.
>
> Well after patch 1, isn't it the case that a guest has no way of
> clearing the suppress_ve bit?
>
> I was going to say we want the XSM_TARGET "default action" (which
> allows a guest to do things on itself, or a privileged domain to do it
> to any domain); but I think really we probably we don't want a guest
> to be able to *clear* the suppress_ve bit on a page for which a
> privileged domain has *set*; this would allow a domain to prevent the
> other domain from effectively introspecting on a page.

That sounds right, that would be a scenario that would probably have
to be avoided. That said, it becomes quite complex if we want to have
two entities having access to altp2m, one external and one in-guest. I
don't think that setup is something that was considered when altp2m
was introduced.

>
> This is starting to sound like another conversation I think I remember
> recently about making sure that *only* the guest *or* an introspection
> engine can use the altp2m functionality, but I can't seem to find that
> with a quick look. Tamas, does that ring any bells?

You may be thinking of the discussions regarding the externel_only
mode for altp2m I've added in
https://lists.xen.org/archives/html/xen-devel/2017-04/msg00373.html.

Tamas

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