[PATCH] xen: arm: fully implement multicall interface.

[PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
implement do_multicall_call for both 32 and 64-bit" but it is obviously
insufficient since it doesn't actually wire up the hypercall.

Before doing so we need to make the usual adjustments for ARM and turn the
unsigned longs into xen_ulong_t. There is no difference in the resulting
structure for x86.

There are knock on changes to the trace interface, but again they are nops on
x86.

In the interests of clarity and always using explicitly sized types change the
unsigned int in the hypercall arguments to a uint32_t. There is no actual
change here on any platform.

We should consider backporting this to 4.4.1 in case a guest decides they want
to use a multicall in common code e.g. I suggested such a thing while
reviewing a netback change recently.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: keir@xen.org
Cc: jbeulich@suse.com
Cc: George Dunlap <george.dunlap@eu.citrix.com>
---
 xen/arch/arm/traps.c     |    1 +
 xen/common/multicall.c   |    4 ++--
 xen/common/trace.c       |    2 +-
 xen/include/public/xen.h |    6 +++---
 xen/include/xen/trace.h  |    2 +-
 5 files changed, 8 insertions(+), 7 deletions(-)

diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c
index ec43e65..ca315af 100644
--- a/xen/arch/arm/traps.c
+++ b/xen/arch/arm/traps.c
@@ -1011,6 +1011,7 @@ static arm_hypercall_t arm_hypercall_table[] = {
     HYPERCALL(sysctl, 2),
     HYPERCALL(hvm_op, 2),
     HYPERCALL(grant_table_op, 3),
+    HYPERCALL(multicall, 2),
     HYPERCALL_ARM(vcpu_op, 3),
 };
 
diff --git a/xen/common/multicall.c b/xen/common/multicall.c
index e66c798..fa9d910 100644
--- a/xen/common/multicall.c
+++ b/xen/common/multicall.c
@@ -35,10 +35,10 @@ static void trace_multicall_call(multicall_entry_t *call)
 
 ret_t
 do_multicall(
-    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, unsigned int nr_calls)
+    XEN_GUEST_HANDLE_PARAM(multicall_entry_t) call_list, uint32_t nr_calls)
 {
     struct mc_state *mcs = &current->mc_state;
-    unsigned int     i;
+    uint32_t         i;
     int              rc = 0;
 
     if ( unlikely(__test_and_set_bit(_MCSF_in_multicall, &mcs->flags)) )
diff --git a/xen/common/trace.c b/xen/common/trace.c
index 1814165..f651cf3 100644
--- a/xen/common/trace.c
+++ b/xen/common/trace.c
@@ -817,7 +817,7 @@ unlock:
 }
 
 void __trace_hypercall(uint32_t event, unsigned long op,
-                       const unsigned long *args)
+                       const xen_ulong_t *args)
 {
     struct __packed {
         uint32_t op;
diff --git a/xen/include/public/xen.h b/xen/include/public/xen.h
index 8c5697e..5bba3af 100644
--- a/xen/include/public/xen.h
+++ b/xen/include/public/xen.h
@@ -541,13 +541,13 @@ DEFINE_XEN_GUEST_HANDLE(mmu_update_t);
 /*
  * ` enum neg_errnoval
  * ` HYPERVISOR_multicall(multicall_entry_t call_list[],
- * `                      unsigned int nr_calls);
+ * `                      uint32_t nr_calls);
  *
  * NB. The fields are natural register size for this architecture.
  */
 struct multicall_entry {
-    unsigned long op, result;
-    unsigned long args[6];
+    xen_ulong_t op, result;
+    xen_ulong_t args[6];
 };
 typedef struct multicall_entry multicall_entry_t;
 DEFINE_XEN_GUEST_HANDLE(multicall_entry_t);
diff --git a/xen/include/xen/trace.h b/xen/include/xen/trace.h
index 3b8a7b3..12966ea 100644
--- a/xen/include/xen/trace.h
+++ b/xen/include/xen/trace.h
@@ -45,7 +45,7 @@ static inline void trace_var(u32 event, int cycles, int extra,
 }
 
 void __trace_hypercall(uint32_t event, unsigned long op,
-                       const unsigned long *args);
+                       const xen_ulong_t *args);
 
 /* Convenience macros for calling the trace function. */
 #define TRACE_0D(_e)                            \
-- 
1.7.10.4

[PATCH] arm: xen: implement multicall hypercall support.

[Ian Campbell]

As part of this make the usual change to xen_ulong_t in place of unsigned long.
This change has no impact on x86.

The Linux defintion of struct multicall_entry.result differs from the Xen
definition, I think for good reasons, and used a long rather than an unsigned
long. Therefore introduce a xen_long_t, which is a long on x86 architectures
and a signed 64-bit integer on ARM.

Build tested on amd64 and i386 builds. Runtime tested on ARM.

Signed-off-by: Ian Campbell <ian.campbell@citrix.com>
Cc: Stefano Stabellini <stefano.stabellini@eu.citrix.com>
Cc: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>
Cc: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Cc: David Vrabel <david.vrabel@citrix.com>
---
Tested on ARM with a stupid patch:
	diff --git a/arch/arm/xen/enlighten.c b/arch/arm/xen/enlighten.c
	index b96723e..61c6b94 100644
	--- a/arch/arm/xen/enlighten.c
	+++ b/arch/arm/xen/enlighten.c
	@@ -339,6 +339,36 @@ static int __init xen_pm_init(void)
	 }
	 late_initcall(xen_pm_init);

	+static int __init multicall_test(void)
	+{
	+	struct multicall_entry call[2];
	+	const char *str0 = "This is the first debug string\n";
	+	const char *str1 = "This is the second debug string\n";
	+	int ret;
	+
	+	call[0] = (struct multicall_entry) {
	+		.op = __HYPERVISOR_console_io,
	+		.args[0] = CONSOLEIO_write,
	+		.args[1] = strlen(str0),
	+		.args[2] = (uintptr_t)str0
	+	};
	+	call[1] = (struct multicall_entry) {
	+		.op = __HYPERVISOR_console_io,
	+		.args[0] = CONSOLEIO_write,
	+		.args[1] = strlen(str1),
	+		.args[2] = (uintptr_t)str1
	+	};
	+
	+	ret = HYPERVISOR_multicall(call, ARRAY_SIZE(call));
	+	printk("MULTICALL returned %d\n", ret);
	+	if (ret == 0) {
	+		printk("call[0].result = %"PRI_xen_long"\n", call[0].result);
	+		printk("call[1].result = %"PRI_xen_long"\n", call[1].result);
	+	}
	+	return 0;
	+}
	+late_initcall(multicall_test);
	+
	 /* In the hypervisor.S file. */
	 EXPORT_SYMBOL_GPL(HYPERVISOR_event_channel_op);
	 EXPORT_SYMBOL_GPL(HYPERVISOR_grant_table_op);
---
 arch/arm/include/asm/xen/hypercall.h |    6 +-----
 arch/arm/include/asm/xen/interface.h |    2 ++
 arch/arm/xen/hypercall.S             |    1 +
 arch/x86/include/asm/xen/interface.h |    3 +++
 include/xen/interface/xen.h          |    6 +++---
 5 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
index 7704e28..7658150 100644
--- a/arch/arm/include/asm/xen/hypercall.h
+++ b/arch/arm/include/asm/xen/hypercall.h
@@ -48,6 +48,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
 int HYPERVISOR_physdev_op(int cmd, void *arg);
 int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
 int HYPERVISOR_tmem_op(void *arg);
+int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
 
 static inline void
 MULTI_update_va_mapping(struct multicall_entry *mcl, unsigned long va,
@@ -63,9 +64,4 @@ MULTI_mmu_update(struct multicall_entry *mcl, struct mmu_update *req,
 	BUG();
 }
 
-static inline int
-HYPERVISOR_multicall(void *call_list, int nr_calls)
-{
-	BUG();
-}
 #endif /* _ASM_ARM_XEN_HYPERCALL_H */
diff --git a/arch/arm/include/asm/xen/interface.h b/arch/arm/include/asm/xen/interface.h
index 1151188..5006600 100644
--- a/arch/arm/include/asm/xen/interface.h
+++ b/arch/arm/include/asm/xen/interface.h
@@ -40,6 +40,8 @@ typedef uint64_t xen_pfn_t;
 #define PRI_xen_pfn "llx"
 typedef uint64_t xen_ulong_t;
 #define PRI_xen_ulong "llx"
+typedef int64_t xen_long_t;
+#define PRI_xen_long "llx"
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/arch/arm/xen/hypercall.S b/arch/arm/xen/hypercall.S
index d1cf7b7..44e3a5f 100644
--- a/arch/arm/xen/hypercall.S
+++ b/arch/arm/xen/hypercall.S
@@ -89,6 +89,7 @@ HYPERCALL2(memory_op);
 HYPERCALL2(physdev_op);
 HYPERCALL3(vcpu_op);
 HYPERCALL1(tmem_op);
+HYPERCALL2(multicall);
 
 ENTRY(privcmd_call)
 	stmdb sp!, {r4}
diff --git a/arch/x86/include/asm/xen/interface.h b/arch/x86/include/asm/xen/interface.h
index fd9cb76..3400dba 100644
--- a/arch/x86/include/asm/xen/interface.h
+++ b/arch/x86/include/asm/xen/interface.h
@@ -54,6 +54,9 @@ typedef unsigned long xen_pfn_t;
 #define PRI_xen_pfn "lx"
 typedef unsigned long xen_ulong_t;
 #define PRI_xen_ulong "lx"
+typedef long xen_long_t;
+#define PRI_xen_long "lx"
+
 /* Guest handles for primitive C types. */
 __DEFINE_GUEST_HANDLE(uchar, unsigned char);
 __DEFINE_GUEST_HANDLE(uint,  unsigned int);
diff --git a/include/xen/interface/xen.h b/include/xen/interface/xen.h
index 0cd5ca3..de08213 100644
--- a/include/xen/interface/xen.h
+++ b/include/xen/interface/xen.h
@@ -275,9 +275,9 @@ DEFINE_GUEST_HANDLE_STRUCT(mmu_update);
  * NB. The fields are natural register size for this architecture.
  */
 struct multicall_entry {
-    unsigned long op;
-    long result;
-    unsigned long args[6];
+    xen_ulong_t op;
+    xen_long_t result;
+    xen_ulong_t args[6];
 };
 DEFINE_GUEST_HANDLE_STRUCT(multicall_entry);
 
-- 
1.7.10.4

Re: [PATCH] arm: xen: implement multicall hypercall support.

[Julien Grall]

Hi Ian,

On 03/28/2014 02:07 PM, Ian Campbell wrote:
> As part of this make the usual change to xen_ulong_t in place of unsigned long.
> This change has no impact on x86.
> 
> The Linux defintion of struct multicall_entry.result differs from the Xen

definition

[..]

> diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> index 7704e28..7658150 100644
> --- a/arch/arm/include/asm/xen/hypercall.h
> +++ b/arch/arm/include/asm/xen/hypercall.h
> @@ -48,6 +48,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
>  int HYPERVISOR_physdev_op(int cmd, void *arg);
>  int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
>  int HYPERVISOR_tmem_op(void *arg);
> +int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);

The x86 prototype of HYPERVISOR_multicall takes an int as 2nd parameter.
I guess we should stay consistent here.

Regards,

-- 
Julien Grall

Re: [PATCH] arm: xen: implement multicall hypercall support.

[Ian Campbell]

On Fri, 2014-03-28 at 14:51 +0000, Julien Grall wrote:
> > diff --git a/arch/arm/include/asm/xen/hypercall.h b/arch/arm/include/asm/xen/hypercall.h
> > index 7704e28..7658150 100644
> > --- a/arch/arm/include/asm/xen/hypercall.h
> > +++ b/arch/arm/include/asm/xen/hypercall.h
> > @@ -48,6 +48,7 @@ int HYPERVISOR_memory_op(unsigned int cmd, void *arg);
> >  int HYPERVISOR_physdev_op(int cmd, void *arg);
> >  int HYPERVISOR_vcpu_op(int cmd, int vcpuid, void *extra_args);
> >  int HYPERVISOR_tmem_op(void *arg);
> > +int HYPERVISOR_multicall(struct multicall_entry *calls, uint32_t nr);
> 
> The x86 prototype of HYPERVISOR_multicall takes an int as 2nd parameter.
> I guess we should stay consistent here.

Which I'd prefer to do by changing x86 for consistency with the Xen side
of things, unless there are any objections.

Ian.

Re: [PATCH] arm: xen: implement multicall hypercall support.

[David Vrabel]

On 28/03/14 14:07, Ian Campbell wrote:
> As part of this make the usual change to xen_ulong_t in place of unsigned long.
> This change has no impact on x86.
> 
> The Linux defintion of struct multicall_entry.result differs from the Xen
> definition, I think for good reasons, and used a long rather than an unsigned
> long. Therefore introduce a xen_long_t, which is a long on x86 architectures
> and a signed 64-bit integer on ARM.

Shouldn't the Xen definition also be changed to use a long?

David

Re: [PATCH] arm: xen: implement multicall hypercall support.

[Ian Campbell]

On Tue, 2014-04-01 at 12:02 +0100, David Vrabel wrote:
> On 28/03/14 14:07, Ian Campbell wrote:
> > As part of this make the usual change to xen_ulong_t in place of unsigned long.
> > This change has no impact on x86.
> > 
> > The Linux defintion of struct multicall_entry.result differs from the Xen
> > definition, I think for good reasons, and used a long rather than an unsigned
> > long. Therefore introduce a xen_long_t, which is a long on x86 architectures
> > and a signed 64-bit integer on ARM.
> 
> Shouldn't the Xen definition also be changed to use a long?

Maybe?

It seems plausible that this would be the right thing to do if someone
wants.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 03/28/2014 02:07 PM, Ian Campbell wrote:
> I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
> implement do_multicall_call for both 32 and 64-bit" but it is obviously
> insufficient since it doesn't actually wire up the hypercall.
> 
> Before doing so we need to make the usual adjustments for ARM and turn the
> unsigned longs into xen_ulong_t. There is no difference in the resulting
> structure for x86.
> 
> There are knock on changes to the trace interface, but again they are nops on
> x86.

I'm wondering if you also need to modify __trace_multicall_call in
common/compat/multicall.c. (I know it's not used by ARM...).

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Fri, 2014-03-28 at 14:22 +0000, Julien Grall wrote:
> On 03/28/2014 02:07 PM, Ian Campbell wrote:
> > I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
> > implement do_multicall_call for both 32 and 64-bit" but it is obviously
> > insufficient since it doesn't actually wire up the hypercall.
> > 
> > Before doing so we need to make the usual adjustments for ARM and turn the
> > unsigned longs into xen_ulong_t. There is no difference in the resulting
> > structure for x86.
> > 
> > There are knock on changes to the trace interface, but again they are nops on
> > x86.
> 
> I'm wondering if you also need to modify __trace_multicall_call in
> common/compat/multicall.c. (I know it's not used by ARM...).

I did build test x86_64, but perhaps I should make a change here anyway
for consistency. George/Jan?

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Jan Beulich]

>>> On 28.03.14 at 15:37, <Ian.Campbell@citrix.com> wrote:
> On Fri, 2014-03-28 at 14:22 +0000, Julien Grall wrote:
>> On 03/28/2014 02:07 PM, Ian Campbell wrote:
>> > I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
>> > implement do_multicall_call for both 32 and 64-bit" but it is obviously
>> > insufficient since it doesn't actually wire up the hypercall.
>> > 
>> > Before doing so we need to make the usual adjustments for ARM and turn the
>> > unsigned longs into xen_ulong_t. There is no difference in the resulting
>> > structure for x86.
>> > 
>> > There are knock on changes to the trace interface, but again they are nops 
> on
>> > x86.
>> 
>> I'm wondering if you also need to modify __trace_multicall_call in
>> common/compat/multicall.c. (I know it's not used by ARM...).
> 
> I did build test x86_64, but perhaps I should make a change here anyway
> for consistency. George/Jan?

It doesn't matter that much, but for consistency's sake it would seem
desirable.

Jan

Re: [PATCH] xen: arm: fully implement multicall interface.

[George Dunlap]

On 03/28/2014 03:03 PM, Jan Beulich wrote:
>>>> On 28.03.14 at 15:37, <Ian.Campbell@citrix.com> wrote:
>> On Fri, 2014-03-28 at 14:22 +0000, Julien Grall wrote:
>>> On 03/28/2014 02:07 PM, Ian Campbell wrote:
>>>> I'm not sure what I was smoking at the time of 5d74ad1a082e "xen: arm:
>>>> implement do_multicall_call for both 32 and 64-bit" but it is obviously
>>>> insufficient since it doesn't actually wire up the hypercall.
>>>>
>>>> Before doing so we need to make the usual adjustments for ARM and turn the
>>>> unsigned longs into xen_ulong_t. There is no difference in the resulting
>>>> structure for x86.
>>>>
>>>> There are knock on changes to the trace interface, but again they are nops
>> on
>>>> x86.
>>>
>>> I'm wondering if you also need to modify __trace_multicall_call in
>>> common/compat/multicall.c. (I know it's not used by ARM...).
>>
>> I did build test x86_64, but perhaps I should make a change here anyway
>> for consistency. George/Jan?
>
> It doesn't matter that much, but for consistency's sake it would seem
> desirable.

+1

  -G

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Fri, 2014-03-28 at 14:07 +0000, Ian Campbell wrote:=
> Before doing so we need to make the usual adjustments for ARM and turn
> the unsigned longs into xen_ulong_t.

After a discussion with Julien I'm wondering if the actual
do_multicall_call dispatcher (reproduced below) should be explicitly
truncating the args values from 64-bits to 32-bits for 32-bit guests,
since that is the actual size of hypercall arguments for a 32-bit guest.
When running on a 64-bit Xen the guest can only actually see the 32-bit
rN registers so for a normal hypercall their is an implicit truncation
in the h/w exception model.

This interface exposes a full 64-bit sized set of arguments even to
32-bit guests. On 32-bit Xen this is truncated by the call() which takes
register_t's, but this might hide latent issues in guest kernels. On
64-bit Xen those 64-bit values would be passed to the hypercall.

My feeling is that any (exploitable or otherwise) issue due to this
would be due to lack of proper error checking in the hypercall, and
would be equally accessible by a 64-bit guest.

I'm considering whether to add an #ifndef NDEBUG check here which will
reject a multicall from a 32-bit guest where any of the arguments
(arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
can't decide whether -EINVAL or domain_kill() would be more appropriate.
I'm actually leaning towards the latter.

Thoughts?

Ian.

void do_multicall_call(struct multicall_entry *multi)
{
    arm_hypercall_fn_t call = NULL;

    if ( multi->op >= ARRAY_SIZE(arm_hypercall_table) )
    {
        multi->result = -ENOSYS;
        return;
    }

    call = arm_hypercall_table[multi->op].fn;
    if ( call == NULL )
    {
        multi->result = -ENOSYS;
        return;
    }

    multi->result = call(multi->args[0], multi->args[1],
                        multi->args[2], multi->args[3],
                        multi->args[4]);
}

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 03/28/2014 02:45 PM, Ian Campbell wrote:

> My feeling is that any (exploitable or otherwise) issue due to this
> would be due to lack of proper error checking in the hypercall, and
> would be equally accessible by a 64-bit guest.

I don't think it's exploitable. IHMO, the main point is to give a useful
debug to the user rather than an obscure error message because the given
pointer is invalid (perhaps by mistake).

> I'm considering whether to add an #ifndef NDEBUG check here which will
> reject a multicall from a 32-bit guest where any of the arguments
> (arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
> can't decide whether -EINVAL or domain_kill() would be more appropriate.
> I'm actually leaning towards the latter.
> 
> Thoughts?

Killing the domain is a bit tough. But it seems that all failures in
trap.c result to crash the domain.

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Fri, 2014-03-28 at 15:01 +0000, Julien Grall wrote:
> On 03/28/2014 02:45 PM, Ian Campbell wrote:
> 
> > My feeling is that any (exploitable or otherwise) issue due to this
> > would be due to lack of proper error checking in the hypercall, and
> > would be equally accessible by a 64-bit guest.
> 
> I don't think it's exploitable. IHMO, the main point is to give a useful
> debug to the user rather than an obscure error message because the given
> pointer is invalid (perhaps by mistake).

Right.

We also want to avoid the case where it just happens to work in one
configuration (i.e. 32-on-32) but fails in another (i.e. 32-on-64).

> 
> > I'm considering whether to add an #ifndef NDEBUG check here which will
> > reject a multicall from a 32-bit guest where any of the arguments
> > (arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
> > can't decide whether -EINVAL or domain_kill() would be more appropriate.
> > I'm actually leaning towards the latter.
> > 
> > Thoughts?
> 
> Killing the domain is a bit tough.

Sure, but the point is to flush out 32-bit guests which make invalid
assumptions which will be broken when they run on 64-bit vs. 32-bit Xen.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[George Dunlap]

On 03/28/2014 03:07 PM, Ian Campbell wrote:
> On Fri, 2014-03-28 at 15:01 +0000, Julien Grall wrote:
>> On 03/28/2014 02:45 PM, Ian Campbell wrote:
>>
>>> My feeling is that any (exploitable or otherwise) issue due to this
>>> would be due to lack of proper error checking in the hypercall, and
>>> would be equally accessible by a 64-bit guest.
>> I don't think it's exploitable. IHMO, the main point is to give a useful
>> debug to the user rather than an obscure error message because the given
>> pointer is invalid (perhaps by mistake).
> Right.
>
> We also want to avoid the case where it just happens to work in one
> configuration (i.e. 32-on-32) but fails in another (i.e. 32-on-64).
>
>>> I'm considering whether to add an #ifndef NDEBUG check here which will
>>> reject a multicall from a 32-bit guest where any of the arguments
>>> (arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
>>> can't decide whether -EINVAL or domain_kill() would be more appropriate.
>>> I'm actually leaning towards the latter.
>>>
>>> Thoughts?
>> Killing the domain is a bit tough.
> Sure, but the point is to flush out 32-bit guests which make invalid
> assumptions which will be broken when they run on 64-bit vs. 32-bit Xen.

Perhaps return -EINVAL for NDEBUG, and kill if !NDEBUG?

We generally don't want to be killing production VMs unless absolutely 
necessary.  One would of course hope that we had caught any bugs during 
development, but things don't always work the way you'd think...

  -George

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 31/03/14 17:38, George Dunlap wrote:
> On 03/28/2014 03:07 PM, Ian Campbell wrote:
>> On Fri, 2014-03-28 at 15:01 +0000, Julien Grall wrote:
>>> On 03/28/2014 02:45 PM, Ian Campbell wrote:
>>>
>>>> My feeling is that any (exploitable or otherwise) issue due to this
>>>> would be due to lack of proper error checking in the hypercall, and
>>>> would be equally accessible by a 64-bit guest.
>>> I don't think it's exploitable. IHMO, the main point is to give a useful
>>> debug to the user rather than an obscure error message because the given
>>> pointer is invalid (perhaps by mistake).
>> Right.
>>
>> We also want to avoid the case where it just happens to work in one
>> configuration (i.e. 32-on-32) but fails in another (i.e. 32-on-64).
>>
>>>> I'm considering whether to add an #ifndef NDEBUG check here which will
>>>> reject a multicall from a 32-bit guest where any of the arguments
>>>> (arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
>>>> can't decide whether -EINVAL or domain_kill() would be more
>>>> appropriate.
>>>> I'm actually leaning towards the latter.
>>>>
>>>> Thoughts?
>>> Killing the domain is a bit tough.
>> Sure, but the point is to flush out 32-bit guests which make invalid
>> assumptions which will be broken when they run on 64-bit vs. 32-bit Xen.
>
> Perhaps return -EINVAL for NDEBUG, and kill if !NDEBUG?
>
> We generally don't want to be killing production VMs unless absolutely
> necessary.  One would of course hope that we had caught any bugs during
> development, but things don't always work the way you'd think...

Out-of-context, I've noticed that most of trap failure will kill the 
domain. From the ARM ARM , if a coprocessor instruction is failing, we 
should 	generate an Undefined Instruction exception (see P.7.5).

Regards,

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
> 
> On 31/03/14 17:38, George Dunlap wrote:
> > On 03/28/2014 03:07 PM, Ian Campbell wrote:
> >> On Fri, 2014-03-28 at 15:01 +0000, Julien Grall wrote:
> >>> On 03/28/2014 02:45 PM, Ian Campbell wrote:
> >>>
> >>>> My feeling is that any (exploitable or otherwise) issue due to this
> >>>> would be due to lack of proper error checking in the hypercall, and
> >>>> would be equally accessible by a 64-bit guest.
> >>> I don't think it's exploitable. IHMO, the main point is to give a useful
> >>> debug to the user rather than an obscure error message because the given
> >>> pointer is invalid (perhaps by mistake).
> >> Right.
> >>
> >> We also want to avoid the case where it just happens to work in one
> >> configuration (i.e. 32-on-32) but fails in another (i.e. 32-on-64).
> >>
> >>>> I'm considering whether to add an #ifndef NDEBUG check here which will
> >>>> reject a multicall from a 32-bit guest where any of the arguments
> >>>> (arm_hypercall_table[nr].nr_args) are non-zero in their top 32-bit. I
> >>>> can't decide whether -EINVAL or domain_kill() would be more
> >>>> appropriate.
> >>>> I'm actually leaning towards the latter.
> >>>>
> >>>> Thoughts?
> >>> Killing the domain is a bit tough.
> >> Sure, but the point is to flush out 32-bit guests which make invalid
> >> assumptions which will be broken when they run on 64-bit vs. 32-bit Xen.
> >
> > Perhaps return -EINVAL for NDEBUG, and kill if !NDEBUG?
> >
> > We generally don't want to be killing production VMs unless absolutely
> > necessary.  One would of course hope that we had caught any bugs during
> > development, but things don't always work the way you'd think...

More of often than not I'd expect that the guest would shoot itself if
one of these hypercalls failed.

But the approach you suggest is probably the best compromise.

> Out-of-context, I've noticed that most of trap failure will kill the 
> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
> should 	generate an Undefined Instruction exception (see P.7.5).

You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?

I've not checked but I think we only ask for traps for things which we
are supposed to be able to handle, so anything which is trapped which we
can't handle is a bug and would indicate the guest doing something
funky.

Now maybe that is wrong and we are asking for traps which we don't
handle, in which case we should either handle them or stop asking for
them.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 04/01/2014 10:28 AM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
>> Out-of-context, I've noticed that most of trap failure will kill the 
>> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
>> should 	generate an Undefined Instruction exception (see P.7.5).
> 
> You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
> 
> I've not checked but I think we only ask for traps for things which we
> are supposed to be able to handle, so anything which is trapped which we
> can't handle is a bug and would indicate the guest doing something
> funky.

Right, but if the emulation of the instruction fails (see
vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
of sending an UNDEF exception.

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-01 at 11:46 +0100, Julien Grall wrote:
> On 04/01/2014 10:28 AM, Ian Campbell wrote:
> > On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
> >> Out-of-context, I've noticed that most of trap failure will kill the 
> >> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
> >> should 	generate an Undefined Instruction exception (see P.7.5).
> > 
> > You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
> > 
> > I've not checked but I think we only ask for traps for things which we
> > are supposed to be able to handle, so anything which is trapped which we
> > can't handle is a bug and would indicate the guest doing something
> > funky.
> 
> Right, but if the emulation of the instruction fails (see
> vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
> of sending an UNDEF exception.

My point was that the emulation should never fail...

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 04/01/2014 11:49 AM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 11:46 +0100, Julien Grall wrote:
>> On 04/01/2014 10:28 AM, Ian Campbell wrote:
>>> On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
>>>> Out-of-context, I've noticed that most of trap failure will kill the 
>>>> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
>>>> should 	generate an Undefined Instruction exception (see P.7.5).
>>>
>>> You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
>>>
>>> I've not checked but I think we only ask for traps for things which we
>>> are supposed to be able to handle, so anything which is trapped which we
>>> can't handle is a bug and would indicate the guest doing something
>>> funky.
>>
>> Right, but if the emulation of the instruction fails (see
>> vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
>> of sending an UNDEF exception.
> 
> My point was that the emulation should never fail...

The emulation can fail if the guest decides to write on an RO register.

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-01 at 12:00 +0100, Julien Grall wrote:
> On 04/01/2014 11:49 AM, Ian Campbell wrote:
> > On Tue, 2014-04-01 at 11:46 +0100, Julien Grall wrote:
> >> On 04/01/2014 10:28 AM, Ian Campbell wrote:
> >>> On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
> >>>> Out-of-context, I've noticed that most of trap failure will kill the 
> >>>> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
> >>>> should 	generate an Undefined Instruction exception (see P.7.5).
> >>>
> >>> You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
> >>>
> >>> I've not checked but I think we only ask for traps for things which we
> >>> are supposed to be able to handle, so anything which is trapped which we
> >>> can't handle is a bug and would indicate the guest doing something
> >>> funky.
> >>
> >> Right, but if the emulation of the instruction fails (see
> >> vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
> >> of sending an UNDEF exception.
> > 
> > My point was that the emulation should never fail...
> 
> The emulation can fail if the guest decides to write on an RO register.

Hrm yes, I'd forgotten that case.

Is that an undef or some other sort of exception? Perhaps it depends on
the cp register whether it faults or is ignored? In either case that
seems to suggest that it is up to the specific handler to inject the
correct kind of exception and return an appropriate error code to the
generic handler.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 04/01/2014 12:15 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 12:00 +0100, Julien Grall wrote:
>> On 04/01/2014 11:49 AM, Ian Campbell wrote:
>>> On Tue, 2014-04-01 at 11:46 +0100, Julien Grall wrote:
>>>> On 04/01/2014 10:28 AM, Ian Campbell wrote:
>>>>> On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
>>>>>> Out-of-context, I've noticed that most of trap failure will kill the 
>>>>>> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
>>>>>> should 	generate an Undefined Instruction exception (see P.7.5).
>>>>>
>>>>> You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
>>>>>
>>>>> I've not checked but I think we only ask for traps for things which we
>>>>> are supposed to be able to handle, so anything which is trapped which we
>>>>> can't handle is a bug and would indicate the guest doing something
>>>>> funky.
>>>>
>>>> Right, but if the emulation of the instruction fails (see
>>>> vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
>>>> of sending an UNDEF exception.
>>>
>>> My point was that the emulation should never fail...
>>
>> The emulation can fail if the guest decides to write on an RO register.
> 
> Hrm yes, I'd forgotten that case.
> 
> Is that an undef or some other sort of exception? Perhaps it depends on
> the cp register whether it faults or is ignored? In either case that
> seems to suggest that it is up to the specific handler to inject the
> correct kind of exception and return an appropriate error code to the
> generic handler.

The default exception is "undefined instruction". I didn't find any
specific exception following the coprocessor. Actually the only ways to
kill the domain in traps.c:
	- we try to access in read (resp. write) on WO (resp. RO) registers
	- the hypercall tags is wrong
	- the PSCI function is not implemented

I think we can replace every (?) domain_crash_synchronous by
inject_undef*_exception.

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-01 at 13:05 +0100, Julien Grall wrote:
> On 04/01/2014 12:15 PM, Ian Campbell wrote:
> > On Tue, 2014-04-01 at 12:00 +0100, Julien Grall wrote:
> >> On 04/01/2014 11:49 AM, Ian Campbell wrote:
> >>> On Tue, 2014-04-01 at 11:46 +0100, Julien Grall wrote:
> >>>> On 04/01/2014 10:28 AM, Ian Campbell wrote:
> >>>>> On Tue, 2014-04-01 at 10:05 +0100, Julien Grall wrote:
> >>>>>> Out-of-context, I've noticed that most of trap failure will kill the 
> >>>>>> domain. From the ARM ARM , if a coprocessor instruction is failing, we 
> >>>>>> should 	generate an Undefined Instruction exception (see P.7.5).
> >>>>>
> >>>>> You mean HSR_EC_CP15_32, HSR_EC_CP15_64 and HSR_EC_SYSREG?
> >>>>>
> >>>>> I've not checked but I think we only ask for traps for things which we
> >>>>> are supposed to be able to handle, so anything which is trapped which we
> >>>>> can't handle is a bug and would indicate the guest doing something
> >>>>> funky.
> >>>>
> >>>> Right, but if the emulation of the instruction fails (see
> >>>> vtimer_emulate, do_trap_psci,...), Xen will destroy the domain instead
> >>>> of sending an UNDEF exception.
> >>>
> >>> My point was that the emulation should never fail...
> >>
> >> The emulation can fail if the guest decides to write on an RO register.
> > 
> > Hrm yes, I'd forgotten that case.
> > 
> > Is that an undef or some other sort of exception? Perhaps it depends on
> > the cp register whether it faults or is ignored? In either case that
> > seems to suggest that it is up to the specific handler to inject the
> > correct kind of exception and return an appropriate error code to the
> > generic handler.
> 
> The default exception is "undefined instruction". I didn't find any
> specific exception following the coprocessor. Actually the only ways to
> kill the domain in traps.c:
> 	- we try to access in read (resp. write) on WO (resp. RO) registers
> 	- the hypercall tags is wrong
> 	- the PSCI function is not implemented
> 
> I think we can replace every (?) domain_crash_synchronous by
> inject_undef*_exception.

Not in a bulk substitution. Each change would need justifying.

In the PSCI case I'd have thought the PSCI spec said what to do,
probably return some error code in r0? Or maybe not.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 04/01/2014 01:11 PM, Ian Campbell wrote:

> In the PSCI case I'd have thought the PSCI spec said what to do,
> probably return some error code in r0? Or maybe not.

Do you have a link to the spec?

-- 
Julien Grall

Re: [PATCH] xen: arm: fully implement multicall interface.

[Ian Campbell]

On Tue, 2014-04-01 at 13:16 +0100, Julien Grall wrote:
> On 04/01/2014 01:11 PM, Ian Campbell wrote:
> 
> > In the PSCI case I'd have thought the PSCI spec said what to do,
> > probably return some error code in r0? Or maybe not.
> 
> Do you have a link to the spec?

It should be in the ARM infocenter. I think it's public.

Ian.

Re: [PATCH] xen: arm: fully implement multicall interface.

[Julien Grall]

On 04/01/2014 01:16 PM, Ian Campbell wrote:
> On Tue, 2014-04-01 at 13:16 +0100, Julien Grall wrote:
>> On 04/01/2014 01:11 PM, Ian Campbell wrote:
>>
>>> In the PSCI case I'd have thought the PSCI spec said what to do,
>>> probably return some error code in r0? Or maybe not.
>>
>> Do you have a link to the spec?
> 
> It should be in the ARM infocenter. I think it's public.

Thanks, so with PSCI v0.{1,2}, we should return -1 (Function not
implemented) if Xen doesn't support the call.

-- 
Julien Grall