* x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
@ 2012-05-09 10:22 Andrew Cooper
2012-05-09 10:31 ` Ian Campbell
0 siblings, 1 reply; 6+ messages in thread
From: Andrew Cooper @ 2012-05-09 10:22 UTC (permalink / raw)
To: xen-devel@lists.xen.org, Keir Fraser, Jan Beulich
[-- Attachment #1: Type: text/plain, Size: 165 bytes --]
This affects every version of Xen at least as back as 3.4
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
[-- Attachment #2: x86_64-ist-off-by-one-error.patch --]
[-- Type: text/x-patch, Size: 2525 bytes --]
# HG changeset patch
# Parent 8f1e0cc4a507a52a49a2eb7832a57ecc7e032dce
x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
The Interrupt Stack Table entries in a 64bit TSS are a 1 based data
structure as far as hardware is concerned. As a result, the code
setting up stacks in subarch_percpu_traps_init() fills in the wrong IST
entries.
The result is that the MCE handler executes on the stack set up for
NMIs; the NMI handler executes on a stack set up for Double Faults, and
Double Faults are executed with a stack pointer set to 0.
Once the #DF handler starts to execute, it will usually take a page
fault looking up the address at 0xfffffffffffffff8, which will cause a
triple fault. If a guest has mapped a page in that location, then it
will have some state overwritten, but as the #DF handler always calls
panic(), this is not a problem the guest will have time to care about.
Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
diff -r 8f1e0cc4a507 xen/arch/x86/x86_64/traps.c
--- a/xen/arch/x86/x86_64/traps.c
+++ b/xen/arch/x86/x86_64/traps.c
@@ -386,13 +386,13 @@ void __devinit subarch_percpu_traps_init
BUILD_BUG_ON((IST_MAX + 2) * PAGE_SIZE + PRIMARY_STACK_SIZE > STACK_SIZE);
/* Machine Check handler has its own per-CPU 4kB stack. */
- this_cpu(init_tss).ist[IST_MCE] = (unsigned long)&stack[IST_MCE * PAGE_SIZE];
+ this_cpu(init_tss).ist[IST_MCE-1] = (unsigned long)&stack[IST_MCE * PAGE_SIZE];
/* Double-fault handler has its own per-CPU 4kB stack. */
- this_cpu(init_tss).ist[IST_DF] = (unsigned long)&stack[IST_DF * PAGE_SIZE];
+ this_cpu(init_tss).ist[IST_DF-1] = (unsigned long)&stack[IST_DF * PAGE_SIZE];
/* NMI handler has its own per-CPU 4kB stack. */
- this_cpu(init_tss).ist[IST_NMI] = (unsigned long)&stack[IST_NMI * PAGE_SIZE];
+ this_cpu(init_tss).ist[IST_NMI-1] = (unsigned long)&stack[IST_NMI * PAGE_SIZE];
/* Trampoline for SYSCALL entry from long mode. */
stack = &stack[IST_MAX * PAGE_SIZE]; /* Skip the IST stacks. */
diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -424,7 +424,9 @@ struct tss_struct {
union { u64 rsp1, esp1; };
union { u64 rsp2, esp2; };
u64 reserved1;
- u64 ist[7];
+ u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
+ * corresponds to an IST value of 1 in an Interrupt
+ * Descriptor */
u64 reserved2;
u16 reserved3;
#else
[-- Attachment #3: Type: text/plain, Size: 126 bytes --]
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
2012-05-09 10:22 x86_64: Fix off-by-one error setting up the Interrupt Stack Tables Andrew Cooper
@ 2012-05-09 10:31 ` Ian Campbell
2012-05-09 10:53 ` David Vrabel
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Ian Campbell @ 2012-05-09 10:31 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Keir (Xen.org), Jan Beulich, xen-devel@lists.xen.org
On Wed, 2012-05-09 at 11:22 +0100, Andrew Cooper wrote:
> diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -424,7 +424,9 @@ struct tss_struct {
> union { u64 rsp1, esp1; };
> union { u64 rsp2, esp2; };
> u64 reserved1;
> - u64 ist[7];
> + u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
> + * corresponds to an IST value of 1 in an Interrupt
> + * Descriptor */
Would it be too sneaky to drop "reserved1" and make ist be 8 elements?
then ist[1] would actually be the slot corresponding to a value of 1 in
an IDT entry.
> u64 reserved2;
> u16 reserved3;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
2012-05-09 10:31 ` Ian Campbell
@ 2012-05-09 10:53 ` David Vrabel
2012-05-09 10:53 ` Andrew Cooper
2012-05-09 10:55 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: David Vrabel @ 2012-05-09 10:53 UTC (permalink / raw)
To: Ian Campbell
Cc: Andrew Cooper, Keir (Xen.org), Jan Beulich,
xen-devel@lists.xen.org
On 09/05/12 11:31, Ian Campbell wrote:
> On Wed, 2012-05-09 at 11:22 +0100, Andrew Cooper wrote:
>> diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -424,7 +424,9 @@ struct tss_struct {
>> union { u64 rsp1, esp1; };
>> union { u64 rsp2, esp2; };
>> u64 reserved1;
>> - u64 ist[7];
>> + u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
>> + * corresponds to an IST value of 1 in an Interrupt
>> + * Descriptor */
>
> Would it be too sneaky to drop "reserved1" and make ist be 8 elements?
> then ist[1] would actually be the slot corresponding to a value of 1 in
> an IDT entry.
We did discuss this briefly internally and I thought it would be too
sneaky. But perhaps it is ok if there is also #define IST_RESERVED 0
and a comment.
David
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
2012-05-09 10:31 ` Ian Campbell
2012-05-09 10:53 ` David Vrabel
@ 2012-05-09 10:53 ` Andrew Cooper
2012-05-09 10:55 ` Jan Beulich
2 siblings, 0 replies; 6+ messages in thread
From: Andrew Cooper @ 2012-05-09 10:53 UTC (permalink / raw)
To: Ian Campbell; +Cc: Keir (Xen.org), Jan Beulich, xen-devel@lists.xen.org
On 09/05/12 11:31, Ian Campbell wrote:
> On Wed, 2012-05-09 at 11:22 +0100, Andrew Cooper wrote:
>> diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -424,7 +424,9 @@ struct tss_struct {
>> union { u64 rsp1, esp1; };
>> union { u64 rsp2, esp2; };
>> u64 reserved1;
>> - u64 ist[7];
>> + u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
>> + * corresponds to an IST value of 1 in an Interrupt
>> + * Descriptor */
> Would it be too sneaky to drop "reserved1" and make ist be 8 elements?
> then ist[1] would actually be the slot corresponding to a value of 1 in
> an IDT entry.
I considered that, but given no particular preference, I went with the
visibly safer fix.
I can change it if general opinion is that it would be clearer that way.
~Andrew
>
>> u64 reserved2;
>> u16 reserved3;
--
Andrew Cooper - Dom0 Kernel Engineer, Citrix XenServer
T: +44 (0)1223 225 900, http://www.citrix.com
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
2012-05-09 10:31 ` Ian Campbell
2012-05-09 10:53 ` David Vrabel
2012-05-09 10:53 ` Andrew Cooper
@ 2012-05-09 10:55 ` Jan Beulich
2012-05-09 11:00 ` Ian Campbell
2 siblings, 1 reply; 6+ messages in thread
From: Jan Beulich @ 2012-05-09 10:55 UTC (permalink / raw)
To: Andrew Cooper, Ian Campbell; +Cc: Keir (Xen.org), xen-devel@lists.xen.org
>>> On 09.05.12 at 12:31, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> On Wed, 2012-05-09 at 11:22 +0100, Andrew Cooper wrote:
>> diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -424,7 +424,9 @@ struct tss_struct {
>> union { u64 rsp1, esp1; };
>> union { u64 rsp2, esp2; };
>> u64 reserved1;
>> - u64 ist[7];
>> + u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
>> + * corresponds to an IST value of 1 in an Interrupt
>> + * Descriptor */
>
> Would it be too sneaky to drop "reserved1" and make ist be 8 elements?
> then ist[1] would actually be the slot corresponding to a value of 1 in
> an IDT entry.
While appealing at a first glance, I wouldn't recommend doing so:
Documentation specifies it the way it's defined currently, and Linux
also uses the same definition, so we'd only call for future bugs if
we did it differently in our code.
Jan
>> u64 reserved2;
>> u16 reserved3;
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: x86_64: Fix off-by-one error setting up the Interrupt Stack Tables
2012-05-09 10:55 ` Jan Beulich
@ 2012-05-09 11:00 ` Ian Campbell
0 siblings, 0 replies; 6+ messages in thread
From: Ian Campbell @ 2012-05-09 11:00 UTC (permalink / raw)
To: Jan Beulich; +Cc: Andrew Cooper, Keir (Xen.org), xen-devel@lists.xen.org
On Wed, 2012-05-09 at 11:55 +0100, Jan Beulich wrote:
> >>> On 09.05.12 at 12:31, Ian Campbell <Ian.Campbell@citrix.com> wrote:
> > On Wed, 2012-05-09 at 11:22 +0100, Andrew Cooper wrote:
> >> diff -r 8f1e0cc4a507 xen/include/asm-x86/processor.h
> >> --- a/xen/include/asm-x86/processor.h
> >> +++ b/xen/include/asm-x86/processor.h
> >> @@ -424,7 +424,9 @@ struct tss_struct {
> >> union { u64 rsp1, esp1; };
> >> union { u64 rsp2, esp2; };
> >> u64 reserved1;
> >> - u64 ist[7];
> >> + u64 ist[7]; /* Interrupt Stack Table is 1-based so tss->ist[0]
> >> + * corresponds to an IST value of 1 in an Interrupt
> >> + * Descriptor */
> >
> > Would it be too sneaky to drop "reserved1" and make ist be 8 elements?
> > then ist[1] would actually be the slot corresponding to a value of 1 in
> > an IDT entry.
>
> While appealing at a first glance, I wouldn't recommend doing so:
> Documentation specifies it the way it's defined currently, and Linux
> also uses the same definition, so we'd only call for future bugs if
> we did it differently in our code.
I thought someone would say something like that ;-) OK then.
>
> Jan
>
> >> u64 reserved2;
> >> u16 reserved3;
>
>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-05-09 11:00 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-05-09 10:22 x86_64: Fix off-by-one error setting up the Interrupt Stack Tables Andrew Cooper
2012-05-09 10:31 ` Ian Campbell
2012-05-09 10:53 ` David Vrabel
2012-05-09 10:53 ` Andrew Cooper
2012-05-09 10:55 ` Jan Beulich
2012-05-09 11:00 ` Ian Campbell
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.