Xen devel - May 2012 - x86_64: Fix off-by-one error setting up the Interrupt Stack Tables

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



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel

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;

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

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

>>> 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;

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; 
> 
> 
>