From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/3] x86/hvm: introduce functions for HVMOP_get/set_param allowance checks Date: Fri, 24 Apr 2015 18:19:17 +0100 Message-ID: <553A7B15.1090401@citrix.com> References: <1429892490-5590-1-git-send-email-paul.durrant@citrix.com> <1429892490-5590-3-git-send-email-paul.durrant@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail6.bemta3.messagelabs.com ([195.245.230.39]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1YlhG6-0005gb-U2 for xen-devel@lists.xenproject.org; Fri, 24 Apr 2015 17:19:23 +0000 In-Reply-To: <1429892490-5590-3-git-send-email-paul.durrant@citrix.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Paul Durrant , xen-devel@lists.xenproject.org Cc: Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org 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 > Cc: Keir Fraser > Cc: Jan Beulich > Cc: Andrew Cooper > --- > 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: {