From mboxrd@z Thu Jan 1 00:00:00 1970 From: Andrew Cooper Subject: Re: [PATCH 1/2] x86/HVM: simplify do_hvm_op() Date: Tue, 25 Mar 2014 17:26:56 +0000 Message-ID: <5331BC60.4040402@citrix.com> References: <5331B4270200007800001DFA@nat28.tlf.novell.com> <5331B6D70200007800001E19@nat28.tlf.novell.com> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============4393142015882085774==" Return-path: Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xen.org with esmtp (Exim 4.72) (envelope-from ) id 1WSV7t-0006r6-Uu for xen-devel@lists.xenproject.org; Tue, 25 Mar 2014 17:27:03 +0000 In-Reply-To: <5331B6D70200007800001E19@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: xen-devel , Keir Fraser List-Id: xen-devel@lists.xenproject.org --===============4393142015882085774== Content-Type: multipart/alternative; boundary="------------030408000608080701070207" --------------030408000608080701070207 Content-Type: text/plain; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit On 25/03/14 16:03, Jan Beulich wrote: > - boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and > HVMOP_set_mem_access: all of these already check for overflow, so > there's no need to range check the first _and_ last PFN (checking > the last one suffices) > - copying back interface structures that were previously copied from > guest memory can use __copy_to_...(), since copy_from_...() already > did the address range validation > > Signed-off-by: Jan Beulich Reviewed-by: Andrew Cooper > > --- a/xen/arch/x86/hvm/hvm.c > +++ b/xen/arch/x86/hvm/hvm.c > @@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > a.value = d->arch.hvm_domain.params[a.index]; > break; > } > - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > } > > HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64, > @@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > goto param_fail3; > > rc = -EINVAL; > - if ( (a.first_pfn > domain_get_maximum_gpfn(d)) || > - ((a.first_pfn + a.nr - 1) < a.first_pfn) || > + if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) || > ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) ) > goto param_fail3; > > @@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > /* Check for continuation if it's not the last interation */ > if ( a.nr > 0 && hypercall_preempt_check() ) > { > - if ( copy_to_guest(arg, &a, 1) ) > + if ( __copy_to_guest(arg, &a, 1) ) > rc = -EFAULT; > else > rc = -EAGAIN; > @@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > a.mem_type = HVMMEM_ram_rw; > else > a.mem_type = HVMMEM_mmio_dm; > - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > } > > param_fail_getmemtype: > @@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > goto param_fail4; > > rc = -EINVAL; > - if ( (a.first_pfn > domain_get_maximum_gpfn(d)) || > - ((a.first_pfn + a.nr - 1) < a.first_pfn) || > + if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) || > ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) ) > goto param_fail4; > > @@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > /* Check for continuation if it's not the last interation */ > if ( a.nr > 0 && hypercall_preempt_check() ) > { > - if ( copy_to_guest(arg, &a, 1) ) > + if ( __copy_to_guest(arg, &a, 1) ) > rc = -EFAULT; > else > rc = -EAGAIN; > @@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE > > rc = -EINVAL; > if ( (a.first_pfn != ~0ull) && > - ((a.first_pfn > domain_get_maximum_gpfn(d)) || > - ((a.first_pfn + a.nr - 1) < a.first_pfn) || > - ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) ) > + (((a.first_pfn + a.nr - 1) < a.first_pfn) || > + ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) ) > goto param_fail5; > > rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access); > @@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE > goto param_fail6; > > a.hvmmem_access = access; > - rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > + rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0; > > param_fail6: > rcu_unlock_domain(d); > > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel --------------030408000608080701070207 Content-Type: text/html; charset="ISO-8859-1" Content-Transfer-Encoding: 7bit
On 25/03/14 16:03, Jan Beulich wrote:
- boundary checks in HVMOP_modified_memory, HVMOP_set_mem_type, and
  HVMOP_set_mem_access: all of these already check for overflow, so
  there's no need to range check the first _and_ last PFN (checking
  the last one suffices)
- copying back interface structures that were previously copied from
  guest memory can use __copy_to_...(), since copy_from_...() already
  did the address range validation

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

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


--- a/xen/arch/x86/hvm/hvm.c
+++ b/xen/arch/x86/hvm/hvm.c
@@ -4288,7 +4288,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.value = d->arch.hvm_domain.params[a.index];
                 break;
             }
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
         HVM_DBG_LOG(DBG_LEVEL_HCALL, "%s param %u = %"PRIx64,
@@ -4386,8 +4386,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail3;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail3;
 
@@ -4416,7 +4415,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4465,7 +4464,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
                 a.mem_type =  HVMMEM_ram_rw;
             else
                 a.mem_type =  HVMMEM_mmio_dm;
-            rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+            rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
         }
 
     param_fail_getmemtype:
@@ -4501,8 +4500,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail4;
 
         rc = -EINVAL;
-        if ( (a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+        if ( ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d)) )
             goto param_fail4;
             
@@ -4558,7 +4556,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             /* Check for continuation if it's not the last interation */
             if ( a.nr > 0 && hypercall_preempt_check() )
             {
-                if ( copy_to_guest(arg, &a, 1) )
+                if ( __copy_to_guest(arg, &a, 1) )
                     rc = -EFAULT;
                 else
                     rc = -EAGAIN;
@@ -4595,9 +4593,8 @@ long do_hvm_op(unsigned long op, XEN_GUE
 
         rc = -EINVAL;
         if ( (a.first_pfn != ~0ull) &&
-             ((a.first_pfn > domain_get_maximum_gpfn(d)) ||
-             ((a.first_pfn + a.nr - 1) < a.first_pfn) ||
-             ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
+             (((a.first_pfn + a.nr - 1) < a.first_pfn) ||
+              ((a.first_pfn + a.nr - 1) > domain_get_maximum_gpfn(d))) )
             goto param_fail5;
             
         rc = p2m_set_mem_access(d, a.first_pfn, a.nr, a.hvmmem_access);
@@ -4646,7 +4643,7 @@ long do_hvm_op(unsigned long op, XEN_GUE
             goto param_fail6;
 
         a.hvmmem_access = access;
-        rc = copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
+        rc = __copy_to_guest(arg, &a, 1) ? -EFAULT : 0;
 
     param_fail6:
         rcu_unlock_domain(d);






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

--------------030408000608080701070207-- --===============4393142015882085774== 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 --===============4393142015882085774==--