|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks
On 24/04/15 17:21, Paul Durrant wrote:
> Some parameters can only (validly) be set once. Some cannot be set
> by a guest for its own domain. Consolidate these checks, along with
> the XSM check, in a new hvm_allow_set_param() function for clarity.
>
> Also, introduce hvm_allow_get_param() for similar reasons.
>
> Signed-off-by: Paul Durrant <paul.durrant@xxxxxxxxxx>
> Cc: Keir Fraser <keir@xxxxxxx>
> Cc: Jan Beulich <jbeulich@xxxxxxxx>
> Cc: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> ---
> xen/arch/x86/hvm/hvm.c | 141
> +++++++++++++++++++++++++++---------------------
> 1 file changed, 79 insertions(+), 62 deletions(-)
>
> diff --git a/xen/arch/x86/hvm/hvm.c b/xen/arch/x86/hvm/hvm.c
> index 87c47b1..23c604d 100644
> --- a/xen/arch/x86/hvm/hvm.c
> +++ b/xen/arch/x86/hvm/hvm.c
> @@ -5640,6 +5640,57 @@ static int hvmop_set_evtchn_upcall_vector(
> return 0;
> }
>
> +static int hvm_allow_set_param(struct domain *d,
> + struct xen_hvm_param *a)
const
> +{
> + uint64_t value = d->arch.hvm_domain.params[a->index];
> + int rc = 0;
No need for the 0 initialiser (and I know Coverity will complain about it).
> +
> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
> + if ( rc )
> + return rc;
> +
> + /* The following parameters cannot be set by the guest */
> + switch (a->index)
style
> + {
> + case HVM_PARAM_VIRIDIAN:
> + case HVM_PARAM_IDENT_PT:
> + case HVM_PARAM_DM_DOMAIN:
> + case HVM_PARAM_ACPI_S_STATE:
> + case HVM_PARAM_MEMORY_EVENT_CR0:
> + case HVM_PARAM_MEMORY_EVENT_CR3:
> + case HVM_PARAM_MEMORY_EVENT_CR4:
> + case HVM_PARAM_MEMORY_EVENT_INT3:
> + case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> + case HVM_PARAM_MEMORY_EVENT_MSR:
> + case HVM_PARAM_IOREQ_SERVER_PFN:
> + case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> + if ( d == current->domain )
> + rc = -EPERM;
> + break;
> + default:
> + break;
Please can we turn the guest-settable list into a whitelist rather than
a blacklist. We have had several XSAs in the past where a guest has
been able to modify a value it shouldn't have because new params were
default writable by everyone.
> + }
> +
> + if ( rc )
> + return rc;
> +
> + /* The following parameters can only be changed once */
> + switch (a->index)
> + {
> + case HVM_PARAM_VIRIDIAN:
> + case HVM_PARAM_IOREQ_SERVER_PFN:
> + case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> + if (value != 0 && a->value != value)
> + rc = -EEXIST;
> + break;
> + default:
> + break;
> + }
> +
> + return rc;
> +}
> +
> static int hvmop_set_param(
> XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> {
> @@ -5666,7 +5717,7 @@ static int hvmop_set_param(
> && (a.index != HVM_PARAM_CALLBACK_IRQ) )
> goto out;
>
> - rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_set_param);
> + rc = hvm_allow_set_param(d, &a);
> if ( rc )
> goto out;
>
> @@ -5681,31 +5732,11 @@ static int hvmop_set_param(
> rc = -EINVAL;
> break;
> case HVM_PARAM_VIRIDIAN:
> - /* This should only ever be set once by the tools and read by the
> guest. */
> - rc = -EPERM;
> - if ( d == current->domain )
> - break;
> -
> - if ( a.value != d->arch.hvm_domain.params[a.index] )
> - {
> - rc = -EEXIST;
> - if ( d->arch.hvm_domain.params[a.index] != 0 )
> - break;
> -
> + if ( (a.value & ~HVMPV_feature_mask) ||
> + !(a.value & HVMPV_base_freq) )
> rc = -EINVAL;
> - if ( (a.value & ~HVMPV_feature_mask) ||
> - !(a.value & HVMPV_base_freq) )
> - break;
> - }
> -
> - rc = 0;
> break;
> case HVM_PARAM_IDENT_PT:
> - /* Not reflexive, as we must domain_pause(). */
> - rc = -EPERM;
> - if ( d == current->domain )
> - break;
> -
> rc = -EINVAL;
> if ( d->arch.hvm_domain.params[a.index] != 0 )
> break;
> @@ -5733,22 +5764,12 @@ static int hvmop_set_param(
> domctl_lock_release();
> break;
> case HVM_PARAM_DM_DOMAIN:
> - /* Not reflexive, as we may need to domain_pause(). */
> - rc = -EPERM;
> - if ( d == current->domain )
> - break;
> -
> if ( a.value == DOMID_SELF )
> a.value = current->domain->domain_id;
>
> rc = hvm_set_dm_domain(d, a.value);
> break;
> case HVM_PARAM_ACPI_S_STATE:
> - /* Not reflexive, as we must domain_pause(). */
> - rc = -EPERM;
> - if ( current->domain == d )
> - break;
> -
> rc = 0;
> if ( a.value == 3 )
> hvm_s3_suspend(d);
> @@ -5761,20 +5782,9 @@ static int hvmop_set_param(
> case HVM_PARAM_ACPI_IOPORTS_LOCATION:
> rc = pmtimer_change_ioport(d, a.value);
> break;
> - case HVM_PARAM_MEMORY_EVENT_CR0:
> - case HVM_PARAM_MEMORY_EVENT_CR3:
> - case HVM_PARAM_MEMORY_EVENT_CR4:
> - if ( d == current->domain )
> - rc = -EPERM;
> - break;
> case HVM_PARAM_MEMORY_EVENT_INT3:
> case HVM_PARAM_MEMORY_EVENT_SINGLE_STEP:
> case HVM_PARAM_MEMORY_EVENT_MSR:
> - if ( d == current->domain )
> - {
> - rc = -EPERM;
> - break;
> - }
> if ( a.value & HVMPME_onchangeonly )
> rc = -EINVAL;
> break;
> @@ -5807,22 +5817,12 @@ static int hvmop_set_param(
> rc = -EINVAL;
> break;
> case HVM_PARAM_IOREQ_SERVER_PFN:
> - if ( d == current->domain )
> - {
> - rc = -EPERM;
> - break;
> - }
> d->arch.hvm_domain.ioreq_gmfn.base = a.value;
> break;
> case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> {
> unsigned int i;
>
> - if ( d == current->domain )
> - {
> - rc = -EPERM;
> - break;
> - }
> if ( a.value == 0 ||
> a.value > sizeof(d->arch.hvm_domain.ioreq_gmfn.mask) * 8 )
> {
> @@ -5866,6 +5866,30 @@ static int hvmop_set_param(
> return rc;
> }
>
> +static int hvm_allow_get_param(struct domain *d,
> + struct xen_hvm_param *a)
> +{
> + int rc = 0;
> +
> + rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
> + if ( rc )
> + return rc;
> +
> + /* The following parameters cannot be read by the guest */
> + switch (a->index)
> + {
> + case HVM_PARAM_IOREQ_SERVER_PFN:
> + case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> + if ( d == current->domain )
> + rc = -EPERM;
> + break;
> + default:
> + break;
Again, this would be safer as a whitelist.
~Andrew
> + }
> +
> + return rc;
> +}
> +
> static int hvmop_get_param(
> XEN_GUEST_HANDLE_PARAM(xen_hvm_param_t) arg)
> {
> @@ -5891,7 +5915,7 @@ static int hvmop_get_param(
> && (a.index != HVM_PARAM_CALLBACK_IRQ) )
> goto out;
>
> - rc = xsm_hvm_param(XSM_TARGET, d, HVMOP_get_param);
> + rc = hvm_allow_get_param(d, &a);
> if ( rc )
> goto out;
>
> @@ -5900,13 +5924,6 @@ static int hvmop_get_param(
> case HVM_PARAM_ACPI_S_STATE:
> a.value = d->arch.hvm_domain.is_s3_suspended ? 3 : 0;
> break;
> - case HVM_PARAM_IOREQ_SERVER_PFN:
> - case HVM_PARAM_NR_IOREQ_SERVER_PAGES:
> - if ( d == current->domain )
> - {
> - rc = -EPERM;
> - goto out;
> - }
> case HVM_PARAM_IOREQ_PFN:
> case HVM_PARAM_BUFIOREQ_PFN:
> case HVM_PARAM_BUFIOREQ_EVTCHN: {
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |