Re: [Xen-devel] [PATCH 2/2] x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Fri, Jul 26, 2019 at 03:45:22PM +0100, Andrew Cooper wrote:
> On 26/07/2019 15:38, Roger Pau Monné wrote:
> > On Fri, Jul 26, 2019 at 02:52:40PM +0100, Andrew Cooper wrote:
> >> The XPTI work restricted the visibility of most of memory, but missed a few
> >> aspects when it came to the TSS.
> >>
> >> Given that the TSS is just an object in percpu data, the 4k mapping for it
> >> created in setup_cpu_root_pgt() maps adjacent percpu data, making it all
> >> leakable via Meltdown, even when XPTI is in use.
> >>
> >> Furthermore, no care is taken to check that the TSS doesn't cross a page
> >> boundary.  As it turns out, struct tss_struct is aligned on its size which
> >> does prevent it straddling a page boundary, but this will cease to be true
> >> once CET and Shadow Stack support is added to Xen.
> >>
> >> Move the TSS into the page aligned percpu area, so no adjacent data can be
> >> leaked.  Move the definition from setup.c to traps.c, which is a more
> >> appropriate place for it to live.
> >>
> >> Signed-off-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
> >> ---
> >> CC: Jan Beulich <JBeulich@xxxxxxxx>
> >> CC: Wei Liu <wl@xxxxxxx>
> >> CC: Roger Pau Monné <roger.pau@xxxxxxxxxx>
> >> ---
> >>  xen/arch/x86/setup.c            | 2 --
> >>  xen/arch/x86/traps.c            | 6 ++++++
> >>  xen/arch/x86/xen.lds.S          | 2 ++
> >>  xen/include/asm-x86/processor.h | 4 ++--
> >>  4 files changed, 10 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> >> index d2011910fa..1a2ffc4dc1 100644
> >> --- a/xen/arch/x86/setup.c
> >> +++ b/xen/arch/x86/setup.c
> >> @@ -100,8 +100,6 @@ unsigned long __read_mostly xen_phys_start;
> >>  
> >>  unsigned long __read_mostly xen_virt_end;
> >>  
> >> -DEFINE_PER_CPU(struct tss_struct, init_tss);
> >> -
> >>  char __section(".bss.stack_aligned") __aligned(STACK_SIZE)
> >>      cpu0_stack[STACK_SIZE];
> >>  
> >> diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
> >> index 38d12013db..e4b4587956 100644
> >> --- a/xen/arch/x86/traps.c
> >> +++ b/xen/arch/x86/traps.c
> >> @@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned") 
> >> __aligned(PAGE_SIZE)
> >>  /* Pointer to the IDT of every CPU. */
> >>  idt_entry_t *idt_tables[NR_CPUS] __read_mostly;
> >>  
> >> +/*
> >> + * The TSS is smaller than a page, but we give it a full page to avoid
> >> + * adjacent per-cpu data leaking via Meltdown when XPTI is in use.
> >> + */
> >> +DEFINE_PER_CPU_PAGE_ALIGNED(struct __aligned(PAGE_SIZE) tss_struct, 
> >> init_tss);
> > Can't you bundle the __aligned attribute in
> > DEFINE_PER_CPU_PAGE_ALIGNED itself?
> >
> > #define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
> >     __DEFINE_PER_CPU(type __aligned(PAGE_SIZE), _##name, .page_aligned)
> >
> > I haven't tested this TBH.
> 
> I did.  It doesn't compile, because the attribute follows the declaration.
> 
> Observe that the patch puts __aligned() between struct and tss_struct.
> 
> There is no way to do this inside DEFINE_PER_CPU_PAGE_ALIGNED(), because
> we can't break the type apart to insert __aligned() in the middle.

And doing something like:

#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
    __DEFINE_PER_CPU(type, _##name, .page_aligned) __aligned(PAGE_SIZE)

Won't work either I guess?

I just find it unconformable to have to specify the aligned
attribute in every usage of DEFINE_PER_CPU_PAGE_ALIGNED.

Thanks, Roger.

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel