[PATCH 0/3] per_cpu() fixes

[PATCH 0/3] per_cpu() fixes

[Andrew Cooper]

This patch series is two independent but related changes to the use of
per_cpu() with offline pcpus.

The first patch is a fix for use of get_cpu_idle_time() whereby toolstack
hypercalls could cause an effective NULL structure dereference.

The second patch is a forward looking fix to try and prevent similar issues in
the future.  It causes uses of per_cpu() against an offline pcpu to cause a
#GF rather than to try and dereference a very low address.

The third patch is debugging code to demonstrate the effects of patch 2.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>

-- 
1.7.10.4

[PATCH 1/3] x86/idle: Fix get_cpu_idle_time()'s interaction with offline pcpus.

[Andrew Cooper]

Checking for "idle_vcpu[cpu] != NULL" is insufficient protection against
offline pcpus.  From a hypercall, vcpu_runstate_get() will determine "v !=
current", and try to take the vcpu_schedule_lock().  This will try to look up
per_cpu(schedule_data, v->processor) and promptly suffer a NULL structure
deference as v->processors' __per_cpu_offset is INVALID_PERCPU_AREA.

One example might look like this:

...
Xen call trace:
   [<ffff82c4c0126ddb>] vcpu_runstate_get+0x50/0x113
   [<ffff82c4c0126ec6>] get_cpu_idle_time+0x28/0x2e
   [<ffff82c4c012b5cb>] do_sysctl+0x3db/0xeb8
   [<ffff82c4c023280d>] compat_hypercall+0xbd/0x116

Pagetable walk from 0000000000000040:
 L4[0x000] = 0000000186df8027 0000000000028207
 L3[0x000] = 0000000188e36027 00000000000261c9
 L2[0x000] = 0000000000000000 ffffffffffffffff

****************************************
Panic on CPU 11:
...

get_cpu_idle_time() has been updated to correctly deal with offline pcpus
itself by returning 0, in the same way as it would if it was missing the
idle_vcpu[] pointer.

In doing so, XENPF_getidletime needed updating to correctly retain its
described behaviour of clearing bits in the cpumap for offline pcpus.

As this crash can only be triggered with toolstack hypercalls, it is not a
security issue and just a simple bug.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
CC: Ian Campbell <Ian.Campbell@citrix.com>
CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
---
 xen/arch/x86/platform_hypercall.c |    8 ++++++--
 xen/common/schedule.c             |    9 ++++-----
 2 files changed, 10 insertions(+), 7 deletions(-)

diff --git a/xen/arch/x86/platform_hypercall.c b/xen/arch/x86/platform_hypercall.c
index 7175a82..f00d508 100644
--- a/xen/arch/x86/platform_hypercall.c
+++ b/xen/arch/x86/platform_hypercall.c
@@ -355,10 +355,14 @@ ret_t do_platform_op(XEN_GUEST_HANDLE_PARAM(xen_platform_op_t) u_xenpf_op)
 
         for_each_cpu ( cpu, cpumap )
         {
-            if ( idle_vcpu[cpu] == NULL )
-                cpumask_clear_cpu(cpu, cpumap);
             idletime = get_cpu_idle_time(cpu);
 
+            if ( ! idletime )
+            {
+                cpumask_clear_cpu(cpu, cpumap);
+                continue;
+            }
+
             if ( copy_to_guest_offset(idletimes, cpu, &idletime, 1) )
             {
                 ret = -EFAULT;
diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index a8398bd..1ddfb22 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -176,13 +176,12 @@ void vcpu_runstate_get(struct vcpu *v, struct vcpu_runstate_info *runstate)
 
 uint64_t get_cpu_idle_time(unsigned int cpu)
 {
-    struct vcpu_runstate_info state;
-    struct vcpu *v;
+    struct vcpu_runstate_info state = { 0 };
+    struct vcpu *v = idle_vcpu[cpu];
 
-    if ( (v = idle_vcpu[cpu]) == NULL )
-        return 0;
+    if ( cpu_online(cpu) && v )
+        vcpu_runstate_get(v, &state);
 
-    vcpu_runstate_get(v, &state);
     return state.time[RUNSTATE_running];
 }
 
-- 
1.7.10.4

[PATCH 2/3] x86/percpu: Force INVALID_PERCPU_AREA into the non-canonical address region

[Andrew Cooper]

This causes accidental uses of per_cpu() on a pcpu with an INVALID_PERCPU_AREA
to result in a #GF for attempting to access the middle of the non-canonical
virtual address region.

This is preferable to the current behaviour, where incorrect use of per_cpu()
will result in an effective NULL structure dereference which has security
implication in the context of PV guests.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
CC: Keir Fraser <keir@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Tim Deegan <tim@xen.org>
---
 xen/arch/x86/percpu.c |    9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c
index e545024..1c1dad9 100644
--- a/xen/arch/x86/percpu.c
+++ b/xen/arch/x86/percpu.c
@@ -6,7 +6,14 @@
 #include <xen/rcupdate.h>
 
 unsigned long __per_cpu_offset[NR_CPUS];
-#define INVALID_PERCPU_AREA (-(long)__per_cpu_start)
+
+/*
+ * Force uses of per_cpu() with an invalid area to attempt to access the
+ * middle of the non-canonical address space resulting in a #GP, rather than a
+ * possible #PF at (NULL + a little) which has security implications in the
+ * context of PV guests.
+ */
+#define INVALID_PERCPU_AREA (0x8000000000000000L - (long)__per_cpu_start)
 #define PERCPU_ORDER (get_order_from_bytes(__per_cpu_data_end-__per_cpu_start))
 
 void __init percpu_init_areas(void)
-- 
1.7.10.4

Re: [PATCH 2/3] x86/percpu: Force INVALID_PERCPU_AREA into the non-canonical address region

[Andrew Cooper]

On 26/09/13 10:49, Andrew Cooper wrote:
> This causes accidental uses of per_cpu() on a pcpu with an INVALID_PERCPU_AREA
> to result in a #GF for attempting to access the middle of the non-canonical
> virtual address region.
>
> This is preferable to the current behaviour, where incorrect use of per_cpu()
> will result in an effective NULL structure dereference which has security
> implication in the context of PV guests.

This could do with clarifying somewhat.

The security concern is simply dereferencing a NULL pointer in the
context of a PV guest, not from any specific use of this code.

This patch simply prevents Xen from accidentally dereferencing a NULL
pointer in the case of an offline pcpu.  As there are no guest
hypercalls which should be able to cause this, there is no specific
security issue here.  The previous patch fixes the case where toolstack
hypercalls could cause this behaviour.

~Andrew

>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> ---
>  xen/arch/x86/percpu.c |    9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/xen/arch/x86/percpu.c b/xen/arch/x86/percpu.c
> index e545024..1c1dad9 100644
> --- a/xen/arch/x86/percpu.c
> +++ b/xen/arch/x86/percpu.c
> @@ -6,7 +6,14 @@
>  #include <xen/rcupdate.h>
>  
>  unsigned long __per_cpu_offset[NR_CPUS];
> -#define INVALID_PERCPU_AREA (-(long)__per_cpu_start)
> +
> +/*
> + * Force uses of per_cpu() with an invalid area to attempt to access the
> + * middle of the non-canonical address space resulting in a #GP, rather than a
> + * possible #PF at (NULL + a little) which has security implications in the
> + * context of PV guests.
> + */
> +#define INVALID_PERCPU_AREA (0x8000000000000000L - (long)__per_cpu_start)
>  #define PERCPU_ORDER (get_order_from_bytes(__per_cpu_data_end-__per_cpu_start))
>  
>  void __init percpu_init_areas(void)

[PATCH 3/3] DO NOT APPLY - debugging code for gpf when accessing invalid per_cpu() data

[Andrew Cooper]

---
 xen/common/schedule.c |   25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/xen/common/schedule.c b/xen/common/schedule.c
index 1ddfb22..8b17555 100644
--- a/xen/common/schedule.c
+++ b/xen/common/schedule.c
@@ -36,6 +36,7 @@
 #include <xen/preempt.h>
 #include <public/sched.h>
 #include <xsm/xsm.h>
+#include <xen/keyhandler.h>
 
 /* opt_sched: scheduler - default to credit */
 static char __initdata opt_sched[10] = "credit";
@@ -1529,6 +1530,30 @@ void wait(void)
 #include "compat/schedule.c"
 #endif
 
+static void percpu_gpf(unsigned char key)
+{
+    unsigned int cpu = NR_CPUS - 1;
+    printk("'%c' pressed -> Looking up schedule lock for cpu %d\n", key, cpu);
+
+    if ( spin_is_locked(per_cpu(schedule_data, cpu).schedule_lock) )
+        printk("It is locked\n");
+    else
+        printk("It is unlocked\n");
+}
+
+static struct keyhandler percpu_gpf_keyhandler = {
+    .diagnostic = 1,
+    .u.fn = percpu_gpf,
+    .desc = "per-cpu gpf test"
+};
+
+static int __init percpu_gpf_key_init(void)
+{
+    register_keyhandler('1', &percpu_gpf_keyhandler);
+    return 0;
+}
+__initcall(percpu_gpf_key_init);
+
 #endif /* !COMPAT */
 
 /*
-- 
1.7.10.4

Re: [PATCH 0/3] per_cpu() fixes

[Andrew Cooper]

On 26/09/13 10:49, Andrew Cooper wrote:
> This patch series is two independent but related changes to the use of
> per_cpu() with offline pcpus.
>
> The first patch is a fix for use of get_cpu_idle_time() whereby toolstack
> hypercalls could cause an effective NULL structure dereference.
>
> The second patch is a forward looking fix to try and prevent similar issues in
> the future.  It causes uses of per_cpu() against an offline pcpu to cause a
> #GF rather than to try and dereference a very low address.
>
> The third patch is debugging code to demonstrate the effects of patch 2.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> CC: Keir Fraser <keir@xen.org>
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Tim Deegan <tim@xen.org>
> CC: Ian Campbell <Ian.Campbell@citrix.com>
> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>

Ping?  Any thoughts at all?

~Andrew

Re: [PATCH 0/3] per_cpu() fixes

[Keir Fraser]

On 03/10/2013 18:55, "Andrew Cooper" <andrew.cooper3@citrix.com> wrote:

> On 26/09/13 10:49, Andrew Cooper wrote:
>> This patch series is two independent but related changes to the use of
>> per_cpu() with offline pcpus.
>> 
>> The first patch is a fix for use of get_cpu_idle_time() whereby toolstack
>> hypercalls could cause an effective NULL structure dereference.
>> 
>> The second patch is a forward looking fix to try and prevent similar issues
>> in
>> the future.  It causes uses of per_cpu() against an offline pcpu to cause a
>> #GF rather than to try and dereference a very low address.
>> 
>> The third patch is debugging code to demonstrate the effects of patch 2.
>> 
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> CC: Keir Fraser <keir@xen.org>
>> CC: Jan Beulich <JBeulich@suse.com>
>> CC: Tim Deegan <tim@xen.org>
>> CC: Ian Campbell <Ian.Campbell@citrix.com>
>> CC: Ian Jackson <Ian.Jackson@eu.citrix.com>
>> 
> 
> Ping?  Any thoughts at all?

Sensible patches.

Acked-by: Keir Fraser <keir@xen.org>

> ~Andrew