xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] arm: implement hypercall continuations
@ 2012-07-19  9:35 Ian Campbell
  2012-07-20 12:17 ` Stefano Stabellini
  0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2012-07-19  9:35 UTC (permalink / raw)
  To: xen-devel; +Cc: Ian Campbell

Largely cribbed from x86, register names differ and the return value is r0 ==
the first argument rather than the hypercall number (which is r12).

Multicall variant is untested, arms do_multicall_call is currently a BUG() so
we obviously don't use that yet. I have left a BUG in the hypercall
continuation path too since it will need validation once multicalls are
implemented.

Since the multicall state is local we do not need a globally atomic
{test,set}_bit. However we do need to be atomic WRT interrupts so can't just
use the naive RMW version. Stick with the global atomic implementation for now
but keep the __ as documentaion of the intention.

We cannot clobber all argument registers in a debug build anymore
because continuations expect them to be preserved. Add nr_args field to the
hypercall dispatch array and use it to only clobber the unused hypercall
arguments.

While debugging this I noticed that hypercall dispatch will happily run off the
end of the hypercall dispatch array, add a suitable bounds check. Also make the
use of an hvc immediate argument other than XEN_HYPERCALL_TAG fatal to the
guest.

Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>
---
 xen/arch/arm/domain.c        |   87 ++++++++++++++++++++++++++++++++++++++++++
 xen/arch/arm/dummy.S         |    1 -
 xen/arch/arm/traps.c         |   61 ++++++++++++++++++-----------
 xen/include/asm-arm/bitops.h |    9 ++++
 4 files changed, 134 insertions(+), 24 deletions(-)

diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
index f61568b..80be0cd 100644
--- a/xen/arch/arm/domain.c
+++ b/xen/arch/arm/domain.c
@@ -5,6 +5,7 @@
 #include <xen/softirq.h>
 #include <xen/wait.h>
 #include <xen/errno.h>
+#include <xen/bitops.h>
 
 #include <asm/current.h>
 #include <asm/regs.h>
@@ -224,6 +225,92 @@ void sync_vcpu_execstate(struct vcpu *v)
     /* Nothing to do -- no lazy switching */
 }
 
+#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();                                             \
+    }                                                                       \
+    __arg;                                                                  \
+})
+
+void hypercall_cancel_continuation(void)
+{
+    struct cpu_user_regs *regs = guest_cpu_user_regs();
+    struct mc_state *mcs = &current->mc_state;
+
+    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
+    {
+        __clear_bit(_MCSF_call_preempted, &mcs->flags);
+    }
+    else
+    {
+        regs->pc += 4; /* undo re-execute 'hvc #XEN_HYPERCALL_TAG' */
+    }
+}
+
+unsigned long hypercall_create_continuation(
+    unsigned int op, const char *format, ...)
+{
+    struct mc_state *mcs = &current->mc_state;
+    struct cpu_user_regs *regs;
+    const char *p = format;
+    unsigned long arg, rc;
+    unsigned int i;
+    va_list args;
+
+    /* All hypercalls take at least one argument */
+    BUG_ON( !p || *p == '\0' );
+
+    va_start(args, format);
+
+    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
+    {
+        BUG(); /* XXX multicalls not implemented yet. */
+
+        __set_bit(_MCSF_call_preempted, &mcs->flags);
+
+        for ( i = 0; *p != '\0'; i++ )
+            mcs->call.args[i] = next_arg(p, args);
+
+        /* Return value gets written back to mcs->call.result */
+        rc = mcs->call.result;
+    }
+    else
+    {
+        regs      = guest_cpu_user_regs();
+        regs->r12 = op;
+
+        /* Ensure the hypercall trap instruction is re-executed. */
+        regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
+
+        for ( i = 0; *p != '\0'; i++ )
+        {
+            arg = next_arg(p, args);
+
+            switch ( i )
+            {
+            case 0: regs->r0 = arg;       break;
+            case 1: regs->r1 = arg; break;
+            case 2: regs->r2 = arg; break;
+            case 3: regs->r3 = arg; break;
+            case 4: regs->r4 = arg; break;
+            case 5: regs->r5 = arg; break;
+            }
+        }
+
+        /* Return value gets written back to r0 */
+        rc = regs->r0;
+    }
+
+    va_end(args);
+
+    return rc;
+}
+
 void startup_cpu_idle_loop(void)
 {
     struct vcpu *v = current;
diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
index cab9522..5406077 100644
--- a/xen/arch/arm/dummy.S
+++ b/xen/arch/arm/dummy.S
@@ -46,7 +46,6 @@ DUMMY(domain_relinquish_resources);
 DUMMY(domain_set_time_offset);
 DUMMY(dom_cow);
 DUMMY(gmfn_to_mfn);
-DUMMY(hypercall_create_continuation);
 DUMMY(send_timer_event);
 DUMMY(share_xen_page_with_privileged_guests);
 DUMMY(wallclock_time);
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index f6e6807..50b62c0 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -413,24 +413,31 @@ unsigned long do_arch_0(unsigned int cmd, unsigned long long value)
         return 0;
 }
 
-typedef unsigned long arm_hypercall_t(
+typedef unsigned long (*arm_hypercall_fn_t)(
     unsigned int, unsigned int, unsigned int, unsigned int, unsigned int);
 
-#define HYPERCALL(x)                                        \
-    [ __HYPERVISOR_ ## x ] = (arm_hypercall_t *) do_ ## x
-
-static arm_hypercall_t *arm_hypercall_table[] = {
-    HYPERCALL(memory_op),
-    HYPERCALL(domctl),
-    HYPERCALL(arch_0),
-    HYPERCALL(sched_op),
-    HYPERCALL(console_io),
-    HYPERCALL(xen_version),
-    HYPERCALL(event_channel_op),
-    HYPERCALL(memory_op),
-    HYPERCALL(physdev_op),
-    HYPERCALL(sysctl),
-    HYPERCALL(hvm_op),
+typedef struct {
+    arm_hypercall_fn_t fn;
+    int nr_args;
+} arm_hypercall_t;
+
+#define HYPERCALL(_name, _nr_args)                                   \
+    [ __HYPERVISOR_ ## _name ] =  {                                  \
+        .fn = (arm_hypercall_fn_t) &do_ ## _name,                    \
+        .nr_args = _nr_args,                                         \
+    }
+
+static arm_hypercall_t arm_hypercall_table[] = {
+    HYPERCALL(memory_op, 2),
+    HYPERCALL(domctl, 1),
+    HYPERCALL(arch_0, 2),
+    HYPERCALL(sched_op, 2),
+    HYPERCALL(console_io, 3),
+    HYPERCALL(xen_version, 2),
+    HYPERCALL(event_channel_op, 2),
+    HYPERCALL(physdev_op, 2),
+    HYPERCALL(sysctl, 2),
+    HYPERCALL(hvm_op, 2),
 };
 
 static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
@@ -462,17 +469,18 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
 
 static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
 {
-    arm_hypercall_t *call = NULL;
+    arm_hypercall_fn_t call = NULL;
 
     if ( iss != XEN_HYPERCALL_TAG )
+        domain_crash_synchronous();
+
+    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
     {
-        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
-                __LINE__ , iss);
-        regs->r0 = -EINVAL;
+        regs->r0 = -ENOSYS;
         return;
     }
 
-    call = arm_hypercall_table[regs->r12];
+    call = arm_hypercall_table[regs->r12].fn;
     if ( call == NULL )
     {
         regs->r0 = -ENOSYS;
@@ -482,8 +490,15 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
     regs->r0 = call(regs->r0, regs->r1, regs->r2, regs->r3, regs->r4);
 
 #ifndef NDEBUG
-    /* clobber registers */
-    regs->r1 = regs->r2 = regs->r3 = regs->r4 = regs->r12 = 0xDEADBEEF;
+    /* clobber non-argument registers */
+    switch ( arm_hypercall_table[regs->r12].nr_args ) {
+    case 1: regs->r1 = 0xDEADBEEF;
+    case 2: regs->r2 = 0xDEADBEEF;
+    case 3: regs->r3 = 0xDEADBEEF;
+    case 4: regs->r4 = 0xDEADBEEF;
+            break;
+    default: BUG();
+    }
 #endif
 }
 
diff --git a/xen/include/asm-arm/bitops.h b/xen/include/asm-arm/bitops.h
index e5c1781..87de5db 100644
--- a/xen/include/asm-arm/bitops.h
+++ b/xen/include/asm-arm/bitops.h
@@ -23,6 +23,15 @@ extern int _test_and_change_bit(int nr, volatile void * p);
 #define test_and_clear_bit(n,p)   _test_and_clear_bit(n,p)
 #define test_and_change_bit(n,p)  _test_and_change_bit(n,p)
 
+/*
+ * Non-atomic bit manipulation.
+ *
+ * Implemented using atomics to be interrupt safe. Could alternatively
+ * implement with local interrupt masking.
+ */
+#define __set_bit(n,p)            _set_bit(n,p)
+#define __clear_bit(n,p)          _clear_bit(n,p)
+
 #define BIT(nr)                 (1UL << (nr))
 #define BIT_MASK(nr)            (1UL << ((nr) % BITS_PER_LONG))
 #define BIT_WORD(nr)            ((nr) / BITS_PER_LONG)
-- 
1.7.9.1

^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm: implement hypercall continuations
  2012-07-19  9:35 [PATCH] arm: implement hypercall continuations Ian Campbell
@ 2012-07-20 12:17 ` Stefano Stabellini
  2012-07-20 13:23   ` Ian Campbell
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Stabellini @ 2012-07-20 12:17 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel@lists.xen.org

On Thu, 19 Jul 2012, Ian Campbell wrote:
> Largely cribbed from x86, register names differ and the return value is r0 ==
> the first argument rather than the hypercall number (which is r12).
> 
> Multicall variant is untested, arms do_multicall_call is currently a BUG() so
> we obviously don't use that yet. I have left a BUG in the hypercall
> continuation path too since it will need validation once multicalls are
> implemented.
> 
> Since the multicall state is local we do not need a globally atomic
> {test,set}_bit. However we do need to be atomic WRT interrupts so can't just
> use the naive RMW version. Stick with the global atomic implementation for now
> but keep the __ as documentaion of the intention.
> 
> We cannot clobber all argument registers in a debug build anymore
> because continuations expect them to be preserved. Add nr_args field to the
> hypercall dispatch array and use it to only clobber the unused hypercall
> arguments.
> 
> While debugging this I noticed that hypercall dispatch will happily run off the
> end of the hypercall dispatch array, add a suitable bounds check. Also make the
> use of an hvc immediate argument other than XEN_HYPERCALL_TAG fatal to the
> guest.
> 
> Signed-off-by: Ian Campbell <Ian.Campbell@citrix.com>
> ---
>  xen/arch/arm/domain.c        |   87 ++++++++++++++++++++++++++++++++++++++++++
>  xen/arch/arm/dummy.S         |    1 -
>  xen/arch/arm/traps.c         |   61 ++++++++++++++++++-----------
>  xen/include/asm-arm/bitops.h |    9 ++++
>  4 files changed, 134 insertions(+), 24 deletions(-)
> 
> diff --git a/xen/arch/arm/domain.c b/xen/arch/arm/domain.c
> index f61568b..80be0cd 100644
> --- a/xen/arch/arm/domain.c
> +++ b/xen/arch/arm/domain.c
> @@ -5,6 +5,7 @@
>  #include <xen/softirq.h>
>  #include <xen/wait.h>
>  #include <xen/errno.h>
> +#include <xen/bitops.h>
>  
>  #include <asm/current.h>
>  #include <asm/regs.h>
> @@ -224,6 +225,92 @@ void sync_vcpu_execstate(struct vcpu *v)
>      /* Nothing to do -- no lazy switching */
>  }
>  
> +#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();                                             \
> +    }                                                                       \
> +    __arg;                                                                  \
> +})
> +
> +void hypercall_cancel_continuation(void)
> +{
> +    struct cpu_user_regs *regs = guest_cpu_user_regs();
> +    struct mc_state *mcs = &current->mc_state;
> +
> +    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
> +    {
> +        __clear_bit(_MCSF_call_preempted, &mcs->flags);
> +    }
> +    else
> +    {
> +        regs->pc += 4; /* undo re-execute 'hvc #XEN_HYPERCALL_TAG' */
> +    }
> +}
> +
> +unsigned long hypercall_create_continuation(
> +    unsigned int op, const char *format, ...)
> +{
> +    struct mc_state *mcs = &current->mc_state;
> +    struct cpu_user_regs *regs;
> +    const char *p = format;
> +    unsigned long arg, rc;
> +    unsigned int i;
> +    va_list args;
> +
> +    /* All hypercalls take at least one argument */
> +    BUG_ON( !p || *p == '\0' );
> +
> +    va_start(args, format);
> +
> +    if ( test_bit(_MCSF_in_multicall, &mcs->flags) )
> +    {
> +        BUG(); /* XXX multicalls not implemented yet. */
> +
> +        __set_bit(_MCSF_call_preempted, &mcs->flags);
> +
> +        for ( i = 0; *p != '\0'; i++ )
> +            mcs->call.args[i] = next_arg(p, args);
> +
> +        /* Return value gets written back to mcs->call.result */
> +        rc = mcs->call.result;
> +    }
> +    else
> +    {
> +        regs      = guest_cpu_user_regs();
> +        regs->r12 = op;
> +
> +        /* Ensure the hypercall trap instruction is re-executed. */
> +        regs->pc -= 4;  /* re-execute 'hvc #XEN_HYPERCALL_TAG' */
> +
> +        for ( i = 0; *p != '\0'; i++ )
> +        {
> +            arg = next_arg(p, args);
> +
> +            switch ( i )
> +            {
> +            case 0: regs->r0 = arg;       break;

wrong alignment


> +            case 1: regs->r1 = arg; break;
> +            case 2: regs->r2 = arg; break;
> +            case 3: regs->r3 = arg; break;
> +            case 4: regs->r4 = arg; break;
> +            case 5: regs->r5 = arg; break;
> +            }
> +        }
> +
> +        /* Return value gets written back to r0 */
> +        rc = regs->r0;
> +    }
> +
> +    va_end(args);
> +
> +    return rc;
> +}
> +
>  void startup_cpu_idle_loop(void)
>  {
>      struct vcpu *v = current;
> diff --git a/xen/arch/arm/dummy.S b/xen/arch/arm/dummy.S
> index cab9522..5406077 100644
> --- a/xen/arch/arm/dummy.S
> +++ b/xen/arch/arm/dummy.S
> @@ -46,7 +46,6 @@ DUMMY(domain_relinquish_resources);
>  DUMMY(domain_set_time_offset);
>  DUMMY(dom_cow);
>  DUMMY(gmfn_to_mfn);
> -DUMMY(hypercall_create_continuation);
>  DUMMY(send_timer_event);
>  DUMMY(share_xen_page_with_privileged_guests);
>  DUMMY(wallclock_time);
> diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
> index f6e6807..50b62c0 100644
> --- a/xen/arch/arm/traps.c
> +++ b/xen/arch/arm/traps.c
> @@ -413,24 +413,31 @@ unsigned long do_arch_0(unsigned int cmd, unsigned long long value)
>          return 0;
>  }
>  
> -typedef unsigned long arm_hypercall_t(
> +typedef unsigned long (*arm_hypercall_fn_t)(
>      unsigned int, unsigned int, unsigned int, unsigned int, unsigned int);
>  
> -#define HYPERCALL(x)                                        \
> -    [ __HYPERVISOR_ ## x ] = (arm_hypercall_t *) do_ ## x
> -
> -static arm_hypercall_t *arm_hypercall_table[] = {
> -    HYPERCALL(memory_op),
> -    HYPERCALL(domctl),
> -    HYPERCALL(arch_0),
> -    HYPERCALL(sched_op),
> -    HYPERCALL(console_io),
> -    HYPERCALL(xen_version),
> -    HYPERCALL(event_channel_op),
> -    HYPERCALL(memory_op),
> -    HYPERCALL(physdev_op),
> -    HYPERCALL(sysctl),
> -    HYPERCALL(hvm_op),
> +typedef struct {
> +    arm_hypercall_fn_t fn;
> +    int nr_args;
> +} arm_hypercall_t;
> +
> +#define HYPERCALL(_name, _nr_args)                                   \
> +    [ __HYPERVISOR_ ## _name ] =  {                                  \
> +        .fn = (arm_hypercall_fn_t) &do_ ## _name,                    \
> +        .nr_args = _nr_args,                                         \
> +    }
> +
> +static arm_hypercall_t arm_hypercall_table[] = {
> +    HYPERCALL(memory_op, 2),
> +    HYPERCALL(domctl, 1),
> +    HYPERCALL(arch_0, 2),
> +    HYPERCALL(sched_op, 2),
> +    HYPERCALL(console_io, 3),
> +    HYPERCALL(xen_version, 2),
> +    HYPERCALL(event_channel_op, 2),
> +    HYPERCALL(physdev_op, 2),
> +    HYPERCALL(sysctl, 2),
> +    HYPERCALL(hvm_op, 2),
>  };
>  
>  static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> @@ -462,17 +469,18 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
>  
>  static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
>  {
> -    arm_hypercall_t *call = NULL;
> +    arm_hypercall_fn_t call = NULL;
>  
>      if ( iss != XEN_HYPERCALL_TAG )
> +        domain_crash_synchronous();

Why did you change the behavior of the iss != XEN_HYPERCALL_TAG case?


> +    if ( regs->r12 > ARRAY_SIZE(arm_hypercall_table) )
>      {
> -        printk("%s %d: received an alien hypercall iss=%lx\n", __func__ ,
> -                __LINE__ , iss);
> -        regs->r0 = -EINVAL;
> +        regs->r0 = -ENOSYS;
>          return;
>      }
>  
> -    call = arm_hypercall_table[regs->r12];
> +    call = arm_hypercall_table[regs->r12].fn;
>      if ( call == NULL )
>      {
>          regs->r0 = -ENOSYS;
> @@ -482,8 +490,15 @@ static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
>      regs->r0 = call(regs->r0, regs->r1, regs->r2, regs->r3, regs->r4);
>  
>  #ifndef NDEBUG
> -    /* clobber registers */
> -    regs->r1 = regs->r2 = regs->r3 = regs->r4 = regs->r12 = 0xDEADBEEF;
> +    /* clobber non-argument registers */
> +    switch ( arm_hypercall_table[regs->r12].nr_args ) {
> +    case 1: regs->r1 = 0xDEADBEEF;
> +    case 2: regs->r2 = 0xDEADBEEF;
> +    case 3: regs->r3 = 0xDEADBEEF;
> +    case 4: regs->r4 = 0xDEADBEEF;
> +            break;
> +    default: BUG();
> +    }
>  #endif
>  }

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm: implement hypercall continuations
  2012-07-20 12:17 ` Stefano Stabellini
@ 2012-07-20 13:23   ` Ian Campbell
  2012-07-20 13:28     ` Stefano Stabellini
  0 siblings, 1 reply; 5+ messages in thread
From: Ian Campbell @ 2012-07-20 13:23 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel@lists.xen.org


> > +        for ( i = 0; *p != '\0'; i++ )
> > +        {
> > +            arg = next_arg(p, args);
> > +
> > +            switch ( i )
> > +            {
> > +            case 0: regs->r0 = arg;       break;
> 
> wrong alignment

I had rc = arg and lined it up then chaned it back without realigning,
thanks for pointing it out.

> > @@ -462,17 +469,18 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> >  
> >  static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> >  {
> > -    arm_hypercall_t *call = NULL;
> > +    arm_hypercall_fn_t call = NULL;
> >  
> >      if ( iss != XEN_HYPERCALL_TAG )
> > +        domain_crash_synchronous();
> 
> Why did you change the behavior of the iss != XEN_HYPERCALL_TAG case?

I just noticed it while adding the bounds check. A guest which makes a
hypercall with the wrong tag is either malicious or about to fail
horribly, there's no reason to allow them to keep living.

Ian.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm: implement hypercall continuations
  2012-07-20 13:23   ` Ian Campbell
@ 2012-07-20 13:28     ` Stefano Stabellini
  2012-07-20 13:40       ` Ian Campbell
  0 siblings, 1 reply; 5+ messages in thread
From: Stefano Stabellini @ 2012-07-20 13:28 UTC (permalink / raw)
  To: Ian Campbell; +Cc: xen-devel@lists.xen.org, Stefano Stabellini

On Fri, 20 Jul 2012, Ian Campbell wrote:
> > > +        for ( i = 0; *p != '\0'; i++ )
> > > +        {
> > > +            arg = next_arg(p, args);
> > > +
> > > +            switch ( i )
> > > +            {
> > > +            case 0: regs->r0 = arg;       break;
> > 
> > wrong alignment
> 
> I had rc = arg and lined it up then chaned it back without realigning,
> thanks for pointing it out.
> 
> > > @@ -462,17 +469,18 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> > >  
> > >  static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> > >  {
> > > -    arm_hypercall_t *call = NULL;
> > > +    arm_hypercall_fn_t call = NULL;
> > >  
> > >      if ( iss != XEN_HYPERCALL_TAG )
> > > +        domain_crash_synchronous();
> > 
> > Why did you change the behavior of the iss != XEN_HYPERCALL_TAG case?
> 
> I just noticed it while adding the bounds check. A guest which makes a
> hypercall with the wrong tag is either malicious or about to fail
> horribly, there's no reason to allow them to keep living.

I don't think so: it could just be a misconfigured guest, trying to
initialize KVM support before Xen.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH] arm: implement hypercall continuations
  2012-07-20 13:28     ` Stefano Stabellini
@ 2012-07-20 13:40       ` Ian Campbell
  0 siblings, 0 replies; 5+ messages in thread
From: Ian Campbell @ 2012-07-20 13:40 UTC (permalink / raw)
  To: Stefano Stabellini; +Cc: xen-devel@lists.xen.org

On Fri, 2012-07-20 at 14:28 +0100, Stefano Stabellini wrote:
> On Fri, 20 Jul 2012, Ian Campbell wrote:
> > > > +        for ( i = 0; *p != '\0'; i++ )
> > > > +        {
> > > > +            arg = next_arg(p, args);
> > > > +
> > > > +            switch ( i )
> > > > +            {
> > > > +            case 0: regs->r0 = arg;       break;
> > > 
> > > wrong alignment
> > 
> > I had rc = arg and lined it up then chaned it back without realigning,
> > thanks for pointing it out.
> > 
> > > > @@ -462,17 +469,18 @@ static void do_debug_trap(struct cpu_user_regs *regs, unsigned int code)
> > > >  
> > > >  static void do_trap_hypercall(struct cpu_user_regs *regs, unsigned long iss)
> > > >  {
> > > > -    arm_hypercall_t *call = NULL;
> > > > +    arm_hypercall_fn_t call = NULL;
> > > >  
> > > >      if ( iss != XEN_HYPERCALL_TAG )
> > > > +        domain_crash_synchronous();
> > > 
> > > Why did you change the behavior of the iss != XEN_HYPERCALL_TAG case?
> > 
> > I just noticed it while adding the bounds check. A guest which makes a
> > hypercall with the wrong tag is either malicious or about to fail
> > horribly, there's no reason to allow them to keep living.
> 
> I don't think so: it could just be a misconfigured guest, trying to
> initialize KVM support before Xen.

Or it could be some other guest doing something else entirely, which
we've never heard of and with a different semantics for ENOSYS type
return values etc.

It is clearly bogus for a guest to be making a KVM hypercall on Xen (and
vice versa). We should provide a reliable way to detect the exact
hypervisor and enforce its use.

Ian.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2012-07-20 13:40 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-07-19  9:35 [PATCH] arm: implement hypercall continuations Ian Campbell
2012-07-20 12:17 ` Stefano Stabellini
2012-07-20 13:23   ` Ian Campbell
2012-07-20 13:28     ` Stefano Stabellini
2012-07-20 13:40       ` Ian Campbell

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).