From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: [PATCH 4/4] x86/domctl: Two functional fixes to XEN_DOMCTL_[gs]etvcpuextstate Date: Wed, 4 Jun 2014 18:26:34 +0100 Message-ID: <1401902794-15542-5-git-send-email-andrew.cooper3@citrix.com> References: <1401902794-15542-1-git-send-email-andrew.cooper3@citrix.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1401902794-15542-1-git-send-email-andrew.cooper3@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: Xen-devel Cc: Andrew Cooper , Keir Fraser , Jan Beulich List-Id: xen-devel@lists.xenproject.org Interacting with the vcpu itself should be protected by vcpu_pause(). Buggy/naive toolstacks might encounter adverse interaction with a vcpu context switch, or increase of xcr0_accum. There are no much problems with current in-tree code. Explicitly permit a NULL guest handle as being a request for size. It is the prevailing Xen style, and without it, valgrind's ioctl handler is unable to determine whether evc->buffer actually got written to. Signed-off-by: Andrew Cooper CC: Keir Fraser CC: Jan Beulich --- There are further issues which are specifically not fixed in this patch. There is a race condition between querying evc->size and trying to write the xsave state into a buffer that size, if the vcpu goes and increases xcr0_accum in the meantime. The legacy migration stream is sufficiently inflexible that I can't find a way of doing it in a backwards compatible way which still works in a heterogeneous migration case. As there are further problems anyway with the legacy migration stream anyway (vcpus with different xcr0_accum's will result in stream corruption in xc_domain_restore()), I intend to wait until the v2 code is present and v1 code removed, at which point this issue shall be trivial to fix. --- xen/arch/x86/domctl.c | 53 +++++++++++++++++++++++++++---------------------- 1 file changed, 29 insertions(+), 24 deletions(-) diff --git a/xen/arch/x86/domctl.c b/xen/arch/x86/domctl.c index 8f5b287..99163f9 100644 --- a/xen/arch/x86/domctl.c +++ b/xen/arch/x86/domctl.c @@ -1097,45 +1097,48 @@ long arch_do_domctl( ((v = d->vcpu[evc->vcpu]) == NULL) ) goto vcpuextstate_out; + ret = -EINVAL; + if ( v == current ) /* no vcpu_pause() */ + goto vcpuextstate_out; + if ( domctl->cmd == XEN_DOMCTL_getvcpuextstate ) { - unsigned int size = PV_XSAVE_SIZE(v->arch.xcr0_accum); + unsigned int size; - if ( !evc->size && !evc->xfeature_mask ) + ret = 0; + vcpu_pause(v); + + size = PV_XSAVE_SIZE(v->arch.xcr0_accum); + if ( (!evc->size && !evc->xfeature_mask) || + guest_handle_is_null(evc->buffer) ) { evc->xfeature_mask = xfeature_mask; evc->size = size; - ret = 0; + vcpu_unpause(v); goto vcpuextstate_out; } + if ( evc->size != size || evc->xfeature_mask != xfeature_mask ) - { ret = -EINVAL; - goto vcpuextstate_out; - } - if ( copy_to_guest_offset(domctl->u.vcpuextstate.buffer, - offset, (void *)&v->arch.xcr0, - sizeof(v->arch.xcr0)) ) - { + + if ( !ret && copy_to_guest_offset(evc->buffer, offset, + (void *)&v->arch.xcr0, + sizeof(v->arch.xcr0)) ) ret = -EFAULT; - goto vcpuextstate_out; - } + offset += sizeof(v->arch.xcr0); - if ( copy_to_guest_offset(domctl->u.vcpuextstate.buffer, - offset, (void *)&v->arch.xcr0_accum, - sizeof(v->arch.xcr0_accum)) ) - { + if ( !ret && copy_to_guest_offset(evc->buffer, offset, + (void *)&v->arch.xcr0_accum, + sizeof(v->arch.xcr0_accum)) ) ret = -EFAULT; - goto vcpuextstate_out; - } + offset += sizeof(v->arch.xcr0_accum); - if ( copy_to_guest_offset(domctl->u.vcpuextstate.buffer, - offset, (void *)v->arch.xsave_area, - size - 2 * sizeof(uint64_t)) ) - { + if ( !ret && copy_to_guest_offset(evc->buffer, offset, + (void *)v->arch.xsave_area, + size - 2 * sizeof(uint64_t)) ) ret = -EFAULT; - goto vcpuextstate_out; - } + + vcpu_unpause(v); } else { @@ -1183,12 +1186,14 @@ long arch_do_domctl( if ( evc->size <= PV_XSAVE_SIZE(_xcr0_accum) ) { + vcpu_pause(v); v->arch.xcr0 = _xcr0; v->arch.xcr0_accum = _xcr0_accum; if ( _xcr0_accum & XSTATE_NONLAZY ) v->arch.nonlazy_xstate_used = 1; memcpy(v->arch.xsave_area, _xsave_area, evc->size - 2 * sizeof(uint64_t)); + vcpu_unpause(v); } else ret = -EINVAL; -- 1.7.10.4