[PATCH v4 0/3] x86/hvm: HVMOP_get/set_param improvements

[PATCH v4 0/3] x86/hvm: HVMOP_get/set_param improvements

[Paul Durrant]

The following 3 patches re-structure the code implementing HVMOP_set_param
and HVMOP_get_param.

Patch #1 gives each operation its own function

Patch #2 splits out checks for getting/setting non-reflexive params and
setting params with change-once semantics, as well as the XSM check into
separate functions

Patch #3 swaps from black-lists to white-lists for guest accessible
params

v4:
 - Remove HVM_PARAM_TRIPLE_FAULT_REASON from the params accessible by
   guests.

v3:
 - Fix more style issues
 - Maintain comments concerning strictly non-reflexive params
 - Add missing non-reflexive checks to hvm_allow_get_param()

v2:
 - Fix style issues during code movement in patch #2.
 - End up with white-lists rather than black-lists for guest accessible
   params.
 - Re-work comments on patch #2 slightly.

[PATCH v4 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Paul Durrant]

The level of switch nesting in those ops is getting unreadable. Giving
them their own functions does introduce some code duplication in the
the pre-op checks but the overall result is easier to follow.

This patch is code movement (including style fixes). There is no
functional change.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |  556 +++++++++++++++++++++++++-----------------------
 1 file changed, 294 insertions(+), 262 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index bfde380..c246b45 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5638,6 +5638,294 @@ static int hvmop_set_evtchn_upcall_vector(
     return 0;
 }
 
+static int hvmop_set_param(
+    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+{
+    struct domain *curr_d = current->domain;
+    struct xen_hvm_param a;
+    struct domain *d;
+    struct vcpu *v;
+    int rc;
+
+    if ( copy_from_guest(&a, arg, 1) )
+        return -EFAULT;
+
+    if ( a.index >= HVM_NR_PARAMS )
+        return -EINVAL;
+
+    d = rcu_lock_domain_by_any_id(a.domid);
+    if ( d == NULL )
+        return -ESRCH;
+
+    rc = -EINVAL;
+    if ( !has_hvm_container_domain(d) ||
+         (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
+        goto out;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    if ( rc )
+        goto out;
+
+    switch ( a.index )
+    {
+    case HVM_PARAM_CALLBACK_IRQ:
+        hvm_set_callback_via(d, a.value);
+        hvm_latch_shinfo_size(d);
+        break;
+    case HVM_PARAM_TIMER_MODE:
+        if ( a.value > HVMPTM_one_missed_tick_pending )
+            rc = -EINVAL;
+        break;
+    case HVM_PARAM_VIRIDIAN:
+        /* This should only ever be set once by the tools and read by the guest. */
+        rc = -EPERM;
+        if ( d == curr_d )
+            break;
+
+        if ( a.value != d->arch.hvm_domain.params[a.index] )
+        {
+            rc = -EEXIST;
+            if ( d->arch.hvm_domain.params[a.index] != 0 )
+                break;
+
+            rc = -EINVAL;
+            if ( (a.value & ~HVMPV_feature_mask) ||
+                 !(a.value & HVMPV_base_freq) )
+                break;
+        }
+
+        rc = 0;
+        break;
+    case HVM_PARAM_IDENT_PT:
+        /* Not reflexive, as we must domain_pause(). */
+        rc = -EPERM;
+        if ( d == curr_d )
+            break;
+
+        rc = -EINVAL;
+        if ( d->arch.hvm_domain.params[a.index] != 0 )
+            break;
+
+        rc = 0;
+        if ( !paging_mode_hap(d) )
+            break;
+
+        /*
+         * Update GUEST_CR3 in each VMCS to point at identity map.
+         * All foreign updates to guest state must synchronise on
+         * the domctl_lock.
+         */
+        rc = -ERESTART;
+        if ( !domctl_lock_acquire() )
+            break;
+
+        rc = 0;
+        domain_pause(d);
+        d->arch.hvm_domain.params[a.index] = a.value;
+        for_each_vcpu ( d, v )
+            paging_update_cr3(v);
+        domain_unpause(d);
+
+        domctl_lock_release();
+        break;
+    case HVM_PARAM_DM_DOMAIN:
+        /* Not reflexive, as we may need to domain_pause(). */
+        rc = -EPERM;
+        if ( d == curr_d )
+            break;
+
+        if ( a.value == DOMID_SELF )
+            a.value = curr_d->domain_id;
+
+        rc = hvm_set_dm_domain(d, a.value);
+        break;
+    case HVM_PARAM_ACPI_S_STATE:
+        /* Not reflexive, as we must domain_pause(). */
+        rc = -EPERM;
+        if ( d == curr_d )
+            break;
+
+        rc = 0;
+        if ( a.value == 3 )
+            hvm_s3_suspend(d);
+        else if ( a.value == 0 )
+            hvm_s3_resume(d);
+        else
+            rc = -EINVAL;
+
+        break;
+    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
+        rc = pmtimer_change_ioport(d, a.value);
+        break;
+    case HVM_PARAM_MEMORY_EVENT_CR0:
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+    case HVM_PARAM_MEMORY_EVENT_CR4:
+        if ( d == curr_d )
+            rc = -EPERM;
+        break;
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
+    case HVM_PARAM_MEMORY_EVENT_MSR:
+        if ( d == curr_d )
+        {
+            rc = -EPERM;
+            break;
+        }
+        if ( a.value & HVMPME_onchangeonly )
+            rc = -EINVAL;
+        break;
+    case HVM_PARAM_NESTEDHVM:
+        rc = xsm_hvm_param_nested(XSM_PRIV, d);
+        if ( rc )
+            break;
+        if ( a.value > 1 )
+            rc = -EINVAL;
+        /*
+         * Remove the check below once we have
+         * shadow-on-shadow.
+         */
+        if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
+            rc = -EINVAL;
+        /* Set up NHVM state for any vcpus that are already up. */
+        if ( a.value &&
+             !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
+            for_each_vcpu(d, v)
+                if ( rc == 0 )
+                    rc = nestedhvm_vcpu_initialise(v);
+        if ( !a.value || rc )
+            for_each_vcpu(d, v)
+                nestedhvm_vcpu_destroy(v);
+        break;
+    case HVM_PARAM_BUFIOREQ_EVTCHN:
+        rc = -EINVAL;
+        break;
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+        if ( a.value > SHUTDOWN_MAX )
+            rc = -EINVAL;
+        break;
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+        if ( d == curr_d )
+        {
+            rc = -EPERM;
+            break;
+        }
+        d->arch.hvm_domain.ioreq_gmfn.base = a.value;
+        break;
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+    {
+        unsigned int i;
+
+        if ( d == curr_d )
+        {
+            rc = -EPERM;
+            break;
+        }
+        if ( a.value == 0 ||
+             a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
+        {
+            rc = -EINVAL;
+            break;
+        }
+        for ( i = 0; i < a.value; i++ )
+            set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
+
+        break;
+    }
+    }
+
+    if ( rc != 0 )
+        goto out;
+
+    d->arch.hvm_domain.params[a.index] = a.value;
+
+    switch( a.index )
+    {
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
+        domain_pause(d);
+        domain_unpause(d); /* Causes guest to latch new status */
+        break;
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+        for_each_vcpu ( d, v )
+            hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 mask through CR0 code */
+        break;
+    }
+
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
+                a.index, a.value);
+
+ out:
+    rcu_unlock_domain(d);
+    return rc;
+}
+
+static int hvmop_get_param(
+    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+{
+    struct domain *curr_d = current->domain;
+    struct xen_hvm_param a;
+    struct domain *d;
+    int rc;
+
+    if ( copy_from_guest(&a, arg, 1) )
+        return -EFAULT;
+
+    if ( a.index >= HVM_NR_PARAMS )
+        return -EINVAL;
+
+    d = rcu_lock_domain_by_any_id(a.domid);
+    if ( d == NULL )
+        return -ESRCH;
+
+    rc = -EINVAL;
+    if ( !has_hvm_container_domain(d) ||
+         (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
+        goto out;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    if ( rc )
+        goto out;
+
+    switch ( a.index )
+    {
+    case HVM_PARAM_ACPI_S_STATE:
+        a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
+        break;
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( d == curr_d )
+        {
+            rc = -EPERM;
+            goto out;
+        }
+    case HVM_PARAM_IOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_EVTCHN:
+    {
+        domid_t domid;
+
+        /* May need to create server. */
+        domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
+        rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
+        if ( rc != 0 && rc != -EEXIST )
+            goto out;
+    }
+    /*FALLTHRU*/
+    default:
+        a.value = d->arch.hvm_domain.params[a.index];
+        break;
+    }
+
+    rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+
+    HVM_DBG_LOG(DBG_LEVEL_HCALL, "get param %u = %"PRIx64,
+                a.index, a.value);
+
+ out:
+    rcu_unlock_domain(d);
+    return rc;
+}
+
 /*
  * Note that this value is effectively part of the ABI, even if we don't need
  * to make it a formal part of it: A guest suspended for migration in the
@@ -5649,7 +5937,6 @@ static int hvmop_set_evtchn_upcall_vector(
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
 {
-    struct domain *curr_d = current->domain;
     unsigned long start_iter, mask;
     long rc = 0;
 
@@ -5703,269 +5990,14 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     
     case HVMOP_set_param:
-    case HVMOP_get_param:
-    {
-        struct xen_hvm_param a;
-        struct domain *d;
-        struct vcpu *v;
-
-        if ( copy_from_guest(&a, arg, 1) )
-            return -EFAULT;
-
-        if ( a.index >= HVM_NR_PARAMS )
-            return -EINVAL;
-
-        d = rcu_lock_domain_by_any_id(a.domid);
-        if ( d == NULL )
-            return -ESRCH;
-
-        rc = -EINVAL;
-        if ( !has_hvm_container_domain(d) )
-            goto param_fail;
-
-        if ( is_pvh_domain(d)
-             && (a.index != HVM_PARAM_CALLBACK_IRQ) )
-            goto param_fail;
-
-        rc = xsm_hvm_param(XSM_TARGET, d, op);
-        if ( rc )
-            goto param_fail;
-
-        if ( op == HVMOP_set_param )
-        {
-            rc = 0;
-
-            switch ( a.index )
-            {
-            case HVM_PARAM_CALLBACK_IRQ:
-                hvm_set_callback_via(d, a.value);
-                hvm_latch_shinfo_size(d);
-                break;
-            case HVM_PARAM_TIMER_MODE:
-                if ( a.value > HVMPTM_one_missed_tick_pending )
-                    rc = -EINVAL;
-                break;
-            case HVM_PARAM_VIRIDIAN:
-                /* This should only ever be set once by the tools and read by the guest. */
-                rc = -EPERM;
-                if ( curr_d == d )
-                    break;
-
-                if ( a.value != d->arch.hvm_domain.params[a.index] )
-                {
-                    rc = -EEXIST;
-                    if ( d->arch.hvm_domain.params[a.index] != 0 )
-                        break;
-
-                    rc = -EINVAL;
-                    if ( (a.value & ~HVMPV_feature_mask) ||
-                         !(a.value & HVMPV_base_freq) )
-                        break;
-                }
-
-                rc = 0;
-                break;
-            case HVM_PARAM_IDENT_PT:
-                /* Not reflexive, as we must domain_pause(). */
-                rc = -EPERM;
-                if ( curr_d == d )
-                    break;
-
-                rc = -EINVAL;
-                if ( d->arch.hvm_domain.params[a.index] != 0 )
-                    break;
-
-                rc = 0;
-                if ( !paging_mode_hap(d) )
-                    break;
-
-                /*
-                 * Update GUEST_CR3 in each VMCS to point at identity map.
-                 * All foreign updates to guest state must synchronise on
-                 * the domctl_lock.
-                 */
-                rc = -ERESTART;
-                if ( !domctl_lock_acquire() )
-                    break;
-
-                rc = 0;
-                domain_pause(d);
-                d->arch.hvm_domain.params[a.index] = a.value;
-                for_each_vcpu ( d, v )
-                    paging_update_cr3(v);
-                domain_unpause(d);
-
-                domctl_lock_release();
-                break;
-            case HVM_PARAM_DM_DOMAIN:
-                /* Not reflexive, as we may need to domain_pause(). */
-                rc = -EPERM;
-                if ( curr_d == d )
-                    break;
-
-                if ( a.value == DOMID_SELF )
-                    a.value = curr_d->domain_id;
-
-                rc = hvm_set_dm_domain(d, a.value);
-                break;
-            case HVM_PARAM_ACPI_S_STATE:
-                /* Not reflexive, as we must domain_pause(). */
-                rc = -EPERM;
-                if ( curr_d == d )
-                    break;
-
-                rc = 0;
-                if ( a.value == 3 )
-                    hvm_s3_suspend(d);
-                else if ( a.value == 0 )
-                    hvm_s3_resume(d);
-                else
-                    rc = -EINVAL;
-
-                break;
-            case HVM_PARAM_ACPI_IOPORTS_LOCATION:
-                rc = pmtimer_change_ioport(d, a.value);
-                break;
-            case HVM_PARAM_MEMORY_EVENT_CR0:
-            case HVM_PARAM_MEMORY_EVENT_CR3:
-            case HVM_PARAM_MEMORY_EVENT_CR4:
-                if ( d == current->domain )
-                    rc = -EPERM;
-                break;
-            case HVM_PARAM_MEMORY_EVENT_INT3:
-            case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-            case HVM_PARAM_MEMORY_EVENT_MSR:
-                if ( d == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-                if ( a.value & HVMPME_onchangeonly )
-                    rc = -EINVAL;
-                break;
-            case HVM_PARAM_NESTEDHVM:
-                rc = xsm_hvm_param_nested(XSM_PRIV, d);
-                if ( rc )
-                    break;
-                if ( a.value > 1 )
-                    rc = -EINVAL;
-                /* Remove the check below once we have
-                 * shadow-on-shadow.
-                 */
-                if ( cpu_has_svm && !paging_mode_hap(d) && a.value )
-                    rc = -EINVAL;
-                /* Set up NHVM state for any vcpus that are already up */
-                if ( a.value &&
-                     !d->arch.hvm_domain.params[HVM_PARAM_NESTEDHVM] )
-                    for_each_vcpu(d, v)
-                        if ( rc == 0 )
-                            rc = nestedhvm_vcpu_initialise(v);
-                if ( !a.value || rc )
-                    for_each_vcpu(d, v)
-                        nestedhvm_vcpu_destroy(v);
-                break;
-            case HVM_PARAM_BUFIOREQ_EVTCHN:
-                rc = -EINVAL;
-                break;
-            case HVM_PARAM_TRIPLE_FAULT_REASON:
-                if ( a.value > SHUTDOWN_MAX )
-                    rc = -EINVAL;
-                break;
-            case HVM_PARAM_IOREQ_SERVER_PFN:
-                if ( d == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-                d->arch.hvm_domain.ioreq_gmfn.base = a.value;
-                break;
-            case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
-            {
-                unsigned int i;
-
-                if ( d == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-                if ( a.value == 0 ||
-                     a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
-                {
-                    rc = -EINVAL;
-                    break;
-                }
-                for ( i = 0; i < a.value; i++ )
-                    set_bit(i, &d->arch.hvm_domain.ioreq_gmfn.mask);
-
-                break;
-            }
-            }
-
-            if ( rc == 0 ) 
-            {
-                d->arch.hvm_domain.params[a.index] = a.value;
-
-                switch( a.index )
-                {
-                case HVM_PARAM_MEMORY_EVENT_INT3:
-                case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-                {
-                    domain_pause(d);
-                    domain_unpause(d); /* Causes guest to latch new status */
-                    break;
-                }
-                case HVM_PARAM_MEMORY_EVENT_CR3:
-                {
-                    for_each_vcpu ( d, v )
-                        hvm_funcs.update_guest_cr(v, 0); /* Latches new CR3 mask through CR0 code */
-                    break;
-                }
-                }
-
-            }
-
-        }
-        else
-        {
-            switch ( a.index )
-            {
-            case HVM_PARAM_ACPI_S_STATE:
-                a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
-                break;
-            case HVM_PARAM_IOREQ_SERVER_PFN:
-            case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
-                if ( d == current->domain )
-                {
-                    rc = -EPERM;
-                    break;
-                }
-            case HVM_PARAM_IOREQ_PFN:
-            case HVM_PARAM_BUFIOREQ_PFN:
-            case HVM_PARAM_BUFIOREQ_EVTCHN: {
-                domid_t domid;
-                
-                /* May need to create server */
-                domid = d->arch.hvm_domain.params[HVM_PARAM_DM_DOMAIN];
-                rc = hvm_create_ioreq_server(d, domid, 1, 1, NULL);
-                if ( rc != 0 && rc != -EEXIST )
-                    goto param_fail;
-                /*FALLTHRU*/
-            }
-            default:
-                a.value = d->arch.hvm_domain.params[a.index];
-                break;
-            }
-            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
-        }
-
-        HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
-                    op == HVMOP_set_param ? "set" : "get",
-                    a.index, a.value);
+        rc = hvmop_set_param(
+            guest_handle_cast(arg, xen_hvm_param_t));
+        break;
 
-    param_fail:
-        rcu_unlock_domain(d);
+    case HVMOP_get_param:
+        rc = hvmop_get_param(
+            guest_handle_cast(arg, xen_hvm_param_t));
         break;
-    }
 
     case HVMOP_set_pci_intx_level:
         rc = hvmop_set_pci_intx_level(
-- 
1.7.10.4

Re: [PATCH v4 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Jan Beulich]

>>> On 05.05.15 at 17:47, <paul.durrant@citrix.com> wrote:
> The level of switch nesting in those ops is getting unreadable. Giving
> them their own functions does introduce some code duplication in the
> the pre-op checks but the overall result is easier to follow.
> 
> This patch is code movement (including style fixes). There is no
> functional change.
> 
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
>  xen/arch/x86/hvm/hvm.c |  556 +++++++++++++++++++++++++-----------------------
>  1 file changed, 294 insertions(+), 262 deletions(-)

This doesn't apply to current staging (conflicts with at least
0a246b41cab40e3409c361c1ef6c1fcfaba5fd1f).

Jan

Re: [PATCH v4 1/3] x86/hvm: give HVMOP_set_param and HVMOP_get_param their own functions

[Paul Durrant]

> -----Original Message-----
> From: Jan Beulich [mailto:JBeulich@suse.com]
> Sent: 06 May 2015 13:27
> To: Paul Durrant
> Cc: xen-devel@lists.xenproject.org; Keir (Xen.org)
> Subject: Re: [PATCH v4 1/3] x86/hvm: give HVMOP_set_param and
> HVMOP_get_param their own functions
> 
> >>> On 05.05.15 at 17:47, <paul.durrant@citrix.com> wrote:
> > The level of switch nesting in those ops is getting unreadable. Giving
> > them their own functions does introduce some code duplication in the
> > the pre-op checks but the overall result is easier to follow.
> >
> > This patch is code movement (including style fixes). There is no
> > functional change.
> >
> > Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> > Cc: Keir Fraser <keir@xen.org>
> > Cc: Jan Beulich <jbeulich@suse.com>
> > Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > ---
> >  xen/arch/x86/hvm/hvm.c |  556 +++++++++++++++++++++++++-----------
> ------------
> >  1 file changed, 294 insertions(+), 262 deletions(-)
> 
> This doesn't apply to current staging (conflicts with at least
> 0a246b41cab40e3409c361c1ef6c1fcfaba5fd1f).
> 

Ok. I'll rebase and resend the series.

  Paul

> Jan

[PATCH v4 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks

[Paul Durrant]

Some parameters can only (validly) be set once. Some should not be set
by a guest for its own domain, and others must not be set since they
require the domain to be paused. Consolidate these checks, along with
the XSM check, in a new hvm_allow_set_param() function for clarity.

Also, introduce hvm_allow_get_param() for similar reasons.

NOTE: This patch leaves the guest accessible parameter checks as
      black-lists and does not make any changes to which parameters
      can be accessed. A subsequent patch will tighten up these
      checks by changing from black-lists to white-lists.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |  153 ++++++++++++++++++++++++++++--------------------
 1 file changed, 90 insertions(+), 63 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c246b45..03543dd 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5638,6 +5638,61 @@ static int hvmop_set_evtchn_upcall_vector(
     return 0;
 }
 
+static int hvm_allow_set_param(struct domain *d,
+                               const struct xen_hvm_param *a)
+{
+    uint64_t value = d->arch.hvm_domain.params[a->index];
+    int rc;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /*
+     * The following parameters must not be set by the guest
+     * since the domain may need to be paused.
+     */
+    case HVM_PARAM_IDENT_PT:
+    case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_ACPI_S_STATE:
+    /* The following parameters should not be set by the guest. */
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_MEMORY_EVENT_CR0:
+    case HVM_PARAM_MEMORY_EVENT_CR3:
+    case HVM_PARAM_MEMORY_EVENT_CR4:
+    case HVM_PARAM_MEMORY_EVENT_INT3:
+    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
+    case HVM_PARAM_MEMORY_EVENT_MSR:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( d == current->domain )
+            rc = -EPERM;
+        break;
+    default:
+        break;
+    }
+
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /* The following parameters should only be changed once. */
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( value != 0 && a->value != value )
+            rc = -EEXIST;
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 static int hvmop_set_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
@@ -5662,7 +5717,7 @@ static int hvmop_set_param(
          (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
         goto out;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
+    rc = hvm_allow_set_param(d, &a);
     if ( rc )
         goto out;
 
@@ -5677,31 +5732,11 @@ static int hvmop_set_param(
             rc = -EINVAL;
         break;
     case HVM_PARAM_VIRIDIAN:
-        /* This should only ever be set once by the tools and read by the guest. */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
-        if ( a.value != d->arch.hvm_domain.params[a.index] )
-        {
-            rc = -EEXIST;
-            if ( d->arch.hvm_domain.params[a.index] != 0 )
-                break;
-
+        if ( (a.value & ~HVMPV_feature_mask) ||
+             !(a.value & HVMPV_base_freq) )
             rc = -EINVAL;
-            if ( (a.value & ~HVMPV_feature_mask) ||
-                 !(a.value & HVMPV_base_freq) )
-                break;
-        }
-
-        rc = 0;
         break;
     case HVM_PARAM_IDENT_PT:
-        /* Not reflexive, as we must domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         rc = -EINVAL;
         if ( d->arch.hvm_domain.params[a.index] != 0 )
             break;
@@ -5729,22 +5764,12 @@ static int hvmop_set_param(
         domctl_lock_release();
         break;
     case HVM_PARAM_DM_DOMAIN:
-        /* Not reflexive, as we may need to domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         if ( a.value == DOMID_SELF )
             a.value = curr_d->domain_id;
 
         rc = hvm_set_dm_domain(d, a.value);
         break;
     case HVM_PARAM_ACPI_S_STATE:
-        /* Not reflexive, as we must domain_pause(). */
-        rc = -EPERM;
-        if ( d == curr_d )
-            break;
-
         rc = 0;
         if ( a.value == 3 )
             hvm_s3_suspend(d);
@@ -5757,20 +5782,9 @@ static int hvmop_set_param(
     case HVM_PARAM_ACPI_IOPORTS_LOCATION:
         rc = pmtimer_change_ioport(d, a.value);
         break;
-    case HVM_PARAM_MEMORY_EVENT_CR0:
-    case HVM_PARAM_MEMORY_EVENT_CR3:
-    case HVM_PARAM_MEMORY_EVENT_CR4:
-        if ( d == curr_d )
-            rc = -EPERM;
-        break;
     case HVM_PARAM_MEMORY_EVENT_INT3:
     case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
     case HVM_PARAM_MEMORY_EVENT_MSR:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         if ( a.value & HVMPME_onchangeonly )
             rc = -EINVAL;
         break;
@@ -5804,22 +5818,12 @@ static int hvmop_set_param(
             rc = -EINVAL;
         break;
     case HVM_PARAM_IOREQ_SERVER_PFN:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         d->arch.hvm_domain.ioreq_gmfn.base = a.value;
         break;
     case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
     {
         unsigned int i;
 
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            break;
-        }
         if ( a.value == 0 ||
              a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
         {
@@ -5859,10 +5863,40 @@ static int hvmop_set_param(
     return rc;
 }
 
+static int hvm_allow_get_param(struct domain *d,
+                               const struct xen_hvm_param *a)
+{
+    int rc;
+
+    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    if ( rc )
+        return rc;
+
+    switch ( a->index )
+    {
+    /*
+     * The following parameters must not be read by the guest
+     * since the domain may need to be paused.
+     */
+    case HVM_PARAM_IOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_EVTCHN:
+    /* The following parameters should not be read by the guest. */
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+        if ( d == current->domain )
+            rc = -EPERM;
+        break;
+    default:
+        break;
+    }
+
+    return rc;
+}
+
 static int hvmop_get_param(
     XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
 {
-    struct domain *curr_d = current->domain;
     struct xen_hvm_param a;
     struct domain *d;
     int rc;
@@ -5882,7 +5916,7 @@ static int hvmop_get_param(
          (is_pvh_domain(d) && (a.index != HVM_PARAM_CALLBACK_IRQ)) )
         goto out;
 
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
+    rc = hvm_allow_get_param(d, &a);
     if ( rc )
         goto out;
 
@@ -5891,13 +5925,6 @@ static int hvmop_get_param(
     case HVM_PARAM_ACPI_S_STATE:
         a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
         break;
-    case HVM_PARAM_IOREQ_SERVER_PFN:
-    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
-        if ( d == curr_d )
-        {
-            rc = -EPERM;
-            goto out;
-        }
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-- 
1.7.10.4

[PATCH v4 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

[Paul Durrant]

There are actually very few HVM parameters that a guest needs to read
and even fewer that a guest needs to write. Use white-lists to specify
those parameters and also ensre that, by default, newly introduced
parameters are not accessible.

Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
Cc: Keir Fraser <keir@xen.org>
Cc: Jan Beulich <jbeulich@suse.com>
Cc: Andrew Cooper <andrew.cooper3@citrix.com>
---
 xen/arch/x86/hvm/hvm.c |   37 ++++++++++++++++++++-----------------
 1 file changed, 20 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 03543dd..571809b 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5650,6 +5650,12 @@ static int hvm_allow_set_param(struct domain *d,
 
     switch ( a->index )
     {
+    /* The following parameters can be set by the guest. */
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_VM86_TSS:
+    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
+    case HVM_PARAM_VM_GENERATION_ID_ADDR:
+        break;
     /*
      * The following parameters must not be set by the guest
      * since the domain may need to be paused.
@@ -5657,21 +5663,11 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_IDENT_PT:
     case HVM_PARAM_DM_DOMAIN:
     case HVM_PARAM_ACPI_S_STATE:
-    /* The following parameters should not be set by the guest. */
-    case HVM_PARAM_VIRIDIAN:
-    case HVM_PARAM_MEMORY_EVENT_CR0:
-    case HVM_PARAM_MEMORY_EVENT_CR3:
-    case HVM_PARAM_MEMORY_EVENT_CR4:
-    case HVM_PARAM_MEMORY_EVENT_INT3:
-    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-    case HVM_PARAM_MEMORY_EVENT_MSR:
-    case HVM_PARAM_IOREQ_SERVER_PFN:
-    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+    /* The remaining parameters should not be set by the guest. */
+    default:
         if ( d == current->domain )
             rc = -EPERM;
         break;
-    default:
-        break;
     }
 
     if ( rc )
@@ -5874,6 +5870,16 @@ static int hvm_allow_get_param(struct domain *d,
 
     switch ( a->index )
     {
+    /* The following parameters can be read by the guest. */
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_VM86_TSS:
+    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
+    case HVM_PARAM_VM_GENERATION_ID_ADDR:
+    case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_STORE_EVTCHN:
+    case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_CONSOLE_EVTCHN:
+        break;
     /*
      * The following parameters must not be read by the guest
      * since the domain may need to be paused.
@@ -5881,14 +5887,11 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-    /* The following parameters should not be read by the guest. */
-    case HVM_PARAM_IOREQ_SERVER_PFN:
-    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+    /* The remaining parameters should not be read by the guest. */
+    default:
         if ( d == current->domain )
             rc = -EPERM;
         break;
-    default:
-        break;
     }
 
     return rc;
-- 
1.7.10.4

Re: [PATCH v4 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

[Andrew Cooper]

On 05/05/15 16:47, Paul Durrant wrote:
> There are actually very few HVM parameters that a guest needs to read
> and even fewer that a guest needs to write. Use white-lists to specify
> those parameters and also ensre that, by default, newly introduced
> parameters are not accessible.
>
> Signed-off-by: Paul Durrant <paul.durrant@citrix.com>
> Cc: Keir Fraser <keir@xen.org>
> Cc: Jan Beulich <jbeulich@suse.com>
> Cc: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>

> ---
>  xen/arch/x86/hvm/hvm.c |   37 ++++++++++++++++++++-----------------
>  1 file changed, 20 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 03543dd..571809b 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5650,6 +5650,12 @@ static int hvm_allow_set_param(struct domain *d,
>  
>      switch ( a->index )
>      {
> +    /* The following parameters can be set by the guest. */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_VM86_TSS:
> +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> +    case HVM_PARAM_VM_GENERATION_ID_ADDR:
> +        break;
>      /*
>       * The following parameters must not be set by the guest
>       * since the domain may need to be paused.
> @@ -5657,21 +5663,11 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_IDENT_PT:
>      case HVM_PARAM_DM_DOMAIN:
>      case HVM_PARAM_ACPI_S_STATE:
> -    /* The following parameters should not be set by the guest. */
> -    case HVM_PARAM_VIRIDIAN:
> -    case HVM_PARAM_MEMORY_EVENT_CR0:
> -    case HVM_PARAM_MEMORY_EVENT_CR3:
> -    case HVM_PARAM_MEMORY_EVENT_CR4:
> -    case HVM_PARAM_MEMORY_EVENT_INT3:
> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> -    case HVM_PARAM_MEMORY_EVENT_MSR:
> -    case HVM_PARAM_IOREQ_SERVER_PFN:
> -    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    /* The remaining parameters should not be set by the guest. */
> +    default:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> -    default:
> -        break;
>      }
>  
>      if ( rc )
> @@ -5874,6 +5870,16 @@ static int hvm_allow_get_param(struct domain *d,
>  
>      switch ( a->index )
>      {
> +    /* The following parameters can be read by the guest. */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_VM86_TSS:
> +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> +    case HVM_PARAM_VM_GENERATION_ID_ADDR:
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:
> +        break;
>      /*
>       * The following parameters must not be read by the guest
>       * since the domain may need to be paused.
> @@ -5881,14 +5887,11 @@ static int hvm_allow_get_param(struct domain *d,
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    /* The following parameters should not be read by the guest. */
> -    case HVM_PARAM_IOREQ_SERVER_PFN:
> -    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    /* The remaining parameters should not be read by the guest. */
> +    default:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> -    default:
> -        break;
>      }
>  
>      return rc;