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

[PATCH v3 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

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 v3 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

[PATCH v3 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>
Cc: 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

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

[Andrew Cooper]

On 05/05/15 11:25, Paul Durrant wrote:
> 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>
> Cc: Andrew Cooper <andrew.cooper3@citrix.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:

[PATCH v3 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 |   39 ++++++++++++++++++++++-----------------
 1 file changed, 22 insertions(+), 17 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 03543dd..ccf19a4 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -5650,6 +5650,13 @@ 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_TRIPLE_FAULT_REASON:
+    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 +5664,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 +5871,17 @@ 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_TRIPLE_FAULT_REASON:
+    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 +5889,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 v3 3/3] x86/hvm: Use white-lists for HVM param guest accessibility checks

[Andrew Cooper]

On 05/05/15 11:25, 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>
> ---
>  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
>  1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 03543dd..ccf19a4 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5650,6 +5650,13 @@ 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:

The only case where the VM86_TSS is needed is when VT-x doesn't support
unrestricted mode, in which case this parameter and IDENT_PT must be set
up by the domain builder in order to execute hvmloader.  Neither need to
be settable by the guest.

> +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> +    case HVM_PARAM_TRIPLE_FAULT_REASON:

TRIPLE_FAULT_REASON really shouldn't be settable by the guest (although
it is not a security problem that it currently is - it defaults to the
most expensive toolstack action possible).

Its intended purpose was to catch triple faults as crashes rather than
reboots (i.e. a difference from real hardware).

> +    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 +5664,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:

I think you can safely elide the above cases into default.  All that
matters in this case is that none of the whitelisted parameters need to
pause the domain.

> -    /* 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 +5871,17 @@ 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:

The details here should be read out of the ACPI tables, not from an HVM
Param.

> +    case HVM_PARAM_TRIPLE_FAULT_REASON:

A guest really shouldn't care what action a triple fault will have.

> +    case HVM_PARAM_VM_GENERATION_ID_ADDR:

Again - should be read from the ACPI tables.

~Andrew

> +    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 +5889,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;

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

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 May 2015 11:54
> To: Paul Durrant; xen-devel@lists.xenproject.org
> Cc: Keir (Xen.org); Jan Beulich
> Subject: Re: [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest
> accessibility checks
> 
> On 05/05/15 11:25, 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>
> > ---
> >  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 03543dd..ccf19a4 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5650,6 +5650,13 @@ 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:
> 
> The only case where the VM86_TSS is needed is when VT-x doesn't support
> unrestricted mode, in which case this parameter and IDENT_PT must be set
> up by the domain builder in order to execute hvmloader.  Neither need to
> be settable by the guest.

Really? What about the hvm_param_set() done at hvmloader.c:183? 

> 
> > +    case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> > +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> 
> TRIPLE_FAULT_REASON really shouldn't be settable by the guest (although
> it is not a security problem that it currently is - it defaults to the
> most expensive toolstack action possible).
> 
> Its intended purpose was to catch triple faults as crashes rather than
> reboots (i.e. a difference from real hardware).
> 

Ok. I'll shoot that one then.

> > +    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 +5664,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:
> 
> I think you can safely elide the above cases into default.  All that
> matters in this case is that none of the whitelisted parameters need to
> pause the domain.
> 

I think you and Jan differ there, unless I misunderstood Jan.

> > -    /* 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 +5871,17 @@ 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:
> 
> The details here should be read out of the ACPI tables, not from an HVM
> Param.
> 

Agreed, but IMO anything that's able to be set by the guest should also be readable by the guest.

> > +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> 
> A guest really shouldn't care what action a triple fault will have.
> 

Ok.

> > +    case HVM_PARAM_VM_GENERATION_ID_ADDR:
> 
> Again - should be read from the ACPI tables.
> 

Again, it's set by the guest (or by hvmloader) so it should be readable.

  Paul

> ~Andrew
> 
> > +    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 +5889,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;

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

[Jan Beulich]

>>> On 05.05.15 at 16:09, <Paul.Durrant@citrix.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 May 2015 11:54
>> On 05/05/15 11:25, Paul Durrant wrote:
>> > @@ -5657,21 +5664,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:
>> 
>> I think you can safely elide the above cases into default.  All that
>> matters in this case is that none of the whitelisted parameters need to
>> pause the domain.
>> 
> 
> I think you and Jan differ there, unless I misunderstood Jan.

Right, I specifically asked for these to be retained. It's just that
the comment preceding them is not visible from the patch
context, so that purpose isn't obvious here.

Jan

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

[Andrew Cooper]

On 05/05/15 15:29, Jan Beulich wrote:
>>>> On 05.05.15 at 16:09, <Paul.Durrant@citrix.com> wrote:
>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>> Sent: 05 May 2015 11:54
>>> On 05/05/15 11:25, Paul Durrant wrote:
>>>> @@ -5657,21 +5664,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:
>>> I think you can safely elide the above cases into default.  All that
>>> matters in this case is that none of the whitelisted parameters need to
>>> pause the domain.
>>>
>> I think you and Jan differ there, unless I misunderstood Jan.
> Right, I specifically asked for these to be retained. It's just that
> the comment preceding them is not visible from the patch
> context, so that purpose isn't obvious here.

I would question its usefulness as documentation, given that its
position in the code is now removed from the actual implementation.

However, I am not sufficiently fussed to block an otherwise-good change.

~Andrew

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

[Jan Beulich]

>>> On 05.05.15 at 16:43, <andrew.cooper3@citrix.com> wrote:
> On 05/05/15 15:29, Jan Beulich wrote:
>>>>> On 05.05.15 at 16:09, <Paul.Durrant@citrix.com> wrote:
>>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>>> Sent: 05 May 2015 11:54
>>>> On 05/05/15 11:25, Paul Durrant wrote:
>>>>> @@ -5657,21 +5664,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:
>>>> I think you can safely elide the above cases into default.  All that
>>>> matters in this case is that none of the whitelisted parameters need to
>>>> pause the domain.
>>>>
>>> I think you and Jan differ there, unless I misunderstood Jan.
>> Right, I specifically asked for these to be retained. It's just that
>> the comment preceding them is not visible from the patch
>> context, so that purpose isn't obvious here.
> 
> I would question its usefulness as documentation, given that its
> position in the code is now removed from the actual implementation.

Leaving aside the fact that these perhaps all shouldn't be used
by the guest anyway - if the implementation changed to no
longer require pausing of the domain, the argument for excluding
them from the white list would vanish, and hence they could
become guest accessible if so desired. And the code now being
far away from the actual implementation may even be considered
an extra argument for stating explicitly what would otherwise
possibly be quite obvious.

Jan

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

[Andrew Cooper]

On 05/05/15 15:09, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 May 2015 11:54
>> To: Paul Durrant; xen-devel@lists.xenproject.org
>> Cc: Keir (Xen.org); Jan Beulich
>> Subject: Re: [PATCH v3 3/3] x86/hvm: Use white-lists for HVM param guest
>> accessibility checks
>>
>> On 05/05/15 11:25, 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>
>>> ---
>>>  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
>>>  1 file changed, 22 insertions(+), 17 deletions(-)
>>>
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index 03543dd..ccf19a4 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -5650,6 +5650,13 @@ 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:
>> The only case where the VM86_TSS is needed is when VT-x doesn't support
>> unrestricted mode, in which case this parameter and IDENT_PT must be set
>> up by the domain builder in order to execute hvmloader.  Neither need to
>> be settable by the guest.
> Really? What about the hvm_param_set() done at hvmloader.c:183? 

Oops - I completely missed that when looking for it. :s  In which case
VM86_TSS does need to be settable.

>
>>> -    /* 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 +5871,17 @@ 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:
>> The details here should be read out of the ACPI tables, not from an HVM
>> Param.
>>
> Agreed, but IMO anything that's able to be set by the guest should also be readable by the guest.

A reasonable argument.  Ok.

~Andrew

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

[Tim Deegan]

At 11:53 +0100 on 05 May (1430826812), Andrew Cooper wrote:
> On 05/05/15 11:25, 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>
> > ---
> >  xen/arch/x86/hvm/hvm.c |   39 ++++++++++++++++++++++-----------------
> >  1 file changed, 22 insertions(+), 17 deletions(-)
> >
> > diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> > index 03543dd..ccf19a4 100644
> > --- a/xen/arch/x86/hvm/hvm.c
> > +++ b/xen/arch/x86/hvm/hvm.c
> > @@ -5650,6 +5650,13 @@ 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:
> 
> The only case where the VM86_TSS is needed is when VT-x doesn't support
> unrestricted mode, in which case this parameter and IDENT_PT must be set
> up by the domain builder in order to execute hvmloader.  Neither need to
> be settable by the guest.

IDENT_PT is indeed set up by the toolstack, but VM86_TSS is set by
hvmloader, as it's not needed until hvmloader enters real mode.
It would be OK to make those two params set-once param if you're feeling
keen, but in fact there's no harm in letting the guest change them --
after all it can write to the memory they point to.

Cheers,

Tim.