[PATCH 0/5] xen: Fixes and improvements to HVM_PARAM handling

[PATCH 0/5] xen: Fixes and improvements to HVM_PARAM handling

[Andrew Cooper]

This started with the observation in patch 5, and expanded from there.  The
end result should be rather more predictable and easy to deprecate parameters
from.

An observation which I haven't addressed (and don't have time in the near
future) is that the hvm params array is moderately wasteful on x86 and very
wasteful on ARM.  An option would be to switch to using a named structure,
which would also fix the problem that both arch's hypercall handers are BCBS
gadgets.

Andrew Cooper (5):
  x86/hvm: Switch hvm_allow_get_param() to use a whitelist
  x86/hvm: Switch hvm_allow_set_param() to use a whitelist
  x86/hvm: Make HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
  x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure
  xen/ARM: Restrict access to most HVM_PARAM's

 xen/arch/arm/hvm.c     |  62 +++++++++-
 xen/arch/x86/hvm/hvm.c | 312 +++++++++++++++++++++++++------------------------
 2 files changed, 221 insertions(+), 153 deletions(-)

-- 
2.1.4


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

[PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Andrew Cooper]

There are holes in the HVM_PARAM space, some of which are from deprecated
parameters, but toolstack and device models currently have blanket read
access.

Rearrange hvm_allow_get_param() to have a whitelist of toolstack-readable
parameters, with the default case failing with -EINVAL (which subsumes the
HVM_NR_PARAMS check).

No expected change for the defined, in-use params.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/hvm.c | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index c22bf0b..96a6323 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
 
     switch ( a->index )
     {
-    /* The following parameters can be read by the guest. */
+        /* The following parameters can be read by the guest and toolstack. */
     case HVM_PARAM_CALLBACK_IRQ:
     case HVM_PARAM_VM86_TSS:
     case HVM_PARAM_VM86_TSS_SIZED:
@@ -4363,18 +4363,39 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_ALTP2M:
     case HVM_PARAM_X87_FIP_WIDTH:
         break;
-    /*
-     * The following parameters must not be read by the guest
-     * since the domain may need to be paused.
-     */
+
+        /*
+         * The following parameters are intended for toolstack usage only.
+         * Some require the domain to be paused, and therefore may not read by
+         * the domain.
+         */
+    case HVM_PARAM_PAE_ENABLED:
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
-    /* The remaining parameters should not be read by the guest. */
-    default:
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_TIMER_MODE:
+    case HVM_PARAM_HPET_ENABLED:
+    case HVM_PARAM_IDENT_PT:
+    case HVM_PARAM_DM_DOMAIN:
+    case HVM_PARAM_ACPI_S_STATE:
+    case HVM_PARAM_VPT_ALIGN:
+    case HVM_PARAM_NESTEDHVM:
+    case HVM_PARAM_PAGING_RING_PFN:
+    case HVM_PARAM_MONITOR_RING_PFN:
+    case HVM_PARAM_SHARING_RING_PFN:
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+    case HVM_PARAM_MCA_CAP:
         if ( d == current->domain )
             rc = -EPERM;
         break;
+
+        /* Hole, deprecated, or out-of-range. */
+    default:
+        rc = -EINVAL;
+        break;
     }
 
     return rc;
@@ -4390,9 +4411,6 @@ static int hvmop_get_param(
     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;
-- 
2.1.4


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

[PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Andrew Cooper]

There are holes in the HVM_PARAM space, some of which are from deprecated
parameters, but toolstack and device models currently has (almost) blanket
write access.

Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
parameters, with the default case failing with -EINVAL.  This subsumes the
HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
BUFIOREQ_EVTCHN Xen-write-only value.

No expected change for the defined, in-use params.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------------------
 1 file changed, 31 insertions(+), 22 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 96a6323..d19ae35 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
 
     switch ( a->index )
     {
-    /* The following parameters can be set by the guest. */
+        /* The following parameters can be set by the guest and toolstack. */
     case HVM_PARAM_CALLBACK_IRQ:
     case HVM_PARAM_VM86_TSS:
     case HVM_PARAM_VM86_TSS_SIZED:
@@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_X87_FIP_WIDTH:
         break;
-    /*
-     * The following parameters must not be set by the guest
-     * since the domain may need to be paused.
-     */
+
+        /*
+         * The following parameters are intended for toolstack usage only.
+         * Some require the domain to be paused while others control
+         * permissions in Xen, and therefore may not set by the domain.
+         */
+    case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_PAE_ENABLED:
+    case HVM_PARAM_IOREQ_PFN:
+    case HVM_PARAM_BUFIOREQ_PFN:
+    case HVM_PARAM_VIRIDIAN:
+    case HVM_PARAM_TIMER_MODE:
+    case HVM_PARAM_HPET_ENABLED:
     case HVM_PARAM_IDENT_PT:
     case HVM_PARAM_DM_DOMAIN:
     case HVM_PARAM_ACPI_S_STATE:
-    /* The remaining parameters should not be set by the guest. */
-    default:
+    case HVM_PARAM_VPT_ALIGN:
+    case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_NESTEDHVM:
+    case HVM_PARAM_PAGING_RING_PFN:
+    case HVM_PARAM_MONITOR_RING_PFN:
+    case HVM_PARAM_SHARING_RING_PFN:
+    case HVM_PARAM_TRIPLE_FAULT_REASON:
+    case HVM_PARAM_IOREQ_SERVER_PFN:
+    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
+    case HVM_PARAM_MCA_CAP:
         if ( d == current->domain )
             rc = -EPERM;
         break;
+
+        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
+    default:
+        rc = -EINVAL;
+        break;
     }
 
     if ( rc )
@@ -4130,9 +4152,6 @@ static int hvmop_set_param(
     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;
@@ -4209,15 +4228,7 @@ 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:
-    case HVM_PARAM_MEMORY_EVENT_INT3:
-    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
-    case HVM_PARAM_MEMORY_EVENT_MSR:
-        /* Deprecated */
-        rc = -EOPNOTSUPP;
-        break;
+
     case HVM_PARAM_NESTEDHVM:
         rc = xsm_hvm_param_nested(XSM_PRIV, d);
         if ( rc )
@@ -4253,9 +4264,7 @@ static int hvmop_set_param(
              d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
             rc = -EINVAL;
         break;
-    case HVM_PARAM_BUFIOREQ_EVTCHN:
-        rc = -EINVAL;
-        break;
+
     case HVM_PARAM_TRIPLE_FAULT_REASON:
         if ( a.value > SHUTDOWN_MAX )
             rc = -EINVAL;
-- 
2.1.4


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

[PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Andrew Cooper]

These values are set by the toolstack for each create/restore operation, and
bound by xen{store,console}d before the the guest starts running.

A guest has no reason to modify them at all, and the matching *_PFN parameters
are already read-only.  Adjust the *_EVTCHN permissions to be consistent.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/hvm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index d19ae35..408e695 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4079,8 +4079,6 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_VM86_TSS_SIZED:
     case HVM_PARAM_ACPI_IOPORTS_LOCATION:
     case HVM_PARAM_VM_GENERATION_ID_ADDR:
-    case HVM_PARAM_STORE_EVTCHN:
-    case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_X87_FIP_WIDTH:
         break;
 
@@ -4090,6 +4088,7 @@ static int hvm_allow_set_param(struct domain *d,
          * permissions in Xen, and therefore may not set by the domain.
          */
     case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_STORE_EVTCHN:
     case HVM_PARAM_PAE_ENABLED:
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
@@ -4101,6 +4100,7 @@ static int hvm_allow_set_param(struct domain *d,
     case HVM_PARAM_ACPI_S_STATE:
     case HVM_PARAM_VPT_ALIGN:
     case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_NESTEDHVM:
     case HVM_PARAM_PAGING_RING_PFN:
     case HVM_PARAM_MONITOR_RING_PFN:
-- 
2.1.4


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

[PATCH 4/5] x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure

[Andrew Cooper]

The parameter marshalling and xsm checks are common to both the set and get
paths.  Lift all of the common code out into do_hvm_op() and let
hvmop_{get,set}_param() operate on struct xen_hvm_param directly.

This is somewhat noisy in the functions as each a. reference has to change to
a-> instead.

In addition, drop an empty default statement, insert newlines as appropriate
between cases, and there is no need to update the IDENT_PT on the fastpath,
because the common path after the switch will make the update.

No functional change, but a net shrink of -328 to do_hvm_op().

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>
---
 xen/arch/x86/hvm/hvm.c | 223 +++++++++++++++++++++++--------------------------
 1 file changed, 104 insertions(+), 119 deletions(-)

diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
index 408e695..c891269 100644
--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4065,11 +4065,7 @@ static int hvm_allow_set_param(struct domain *d,
                                const struct xen_hvm_param *a)
 {
     uint64_t value = d->arch.hvm.params[a->index];
-    int rc;
-
-    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
-    if ( rc )
-        return rc;
+    int rc = 0;
 
     switch ( a->index )
     {
@@ -4133,62 +4129,46 @@ static int hvm_allow_set_param(struct domain *d,
         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)
+static int hvmop_set_param(struct domain *d, struct xen_hvm_param *a)
 {
     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;
-
-    d = rcu_lock_domain_by_any_id(a.domid);
-    if ( d == NULL )
-        return -ESRCH;
-
-    rc = -EINVAL;
-    if ( !is_hvm_domain(d) )
-        goto out;
-
-    rc = hvm_allow_set_param(d, &a);
+    rc = hvm_allow_set_param(d, a);
     if ( rc )
         goto out;
 
-    switch ( a.index )
+    switch ( a->index )
     {
     case HVM_PARAM_CALLBACK_IRQ:
-        hvm_set_callback_via(d, a.value);
+        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 )
+        if ( a->value > HVMPTM_one_missed_tick_pending )
             rc = -EINVAL;
         break;
+
     case HVM_PARAM_VIRIDIAN:
-        if ( (a.value & ~HVMPV_feature_mask) ||
-             !(a.value & HVMPV_base_freq) )
+        if ( (a->value & ~HVMPV_feature_mask) ||
+             !(a->value & HVMPV_base_freq) )
             rc = -EINVAL;
         break;
+
     case HVM_PARAM_IDENT_PT:
         /*
          * Only actually required for VT-x lacking unrestricted_guest
          * capabilities.  Short circuit the pause if possible.
          */
         if ( !paging_mode_hap(d) || !cpu_has_vmx )
-        {
-            d->arch.hvm.params[a.index] = a.value;
             break;
-        }
 
         /*
          * Update GUEST_CR3 in each VMCS to point at identity map.
@@ -4201,117 +4181,123 @@ static int hvmop_set_param(
 
         rc = 0;
         domain_pause(d);
-        d->arch.hvm.params[a.index] = a.value;
+        d->arch.hvm.params[a->index] = a->value;
         for_each_vcpu ( d, v )
             paging_update_cr3(v, false);
         domain_unpause(d);
 
         domctl_lock_release();
         break;
+
     case HVM_PARAM_DM_DOMAIN:
         /* The only value this should ever be set to is DOMID_SELF */
-        if ( a.value != DOMID_SELF )
+        if ( a->value != DOMID_SELF )
             rc = -EINVAL;
 
-        a.value = curr_d->domain_id;
+        a->value = curr_d->domain_id;
         break;
+
     case HVM_PARAM_ACPI_S_STATE:
         rc = 0;
-        if ( a.value == 3 )
+        if ( a->value == 3 )
             hvm_s3_suspend(d);
-        else if ( a.value == 0 )
+        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);
+        rc = pmtimer_change_ioport(d, a->value);
         break;
 
     case HVM_PARAM_NESTEDHVM:
         rc = xsm_hvm_param_nested(XSM_PRIV, d);
         if ( rc )
             break;
-        if ( a.value > 1 )
+        if ( a->value > 1 )
             rc = -EINVAL;
         /*
          * Remove the check below once we have
          * shadow-on-shadow.
          */
-        if ( !paging_mode_hap(d) && a.value )
+        if ( !paging_mode_hap(d) && a->value )
             rc = -EINVAL;
-        if ( a.value &&
+        if ( a->value &&
              d->arch.hvm.params[HVM_PARAM_ALTP2M] )
             rc = -EINVAL;
         /* Set up NHVM state for any vcpus that are already up. */
-        if ( a.value &&
+        if ( a->value &&
              !d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
             for_each_vcpu(d, v)
                 if ( rc == 0 )
                     rc = nestedhvm_vcpu_initialise(v);
-        if ( !a.value || rc )
+        if ( !a->value || rc )
             for_each_vcpu(d, v)
                 nestedhvm_vcpu_destroy(v);
         break;
+
     case HVM_PARAM_ALTP2M:
         rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d);
         if ( rc )
             break;
-        if ( a.value > XEN_ALTP2M_limited )
+        if ( a->value > XEN_ALTP2M_limited )
             rc = -EINVAL;
-        if ( a.value &&
+        if ( a->value &&
              d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
             rc = -EINVAL;
         break;
 
     case HVM_PARAM_TRIPLE_FAULT_REASON:
-        if ( a.value > SHUTDOWN_MAX )
+        if ( a->value > SHUTDOWN_MAX )
             rc = -EINVAL;
         break;
+
     case HVM_PARAM_IOREQ_SERVER_PFN:
-        d->arch.hvm.ioreq_gfn.base = a.value;
+        d->arch.hvm.ioreq_gfn.base = a->value;
         break;
+
     case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
     {
         unsigned int i;
 
-        if ( a.value == 0 ||
-             a.value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 )
+        if ( a->value == 0 ||
+             a->value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 )
         {
             rc = -EINVAL;
             break;
         }
-        for ( i = 0; i < a.value; i++ )
+        for ( i = 0; i < a->value; i++ )
             set_bit(i, &d->arch.hvm.ioreq_gfn.mask);
 
         break;
     }
+
     case HVM_PARAM_X87_FIP_WIDTH:
-        if ( a.value != 0 && a.value != 4 && a.value != 8 )
+        if ( a->value != 0 && a->value != 4 && a->value != 8 )
         {
             rc = -EINVAL;
             break;
         }
-        d->arch.x87_fip_width = a.value;
+        d->arch.x87_fip_width = a->value;
         break;
 
     case HVM_PARAM_VM86_TSS:
         /* Hardware would silently truncate high bits. */
-        if ( a.value != (uint32_t)a.value )
+        if ( a->value != (uint32_t)a->value )
         {
             if ( d == curr_d )
                 domain_crash(d);
             rc = -EINVAL;
         }
         /* Old hvmloader binaries hardcode the size to 128 bytes. */
-        if ( a.value )
-            a.value |= (128ULL << 32) | VM86_TSS_UPDATED;
-        a.index = HVM_PARAM_VM86_TSS_SIZED;
+        if ( a->value )
+            a->value |= (128ULL << 32) | VM86_TSS_UPDATED;
+        a->index = HVM_PARAM_VM86_TSS_SIZED;
         break;
 
     case HVM_PARAM_VM86_TSS_SIZED:
-        if ( (a.value >> 32) < sizeof(struct tss32) )
+        if ( (a->value >> 32) < sizeof(struct tss32) )
         {
             if ( d == curr_d )
                 domain_crash(d);
@@ -4322,41 +4308,34 @@ static int hvmop_set_param(
          * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap
          * plus one padding byte).
          */
-        if ( (a.value >> 32) > sizeof(struct tss32) +
-                               (0x100 / 8) + (0x10000 / 8) + 1 )
-            a.value = (uint32_t)a.value |
-                      ((sizeof(struct tss32) + (0x100 / 8) +
-                                               (0x10000 / 8) + 1) << 32);
-        a.value |= VM86_TSS_UPDATED;
+        if ( (a->value >> 32) > sizeof(struct tss32) +
+                                (0x100 / 8) + (0x10000 / 8) + 1 )
+            a->value = (uint32_t)a->value |
+                       ((sizeof(struct tss32) + (0x100 / 8) +
+                                                (0x10000 / 8) + 1) << 32);
+        a->value |= VM86_TSS_UPDATED;
         break;
 
     case HVM_PARAM_MCA_CAP:
-        rc = vmce_enable_mca_cap(d, a.value);
+        rc = vmce_enable_mca_cap(d, a->value);
         break;
     }
 
     if ( rc != 0 )
         goto out;
 
-    d->arch.hvm.params[a.index] = a.value;
+    d->arch.hvm.params[a->index] = a->value;
 
     HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
-                a.index, a.value);
+                a->index, a->value);
 
  out:
-    rcu_unlock_domain(d);
     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 can be read by the guest and toolstack. */
@@ -4371,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_CONSOLE_EVTCHN:
     case HVM_PARAM_ALTP2M:
     case HVM_PARAM_X87_FIP_WIDTH:
-        break;
+        return 0;
 
         /*
          * The following parameters are intended for toolstack usage only.
@@ -4397,59 +4376,41 @@ static int hvm_allow_get_param(struct domain *d,
     case HVM_PARAM_IOREQ_SERVER_PFN:
     case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
     case HVM_PARAM_MCA_CAP:
-        if ( d == current->domain )
-            rc = -EPERM;
-        break;
+        return d == current->domain ? -EPERM : 0;
 
         /* Hole, deprecated, or out-of-range. */
     default:
-        rc = -EINVAL;
-        break;
+        return -EINVAL;
     }
-
-    return rc;
 }
 
-static int hvmop_get_param(
-    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
+static int hvmop_get_param(struct domain *d, struct xen_hvm_param *a)
 {
-    struct xen_hvm_param a;
-    struct domain *d;
     int rc;
 
-    if ( copy_from_guest(&a, arg, 1) )
-        return -EFAULT;
-
-    d = rcu_lock_domain_by_any_id(a.domid);
-    if ( d == NULL )
-        return -ESRCH;
-
-    rc = -EINVAL;
-    if ( !is_hvm_domain(d) )
-        goto out;
-
-    rc = hvm_allow_get_param(d, &a);
+    rc = hvm_allow_get_param(d, a);
     if ( rc )
-        goto out;
+        return rc;
 
-    switch ( a.index )
+    switch ( a->index )
     {
     case HVM_PARAM_ACPI_S_STATE:
-        a.value = d->arch.hvm.is_s3_suspended ? 3 : 0;
+        a->value = d->arch.hvm.is_s3_suspended ? 3 : 0;
         break;
 
     case HVM_PARAM_VM86_TSS:
-        a.value = (uint32_t)d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED];
+        a->value = (uint32_t)d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED];
         break;
 
     case HVM_PARAM_VM86_TSS_SIZED:
-        a.value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] &
-                  ~VM86_TSS_UPDATED;
+        a->value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] &
+                   ~VM86_TSS_UPDATED;
         break;
 
     case HVM_PARAM_X87_FIP_WIDTH:
-        a.value = d->arch.x87_fip_width;
+        a->value = d->arch.x87_fip_width;
         break;
+
     case HVM_PARAM_IOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_PFN:
     case HVM_PARAM_BUFIOREQ_EVTCHN:
@@ -4465,23 +4426,19 @@ static int hvmop_get_param(
             rc = hvm_create_ioreq_server(d, true,
                                          HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
             if ( rc != 0 && rc != -EEXIST )
-                goto out;
+                return rc;
         }
 
-    /*FALLTHRU*/
+        /* Fallthrough */
     default:
-        a.value = d->arch.hvm.params[a.index];
+        a->value = d->arch.hvm.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);
+                a->index, a->value);
 
- out:
-    rcu_unlock_domain(d);
-    return rc;
+    return 0;
 }
 
 /*
@@ -4896,14 +4853,42 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         break;
     
     case HVMOP_set_param:
-        rc = hvmop_set_param(
-            guest_handle_cast(arg, xen_hvm_param_t));
-        break;
-
     case HVMOP_get_param:
-        rc = hvmop_get_param(
-            guest_handle_cast(arg, xen_hvm_param_t));
+    {
+        struct xen_hvm_param a;
+        struct domain *d;
+
+        rc = -EFAULT;
+        if ( copy_from_guest(&a, arg, 1) )
+            break;
+
+        rc = -ESRCH;
+        d = rcu_lock_domain_by_any_id(a.domid);
+        if ( d == NULL )
+            break;
+
+        rc = -EINVAL;
+        if ( !is_hvm_domain(d) )
+            goto param_out;
+
+        rc = xsm_hvm_param(XSM_TARGET, d, op);
+        if ( rc )
+            goto param_out;
+
+        if ( op == HVMOP_set_param )
+            rc = hvmop_set_param(d, &a);
+        else
+        {
+            rc = hvmop_get_param(d, &a);
+
+            if ( !rc && __copy_to_guest(arg, &a, 1) )
+                rc = -EFAULT;
+        }
+
+    param_out:
+        rcu_unlock_domain(d);
         break;
+    }
 
     case HVMOP_flush_tlbs:
         rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -EINVAL;
-- 
2.1.4


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

[PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Andrew Cooper]

ARM currently has no restrictions on toolstack and guest access to the entire
HVM_PARAM block.  As the paging/monitor/sharing features aren't under security
support, this doesn't need an XSA.

The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed read-only to
the guest, while the *_RING_PFN details are restricted to only toolstack
access.  No other parameters are used.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wei.liu2@citrix.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Paul Durrant <paul.durrant@citrix.com>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien.grall@arm.com>

This is only compile tested, and based on my reading of the source.  There
might be other PARAMS needing including.
---
 xen/arch/arm/hvm.c | 62 +++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 59 insertions(+), 3 deletions(-)

diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
index 76b27c9..3581ba2 100644
--- a/xen/arch/arm/hvm.c
+++ b/xen/arch/arm/hvm.c
@@ -31,6 +31,57 @@
 
 #include <asm/hypercall.h>
 
+static int hvm_allow_set_param(const struct domain *d, unsigned int param)
+{
+    switch ( param )
+    {
+        /*
+         * The following parameters are intended for toolstack usage only.
+         * They may not be set by the domain.
+         */
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_STORE_EVTCHN:
+    case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_CONSOLE_EVTCHN:
+    case HVM_PARAM_PAGING_RING_PFN:
+    case HVM_PARAM_MONITOR_RING_PFN:
+    case HVM_PARAM_SHARING_RING_PFN:
+        return d == current->domain ? -EPERM : 0;
+
+        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
+    default:
+        return -EINVAL;
+    }
+}
+
+static int hvm_allow_get_param(const struct domain *d, unsigned int param)
+{
+    switch ( param )
+    {
+        /* The following parameters can be read by the guest and toolstack. */
+    case HVM_PARAM_CALLBACK_IRQ:
+    case HVM_PARAM_STORE_PFN:
+    case HVM_PARAM_STORE_EVTCHN:
+    case HVM_PARAM_CONSOLE_PFN:
+    case HVM_PARAM_CONSOLE_EVTCHN:
+        return 0;
+
+        /*
+         * The following parameters are intended for toolstack usage only.
+         * They may not be read by the domain.
+         */
+    case HVM_PARAM_PAGING_RING_PFN:
+    case HVM_PARAM_MONITOR_RING_PFN:
+    case HVM_PARAM_SHARING_RING_PFN:
+        return d == current->domain ? -EPERM : 0;
+
+        /* Hole, deprecated, or out-of-range. */
+    default:
+        return -EINVAL;
+    }
+}
+
 long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 {
     long rc = 0;
@@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
         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;
@@ -59,10 +107,18 @@ long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void) arg)
 
         if ( op == HVMOP_set_param )
         {
+            rc = hvm_allow_set_param(d, a.index);
+            if ( rc )
+                goto param_fail;
+
             d->arch.hvm.params[a.index] = a.value;
         }
         else
         {
+            rc = hvm_allow_get_param(d, a.index);
+            if ( rc )
+                goto param_fail;
+
             a.value = d->arch.hvm.params[a.index];
             rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
-- 
2.1.4


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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a
> whitelist
> 
> There are holes in the HVM_PARAM space, some of which are from
> deprecated
> parameters, but toolstack and device models currently have blanket read
> access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-readable
> parameters, with the default case failing with -EINVAL (which subsumes the
> HVM_NR_PARAMS check).
> 
> No expected change for the defined, in-use params.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c22bf0b..96a6323 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
> 
>      switch ( a->index )
>      {
> -    /* The following parameters can be read by the guest. */
> +        /* The following parameters can be read by the guest and toolstack. */

Intentional indentation change? I guess so since you do it again below, but just checking.

>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:
>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4363,18 +4363,39 @@ static int hvm_allow_get_param(struct domain
> *d,
>      case HVM_PARAM_ALTP2M:
>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> -    /*
> -     * The following parameters must not be read by the guest
> -     * since the domain may need to be paused.
> -     */
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * Some require the domain to be paused, and therefore may not read
> by
> +         * the domain.
> +         */
> +    case HVM_PARAM_PAE_ENABLED:
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> -    /* The remaining parameters should not be read by the guest. */
> -    default:
> +    case HVM_PARAM_VIRIDIAN:
> +    case HVM_PARAM_TIMER_MODE:
> +    case HVM_PARAM_HPET_ENABLED:
> +    case HVM_PARAM_IDENT_PT:
> +    case HVM_PARAM_DM_DOMAIN:
> +    case HVM_PARAM_ACPI_S_STATE:
> +    case HVM_PARAM_VPT_ALIGN:
> +    case HVM_PARAM_NESTEDHVM:
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> +    case HVM_PARAM_IOREQ_SERVER_PFN:
> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    case HVM_PARAM_MCA_CAP:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> +
> +        /* Hole, deprecated, or out-of-range. */
> +    default:
> +        rc = -EINVAL;
> +        break;
>      }
> 
>      return rc;
> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>      if ( copy_from_guest(&a, arg, 1) )
>          return -EFAULT;
> 
> -    if ( a.index >= HVM_NR_PARAMS )
> -        return -EINVAL;
> -

ASSERT, just in case someone screws up the allow function in future?

>      d = rcu_lock_domain_by_any_id(a.domid);
>      if ( d == NULL )
>          return -ESRCH;
> --
> 2.1.4

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

Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a
> whitelist
> 
> There are holes in the HVM_PARAM space, some of which are from
> deprecated
> parameters, but toolstack and device models currently has (almost) blanket

s/has/have

> write access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable

s/get/set

> parameters, with the default case failing with -EINVAL.  This subsumes the
> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated
> block, and the
> BUFIOREQ_EVTCHN Xen-write-only value.
> 
> No expected change for the defined, in-use params.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------
> ------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 96a6323..d19ae35 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
> 
>      switch ( a->index )
>      {
> -    /* The following parameters can be set by the guest. */
> +        /* The following parameters can be set by the guest and toolstack. */

Personally I'm finding this indentation of comment incongruous. Such comments are logically outside of any particular case statement so I think they should have the same level of indentation as the case statements themselves.

>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:
>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain
> *d,
>      case HVM_PARAM_CONSOLE_EVTCHN:
>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> -    /*
> -     * The following parameters must not be set by the guest
> -     * since the domain may need to be paused.
> -     */
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * Some require the domain to be paused while others control
> +         * permissions in Xen, and therefore may not set by the domain.
> +         */
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_PAE_ENABLED:
> +    case HVM_PARAM_IOREQ_PFN:
> +    case HVM_PARAM_BUFIOREQ_PFN:
> +    case HVM_PARAM_VIRIDIAN:
> +    case HVM_PARAM_TIMER_MODE:
> +    case HVM_PARAM_HPET_ENABLED:
>      case HVM_PARAM_IDENT_PT:
>      case HVM_PARAM_DM_DOMAIN:
>      case HVM_PARAM_ACPI_S_STATE:
> -    /* The remaining parameters should not be set by the guest. */
> -    default:
> +    case HVM_PARAM_VPT_ALIGN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_NESTEDHVM:
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> +    case HVM_PARAM_IOREQ_SERVER_PFN:
> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    case HVM_PARAM_MCA_CAP:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> +
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        rc = -EINVAL;
> +        break;
>      }
> 
>      if ( rc )
> @@ -4130,9 +4152,6 @@ static int hvmop_set_param(
>      if ( copy_from_guest(&a, arg, 1) )
>          return -EFAULT;
> 
> -    if ( a.index >= HVM_NR_PARAMS )
> -        return -EINVAL;
> -

Again, I think an ASSERT here would be good.

>      d = rcu_lock_domain_by_any_id(a.domid);
>      if ( d == NULL )
>          return -ESRCH;
> @@ -4209,15 +4228,7 @@ 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:
> -    case HVM_PARAM_MEMORY_EVENT_INT3:
> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> -    case HVM_PARAM_MEMORY_EVENT_MSR:
> -        /* Deprecated */
> -        rc = -EOPNOTSUPP;
> -        break;

Does anything rely on this EOPNOTSUPP vs the EINVAL that would be returned after this patch is applied?

  Paul

> +
>      case HVM_PARAM_NESTEDHVM:
>          rc = xsm_hvm_param_nested(XSM_PRIV, d);
>          if ( rc )
> @@ -4253,9 +4264,7 @@ static int hvmop_set_param(
>               d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
>              rc = -EINVAL;
>          break;
> -    case HVM_PARAM_BUFIOREQ_EVTCHN:
> -        rc = -EINVAL;
> -        break;
> +
>      case HVM_PARAM_TRIPLE_FAULT_REASON:
>          if ( a.value > SHUTDOWN_MAX )
>              rc = -EINVAL;
> --
> 2.1.4

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

Re: [PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 3/5] x86/hvm: Make
> HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
> 
> These values are set by the toolstack for each create/restore operation, and
> bound by xen{store,console}d before the the guest starts running.
> 
> A guest has no reason to modify them at all, and the matching *_PFN
> parameters
> are already read-only.  Adjust the *_EVTCHN permissions to be consistent.

Unfortunately this patch will break the Windows PV driver function here:

http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xenbus/evtchn.c;hb=HEAD#l1037

Unfortunately the values really do change across a reset. It would be possible to use volatile (disappear on reboot) registry keys to store the updated values instead but I don't really see any harm in allowing the guest to update the values to be correct, unless we want to change Xen to do the job so the guest doesn't have to go through this dance.

  Paul

> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index d19ae35..408e695 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4079,8 +4079,6 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_VM86_TSS_SIZED:
>      case HVM_PARAM_ACPI_IOPORTS_LOCATION:
>      case HVM_PARAM_VM_GENERATION_ID_ADDR:
> -    case HVM_PARAM_STORE_EVTCHN:
> -    case HVM_PARAM_CONSOLE_EVTCHN:
>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> 
> @@ -4090,6 +4088,7 @@ static int hvm_allow_set_param(struct domain *d,
>           * permissions in Xen, and therefore may not set by the domain.
>           */
>      case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
>      case HVM_PARAM_PAE_ENABLED:
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
> @@ -4101,6 +4100,7 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_ACPI_S_STATE:
>      case HVM_PARAM_VPT_ALIGN:
>      case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:
>      case HVM_PARAM_NESTEDHVM:
>      case HVM_PARAM_PAGING_RING_PFN:
>      case HVM_PARAM_MONITOR_RING_PFN:
> --
> 2.1.4

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

Re: [PATCH 4/5] x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 4/5] x86/hvm: Misc non-functional cleanup to the
> HVM_PARAM infrastructure
> 
> The parameter marshalling and xsm checks are common to both the set and
> get
> paths.  Lift all of the common code out into do_hvm_op() and let
> hvmop_{get,set}_param() operate on struct xen_hvm_param directly.
> 
> This is somewhat noisy in the functions as each a. reference has to change to
> a-> instead.
> 
> In addition, drop an empty default statement, insert newlines as appropriate
> between cases, and there is no need to update the IDENT_PT on the
> fastpath,
> because the common path after the switch will make the update.
> 
> No functional change, but a net shrink of -328 to do_hvm_op().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 223 +++++++++++++++++++++++-----------------
> ---------
>  1 file changed, 104 insertions(+), 119 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 408e695..c891269 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4065,11 +4065,7 @@ static int hvm_allow_set_param(struct domain *d,
>                                 const struct xen_hvm_param *a)
>  {
>      uint64_t value = d->arch.hvm.params[a->index];
> -    int rc;
> -
> -    rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
> -    if ( rc )
> -        return rc;
> +    int rc = 0;
> 
>      switch ( a->index )
>      {
> @@ -4133,62 +4129,46 @@ static int hvm_allow_set_param(struct domain
> *d,
>          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)
> +static int hvmop_set_param(struct domain *d, struct xen_hvm_param *a)
>  {
>      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;
> -
> -    d = rcu_lock_domain_by_any_id(a.domid);
> -    if ( d == NULL )
> -        return -ESRCH;
> -
> -    rc = -EINVAL;
> -    if ( !is_hvm_domain(d) )
> -        goto out;
> -
> -    rc = hvm_allow_set_param(d, &a);
> +    rc = hvm_allow_set_param(d, a);
>      if ( rc )
>          goto out;
> 
> -    switch ( a.index )
> +    switch ( a->index )
>      {
>      case HVM_PARAM_CALLBACK_IRQ:
> -        hvm_set_callback_via(d, a.value);
> +        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 )
> +        if ( a->value > HVMPTM_one_missed_tick_pending )
>              rc = -EINVAL;
>          break;
> +
>      case HVM_PARAM_VIRIDIAN:
> -        if ( (a.value & ~HVMPV_feature_mask) ||
> -             !(a.value & HVMPV_base_freq) )
> +        if ( (a->value & ~HVMPV_feature_mask) ||
> +             !(a->value & HVMPV_base_freq) )
>              rc = -EINVAL;
>          break;
> +
>      case HVM_PARAM_IDENT_PT:
>          /*
>           * Only actually required for VT-x lacking unrestricted_guest
>           * capabilities.  Short circuit the pause if possible.
>           */
>          if ( !paging_mode_hap(d) || !cpu_has_vmx )
> -        {
> -            d->arch.hvm.params[a.index] = a.value;
>              break;
> -        }
> 
>          /*
>           * Update GUEST_CR3 in each VMCS to point at identity map.
> @@ -4201,117 +4181,123 @@ static int hvmop_set_param(
> 
>          rc = 0;
>          domain_pause(d);
> -        d->arch.hvm.params[a.index] = a.value;
> +        d->arch.hvm.params[a->index] = a->value;
>          for_each_vcpu ( d, v )
>              paging_update_cr3(v, false);
>          domain_unpause(d);
> 
>          domctl_lock_release();
>          break;
> +
>      case HVM_PARAM_DM_DOMAIN:
>          /* The only value this should ever be set to is DOMID_SELF */
> -        if ( a.value != DOMID_SELF )
> +        if ( a->value != DOMID_SELF )
>              rc = -EINVAL;
> 
> -        a.value = curr_d->domain_id;
> +        a->value = curr_d->domain_id;
>          break;
> +
>      case HVM_PARAM_ACPI_S_STATE:
>          rc = 0;
> -        if ( a.value == 3 )
> +        if ( a->value == 3 )
>              hvm_s3_suspend(d);
> -        else if ( a.value == 0 )
> +        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);
> +        rc = pmtimer_change_ioport(d, a->value);
>          break;
> 
>      case HVM_PARAM_NESTEDHVM:
>          rc = xsm_hvm_param_nested(XSM_PRIV, d);
>          if ( rc )
>              break;
> -        if ( a.value > 1 )
> +        if ( a->value > 1 )
>              rc = -EINVAL;
>          /*
>           * Remove the check below once we have
>           * shadow-on-shadow.
>           */
> -        if ( !paging_mode_hap(d) && a.value )
> +        if ( !paging_mode_hap(d) && a->value )
>              rc = -EINVAL;
> -        if ( a.value &&
> +        if ( a->value &&
>               d->arch.hvm.params[HVM_PARAM_ALTP2M] )
>              rc = -EINVAL;
>          /* Set up NHVM state for any vcpus that are already up. */
> -        if ( a.value &&
> +        if ( a->value &&
>               !d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
>              for_each_vcpu(d, v)
>                  if ( rc == 0 )
>                      rc = nestedhvm_vcpu_initialise(v);
> -        if ( !a.value || rc )
> +        if ( !a->value || rc )
>              for_each_vcpu(d, v)
>                  nestedhvm_vcpu_destroy(v);
>          break;
> +
>      case HVM_PARAM_ALTP2M:
>          rc = xsm_hvm_param_altp2mhvm(XSM_PRIV, d);
>          if ( rc )
>              break;
> -        if ( a.value > XEN_ALTP2M_limited )
> +        if ( a->value > XEN_ALTP2M_limited )
>              rc = -EINVAL;
> -        if ( a.value &&
> +        if ( a->value &&
>               d->arch.hvm.params[HVM_PARAM_NESTEDHVM] )
>              rc = -EINVAL;
>          break;
> 
>      case HVM_PARAM_TRIPLE_FAULT_REASON:
> -        if ( a.value > SHUTDOWN_MAX )
> +        if ( a->value > SHUTDOWN_MAX )
>              rc = -EINVAL;
>          break;
> +
>      case HVM_PARAM_IOREQ_SERVER_PFN:
> -        d->arch.hvm.ioreq_gfn.base = a.value;
> +        d->arch.hvm.ioreq_gfn.base = a->value;
>          break;
> +
>      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>      {
>          unsigned int i;
> 
> -        if ( a.value == 0 ||
> -             a.value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 )
> +        if ( a->value == 0 ||
> +             a->value > sizeof(d->arch.hvm.ioreq_gfn.mask) * 8 )
>          {
>              rc = -EINVAL;
>              break;
>          }
> -        for ( i = 0; i < a.value; i++ )
> +        for ( i = 0; i < a->value; i++ )
>              set_bit(i, &d->arch.hvm.ioreq_gfn.mask);
> 
>          break;
>      }
> +
>      case HVM_PARAM_X87_FIP_WIDTH:
> -        if ( a.value != 0 && a.value != 4 && a.value != 8 )
> +        if ( a->value != 0 && a->value != 4 && a->value != 8 )
>          {
>              rc = -EINVAL;
>              break;
>          }
> -        d->arch.x87_fip_width = a.value;
> +        d->arch.x87_fip_width = a->value;
>          break;
> 
>      case HVM_PARAM_VM86_TSS:
>          /* Hardware would silently truncate high bits. */
> -        if ( a.value != (uint32_t)a.value )
> +        if ( a->value != (uint32_t)a->value )
>          {
>              if ( d == curr_d )
>                  domain_crash(d);
>              rc = -EINVAL;
>          }
>          /* Old hvmloader binaries hardcode the size to 128 bytes. */
> -        if ( a.value )
> -            a.value |= (128ULL << 32) | VM86_TSS_UPDATED;
> -        a.index = HVM_PARAM_VM86_TSS_SIZED;
> +        if ( a->value )
> +            a->value |= (128ULL << 32) | VM86_TSS_UPDATED;
> +        a->index = HVM_PARAM_VM86_TSS_SIZED;
>          break;
> 
>      case HVM_PARAM_VM86_TSS_SIZED:
> -        if ( (a.value >> 32) < sizeof(struct tss32) )
> +        if ( (a->value >> 32) < sizeof(struct tss32) )
>          {
>              if ( d == curr_d )
>                  domain_crash(d);
> @@ -4322,41 +4308,34 @@ static int hvmop_set_param(
>           * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap
>           * plus one padding byte).
>           */
> -        if ( (a.value >> 32) > sizeof(struct tss32) +
> -                               (0x100 / 8) + (0x10000 / 8) + 1 )
> -            a.value = (uint32_t)a.value |
> -                      ((sizeof(struct tss32) + (0x100 / 8) +
> -                                               (0x10000 / 8) + 1) << 32);
> -        a.value |= VM86_TSS_UPDATED;
> +        if ( (a->value >> 32) > sizeof(struct tss32) +
> +                                (0x100 / 8) + (0x10000 / 8) + 1 )
> +            a->value = (uint32_t)a->value |
> +                       ((sizeof(struct tss32) + (0x100 / 8) +
> +                                                (0x10000 / 8) + 1) << 32);
> +        a->value |= VM86_TSS_UPDATED;
>          break;
> 
>      case HVM_PARAM_MCA_CAP:
> -        rc = vmce_enable_mca_cap(d, a.value);
> +        rc = vmce_enable_mca_cap(d, a->value);
>          break;
>      }
> 
>      if ( rc != 0 )
>          goto out;
> 
> -    d->arch.hvm.params[a.index] = a.value;
> +    d->arch.hvm.params[a->index] = a->value;
> 
>      HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
> -                a.index, a.value);
> +                a->index, a->value);
> 
>   out:
> -    rcu_unlock_domain(d);
>      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 can be read by the guest and toolstack. */
> @@ -4371,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
>      case HVM_PARAM_CONSOLE_EVTCHN:
>      case HVM_PARAM_ALTP2M:
>      case HVM_PARAM_X87_FIP_WIDTH:
> -        break;
> +        return 0;
> 
>          /*
>           * The following parameters are intended for toolstack usage only.
> @@ -4397,59 +4376,41 @@ static int hvm_allow_get_param(struct domain
> *d,
>      case HVM_PARAM_IOREQ_SERVER_PFN:
>      case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>      case HVM_PARAM_MCA_CAP:
> -        if ( d == current->domain )
> -            rc = -EPERM;
> -        break;
> +        return d == current->domain ? -EPERM : 0;
> 
>          /* Hole, deprecated, or out-of-range. */
>      default:
> -        rc = -EINVAL;
> -        break;
> +        return -EINVAL;
>      }
> -
> -    return rc;
>  }
> 
> -static int hvmop_get_param(
> -    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> +static int hvmop_get_param(struct domain *d, struct xen_hvm_param *a)
>  {
> -    struct xen_hvm_param a;
> -    struct domain *d;
>      int rc;
> 
> -    if ( copy_from_guest(&a, arg, 1) )
> -        return -EFAULT;
> -
> -    d = rcu_lock_domain_by_any_id(a.domid);
> -    if ( d == NULL )
> -        return -ESRCH;
> -
> -    rc = -EINVAL;
> -    if ( !is_hvm_domain(d) )
> -        goto out;
> -
> -    rc = hvm_allow_get_param(d, &a);
> +    rc = hvm_allow_get_param(d, a);
>      if ( rc )
> -        goto out;
> +        return rc;
> 
> -    switch ( a.index )
> +    switch ( a->index )
>      {
>      case HVM_PARAM_ACPI_S_STATE:
> -        a.value = d->arch.hvm.is_s3_suspended ? 3 : 0;
> +        a->value = d->arch.hvm.is_s3_suspended ? 3 : 0;
>          break;
> 
>      case HVM_PARAM_VM86_TSS:
> -        a.value = (uint32_t)d-
> >arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED];
> +        a->value = (uint32_t)d-
> >arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED];
>          break;
> 
>      case HVM_PARAM_VM86_TSS_SIZED:
> -        a.value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] &
> -                  ~VM86_TSS_UPDATED;
> +        a->value = d->arch.hvm.params[HVM_PARAM_VM86_TSS_SIZED] &
> +                   ~VM86_TSS_UPDATED;
>          break;
> 
>      case HVM_PARAM_X87_FIP_WIDTH:
> -        a.value = d->arch.x87_fip_width;
> +        a->value = d->arch.x87_fip_width;
>          break;
> +
>      case HVM_PARAM_IOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_PFN:
>      case HVM_PARAM_BUFIOREQ_EVTCHN:
> @@ -4465,23 +4426,19 @@ static int hvmop_get_param(
>              rc = hvm_create_ioreq_server(d, true,
>                                           HVM_IOREQSRV_BUFIOREQ_LEGACY, NULL);
>              if ( rc != 0 && rc != -EEXIST )
> -                goto out;
> +                return rc;
>          }
> 
> -    /*FALLTHRU*/
> +        /* Fallthrough */
>      default:
> -        a.value = d->arch.hvm.params[a.index];
> +        a->value = d->arch.hvm.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);
> +                a->index, a->value);
> 
> - out:
> -    rcu_unlock_domain(d);
> -    return rc;
> +    return 0;
>  }
> 
>  /*
> @@ -4896,14 +4853,42 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          break;
> 
>      case HVMOP_set_param:
> -        rc = hvmop_set_param(
> -            guest_handle_cast(arg, xen_hvm_param_t));
> -        break;
> -
>      case HVMOP_get_param:
> -        rc = hvmop_get_param(
> -            guest_handle_cast(arg, xen_hvm_param_t));
> +    {
> +        struct xen_hvm_param a;
> +        struct domain *d;
> +
> +        rc = -EFAULT;
> +        if ( copy_from_guest(&a, arg, 1) )
> +            break;
> +
> +        rc = -ESRCH;
> +        d = rcu_lock_domain_by_any_id(a.domid);
> +        if ( d == NULL )
> +            break;
> +
> +        rc = -EINVAL;
> +        if ( !is_hvm_domain(d) )
> +            goto param_out;
> +
> +        rc = xsm_hvm_param(XSM_TARGET, d, op);
> +        if ( rc )
> +            goto param_out;
> +
> +        if ( op == HVMOP_set_param )
> +            rc = hvmop_set_param(d, &a);
> +        else
> +        {
> +            rc = hvmop_get_param(d, &a);
> +
> +            if ( !rc && __copy_to_guest(arg, &a, 1) )
> +                rc = -EFAULT;
> +        }
> +
> +    param_out:
> +        rcu_unlock_domain(d);
>          break;
> +    }
> 
>      case HVMOP_flush_tlbs:
>          rc = guest_handle_is_null(arg) ? hvmop_flush_tlb_all() : -EINVAL;
> --
> 2.1.4

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

Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> Sent: 05 September 2018 19:12
> To: Xen-devel <xen-devel@lists.xen.org>
> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> 
> ARM currently has no restrictions on toolstack and guest access to the entire
> HVM_PARAM block.  As the paging/monitor/sharing features aren't under
> security
> support, this doesn't need an XSA.
> 
> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed
> read-only to
> the guest, while the *_RING_PFN details are restricted to only toolstack
> access.  No other parameters are used.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> 
> This is only compile tested, and based on my reading of the source.  There
> might be other PARAMS needing including.
> ---
>  xen/arch/arm/hvm.c | 62
> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 59 insertions(+), 3 deletions(-)
> 
> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> index 76b27c9..3581ba2 100644
> --- a/xen/arch/arm/hvm.c
> +++ b/xen/arch/arm/hvm.c
> @@ -31,6 +31,57 @@
> 
>  #include <asm/hypercall.h>
> 
> +static int hvm_allow_set_param(const struct domain *d, unsigned int
> param)
> +{
> +    switch ( param )
> +    {
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * They may not be set by the domain.
> +         */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:

Probably should remove the EVTCHN params from this list after fixing patch #3.

> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +        return d == current->domain ? -EPERM : 0;
> +
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
> +static int hvm_allow_get_param(const struct domain *d, unsigned int
> param)
> +{
> +    switch ( param )
> +    {
> +        /* The following parameters can be read by the guest and toolstack. */
> +    case HVM_PARAM_CALLBACK_IRQ:
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_STORE_EVTCHN:
> +    case HVM_PARAM_CONSOLE_PFN:
> +    case HVM_PARAM_CONSOLE_EVTCHN:
> +        return 0;
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * They may not be read by the domain.
> +         */
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +        return d == current->domain ? -EPERM : 0;
> +
> +        /* Hole, deprecated, or out-of-range. */
> +    default:
> +        return -EINVAL;
> +    }
> +}
> +
>  long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
> arg)
>  {
>      long rc = 0;
> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
>          if ( copy_from_guest(&a, arg, 1) )
>              return -EFAULT;
> 
> -        if ( a.index >= HVM_NR_PARAMS )
> -            return -EINVAL;
> -

ASSERT here.

  Paul

>          d = rcu_lock_domain_by_any_id(a.domid);
>          if ( d == NULL )
>              return -ESRCH;
> @@ -59,10 +107,18 @@ long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void) arg)
> 
>          if ( op == HVMOP_set_param )
>          {
> +            rc = hvm_allow_set_param(d, a.index);
> +            if ( rc )
> +                goto param_fail;
> +
>              d->arch.hvm.params[a.index] = a.value;
>          }
>          else
>          {
> +            rc = hvm_allow_get_param(d, a.index);
> +            if ( rc )
> +                goto param_fail;
> +
>              a.value = d->arch.hvm.params[a.index];
>              rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
>          }
> --
> 2.1.4

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

Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Julien Grall]

Hi Paul,

On 06/09/18 10:29, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 September 2018 19:12
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
>>
>> ARM currently has no restrictions on toolstack and guest access to the entire
>> HVM_PARAM block.  As the paging/monitor/sharing features aren't under
>> security
>> support, this doesn't need an XSA.
>>
>> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed
>> read-only to
>> the guest, while the *_RING_PFN details are restricted to only toolstack
>> access.  No other parameters are used.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>>
>> This is only compile tested, and based on my reading of the source.  There
>> might be other PARAMS needing including.
>> ---
>>   xen/arch/arm/hvm.c | 62
>> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>   1 file changed, 59 insertions(+), 3 deletions(-)
>>
>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>> index 76b27c9..3581ba2 100644
>> --- a/xen/arch/arm/hvm.c
>> +++ b/xen/arch/arm/hvm.c
>> @@ -31,6 +31,57 @@
>>
>>   #include <asm/hypercall.h>
>>
>> +static int hvm_allow_set_param(const struct domain *d, unsigned int
>> param)
>> +{
>> +    switch ( param )
>> +    {
>> +        /*
>> +         * The following parameters are intended for toolstack usage only.
>> +         * They may not be set by the domain.
>> +         */
>> +    case HVM_PARAM_CALLBACK_IRQ:
>> +    case HVM_PARAM_STORE_PFN:
>> +    case HVM_PARAM_STORE_EVTCHN:
>> +    case HVM_PARAM_CONSOLE_PFN:
>> +    case HVM_PARAM_CONSOLE_EVTCHN:
> 
> Probably should remove the EVTCHN params from this list after fixing patch #3.
> 
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>> +        return d == current->domain ? -EPERM : 0;
>> +
>> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +}
>> +
>> +static int hvm_allow_get_param(const struct domain *d, unsigned int
>> param)
>> +{
>> +    switch ( param )
>> +    {
>> +        /* The following parameters can be read by the guest and toolstack. */
>> +    case HVM_PARAM_CALLBACK_IRQ:
>> +    case HVM_PARAM_STORE_PFN:
>> +    case HVM_PARAM_STORE_EVTCHN:
>> +    case HVM_PARAM_CONSOLE_PFN:
>> +    case HVM_PARAM_CONSOLE_EVTCHN:
>> +        return 0;
>> +
>> +        /*
>> +         * The following parameters are intended for toolstack usage only.
>> +         * They may not be read by the domain.
>> +         */
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>> +        return d == current->domain ? -EPERM : 0;
>> +
>> +        /* Hole, deprecated, or out-of-range. */
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +}
>> +
>>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
>> arg)
>>   {
>>       long rc = 0;
>> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>           if ( copy_from_guest(&a, arg, 1) )
>>               return -EFAULT;
>>
>> -        if ( a.index >= HVM_NR_PARAMS )
>> -            return -EINVAL;
>> -
> 
> ASSERT here.

I don't think this would be correct. This is an input from the guest, so 
if you do fuzzing you will end up to get an hypervisor crash rather than 
returning an error.

A potential place for an ASSERT would be just before accessing 
hvm.params. But then, technically the index should have been sanitized 
by hvm_allow_{get,set}_param.

Cheers,

-- 
Julien Grall

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

Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Andrew Cooper]

On 06/09/18 11:36, Julien Grall wrote:
> Hi Paul,
>
> On 06/09/18 10:29, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>> Sent: 05 September 2018 19:12
>>> To: Xen-devel <xen-devel@lists.xen.org>
>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
>>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
>>> Stabellini <sstabellini@kernel.org>; Julien Grall
>>> <julien.grall@arm.com>
>>> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
>>>
>>> ARM currently has no restrictions on toolstack and guest access to
>>> the entire
>>> HVM_PARAM block.  As the paging/monitor/sharing features aren't under
>>> security
>>> support, this doesn't need an XSA.
>>>
>>> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details exposed
>>> read-only to
>>> the guest, while the *_RING_PFN details are restricted to only
>>> toolstack
>>> access.  No other parameters are used.
>>>
>>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>>> ---
>>> CC: Jan Beulich <JBeulich@suse.com>
>>> CC: Wei Liu <wei.liu2@citrix.com>
>>> CC: Roger Pau Monné <roger.pau@citrix.com>
>>> CC: Paul Durrant <paul.durrant@citrix.com>
>>> CC: Stefano Stabellini <sstabellini@kernel.org>
>>> CC: Julien Grall <julien.grall@arm.com>
>>>
>>> This is only compile tested, and based on my reading of the source. 
>>> There
>>> might be other PARAMS needing including.
>>> ---
>>>   xen/arch/arm/hvm.c | 62
>>> +++++++++++++++++++++++++++++++++++++++++++++++++++---
>>>   1 file changed, 59 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
>>> index 76b27c9..3581ba2 100644
>>> --- a/xen/arch/arm/hvm.c
>>> +++ b/xen/arch/arm/hvm.c
>>> @@ -31,6 +31,57 @@
>>>
>>>   #include <asm/hypercall.h>
>>>
>>> +static int hvm_allow_set_param(const struct domain *d, unsigned int
>>> param)
>>> +{
>>> +    switch ( param )
>>> +    {
>>> +        /*
>>> +         * The following parameters are intended for toolstack
>>> usage only.
>>> +         * They may not be set by the domain.
>>> +         */
>>> +    case HVM_PARAM_CALLBACK_IRQ:
>>> +    case HVM_PARAM_STORE_PFN:
>>> +    case HVM_PARAM_STORE_EVTCHN:
>>> +    case HVM_PARAM_CONSOLE_PFN:
>>> +    case HVM_PARAM_CONSOLE_EVTCHN:
>>
>> Probably should remove the EVTCHN params from this list after fixing
>> patch #3.
>>
>>> +    case HVM_PARAM_PAGING_RING_PFN:
>>> +    case HVM_PARAM_MONITOR_RING_PFN:
>>> +    case HVM_PARAM_SHARING_RING_PFN:
>>> +        return d == current->domain ? -EPERM : 0;
>>> +
>>> +        /* Writeable only by Xen, hole, deprecated, or
>>> out-of-range. */
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +}
>>> +
>>> +static int hvm_allow_get_param(const struct domain *d, unsigned int
>>> param)
>>> +{
>>> +    switch ( param )
>>> +    {
>>> +        /* The following parameters can be read by the guest and
>>> toolstack. */
>>> +    case HVM_PARAM_CALLBACK_IRQ:
>>> +    case HVM_PARAM_STORE_PFN:
>>> +    case HVM_PARAM_STORE_EVTCHN:
>>> +    case HVM_PARAM_CONSOLE_PFN:
>>> +    case HVM_PARAM_CONSOLE_EVTCHN:
>>> +        return 0;
>>> +
>>> +        /*
>>> +         * The following parameters are intended for toolstack
>>> usage only.
>>> +         * They may not be read by the domain.
>>> +         */
>>> +    case HVM_PARAM_PAGING_RING_PFN:
>>> +    case HVM_PARAM_MONITOR_RING_PFN:
>>> +    case HVM_PARAM_SHARING_RING_PFN:
>>> +        return d == current->domain ? -EPERM : 0;
>>> +
>>> +        /* Hole, deprecated, or out-of-range. */
>>> +    default:
>>> +        return -EINVAL;
>>> +    }
>>> +}
>>> +
>>>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
>>> arg)
>>>   {
>>>       long rc = 0;
>>> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
>>> XEN_GUEST_HANDLE_PARAM(void) arg)
>>>           if ( copy_from_guest(&a, arg, 1) )
>>>               return -EFAULT;
>>>
>>> -        if ( a.index >= HVM_NR_PARAMS )
>>> -            return -EINVAL;
>>> -
>>
>> ASSERT here.
>
> I don't think this would be correct. This is an input from the guest,
> so if you do fuzzing you will end up to get an hypervisor crash rather
> than returning an error.
>
> A potential place for an ASSERT would be just before accessing
> hvm.params. But then, technically the index should have been sanitized
> by hvm_allow_{get,set}_param.

Yeah - across all of these ASSERT() requests - using an assert for a
boundary check doesn't do anything in the case where it matters most,
and in this case, Julien is correct that it is a fully guest-controlled
number at this point.

~Andrew

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

Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 06 September 2018 11:37
> To: Paul Durrant <Paul.Durrant@citrix.com>; Andrew Cooper
> <Andrew.Cooper3@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> 
> Hi Paul,
> 
> On 06/09/18 10:29, Paul Durrant wrote:
> >> -----Original Message-----
> >> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >> Sent: 05 September 2018 19:12
> >> To: Xen-devel <xen-devel@lists.xen.org>
> >> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> >> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Stefano
> >> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> >> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> >>
> >> ARM currently has no restrictions on toolstack and guest access to the
> entire
> >> HVM_PARAM block.  As the paging/monitor/sharing features aren't
> under
> >> security
> >> support, this doesn't need an XSA.
> >>
> >> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details
> exposed
> >> read-only to
> >> the guest, while the *_RING_PFN details are restricted to only toolstack
> >> access.  No other parameters are used.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: Julien Grall <julien.grall@arm.com>
> >>
> >> This is only compile tested, and based on my reading of the source.  There
> >> might be other PARAMS needing including.
> >> ---
> >>   xen/arch/arm/hvm.c | 62
> >> +++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>   1 file changed, 59 insertions(+), 3 deletions(-)
> >>
> >> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> >> index 76b27c9..3581ba2 100644
> >> --- a/xen/arch/arm/hvm.c
> >> +++ b/xen/arch/arm/hvm.c
> >> @@ -31,6 +31,57 @@
> >>
> >>   #include <asm/hypercall.h>
> >>
> >> +static int hvm_allow_set_param(const struct domain *d, unsigned int
> >> param)
> >> +{
> >> +    switch ( param )
> >> +    {
> >> +        /*
> >> +         * The following parameters are intended for toolstack usage only.
> >> +         * They may not be set by the domain.
> >> +         */
> >> +    case HVM_PARAM_CALLBACK_IRQ:
> >> +    case HVM_PARAM_STORE_PFN:
> >> +    case HVM_PARAM_STORE_EVTCHN:
> >> +    case HVM_PARAM_CONSOLE_PFN:
> >> +    case HVM_PARAM_CONSOLE_EVTCHN:
> >
> > Probably should remove the EVTCHN params from this list after fixing patch
> #3.
> >
> >> +    case HVM_PARAM_PAGING_RING_PFN:
> >> +    case HVM_PARAM_MONITOR_RING_PFN:
> >> +    case HVM_PARAM_SHARING_RING_PFN:
> >> +        return d == current->domain ? -EPERM : 0;
> >> +
> >> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> >> +    default:
> >> +        return -EINVAL;
> >> +    }
> >> +}
> >> +
> >> +static int hvm_allow_get_param(const struct domain *d, unsigned int
> >> param)
> >> +{
> >> +    switch ( param )
> >> +    {
> >> +        /* The following parameters can be read by the guest and toolstack.
> */
> >> +    case HVM_PARAM_CALLBACK_IRQ:
> >> +    case HVM_PARAM_STORE_PFN:
> >> +    case HVM_PARAM_STORE_EVTCHN:
> >> +    case HVM_PARAM_CONSOLE_PFN:
> >> +    case HVM_PARAM_CONSOLE_EVTCHN:
> >> +        return 0;
> >> +
> >> +        /*
> >> +         * The following parameters are intended for toolstack usage only.
> >> +         * They may not be read by the domain.
> >> +         */
> >> +    case HVM_PARAM_PAGING_RING_PFN:
> >> +    case HVM_PARAM_MONITOR_RING_PFN:
> >> +    case HVM_PARAM_SHARING_RING_PFN:
> >> +        return d == current->domain ? -EPERM : 0;
> >> +
> >> +        /* Hole, deprecated, or out-of-range. */
> >> +    default:
> >> +        return -EINVAL;
> >> +    }
> >> +}
> >> +
> >>   long do_hvm_op(unsigned long op, XEN_GUEST_HANDLE_PARAM(void)
> >> arg)
> >>   {
> >>       long rc = 0;
> >> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
> >> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>           if ( copy_from_guest(&a, arg, 1) )
> >>               return -EFAULT;
> >>
> >> -        if ( a.index >= HVM_NR_PARAMS )
> >> -            return -EINVAL;
> >> -
> >
> > ASSERT here.
> 
> I don't think this would be correct. This is an input from the guest, so
> if you do fuzzing you will end up to get an hypervisor crash rather than
> returning an error.

Ah, true.

> 
> A potential place for an ASSERT would be just before accessing
> hvm.params. But then, technically the index should have been sanitized
> by hvm_allow_{get,set}_param.
> 

Yes, that's where I meant to suggest adding the ASSERT. I wanted it to verify that sanitization.

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's

[Paul Durrant]

> -----Original Message-----
> From: Andrew Cooper
> Sent: 06 September 2018 11:40
> To: Julien Grall <julien.grall@arm.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> 
> On 06/09/18 11:36, Julien Grall wrote:
> > Hi Paul,
> >
> > On 06/09/18 10:29, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>> Sent: 05 September 2018 19:12
> >>> To: Xen-devel <xen-devel@lists.xen.org>
> >>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> >>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Stefano
> >>> Stabellini <sstabellini@kernel.org>; Julien Grall
> >>> <julien.grall@arm.com>
> >>> Subject: [PATCH 5/5] xen/ARM: Restrict access to most HVM_PARAM's
> >>>
> >>> ARM currently has no restrictions on toolstack and guest access to
> >>> the entire
> >>> HVM_PARAM block.  As the paging/monitor/sharing features aren't
> under
> >>> security
> >>> support, this doesn't need an XSA.
> >>>
> >>> The CALLBACK_IRQ and {STORE,CONSOLE}_{PFN,EVTCHN} details
> exposed
> >>> read-only to
> >>> the guest, while the *_RING_PFN details are restricted to only
> >>> toolstack
> >>> access.  No other parameters are used.
> >>>
> >>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> >>> ---
> >>> CC: Jan Beulich <JBeulich@suse.com>
> >>> CC: Wei Liu <wei.liu2@citrix.com>
> >>> CC: Roger Pau Monné <roger.pau@citrix.com>
> >>> CC: Paul Durrant <paul.durrant@citrix.com>
> >>> CC: Stefano Stabellini <sstabellini@kernel.org>
> >>> CC: Julien Grall <julien.grall@arm.com>
> >>>
> >>> This is only compile tested, and based on my reading of the source.
> >>> There
> >>> might be other PARAMS needing including.
> >>> ---
> >>>   xen/arch/arm/hvm.c | 62
> >>> +++++++++++++++++++++++++++++++++++++++++++++++++++---
> >>>   1 file changed, 59 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/xen/arch/arm/hvm.c b/xen/arch/arm/hvm.c
> >>> index 76b27c9..3581ba2 100644
> >>> --- a/xen/arch/arm/hvm.c
> >>> +++ b/xen/arch/arm/hvm.c
> >>> @@ -31,6 +31,57 @@
> >>>
> >>>   #include <asm/hypercall.h>
> >>>
> >>> +static int hvm_allow_set_param(const struct domain *d, unsigned int
> >>> param)
> >>> +{
> >>> +    switch ( param )
> >>> +    {
> >>> +        /*
> >>> +         * The following parameters are intended for toolstack
> >>> usage only.
> >>> +         * They may not be set by the domain.
> >>> +         */
> >>> +    case HVM_PARAM_CALLBACK_IRQ:
> >>> +    case HVM_PARAM_STORE_PFN:
> >>> +    case HVM_PARAM_STORE_EVTCHN:
> >>> +    case HVM_PARAM_CONSOLE_PFN:
> >>> +    case HVM_PARAM_CONSOLE_EVTCHN:
> >>
> >> Probably should remove the EVTCHN params from this list after fixing
> >> patch #3.
> >>
> >>> +    case HVM_PARAM_PAGING_RING_PFN:
> >>> +    case HVM_PARAM_MONITOR_RING_PFN:
> >>> +    case HVM_PARAM_SHARING_RING_PFN:
> >>> +        return d == current->domain ? -EPERM : 0;
> >>> +
> >>> +        /* Writeable only by Xen, hole, deprecated, or
> >>> out-of-range. */
> >>> +    default:
> >>> +        return -EINVAL;
> >>> +    }
> >>> +}
> >>> +
> >>> +static int hvm_allow_get_param(const struct domain *d, unsigned int
> >>> param)
> >>> +{
> >>> +    switch ( param )
> >>> +    {
> >>> +        /* The following parameters can be read by the guest and
> >>> toolstack. */
> >>> +    case HVM_PARAM_CALLBACK_IRQ:
> >>> +    case HVM_PARAM_STORE_PFN:
> >>> +    case HVM_PARAM_STORE_EVTCHN:
> >>> +    case HVM_PARAM_CONSOLE_PFN:
> >>> +    case HVM_PARAM_CONSOLE_EVTCHN:
> >>> +        return 0;
> >>> +
> >>> +        /*
> >>> +         * The following parameters are intended for toolstack
> >>> usage only.
> >>> +         * They may not be read by the domain.
> >>> +         */
> >>> +    case HVM_PARAM_PAGING_RING_PFN:
> >>> +    case HVM_PARAM_MONITOR_RING_PFN:
> >>> +    case HVM_PARAM_SHARING_RING_PFN:
> >>> +        return d == current->domain ? -EPERM : 0;
> >>> +
> >>> +        /* Hole, deprecated, or out-of-range. */
> >>> +    default:
> >>> +        return -EINVAL;
> >>> +    }
> >>> +}
> >>> +
> >>>   long do_hvm_op(unsigned long op,
> XEN_GUEST_HANDLE_PARAM(void)
> >>> arg)
> >>>   {
> >>>       long rc = 0;
> >>> @@ -46,9 +97,6 @@ long do_hvm_op(unsigned long op,
> >>> XEN_GUEST_HANDLE_PARAM(void) arg)
> >>>           if ( copy_from_guest(&a, arg, 1) )
> >>>               return -EFAULT;
> >>>
> >>> -        if ( a.index >= HVM_NR_PARAMS )
> >>> -            return -EINVAL;
> >>> -
> >>
> >> ASSERT here.
> >
> > I don't think this would be correct. This is an input from the guest,
> > so if you do fuzzing you will end up to get an hypervisor crash rather
> > than returning an error.
> >
> > A potential place for an ASSERT would be just before accessing
> > hvm.params. But then, technically the index should have been sanitized
> > by hvm_allow_{get,set}_param.
> 
> Yeah - across all of these ASSERT() requests - using an assert for a
> boundary check doesn't do anything in the case where it matters most,
> and in this case, Julien is correct that it is a fully guest-controlled
> number at this point.
> 

I'm just uneasy about removing a bounds check. If a logic error creeps into an 'allow' function in future then we'd have no protection. An ASSERT would at least make such a logic error obvious.

  Paul

> ~Andrew
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Andrew Cooper]

On 06/09/18 09:56, Paul Durrant wrote:
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index c22bf0b..96a6323 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
>>
>>      switch ( a->index )
>>      {
>> -    /* The following parameters can be read by the guest. */
>> +        /* The following parameters can be read by the guest and toolstack. */
> Intentional indentation change? I guess so since you do it again below, but just checking.

Yes - this is where BSD style puts comments, as you are inside the
braced scope of the switch statement.

Notice furthermore that there is a correction to the end of the comment.

>
>>      case HVM_PARAM_CALLBACK_IRQ:
>>      case HVM_PARAM_VM86_TSS:
>>      case HVM_PARAM_VM86_TSS_SIZED:
>> @@ -4363,18 +4363,39 @@ static int hvm_allow_get_param(struct domain
>> *d,
>>      case HVM_PARAM_ALTP2M:
>>      case HVM_PARAM_X87_FIP_WIDTH:
>>          break;
>> -    /*
>> -     * The following parameters must not be read by the guest
>> -     * since the domain may need to be paused.
>> -     */
>> +
>> +        /*
>> +         * The following parameters are intended for toolstack usage only.
>> +         * Some require the domain to be paused, and therefore may not read
>> by
>> +         * the domain.
>> +         */
>> +    case HVM_PARAM_PAE_ENABLED:
>>      case HVM_PARAM_IOREQ_PFN:
>>      case HVM_PARAM_BUFIOREQ_PFN:
>>      case HVM_PARAM_BUFIOREQ_EVTCHN:
>> -    /* The remaining parameters should not be read by the guest. */
>> -    default:
>> +    case HVM_PARAM_VIRIDIAN:
>> +    case HVM_PARAM_TIMER_MODE:
>> +    case HVM_PARAM_HPET_ENABLED:
>> +    case HVM_PARAM_IDENT_PT:
>> +    case HVM_PARAM_DM_DOMAIN:
>> +    case HVM_PARAM_ACPI_S_STATE:
>> +    case HVM_PARAM_VPT_ALIGN:
>> +    case HVM_PARAM_NESTEDHVM:
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
>> +    case HVM_PARAM_IOREQ_SERVER_PFN:
>> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>> +    case HVM_PARAM_MCA_CAP:
>>          if ( d == current->domain )
>>              rc = -EPERM;
>>          break;
>> +
>> +        /* Hole, deprecated, or out-of-range. */
>> +    default:
>> +        rc = -EINVAL;
>> +        break;
>>      }
>>
>>      return rc;
>> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>>      if ( copy_from_guest(&a, arg, 1) )
>>          return -EFAULT;
>>
>> -    if ( a.index >= HVM_NR_PARAMS )
>> -        return -EINVAL;
>> -
> ASSERT, just in case someone screws up the allow function in future?

That's not going to help in any practical way.  This check does really
exist, and is part of the switch statement.

~Andrew

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

Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Andrew Cooper]

On 06/09/18 10:08, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 September 2018 19:12
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a
>> whitelist
>>
>> There are holes in the HVM_PARAM space, some of which are from
>> deprecated
>> parameters, but toolstack and device models currently has (almost) blanket
> s/has/have
>
>> write access.
>>
>> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
> s/get/set

Both fixed.

>>      d = rcu_lock_domain_by_any_id(a.domid);
>>      if ( d == NULL )
>>          return -ESRCH;
>> @@ -4209,15 +4228,7 @@ 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:
>> -    case HVM_PARAM_MEMORY_EVENT_INT3:
>> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>> -    case HVM_PARAM_MEMORY_EVENT_MSR:
>> -        /* Deprecated */
>> -        rc = -EOPNOTSUPP;
>> -        break;
> Does anything rely on this EOPNOTSUPP vs the EINVAL that would be returned after this patch is applied?

Nothing I can spot, although in searching, I see that
xc_hvm_param_deprecated_check() is a thing.

~Andrew

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

Re: [PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Andrew Cooper]

On 06/09/18 10:16, Paul Durrant wrote:
>> -----Original Message-----
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 September 2018 19:12
>> To: Xen-devel <xen-devel@lists.xen.org>
>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
>> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
>> Subject: [PATCH 3/5] x86/hvm: Make
>> HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
>>
>> These values are set by the toolstack for each create/restore operation, and
>> bound by xen{store,console}d before the the guest starts running.
>>
>> A guest has no reason to modify them at all, and the matching *_PFN
>> parameters
>> are already read-only.  Adjust the *_EVTCHN permissions to be consistent.
> Unfortunately this patch will break the Windows PV driver function here:
>
> http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xenbus/evtchn.c;hb=HEAD#l1037
>
> Unfortunately the values really do change across a reset. It would be possible to use volatile (disappear on reboot) registry keys to store the updated values instead but I don't really see any harm in allowing the guest to update the values to be correct, unless we want to change Xen to do the job so the guest doesn't have to go through this dance.

:(  Everything is terrible.

This is a general problem, not x86 specific, so I'll drop this patch and
make a similar adjustment to the ARM one.

~Andrew

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

Re: [PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Julien Grall]

On 06/09/18 16:29, Andrew Cooper wrote:
> On 06/09/18 10:16, Paul Durrant wrote:
>>> -----Original Message-----
>>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>>> Sent: 05 September 2018 19:12
>>> To: Xen-devel <xen-devel@lists.xen.org>
>>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
>>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
>>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>; Stefano
>>> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
>>> Subject: [PATCH 3/5] x86/hvm: Make
>>> HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
>>>
>>> These values are set by the toolstack for each create/restore operation, and
>>> bound by xen{store,console}d before the the guest starts running.
>>>
>>> A guest has no reason to modify them at all, and the matching *_PFN
>>> parameters
>>> are already read-only.  Adjust the *_EVTCHN permissions to be consistent.
>> Unfortunately this patch will break the Windows PV driver function here:
>>
>> http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/xenbus/evtchn.c;hb=HEAD#l1037
>>
>> Unfortunately the values really do change across a reset. It would be possible to use volatile (disappear on reboot) registry keys to store the updated values instead but I don't really see any harm in allowing the guest to update the values to be correct, unless we want to change Xen to do the job so the guest doesn't have to go through this dance.
> 
> :(  Everything is terrible.
> 
> This is a general problem, not x86 specific, so I'll drop this patch and
> make a similar adjustment to the ARM one.

I am a bit confused. I would have thought this was updated by the 
toolstack at reset. So why would the guest update them?

Cheers,

-- 
Julien Grall

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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Jan Beulich]

>>> On 06.09.18 at 17:21, <andrew.cooper3@citrix.com> wrote:
> On 06/09/18 09:56, Paul Durrant wrote:
>>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>>> index c22bf0b..96a6323 100644
>>> --- a/xen/arch/x86/hvm/hvm.c
>>> +++ b/xen/arch/x86/hvm/hvm.c
>>> @@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
>>>
>>>      switch ( a->index )
>>>      {
>>> -    /* The following parameters can be read by the guest. */
>>> +        /* The following parameters can be read by the guest and toolstack. */
>> Intentional indentation change? I guess so since you do it again below, but just checking.
> 
> Yes - this is where BSD style puts comments, as you are inside the
> braced scope of the switch statement.

Well, I'm inclined to support Paul here, no matter what BSD style may
have to say. Our own style description doesn't spell out anything
either way...

Jan



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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Jan Beulich]

>>> On 06.09.18 at 17:21, <andrew.cooper3@citrix.com> wrote:
> On 06/09/18 09:56, Paul Durrant wrote:
>>> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>>>      if ( copy_from_guest(&a, arg, 1) )
>>>          return -EFAULT;
>>>
>>> -    if ( a.index >= HVM_NR_PARAMS )
>>> -        return -EINVAL;
>>> -
>> ASSERT, just in case someone screws up the allow function in future?
> 
> That's not going to help in any practical way.  This check does really
> exist, and is part of the switch statement.

Which switch() statement? The one in the allow function includes this,
but the one here simply has

    default:
        a.value = d->arch.hvm.params[a.index];
        break;

Jan



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

Re: [PATCH 4/5] x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure

[Jan Beulich]

>>> On 06.09.18 at 11:26, <Paul.Durrant@citrix.com> wrote:
>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
>> Sent: 05 September 2018 19:12
>> 
>> The parameter marshalling and xsm checks are common to both the set and
>> get
>> paths.  Lift all of the common code out into do_hvm_op() and let
>> hvmop_{get,set}_param() operate on struct xen_hvm_param directly.
>> 
>> This is somewhat noisy in the functions as each a. reference has to change to
>> a-> instead.
>> 
>> In addition, drop an empty default statement, insert newlines as appropriate
>> between cases, and there is no need to update the IDENT_PT on the
>> fastpath,
>> because the common path after the switch will make the update.
>> 
>> No functional change, but a net shrink of -328 to do_hvm_op().
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> 
> Reviewed-by: Paul Durrant <paul.durrant@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>



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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Roger Pau Monné]

On Wed, Sep 05, 2018 at 07:12:00PM +0100, Andrew Cooper wrote:
> There are holes in the HVM_PARAM space, some of which are from deprecated
> parameters, but toolstack and device models currently have blanket read
> access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-readable
> parameters, with the default case failing with -EINVAL (which subsumes the
> HVM_NR_PARAMS check).
> 
> No expected change for the defined, in-use params.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just one nit.

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 38 ++++++++++++++++++++++++++++----------
>  1 file changed, 28 insertions(+), 10 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index c22bf0b..96a6323 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4350,7 +4350,7 @@ static int hvm_allow_get_param(struct domain *d,
>  
>      switch ( a->index )
>      {
> -    /* The following parameters can be read by the guest. */
> +        /* The following parameters can be read by the guest and toolstack. */
>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:
>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4363,18 +4363,39 @@ static int hvm_allow_get_param(struct domain *d,
>      case HVM_PARAM_ALTP2M:
>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> -    /*
> -     * The following parameters must not be read by the guest
> -     * since the domain may need to be paused.
> -     */
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * Some require the domain to be paused, and therefore may not read by
                                                                         ^ be

Roger.

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

Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Roger Pau Monné]

On Wed, Sep 05, 2018 at 07:12:01PM +0100, Andrew Cooper wrote:
> There are holes in the HVM_PARAM space, some of which are from deprecated
> parameters, but toolstack and device models currently has (almost) blanket
> write access.
> 
> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
> parameters, with the default case failing with -EINVAL.  This subsumes the
> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
> BUFIOREQ_EVTCHN Xen-write-only value.
> 
> No expected change for the defined, in-use params.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wei.liu2@citrix.com>
> CC: Roger Pau Monné <roger.pau@citrix.com>
> CC: Paul Durrant <paul.durrant@citrix.com>
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien.grall@arm.com>
> ---
>  xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 22 deletions(-)
> 
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 96a6323..d19ae35 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
>  
>      switch ( a->index )
>      {
> -    /* The following parameters can be set by the guest. */
> +        /* The following parameters can be set by the guest and toolstack. */
>      case HVM_PARAM_CALLBACK_IRQ:
>      case HVM_PARAM_VM86_TSS:

Note sure about the point of letting the guest set the unreal mode
TSS, but anyway this is not the scope of the patch.

>      case HVM_PARAM_VM86_TSS_SIZED:
> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain *d,
>      case HVM_PARAM_CONSOLE_EVTCHN:

Also it's quite weird that we allow the guest to set the console
evtchn...

>      case HVM_PARAM_X87_FIP_WIDTH:
>          break;
> -    /*
> -     * The following parameters must not be set by the guest
> -     * since the domain may need to be paused.
> -     */
> +
> +        /*
> +         * The following parameters are intended for toolstack usage only.
> +         * Some require the domain to be paused while others control
> +         * permissions in Xen, and therefore may not set by the domain.
> +         */
> +    case HVM_PARAM_STORE_PFN:
> +    case HVM_PARAM_PAE_ENABLED:
> +    case HVM_PARAM_IOREQ_PFN:
> +    case HVM_PARAM_BUFIOREQ_PFN:
> +    case HVM_PARAM_VIRIDIAN:
> +    case HVM_PARAM_TIMER_MODE:
> +    case HVM_PARAM_HPET_ENABLED:
>      case HVM_PARAM_IDENT_PT:
>      case HVM_PARAM_DM_DOMAIN:
>      case HVM_PARAM_ACPI_S_STATE:
> -    /* The remaining parameters should not be set by the guest. */
> -    default:
> +    case HVM_PARAM_VPT_ALIGN:
> +    case HVM_PARAM_CONSOLE_PFN:

... but not the console page. I think the guest shouldn't be allowed to
set any of those.

> +    case HVM_PARAM_NESTEDHVM:
> +    case HVM_PARAM_PAGING_RING_PFN:
> +    case HVM_PARAM_MONITOR_RING_PFN:
> +    case HVM_PARAM_SHARING_RING_PFN:
> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
> +    case HVM_PARAM_IOREQ_SERVER_PFN:
> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> +    case HVM_PARAM_MCA_CAP:
>          if ( d == current->domain )
>              rc = -EPERM;
>          break;
> +
> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
> +    default:
> +        rc = -EINVAL;
> +        break;
>      }
>  
>      if ( rc )
> @@ -4130,9 +4152,6 @@ static int hvmop_set_param(
>      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;
> @@ -4209,15 +4228,7 @@ 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:
> -    case HVM_PARAM_MEMORY_EVENT_INT3:
> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> -    case HVM_PARAM_MEMORY_EVENT_MSR:
> -        /* Deprecated */
> -        rc = -EOPNOTSUPP;
> -        break;

I assume there's no toolstack logic that relies on those parameters
returning EOPNOTSUPP?

Thanks, Roger.

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

Re: [PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Roger Pau Monné]

On Wed, Sep 05, 2018 at 07:12:02PM +0100, Andrew Cooper wrote:
> These values are set by the toolstack for each create/restore operation, and
> bound by xen{store,console}d before the the guest starts running.
> 
> A guest has no reason to modify them at all, and the matching *_PFN parameters
> are already read-only.  Adjust the *_EVTCHN permissions to be consistent.
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Oh, that answers one of my questions in the previous patch :)

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

I wonder however if such parameters being writable was part of the
public ABI, since it's exposed to guests and now we are changing the
behavior.

Thanks, Roger.

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

Re: [PATCH 3/5] x86/hvm: Make HVM_PARAM_{STORE, CONSOLE}_EVTCHN read-only to the guest

[Paul Durrant]

> -----Original Message-----
> From: Julien Grall [mailto:julien.grall@arm.com]
> Sent: 06 September 2018 18:28
> To: Andrew Cooper <Andrew.Cooper3@citrix.com>; Paul Durrant
> <Paul.Durrant@citrix.com>; Xen-devel <xen-devel@lists.xen.org>
> Cc: Jan Beulich <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger
> Pau Monne <roger.pau@citrix.com>; Stefano Stabellini
> <sstabellini@kernel.org>
> Subject: Re: [PATCH 3/5] x86/hvm: Make
> HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
> 
> 
> 
> On 06/09/18 16:29, Andrew Cooper wrote:
> > On 06/09/18 10:16, Paul Durrant wrote:
> >>> -----Original Message-----
> >>> From: Andrew Cooper [mailto:andrew.cooper3@citrix.com]
> >>> Sent: 05 September 2018 19:12
> >>> To: Xen-devel <xen-devel@lists.xen.org>
> >>> Cc: Andrew Cooper <Andrew.Cooper3@citrix.com>; Jan Beulich
> >>> <JBeulich@suse.com>; Wei Liu <wei.liu2@citrix.com>; Roger Pau Monne
> >>> <roger.pau@citrix.com>; Paul Durrant <Paul.Durrant@citrix.com>;
> Stefano
> >>> Stabellini <sstabellini@kernel.org>; Julien Grall <julien.grall@arm.com>
> >>> Subject: [PATCH 3/5] x86/hvm: Make
> >>> HVM_PARAM_{STORE,CONSOLE}_EVTCHN read-only to the guest
> >>>
> >>> These values are set by the toolstack for each create/restore operation,
> and
> >>> bound by xen{store,console}d before the the guest starts running.
> >>>
> >>> A guest has no reason to modify them at all, and the matching *_PFN
> >>> parameters
> >>> are already read-only.  Adjust the *_EVTCHN permissions to be
> consistent.
> >> Unfortunately this patch will break the Windows PV driver function here:
> >>
> >>
> http://xenbits.xen.org/gitweb/?p=pvdrivers/win/xenbus.git;a=blob;f=src/x
> enbus/evtchn.c;hb=HEAD#l1037
> >>
> >> Unfortunately the values really do change across a reset. It would be
> possible to use volatile (disappear on reboot) registry keys to store the
> updated values instead but I don't really see any harm in allowing the guest
> to update the values to be correct, unless we want to change Xen to do the
> job so the guest doesn't have to go through this dance.
> >
> > :(  Everything is terrible.
> >
> > This is a general problem, not x86 specific, so I'll drop this patch and
> > make a similar adjustment to the ARM one.
> 
> I am a bit confused. I would have thought this was updated by the
> toolstack at reset. So why would the guest update them?
> 

The problem comes when establishing the evtchn ABI. In the Windows case, when the XENBUS driver loads, it will establish the ABI )which is FIFO by default but can be overridden to 2-level). The XENBUS driver can be manually unloaded and re-loaded (e.g. for upgrade purposes) so it is also necessary to tear down the ABI during unload. This means it is necessary to go through an event channel reset operation, and this has the side-effect of tearing down the store and console event channels which were established by the toolstack before the guest started to boot.
It is therefore necessary for the guest, prior to requesting the reset, to query the store and console event channels to get information about the remote domain and port and then, after reset, perform an interdomain bind operation to re-establish the channels. When the guest goes through this dance there is no guarantee that it will get the same local port number for each of the channels is it had previously. Thus, either the information has to be stored somewhere locally such that a new instantiation of the XENBUS driver can find it (e.g. a volatile registry key) or the HVM param needs to be updated to match reality.

HTH,

  Paul

> Cheers,
> 
> --
> Julien Grall
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel

Re: [PATCH 4/5] x86/hvm: Misc non-functional cleanup to the HVM_PARAM infrastructure

[Roger Pau Monné]

On Wed, Sep 05, 2018 at 07:12:03PM +0100, Andrew Cooper wrote:
> The parameter marshalling and xsm checks are common to both the set and get
> paths.  Lift all of the common code out into do_hvm_op() and let
> hvmop_{get,set}_param() operate on struct xen_hvm_param directly.
> 
> This is somewhat noisy in the functions as each a. reference has to change to
> a-> instead.
> 
> In addition, drop an empty default statement, insert newlines as appropriate
> between cases, and there is no need to update the IDENT_PT on the fastpath,
> because the common path after the switch will make the update.
> 
> No functional change, but a net shrink of -328 to do_hvm_op().
> 
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>

Just two suggestions below.

> @@ -4322,41 +4308,34 @@ static int hvmop_set_param(
>           * 256 bits interrupt redirection bitmap + 64k bits I/O bitmap
>           * plus one padding byte).
>           */
> -        if ( (a.value >> 32) > sizeof(struct tss32) +
> -                               (0x100 / 8) + (0x10000 / 8) + 1 )
> -            a.value = (uint32_t)a.value |
> -                      ((sizeof(struct tss32) + (0x100 / 8) +
> -                                               (0x10000 / 8) + 1) << 32);
> -        a.value |= VM86_TSS_UPDATED;
> +        if ( (a->value >> 32) > sizeof(struct tss32) +
> +                                (0x100 / 8) + (0x10000 / 8) + 1 )
> +            a->value = (uint32_t)a->value |
> +                       ((sizeof(struct tss32) + (0x100 / 8) +
> +                                                (0x10000 / 8) + 1) << 32);
> +        a->value |= VM86_TSS_UPDATED;
>          break;
>  
>      case HVM_PARAM_MCA_CAP:
> -        rc = vmce_enable_mca_cap(d, a.value);
> +        rc = vmce_enable_mca_cap(d, a->value);
>          break;
>      }
>  
>      if ( rc != 0 )
>          goto out;
>  
> -    d->arch.hvm.params[a.index] = a.value;
> +    d->arch.hvm.params[a->index] = a->value;
>  
>      HVM_DBG_LOG(DBG_LEVEL_HCALL, "set param %u = %"PRIx64,
> -                a.index, a.value);
> +                a->index, a->value);
>  
>   out:
> -    rcu_unlock_domain(d);
>      return rc;

If the out label is just return rc, and unless there's no further
patches adding code here I would consider removing it.

> -static int hvmop_get_param(
> -    XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> +static int hvmop_get_param(struct domain *d, struct xen_hvm_param *a)
>  {
> -    struct xen_hvm_param a;
> -    struct domain *d;
>      int rc;
>  
> -    if ( copy_from_guest(&a, arg, 1) )
> -        return -EFAULT;
> -
> -    d = rcu_lock_domain_by_any_id(a.domid);
> -    if ( d == NULL )
> -        return -ESRCH;
> -
> -    rc = -EINVAL;
> -    if ( !is_hvm_domain(d) )
> -        goto out;
> -
> -    rc = hvm_allow_get_param(d, &a);
> +    rc = hvm_allow_get_param(d, a);

You could move this initialization at declaration time.

Thanks, Roger.

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

Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Andrew Cooper]

On 07/09/18 17:01, Roger Pau Monné wrote:
> On Wed, Sep 05, 2018 at 07:12:01PM +0100, Andrew Cooper wrote:
>> There are holes in the HVM_PARAM space, some of which are from deprecated
>> parameters, but toolstack and device models currently has (almost) blanket
>> write access.
>>
>> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
>> parameters, with the default case failing with -EINVAL.  This subsumes the
>> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
>> BUFIOREQ_EVTCHN Xen-write-only value.
>>
>> No expected change for the defined, in-use params.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
>
>> ---
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Wei Liu <wei.liu2@citrix.com>
>> CC: Roger Pau Monné <roger.pau@citrix.com>
>> CC: Paul Durrant <paul.durrant@citrix.com>
>> CC: Stefano Stabellini <sstabellini@kernel.org>
>> CC: Julien Grall <julien.grall@arm.com>
>> ---
>>  xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------------------
>>  1 file changed, 31 insertions(+), 22 deletions(-)
>>
>> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
>> index 96a6323..d19ae35 100644
>> --- a/xen/arch/x86/hvm/hvm.c
>> +++ b/xen/arch/x86/hvm/hvm.c
>> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
>>  
>>      switch ( a->index )
>>      {
>> -    /* The following parameters can be set by the guest. */
>> +        /* The following parameters can be set by the guest and toolstack. */
>>      case HVM_PARAM_CALLBACK_IRQ:
>>      case HVM_PARAM_VM86_TSS:
> Note sure about the point of letting the guest set the unreal mode
> TSS, but anyway this is not the scope of the patch.

Because hvmloader still sets it up for HVM guests.

Neither you nor Jan took my hints (when doing various related work) that
unifying the PVH and HVM paths in the domain builder (alongside
IDENT_PT) would be a GoodThing(tm).

OTOH, we do now actually have a fairly simple cleanup task which a
student could be guided through doing, which would allow us to remove
guest access to these two params.

>
>>      case HVM_PARAM_VM86_TSS_SIZED:
>> @@ -4083,18 +4083,40 @@ static int hvm_allow_set_param(struct domain *d,
>>      case HVM_PARAM_CONSOLE_EVTCHN:
> Also it's quite weird that we allow the guest to set the console
> evtchn...
>
>>      case HVM_PARAM_X87_FIP_WIDTH:
>>          break;
>> -    /*
>> -     * The following parameters must not be set by the guest
>> -     * since the domain may need to be paused.
>> -     */
>> +
>> +        /*
>> +         * The following parameters are intended for toolstack usage only.
>> +         * Some require the domain to be paused while others control
>> +         * permissions in Xen, and therefore may not set by the domain.
>> +         */
>> +    case HVM_PARAM_STORE_PFN:
>> +    case HVM_PARAM_PAE_ENABLED:
>> +    case HVM_PARAM_IOREQ_PFN:
>> +    case HVM_PARAM_BUFIOREQ_PFN:
>> +    case HVM_PARAM_VIRIDIAN:
>> +    case HVM_PARAM_TIMER_MODE:
>> +    case HVM_PARAM_HPET_ENABLED:
>>      case HVM_PARAM_IDENT_PT:
>>      case HVM_PARAM_DM_DOMAIN:
>>      case HVM_PARAM_ACPI_S_STATE:
>> -    /* The remaining parameters should not be set by the guest. */
>> -    default:
>> +    case HVM_PARAM_VPT_ALIGN:
>> +    case HVM_PARAM_CONSOLE_PFN:
> ... but not the console page. I think the guest shouldn't be allowed to
> set any of those.

I see you saw why, but you need to read Paul's reply.

Any user of EVTCHNOP_reset needs to recreate the toolstack event
channels and rewrite *_EVTCHN to allow the next entity (in Paul's
example, the windows crash driver) to start back up correctly.

Of course, the same cant be said for a theoretical GNTTABOP_reset.  (If
it seems like this is a sprawling mess of swamps, that's because its all
terrible.)

>
>> +    case HVM_PARAM_NESTEDHVM:
>> +    case HVM_PARAM_PAGING_RING_PFN:
>> +    case HVM_PARAM_MONITOR_RING_PFN:
>> +    case HVM_PARAM_SHARING_RING_PFN:
>> +    case HVM_PARAM_TRIPLE_FAULT_REASON:
>> +    case HVM_PARAM_IOREQ_SERVER_PFN:
>> +    case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
>> +    case HVM_PARAM_MCA_CAP:
>>          if ( d == current->domain )
>>              rc = -EPERM;
>>          break;
>> +
>> +        /* Writeable only by Xen, hole, deprecated, or out-of-range. */
>> +    default:
>> +        rc = -EINVAL;
>> +        break;
>>      }
>>  
>>      if ( rc )
>> @@ -4130,9 +4152,6 @@ static int hvmop_set_param(
>>      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;
>> @@ -4209,15 +4228,7 @@ 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:
>> -    case HVM_PARAM_MEMORY_EVENT_INT3:
>> -    case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
>> -    case HVM_PARAM_MEMORY_EVENT_MSR:
>> -        /* Deprecated */
>> -        rc = -EOPNOTSUPP;
>> -        break;
> I assume there's no toolstack logic that relies on those parameters
> returning EOPNOTSUPP?

No.  There is a libxc function which intercepts these params and fails
early with -EOPNOTSUPP, so hypercalls never hit Xen, but I can't find
any code which requests these PARAMs now.

~Andrew

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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Andrew Cooper]

On 07/09/18 09:55, Jan Beulich wrote:
>>>> On 06.09.18 at 17:21, <andrew.cooper3@citrix.com> wrote:
>> On 06/09/18 09:56, Paul Durrant wrote:
>>>> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>>>>      if ( copy_from_guest(&a, arg, 1) )
>>>>          return -EFAULT;
>>>>
>>>> -    if ( a.index >= HVM_NR_PARAMS )
>>>> -        return -EINVAL;
>>>> -
>>> ASSERT, just in case someone screws up the allow function in future?
>> That's not going to help in any practical way.  This check does really
>> exist, and is part of the switch statement.
> Which switch() statement? The one in the allow function includes this,
> but the one here simply has
>
>     default:
>         a.value = d->arch.hvm.params[a.index];
>         break;

A boundary check on a.index logically falls within the remit of
hvm_allow_get_param()

~Andrew

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

Re: [PATCH 1/5] x86/hvm: Switch hvm_allow_get_param() to use a whitelist

[Jan Beulich]

>>> On 07.09.18 at 20:18, <andrew.cooper3@citrix.com> wrote:
> On 07/09/18 09:55, Jan Beulich wrote:
>>>>> On 06.09.18 at 17:21, <andrew.cooper3@citrix.com> wrote:
>>> On 06/09/18 09:56, Paul Durrant wrote:
>>>>> @@ -4390,9 +4411,6 @@ static int hvmop_get_param(
>>>>>      if ( copy_from_guest(&a, arg, 1) )
>>>>>          return -EFAULT;
>>>>>
>>>>> -    if ( a.index >= HVM_NR_PARAMS )
>>>>> -        return -EINVAL;
>>>>> -
>>>> ASSERT, just in case someone screws up the allow function in future?
>>> That's not going to help in any practical way.  This check does really
>>> exist, and is part of the switch statement.
>> Which switch() statement? The one in the allow function includes this,
>> but the one here simply has
>>
>>     default:
>>         a.value = d->arch.hvm.params[a.index];
>>         break;
> 
> A boundary check on a.index logically falls within the remit of
> hvm_allow_get_param()

Correct. Hence - due to the split between the two functions - the
desire to have a validating ASSERT() in the other function.

Jan



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

Re: [PATCH 2/5] x86/hvm: Switch hvm_allow_set_param() to use a whitelist

[Roger Pau Monné]

On Fri, Sep 07, 2018 at 07:13:08PM +0100, Andrew Cooper wrote:
> On 07/09/18 17:01, Roger Pau Monné wrote:
> > On Wed, Sep 05, 2018 at 07:12:01PM +0100, Andrew Cooper wrote:
> >> There are holes in the HVM_PARAM space, some of which are from deprecated
> >> parameters, but toolstack and device models currently has (almost) blanket
> >> write access.
> >>
> >> Rearrange hvm_allow_get_param() to have a whitelist of toolstack-writeable
> >> parameters, with the default case failing with -EINVAL.  This subsumes the
> >> HVM_NR_PARAMS check, as well as the MEMORY_EVENT_* deprecated block, and the
> >> BUFIOREQ_EVTCHN Xen-write-only value.
> >>
> >> No expected change for the defined, in-use params.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> > Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
> >
> >> ---
> >> CC: Jan Beulich <JBeulich@suse.com>
> >> CC: Wei Liu <wei.liu2@citrix.com>
> >> CC: Roger Pau Monné <roger.pau@citrix.com>
> >> CC: Paul Durrant <paul.durrant@citrix.com>
> >> CC: Stefano Stabellini <sstabellini@kernel.org>
> >> CC: Julien Grall <julien.grall@arm.com>
> >> ---
> >>  xen/arch/x86/hvm/hvm.c | 53 +++++++++++++++++++++++++++++---------------------
> >>  1 file changed, 31 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> >> index 96a6323..d19ae35 100644
> >> --- a/xen/arch/x86/hvm/hvm.c
> >> +++ b/xen/arch/x86/hvm/hvm.c
> >> @@ -4073,7 +4073,7 @@ static int hvm_allow_set_param(struct domain *d,
> >>  
> >>      switch ( a->index )
> >>      {
> >> -    /* The following parameters can be set by the guest. */
> >> +        /* The following parameters can be set by the guest and toolstack. */
> >>      case HVM_PARAM_CALLBACK_IRQ:
> >>      case HVM_PARAM_VM86_TSS:
> > Note sure about the point of letting the guest set the unreal mode
> > TSS, but anyway this is not the scope of the patch.
> 
> Because hvmloader still sets it up for HVM guests.
> 
> Neither you nor Jan took my hints (when doing various related work) that
> unifying the PVH and HVM paths in the domain builder (alongside
> IDENT_PT) would be a GoodThing(tm).
> 
> OTOH, we do now actually have a fairly simple cleanup task which a
> student could be guided through doing, which would allow us to remove
> guest access to these two params.

Hm, right. The main problem I see with this is that the hypervisor has
no knowledge of the memory map when building a DomU (all this is in
the toolstack), so it's quite hard to figure out where to place the
TSS or the identity page tables.

We could make the special pages addresses somehow part of the public
headers, so that there's a fixed address known by both the toolstack
and the hypervisor about the location of those magic pages.

Roger.

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