Re: [Xen-devel] [PATCH 02/11] hvmctl: convert HVMOP_set_pci_intx_level

[Wei Liu]

On Mon, Jun 20, 2016 at 06:53:23AM -0600, Jan Beulich wrote:
> Note that this adds validation of the "domain" interface structure
> field, which previously got ignored.
> 
> Note further that this retains the hvmop interface definitions as those
> had (wrongly) been exposed to non-tool stack consumers (albeit the
> operation wouldn't have succeeded when requested by a domain for
> itself).
> 
> Signed-off-by: Jan Beulich 
> ---
> TBD: xen/xsm/flask/policy/access_vectors says "also needs hvmctl", but
>  I don't see how this has been done so far. With the change here,
>  doing two checks in flask_hvm_control() (the generic one always
>  and a specific one if needed) would of course be simple, but it's
>  unclear how subsequently added sub-ops should then be dealt with
>  (which don't have a similar remark).
> 
> --- a/tools/libxc/xc_misc.c
> +++ b/tools/libxc/xc_misc.c
> @@ -473,30 +473,14 @@ int xc_hvm_set_pci_intx_level(
>  uint8_t domain, uint8_t bus, uint8_t device, uint8_t intx,
>  unsigned int level)
>  {
> -DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_pci_intx_level, arg);
> -int rc;
> +DECLARE_HVMCTL(set_pci_intx_level, dom,
> +   .domain = domain,
> +   .bus= bus,
> +   .device = device,
> +   .intx   = intx,
> +   .level =  level);

Minor nit: the "=" is not aligned.

For tool and hypervisor code changes, sans the XSM changes:

Reviewed-by: Wei Liu 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/11] hvmctl: convert HVMOP_set_pci_intx_level

[Jan Beulich]

>>> On 20.06.16 at 16:48,  wrote:
> Daniel De Graaf writes ("Re: [PATCH 02/11] hvmctl: convert 
> HVMOP_set_pci_intx_level"):
>> On 06/20/2016 08:53 AM, Jan Beulich wrote:
>> > Note that this adds validation of the "domain" interface structure
>> > field, which previously got ignored.
>> >
>> > Note further that this retains the hvmop interface definitions as those
>> > had (wrongly) been exposed to non-tool stack consumers (albeit the
>> > operation wouldn't have succeeded when requested by a domain for
>> > itself).
>> >
>> > Signed-off-by: Jan Beulich 
>> > ---
>> > TBD: xen/xsm/flask/policy/access_vectors says "also needs hvmctl", but
>> >  I don't see how this has been done so far. With the change here,
>> >  doing two checks in flask_hvm_control() (the generic one always
>> >  and a specific one if needed) would of course be simple, but it's
>> >  unclear how subsequently added sub-ops should then be dealt with
>> >  (which don't have a similar remark).
>> 
>> I am not sure why that remark is there: it seems like it refers to an
>> overall check in the HVM operation hypercall, which does not exist.
>> 
>> There is no reason to have an operation protected by two different
>> access checks, so I think that both the previous and patched code
>> are correct and the "also needs hvmctl" comment should be removed.
>> With that, Acked-by: Daniel De Graaf 
> 
> This is a slight digression, but is it intended that all of these
> hvmctl's are safe to expose to a deprivileged device model process in
> dom0, or to a device model stub domain ?

Yes, afaict (they've been exposed the same way before).

Jan


___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/11] hvmctl: convert HVMOP_set_pci_intx_level

[Ian Jackson]

Daniel De Graaf writes ("Re: [PATCH 02/11] hvmctl: convert 
HVMOP_set_pci_intx_level"):
> On 06/20/2016 08:53 AM, Jan Beulich wrote:
> > Note that this adds validation of the "domain" interface structure
> > field, which previously got ignored.
> >
> > Note further that this retains the hvmop interface definitions as those
> > had (wrongly) been exposed to non-tool stack consumers (albeit the
> > operation wouldn't have succeeded when requested by a domain for
> > itself).
> >
> > Signed-off-by: Jan Beulich 
> > ---
> > TBD: xen/xsm/flask/policy/access_vectors says "also needs hvmctl", but
> >  I don't see how this has been done so far. With the change here,
> >  doing two checks in flask_hvm_control() (the generic one always
> >  and a specific one if needed) would of course be simple, but it's
> >  unclear how subsequently added sub-ops should then be dealt with
> >  (which don't have a similar remark).
> 
> I am not sure why that remark is there: it seems like it refers to an
> overall check in the HVM operation hypercall, which does not exist.
> 
> There is no reason to have an operation protected by two different
> access checks, so I think that both the previous and patched code
> are correct and the "also needs hvmctl" comment should be removed.
> With that, Acked-by: Daniel De Graaf 

This is a slight digression, but is it intended that all of these
hvmctl's are safe to expose to a deprivileged device model process in
dom0, or to a device model stub domain ?

Ian.

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

Re: [Xen-devel] [PATCH 02/11] hvmctl: convert HVMOP_set_pci_intx_level

[Daniel De Graaf]

On 06/20/2016 08:53 AM, Jan Beulich wrote:

Note that this adds validation of the "domain" interface structure
field, which previously got ignored.

Note further that this retains the hvmop interface definitions as those
had (wrongly) been exposed to non-tool stack consumers (albeit the
operation wouldn't have succeeded when requested by a domain for
itself).

Signed-off-by: Jan Beulich 
---
TBD: xen/xsm/flask/policy/access_vectors says "also needs hvmctl", but
 I don't see how this has been done so far. With the change here,
 doing two checks in flask_hvm_control() (the generic one always
 and a specific one if needed) would of course be simple, but it's
 unclear how subsequently added sub-ops should then be dealt with
 (which don't have a similar remark).


I am not sure why that remark is there: it seems like it refers to an
overall check in the HVM operation hypercall, which does not exist.

There is no reason to have an operation protected by two different
access checks, so I think that both the previous and patched code
are correct and the "also needs hvmctl" comment should be removed.
With that, Acked-by: Daniel De Graaf 

___
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

[Xen-devel] [PATCH 02/11] hvmctl: convert HVMOP_set_pci_intx_level

[Jan Beulich]

Note that this adds validation of the "domain" interface structure
field, which previously got ignored.

Note further that this retains the hvmop interface definitions as those
had (wrongly) been exposed to non-tool stack consumers (albeit the
operation wouldn't have succeeded when requested by a domain for
itself).

Signed-off-by: Jan Beulich 
---
TBD: xen/xsm/flask/policy/access_vectors says "also needs hvmctl", but
 I don't see how this has been done so far. With the change here,
 doing two checks in flask_hvm_control() (the generic one always
 and a specific one if needed) would of course be simple, but it's
 unclear how subsequently added sub-ops should then be dealt with
 (which don't have a similar remark).

--- a/tools/libxc/xc_misc.c
+++ b/tools/libxc/xc_misc.c
@@ -473,30 +473,14 @@ int xc_hvm_set_pci_intx_level(
 uint8_t domain, uint8_t bus, uint8_t device, uint8_t intx,
 unsigned int level)
 {
-DECLARE_HYPERCALL_BUFFER(struct xen_hvm_set_pci_intx_level, arg);
-int rc;
+DECLARE_HVMCTL(set_pci_intx_level, dom,
+   .domain = domain,
+   .bus= bus,
+   .device = device,
+   .intx   = intx,
+   .level =  level);
 
-arg = xc_hypercall_buffer_alloc(xch, arg, sizeof(*arg));
-if ( arg == NULL )
-{
-PERROR("Could not allocate memory for xc_hvm_set_pci_intx_level 
hypercall");
-return -1;
-}
-
-arg->domid  = dom;
-arg->domain = domain;
-arg->bus= bus;
-arg->device = device;
-arg->intx   = intx;
-arg->level  = level;
-
-rc = xencall2(xch->xcall, __HYPERVISOR_hvm_op,
-  HVMOP_set_pci_intx_level,
-  HYPERCALL_BUFFER_AS_ARG(arg));
-
-xc_hypercall_buffer_free(xch, arg);
-
-return rc;
+return do_hvmctl(xch, );
 }
 
 int xc_hvm_set_isa_irq_level(
--- a/tools/libxc/xc_private.h
+++ b/tools/libxc/xc_private.h
@@ -34,6 +34,8 @@
 #define XC_INTERNAL_COMPAT_MAP_FOREIGN_API
 #include "xenctrl.h"
 
+#include 
+
 #include 
 #include 
 
@@ -61,6 +63,13 @@ struct iovec {
 
 #define DECLARE_DOMCTL struct xen_domctl domctl
 #define DECLARE_SYSCTL struct xen_sysctl sysctl
+#define DECLARE_HVMCTL(op, dom, init...) \
+struct xen_hvmctl hvmctl = { \
+.interface_version = XEN_HVMCTL_INTERFACE_VERSION, \
+.domain = (dom), \
+.cmd = XEN_HVMCTL_##op, \
+.u.op = { init } \
+}
 #define DECLARE_PHYSDEV_OP struct physdev_op physdev_op
 #define DECLARE_FLASK_OP struct xen_flask_op op
 #define DECLARE_PLATFORM_OP struct xen_platform_op platform_op
@@ -311,6 +320,31 @@ static inline int do_sysctl(xc_interface
 return ret;
 }
 
+static inline int do_hvmctl(xc_interface *xch, struct xen_hvmctl *hvmctl)
+{
+int ret = -1;
+DECLARE_HYPERCALL_BOUNCE(hvmctl, sizeof(*hvmctl), 
XC_HYPERCALL_BUFFER_BOUNCE_BOTH);
+
+if ( xc_hypercall_bounce_pre(xch, hvmctl) )
+{
+PERROR("Could not bounce buffer for hvmctl hypercall");
+return -1;
+}
+
+ret = xencall1(xch->xcall, __HYPERVISOR_hvmctl,
+   HYPERCALL_BUFFER_AS_ARG(hvmctl));
+if ( ret < 0 )
+{
+if ( errno == EACCES )
+DPRINTF("hvmctl operation failed -- need to"
+" rebuild the user-space tool set?\n");
+}
+
+xc_hypercall_bounce_post(xch, hvmctl);
+
+return ret;
+}
+
 static inline int do_platform_op(xc_interface *xch,
  struct xen_platform_op *platform_op)
 {
--- a/xen/arch/x86/hvm/control.c
+++ b/xen/arch/x86/hvm/control.c
@@ -19,6 +19,30 @@
 #include 
 #include 
 
+static int set_pci_intx_level(struct domain *d,
+  const struct xen_hvm_set_pci_intx_level *op)
+{
+if ( op->domain || op->bus || (op->device > 31) || (op->intx > 3) )
+return -EINVAL;
+
+if ( !is_hvm_domain(d) )
+return -EINVAL;
+
+switch ( op->level )
+{
+case 0:
+hvm_pci_intx_deassert(d, op->device, op->intx);
+break;
+case 1:
+hvm_pci_intx_assert(d, op->device, op->intx);
+break;
+default:
+return -EINVAL;
+}
+
+return 0;
+}
+
 /*
  * Note that this value is effectively part of the ABI, even if we don't need
  * to make it a formal part of it.  Hence this value may only be changed if
@@ -64,6 +88,10 @@ long do_hvmctl(XEN_GUEST_HANDLE_PARAM(xe
 
 switch ( op.cmd )
 {
+case XEN_HVMCTL_set_pci_intx_level:
+rc = set_pci_intx_level(d, _pci_intx_level);
+break;
+
 default:
 rc = -EOPNOTSUPP;
 break;
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4303,50 +4303,6 @@ void hvm_hypercall_page_initialise(struc
 hvm_funcs.init_hypercall_page(d, hypercall_page);
 }
 
-static int hvmop_set_pci_intx_level(
-XEN_GUEST_HANDLE_PARAM(xen_hvm_set_pci_intx_level_t) uop)
-{
-struct xen_hvm_set_pci_intx_level op;
-struct domain *d;