From: Andrew Cooper <andrew.cooper3@citrix.com>
To: Xen-devel <xen-devel@lists.xen.org>
Cc: Keir Fraser <keir@xen.org>,
Ian Campbell <Ian.Campbell@citrix.com>,
Andrew Cooper <andrew.cooper3@citrix.com>,
Ian Jackson <Ian.Jackson@eu.citrix.com>, Tim Deegan <tim@xen.org>,
Jan Beulich <JBeulich@suse.com>
Subject: [PATCH 1/3] x86/idle: Fix get_cpu_idle_time()'s interaction with offline pcpus.
Date: Thu, 26 Sep 2013 10:49:24 +0100 [thread overview]
Message-ID: <1380188966-26665-2-git-send-email-andrew.cooper3@citrix.com> (raw)
In-Reply-To: <1380188966-26665-1-git-send-email-andrew.cooper3@citrix.com>
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
next prev parent reply other threads:[~2013-09-26 9:49 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-09-26 9:49 [PATCH 0/3] per_cpu() fixes Andrew Cooper
2013-09-26 9:49 ` Andrew Cooper [this message]
2013-09-26 9:49 ` [PATCH 2/3] x86/percpu: Force INVALID_PERCPU_AREA into the non-canonical address region Andrew Cooper
2013-09-26 10:04 ` Andrew Cooper
2013-09-26 9:49 ` [PATCH 3/3] DO NOT APPLY - debugging code for gpf when accessing invalid per_cpu() data Andrew Cooper
2013-10-03 17:55 ` [PATCH 0/3] per_cpu() fixes Andrew Cooper
2013-10-03 19:52 ` Keir Fraser
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1380188966-26665-2-git-send-email-andrew.cooper3@citrix.com \
--to=andrew.cooper3@citrix.com \
--cc=Ian.Campbell@citrix.com \
--cc=Ian.Jackson@eu.citrix.com \
--cc=JBeulich@suse.com \
--cc=keir@xen.org \
--cc=tim@xen.org \
--cc=xen-devel@lists.xen.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
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).