From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 2/3] domctl: also pause domain for extended context updates Date: Wed, 5 Feb 2014 15:05:07 +0000 Message-ID: <52F25323.9080208@citrix.com> References: <52F25D4202000078001196AB@nat28.tlf.novell.com> <52F25E9902000078001196E8@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4778219406206390804==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WB4Dj-0000xv-H7 for xen-devel@lists.xenproject.org; Wed, 05 Feb 2014 15:16:59 +0000 In-Reply-To: <52F25E9902000078001196E8@nat28.tlf.novell.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: Jan Beulich Cc: George Dunlap , xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============4778219406206390804== Content-Type: multipart/alternative; boundary="------------040702000603030909050407" --------------040702000603030909050407 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 05/02/14 14:54, Jan Beulich wrote: > This is not just for consistency with "base" context updates, but > actually needed so that guest side accesses can't race with control > domain side updates. > > This would have been a security issue if XSA-77 hadn't waived them on > the affected domctl operation. > > While looking at the code I also spotted a redundant NULL check in the > "base" context update handling code, which is being removed. > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/domctl.c > +++ b/xen/arch/x86/domctl.c > @@ -853,6 +853,8 @@ long arch_do_domctl( > } > else > { > + if ( d == current->domain ) /* no domain_pause() */ > + break; > ret = -EINVAL; > if ( evc->size < offsetof(typeof(*evc), vmce) ) > break; > @@ -861,6 +863,7 @@ long arch_do_domctl( > if ( !is_canonical_address(evc->sysenter_callback_eip) || > !is_canonical_address(evc->syscall32_callback_eip) ) > break; > + domain_pause(d); > fixup_guest_code_selector(d, evc->sysenter_callback_cs); > v->arch.pv_vcpu.sysenter_callback_cs = > evc->sysenter_callback_cs; > @@ -881,6 +884,8 @@ long arch_do_domctl( > (evc->syscall32_callback_cs & ~3) || > evc->syscall32_callback_eip ) > break; > + else > + domain_pause(d); > > BUILD_BUG_ON(offsetof(struct xen_domctl_ext_vcpucontext, > mcg_cap) != > @@ -899,6 +904,8 @@ long arch_do_domctl( > } > else > ret = 0; > + > + domain_unpause(d); > } > } > break; > --- a/xen/common/domctl.c > +++ b/xen/common/domctl.c > @@ -334,10 +334,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe > unsigned int vcpu = op->u.vcpucontext.vcpu; > struct vcpu *v; > > - ret = -ESRCH; > - if ( d == NULL ) > - break; > - > ret = -EINVAL; > if ( (d == current->domain) || /* no domain_pause() */ > (vcpu >= d->max_vcpus) || ((v = d->vcpu[vcpu]) == NULL) ) > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------040702000603030909050407 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 05/02/14 14:54, Jan Beulich wrote:
This is not just for consistency with "base" context updates, but
actually needed so that guest side accesses can't race with control
domain side updates.

This would have been a security issue if XSA-77 hadn't waived them on
the affected domctl operation.

While looking at the code I also spotted a redundant NULL check in the
"base" context update handling code, which is being removed.

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

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>


--- a/xen/arch/x86/domctl.c
+++ b/xen/arch/x86/domctl.c
@@ -853,6 +853,8 @@ long arch_do_domctl(
         }
         else
         {
+            if ( d == current->domain ) /* no domain_pause() */
+                break;
             ret = -EINVAL;
             if ( evc->size < offsetof(typeof(*evc), vmce) )
                 break;
@@ -861,6 +863,7 @@ long arch_do_domctl(
                 if ( !is_canonical_address(evc->sysenter_callback_eip) ||
                      !is_canonical_address(evc->syscall32_callback_eip) )
                     break;
+                domain_pause(d);
                 fixup_guest_code_selector(d, evc->sysenter_callback_cs);
                 v->arch.pv_vcpu.sysenter_callback_cs      =
                     evc->sysenter_callback_cs;
@@ -881,6 +884,8 @@ long arch_do_domctl(
                       (evc->syscall32_callback_cs & ~3) ||
                       evc->syscall32_callback_eip )
                 break;
+            else
+                domain_pause(d);
 
             BUILD_BUG_ON(offsetof(struct xen_domctl_ext_vcpucontext,
                                   mcg_cap) !=
@@ -899,6 +904,8 @@ long arch_do_domctl(
             }
             else
                 ret = 0;
+
+            domain_unpause(d);
         }
     }
     break;
--- a/xen/common/domctl.c
+++ b/xen/common/domctl.c
@@ -334,10 +334,6 @@ long do_domctl(XEN_GUEST_HANDLE_PARAM(xe
         unsigned int vcpu = op->u.vcpucontext.vcpu;
         struct vcpu *v;
 
-        ret = -ESRCH;
-        if ( d == NULL )
-            break;
-
         ret = -EINVAL;
         if ( (d == current->domain) || /* no domain_pause() */
              (vcpu >= d->max_vcpus) || ((v = d->vcpu[vcpu]) == NULL) )






_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

--------------040702000603030909050407-- --===============4778219406206390804== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Xen-devel mailing list Xen-devel@lists.xen.org http://lists.xen.org/xen-devel --===============4778219406206390804==--