Linux Virtualization - Oct 2013 - [PATCH 2/3] x86: tss: Eliminate fragile calculation of TSS segment limit

On Mon, Oct 21, 2013 at 7:34 PM, Josh Triplett <josh at joshtriplett.org>
wrote:
> __set_tss_desc has a complex calculation of the TSS segment limit,
> duplicating the quirky details of the I/O bitmap array length, and
> requiring a complex comment to explain.  Replace that calculation with a
> simpler one based on the offsetof the "stack" field that follows
the
> array.
>
> That then removes the last use of IO_BITMAP_OFFSET, so delete it.
>
> Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> ---
>  arch/x86/include/asm/desc.h      | 11 +----------
>  arch/x86/include/asm/processor.h |  3 ++-
>  2 files changed, 3 insertions(+), 11 deletions(-)
>
> diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> index b90e5df..17ac92f 100644
> --- a/arch/x86/include/asm/desc.h
> +++ b/arch/x86/include/asm/desc.h
> @@ -177,16 +177,7 @@ static inline void __set_tss_desc(unsigned cpu,
unsigned int entry, void *addr)
>         struct desc_struct *d = get_cpu_gdt_table(cpu);
>         tss_desc tss;
>
> -       /*
> -        * sizeof(unsigned long) coming from an extra "long" at
the end
> -        * of the iobitmap. See tss_struct definition in processor.h
> -        *
> -        * -1? seg base+limit should be pointing to the address of the
> -        * last valid byte
I think it might be better to keep at least a minimal comment near the
TSS_LIMIT declaration, just to explain the "-1" part, which is not
entirely obvious from just reading the code.

-Kees
> -        */
> -       set_tssldt_descriptor(&tss, (unsigned long)addr, DESC_TSS,
> -                             IO_BITMAP_OFFSET + IO_BITMAP_BYTES +
> -                             sizeof(unsigned long) - 1);
> +       set_tssldt_descriptor(&tss, (unsigned long)addr, DESC_TSS,
TSS_LIMIT);
>         write_gdt_entry(d, entry, &tss, DESC_TSS);
>  }
>
> diff --git a/arch/x86/include/asm/processor.h
b/arch/x86/include/asm/processor.h
> index 987c75e..03d3003 100644
> --- a/arch/x86/include/asm/processor.h
> +++ b/arch/x86/include/asm/processor.h
> @@ -259,9 +259,10 @@ struct x86_hw_tss {
>  #define IO_BITMAP_BITS                 65536
>  #define IO_BITMAP_BYTES                        (IO_BITMAP_BITS/8)
>  #define IO_BITMAP_LONGS                       
(IO_BITMAP_BYTES/sizeof(long))
> -#define IO_BITMAP_OFFSET               offsetof(struct tss_struct,
io_bitmap)
>  #define INVALID_IO_BITMAP_OFFSET       0x8000
>
> +#define TSS_LIMIT      (offsetof(struct tss_struct, stack) - 1)
> +
>  struct tss_struct {
>         /*
>          * The hardware state:
> --
> 1.8.4.rc3
>


-- 
Kees Cook
Chrome OS Security

On 10/30/2013 03:22 PM, Kees Cook wrote:
>>
>> -       /*
>> -        * sizeof(unsigned long) coming from an extra "long"
at the end
>> -        * of the iobitmap. See tss_struct definition in processor.h
>> -        *
>> -        * -1? seg base+limit should be pointing to the address of the
>> -        * last valid byte
> 
> I think it might be better to keep at least a minimal comment near the
> TSS_LIMIT declaration, just to explain the "-1" part, which is
not
> entirely obvious from just reading the code.
> 
Agreed, although it doesn't need to be an unsigned long at all... the
CPU will only ever access one extra byte past the end.

	-hpa

On Wed, Oct 30, 2013 at 03:22:33PM -0700, Kees Cook
wrote:
> On Mon, Oct 21, 2013 at 7:34 PM, Josh Triplett <josh at
joshtriplett.org> wrote:
> > __set_tss_desc has a complex calculation of the TSS segment limit,
> > duplicating the quirky details of the I/O bitmap array length, and
> > requiring a complex comment to explain.  Replace that calculation with
a
> > simpler one based on the offsetof the "stack" field that
follows the
> > array.
> >
> > That then removes the last use of IO_BITMAP_OFFSET, so delete it.
> >
> > Signed-off-by: Josh Triplett <josh at joshtriplett.org>
> > ---
> >  arch/x86/include/asm/desc.h      | 11 +----------
> >  arch/x86/include/asm/processor.h |  3 ++-
> >  2 files changed, 3 insertions(+), 11 deletions(-)
> >
> > diff --git a/arch/x86/include/asm/desc.h b/arch/x86/include/asm/desc.h
> > index b90e5df..17ac92f 100644
> > --- a/arch/x86/include/asm/desc.h
> > +++ b/arch/x86/include/asm/desc.h
> > @@ -177,16 +177,7 @@ static inline void __set_tss_desc(unsigned cpu,
unsigned int entry, void *addr)
> >         struct desc_struct *d = get_cpu_gdt_table(cpu);
> >         tss_desc tss;
> >
> > -       /*
> > -        * sizeof(unsigned long) coming from an extra "long"
at the end
> > -        * of the iobitmap. See tss_struct definition in processor.h
> > -        *
> > -        * -1? seg base+limit should be pointing to the address of the
> > -        * last valid byte
> 
> I think it might be better to keep at least a minimal comment near the
> TSS_LIMIT declaration, just to explain the "-1" part, which is
not
> entirely obvious from just reading the code.
Fair enough; I've added an appropriate comment next to TSS_LIMIT, and
I'll include that in PATCHv2, which I'll send out as soon as I see
any feedback on patch 3/3.

- Josh Triplett

On Wed, Oct 30, 2013 at 03:53:11PM -0700, H. Peter Anvin
wrote:
> On 10/30/2013 03:22 PM, Kees Cook wrote:
> >>
> >> -       /*
> >> -        * sizeof(unsigned long) coming from an extra
"long" at the end
> >> -        * of the iobitmap. See tss_struct definition in
processor.h
> >> -        *
> >> -        * -1? seg base+limit should be pointing to the address of
the
> >> -        * last valid byte
> > 
> > I think it might be better to keep at least a minimal comment near the
> > TSS_LIMIT declaration, just to explain the "-1" part, which
is not
> > entirely obvious from just reading the code.
> > 
> 
> Agreed, although it doesn't need to be an unsigned long at all... the
> CPU will only ever access one extra byte past the end.
True, but the thing immediately following the iobitmap is a stack, which
needs aligning, so the array does need to contain a full additional
unsigned long even if the CPU only accesses a byte of it.  In any case,
that isn't the reason for the -1, just the reason for the
sizeof(unsigned long) mentioned in the comment above, which goes away
now that TSS_LIMIT uses the offset of the *following* field rather than
recalculating the size of the iobitmap.

- Josh Triplett