[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Xen-changelog] [xen staging-4.8] xen/hypercall: Don't use BUG() for parameter checking in hypercall_create_continuation()



commit 151406a30b49986569925c9e14a5e581ed5a9435
Author:     Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
AuthorDate: Mon Nov 4 15:21:44 2019 +0100
Commit:     Jan Beulich <jbeulich@xxxxxxxx>
CommitDate: Mon Nov 4 15:21:44 2019 +0100

    xen/hypercall: Don't use BUG() for parameter checking in 
hypercall_create_continuation()
    
    Since c/s 1d429034 "hypercall: update vcpu_op to take an unsigned vcpuid",
    which incorrectly swapped 'i' for 'u' in the parameter type list, guests 
have
    been able to hit the BUG() in next_args()'s default case.
    
    Correct these back to 'i'.
    
    In addition, make adjustments to prevent this class of issue from occurring 
in
    the future - crashing Xen is not an appropriate form of parameter checking.
    
    Capitalise NEXT_ARG() to catch all uses, to highlight that it is a macro 
doing
    non-function-like things behind the scenes, and undef it when appropriate.
    Implement a bad_fmt: block which prints an error, asserts unreachable, and
    crashes the guest.
    
    On the ARM side, drop all parameter checking of p.  It is asymmetric with 
the
    x86 side, and akin to expecting memcpy() or sprintf() to check their src/fmt
    parameter before use.  A caller passing "" or something other than a string
    literal will be obvious during code review.
    
    This is XSA-296.
    
    Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
    Acked-by: Julien Grall <julien.grall@xxxxxxx>
    master commit: 0bf9f8d3e399a0e1d2b717f71b4776172446184b
    master date: 2019-10-31 16:07:11 +0100
---
 xen/arch/arm/domain.c      | 22 ++++++++++++++--------
 xen/arch/x86/domain.c      | 19 ++++++++++++++-----
 xen/common/compat/domain.c |  2 +-
 xen/common/domain.c        |  2 +-
 4 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index d9e796dcbe..f6678d2227 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -336,14 +336,15 @@ void sync_vcpu_execstate(struct vcpu *v)
     /* Nothing to do -- no lazy switching */
 }
 
-#define next_arg(fmt, args) ({                                              \
+#define NEXT_ARG(fmt, args)                                                 \
+({                                                                          \
     unsigned long __arg;                                                    \
     switch ( *(fmt)++ )                                                     \
     {                                                                       \
     case 'i': __arg = (unsigned long)va_arg(args, unsigned int);  break;    \
     case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break;    \
     case 'h': __arg = (unsigned long)va_arg(args, void *);        break;    \
-    default:  __arg = 0; BUG();                                             \
+    default:  goto bad_fmt;                                                 \
     }                                                                       \
     __arg;                                                                  \
 })
@@ -373,9 +374,6 @@ unsigned long hypercall_create_continuation(
     unsigned int i;
     va_list args;
 
-    /* All hypercalls take at least one argument */
-    BUG_ON( !p || *p == '\0' );
-
     va_start(args, format);
 
     if ( mcs->flags & MCSF_in_multicall )
@@ -383,7 +381,7 @@ unsigned long hypercall_create_continuation(
         __set_bit(_MCSF_call_preempted, &mcs->flags);
 
         for ( i = 0; *p != '\0'; i++ )
-            mcs->call.args[i] = next_arg(p, args);
+            mcs->call.args[i] = NEXT_ARG(p, args);
 
         /* Return value gets written back to mcs->call.result */
         rc = mcs->call.result;
@@ -402,7 +400,7 @@ unsigned long hypercall_create_continuation(
 
             for ( i = 0; *p != '\0'; i++ )
             {
-                arg = next_arg(p, args);
+                arg = NEXT_ARG(p, args);
 
                 switch ( i )
                 {
@@ -425,7 +423,7 @@ unsigned long hypercall_create_continuation(
 
             for ( i = 0; *p != '\0'; i++ )
             {
-                arg = next_arg(p, args);
+                arg = NEXT_ARG(p, args);
 
                 switch ( i )
                 {
@@ -446,8 +444,16 @@ unsigned long hypercall_create_continuation(
     va_end(args);
 
     return rc;
+
+ bad_fmt:
+    gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+    ASSERT_UNREACHABLE();
+    domain_crash(current->domain);
+    return 0;
 }
 
+#undef NEXT_ARG
+
 void startup_cpu_idle_loop(void)
 {
     struct vcpu *v = current;
diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
index ddeb68f967..3946ea38fd 100644
--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -2403,14 +2403,15 @@ void sync_vcpu_execstate(struct vcpu *v)
     flush_tlb_mask(v->vcpu_dirty_cpumask);
 }
 
-#define next_arg(fmt, args) ({                                              \
+#define NEXT_ARG(fmt, args)                                                 \
+({                                                                          \
     unsigned long __arg;                                                    \
     switch ( *(fmt)++ )                                                     \
     {                                                                       \
     case 'i': __arg = (unsigned long)va_arg(args, unsigned int);  break;    \
     case 'l': __arg = (unsigned long)va_arg(args, unsigned long); break;    \
     case 'h': __arg = (unsigned long)va_arg(args, void *);        break;    \
-    default:  __arg = 0; BUG();                                             \
+    default:  goto bad_fmt;                                                 \
     }                                                                       \
     __arg;                                                                  \
 })
@@ -2449,7 +2450,7 @@ unsigned long hypercall_create_continuation(
         __set_bit(_MCSF_call_preempted, &mcs->flags);
 
         for ( i = 0; *p != '\0'; i++ )
-            mcs->call.args[i] = next_arg(p, args);
+            mcs->call.args[i] = NEXT_ARG(p, args);
     }
     else
     {
@@ -2470,7 +2471,7 @@ unsigned long hypercall_create_continuation(
         {
             for ( i = 0; *p != '\0'; i++ )
             {
-                arg = next_arg(p, args);
+                arg = NEXT_ARG(p, args);
                 switch ( i )
                 {
                 case 0: regs->rdi = arg; break;
@@ -2486,7 +2487,7 @@ unsigned long hypercall_create_continuation(
         {
             for ( i = 0; *p != '\0'; i++ )
             {
-                arg = next_arg(p, args);
+                arg = NEXT_ARG(p, args);
                 switch ( i )
                 {
                 case 0: regs->ebx = arg; break;
@@ -2503,8 +2504,16 @@ unsigned long hypercall_create_continuation(
     va_end(args);
 
     return op;
+
+ bad_fmt:
+    gprintk(XENLOG_ERR, "Bad hypercall continuation format '%c'\n", *p);
+    ASSERT_UNREACHABLE();
+    domain_crash(current->domain);
+    return 0;
 }
 
+#undef NEXT_ARG
+
 int hypercall_xlat_continuation(unsigned int *id, unsigned int nr,
                                 unsigned int mask, ...)
 {
diff --git a/xen/common/compat/domain.c b/xen/common/compat/domain.c
index 88bfdc836d..d446ed131b 100644
--- a/xen/common/compat/domain.c
+++ b/xen/common/compat/domain.c
@@ -81,7 +81,7 @@ int compat_vcpu_op(int cmd, unsigned int vcpuid, 
XEN_GUEST_HANDLE_PARAM(void) ar
         }
 
         if ( rc == -ERESTART )
-            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
                                                cmd, vcpuid, arg);
 
         break;
diff --git a/xen/common/domain.c b/xen/common/domain.c
index 740163ee77..28d7903a96 100644
--- a/xen/common/domain.c
+++ b/xen/common/domain.c
@@ -1277,7 +1277,7 @@ long do_vcpu_op(int cmd, unsigned int vcpuid, 
XEN_GUEST_HANDLE_PARAM(void) arg)
 
         rc = arch_initialise_vcpu(v, arg);
         if ( rc == -ERESTART )
-            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iuh",
+            rc = hypercall_create_continuation(__HYPERVISOR_vcpu_op, "iih",
                                                cmd, vcpuid, arg);
 
         break;
--
generated by git-patchbot for /home/xen/git/xen.git#staging-4.8

_______________________________________________
Xen-changelog mailing list
Xen-changelog@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/xen-changelog

 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.