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 = ¤t->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 = ¤t->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
Stefano Stabellini
2012-Jul-20 12:17 UTC
Re: [PATCH] arm: implement hypercall continuations
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 = ¤t->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 = ¤t->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 > }
> > + for ( i = 0; *p != ''\0''; i++ ) > > + { > > + arg = next_arg(p, args); > > + > > + switch ( i ) > > + { > > + case 0: regs->r0 = arg; break; > > wrong alignmentI 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.
Stefano Stabellini
2012-Jul-20 13:28 UTC
Re: [PATCH] arm: implement hypercall continuations
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.
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.