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

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

[Jan Beulich]

[-- Attachment #1: Type: text/plain, Size: 239 bytes --]

From: Andrew Cooper <andrew.cooper3@citrix.com>

1: xen/link: Introduce .bss.percpu.page_aligned
2: x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

Addressing my own v3 review comments, albeit in a slightly extended
form.

Jan

[-- Attachment #2: bss-per-CPU-page-aligned.patch --]
[-- Type: text/plain, Size: 5343 bytes --]

xen/link: Introduce .bss.percpu.page_aligned

Future changes are going to need to page align some percpu data.

Shuffle the exact link order of items within the BSS to give
.bss.percpu.page_aligned appropriate alignment, even on CPU0, which uses
.bss.percpu itself.

Insert explicit alignment such that the result is safe even with objects
shorter than a page in length.  The POINTER_ALIGN for __bss_end is to cover
the lack of SMP_CACHE_BYTES alignment, as the loops which zero the BSS use
pointer-sized stores on all architectures.

In addition, we need to be able to specify an alignment attribute to
__DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
that it is now possible to grep for .bss.percpu and find all the users.

Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which specifies the
section attribute and verifies the type's alignment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Make DEFINE_PER_CPU_PAGE_ALIGNED() verify the alignment rather than
specifying it. It is the underlying type which should be suitably aligned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
A sample build including the subsequent patch is now:

ffff82d08092d000 B zero_page
ffff82d08092e000 B per_cpu__init_tss
ffff82d08092e000 B __per_cpu_start
ffff82d08092f000 B per_cpu__cpupool
ffff82d08092f008 b per_cpu__continue_info
ffff82d08092f010 b per_cpu__grant_rwlock

which demonstrates the correct alignment of data in .bss.percpu even when
following a non-page-sized object in .bss.percpu.page_aligned.

v4:
 * Drop stray trailing ALIGN(). Make DEFINE_PER_CPU_PAGE_ALIGNED() verify
   the alignment rather than specifying it.

v3:
 * Insert explicit alignment.
 * Reduce __bss_end's alignment to just POINTER_ALIGN.

v2:
 * Rework __DEFINE_PER_CPU() to allow for further attributes to be passed.
 * Specify __aligned(PAGE_SIZE) as part of DEFINE_PER_CPU_PAGE_ALIGNED().

--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -200,14 +200,16 @@ SECTIONS
        *(.bss.stack_aligned)
        . = ALIGN(PAGE_SIZE);
        *(.bss.page_aligned)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
   _end = . ;
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -292,14 +292,16 @@ SECTIONS
        __bss_start = .;
        *(.bss.stack_aligned)
        *(.bss.page_aligned*)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
        __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
        *(.bss.percpu)
        . = ALIGN(SMP_CACHE_BYTES);
        *(.bss.percpu.read_mostly)
        . = ALIGN(SMP_CACHE_BYTES);
        __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
        __bss_end = .;
   } :text
   _end = . ;
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -10,10 +10,8 @@ extern char __per_cpu_start[], __per_cpu
 extern unsigned long __per_cpu_offset[NR_CPUS];
 void percpu_init_areas(void);
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 #define per_cpu(var, cpu)  \
     (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR
 void percpu_init_areas(void);
 #endif
 
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
 
 /* var is in discarded region: offset to particular copy we want */
 #define per_cpu(var, cpu)  \
--- a/xen/include/xen/percpu.h
+++ b/xen/include/xen/percpu.h
@@ -9,9 +9,17 @@
  * The _##name concatenation is being used here to prevent 'name' from getting
  * macro expanded, while still allowing a per-architecture symbol name prefix.
  */
-#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
+#define DEFINE_PER_CPU(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
+
+#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
+    typedef char name ## _chk_t[BUILD_BUG_ON_ZERO(__alignof(type) & \
+                                                  (PAGE_SIZE - 1))]; \
+    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"), \
+                     type, _ ## name)
+
 #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
-	__DEFINE_PER_CPU(type, _##name, .read_mostly)
+    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
 
 #define get_per_cpu_var(var)  (per_cpu__##var)
 

[-- Attachment #3: x86-TSS-adjacent-leak.patch --]
[-- Type: text/plain, Size: 4005 bytes --]

x86/xpti: Don't leak TSS-adjacent percpu data via Meltdown

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.

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@citrix.com>

Introduce / use struct tss_page.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Especially with how the previous patch now works I'm unconvinced of
     the utility of the linker script alignment check. It in particular
     doesn't check the property we're after in this patch, i.e. the fact
     that there's nothing else in the same page.
NB: Sadly get_per_cpu_var() can't also be used on the "left" side of a
    #define.
---
v4:
 * Introduce / use struct tss_page.

v3:
 * Drop the remark about CET.  It is no longer accurate in the latest version
   of the CET spec.

v2:
 * Rebase over changes to include __aligned() within
   DEFINE_PER_CPU_PAGE_ALIGNED()
 * Drop now-unused xen/percpu.h from setup.c

--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -16,7 +16,6 @@
 #include <xen/domain_page.h>
 #include <xen/version.h>
 #include <xen/gdbstub.h>
-#include <xen/percpu.h>
 #include <xen/hypercall.h>
 #include <xen/keyhandler.h>
 #include <xen/numa.h>
@@ -100,8 +99,6 @@ unsigned long __read_mostly xen_phys_sta
 
 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];
 
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned
 /* 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 tss_page, init_tss_page);
+
 bool (*ioemul_handle_quirk)(
     u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
 
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -368,6 +368,8 @@ ASSERT(IS_ALIGNED(__2M_rwdata_end,   SEC
 
 ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
 
+ASSERT(IS_ALIGNED(per_cpu__init_tss_page, PAGE_SIZE), "per_cpu(init_tss) misaligned")
+
 ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
 ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZE), "__init_end misaligned")
 
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -411,7 +411,7 @@ static always_inline void __mwait(unsign
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct __packed __cacheline_aligned tss_struct {
+struct __packed tss_struct {
     uint32_t :32;
     uint64_t rsp0, rsp1, rsp2;
     uint64_t :64;
@@ -425,6 +425,11 @@ struct __packed __cacheline_aligned tss_
     /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
     uint8_t __cacheline_filler[24];
 };
+struct tss_page {
+    struct tss_struct __aligned(PAGE_SIZE) tss;
+};
+DECLARE_PER_CPU(struct tss_page, init_tss_page);
+#define per_cpu__init_tss get_per_cpu_var(init_tss_page.tss)
 
 #define IST_NONE 0UL
 #define IST_DF   1UL
@@ -463,7 +468,6 @@ static inline void disable_each_ist(idt_
 extern idt_entry_t idt_table[];
 extern idt_entry_t *idt_tables[];
 
-DECLARE_PER_CPU(struct tss_struct, init_tss);
 DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
 
 extern void write_ptbase(struct vcpu *v);

[-- Attachment #4: Type: text/plain, Size: 157 bytes --]

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

[Xen-devel] [PATCH v4 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

From: Andrew Cooper <andrew.cooper3@citrix.com>

Future changes are going to need to page align some percpu data.

Shuffle the exact link order of items within the BSS to give
.bss.percpu.page_aligned appropriate alignment, even on CPU0, which uses
.bss.percpu itself.

Insert explicit alignment such that the result is safe even with objects
shorter than a page in length.  The POINTER_ALIGN for __bss_end is to cover
the lack of SMP_CACHE_BYTES alignment, as the loops which zero the BSS use
pointer-sized stores on all architectures.

In addition, we need to be able to specify an alignment attribute to
__DEFINE_PER_CPU().  Rework it so the caller passes in all attributes, and
adjust DEFINE_PER_CPU{,_READ_MOSTLY}() to match.  This has the added bonus
that it is now possible to grep for .bss.percpu and find all the users.

Finally, introduce DEFINE_PER_CPU_PAGE_ALIGNED() which specifies the
section attribute and verifies the type's alignment.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>

Make DEFINE_PER_CPU_PAGE_ALIGNED() verify the alignment rather than
specifying it. It is the underlying type which should be suitably aligned.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
Acked-by: Julien Grall <julien.grall@arm.com>
---
A sample build including the subsequent patch is now:

ffff82d08092d000 B zero_page
ffff82d08092e000 B per_cpu__init_tss
ffff82d08092e000 B __per_cpu_start
ffff82d08092f000 B per_cpu__cpupool
ffff82d08092f008 b per_cpu__continue_info
ffff82d08092f010 b per_cpu__grant_rwlock

which demonstrates the correct alignment of data in .bss.percpu even when
following a non-page-sized object in .bss.percpu.page_aligned.

v4:
  * Drop stray trailing ALIGN(). Make DEFINE_PER_CPU_PAGE_ALIGNED() verify
    the alignment rather than specifying it.

v3:
  * Insert explicit alignment.
  * Reduce __bss_end's alignment to just POINTER_ALIGN.

v2:
  * Rework __DEFINE_PER_CPU() to allow for further attributes to be passed.
  * Specify __aligned(PAGE_SIZE) as part of DEFINE_PER_CPU_PAGE_ALIGNED().

--- a/xen/arch/arm/xen.lds.S
+++ b/xen/arch/arm/xen.lds.S
@@ -200,14 +200,16 @@ SECTIONS
         *(.bss.stack_aligned)
         . = ALIGN(PAGE_SIZE);
         *(.bss.page_aligned)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
         __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
         *(.bss.percpu)
         . = ALIGN(SMP_CACHE_BYTES);
         *(.bss.percpu.read_mostly)
         . = ALIGN(SMP_CACHE_BYTES);
         __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
         __bss_end = .;
    } :text
    _end = . ;
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -292,14 +292,16 @@ SECTIONS
         __bss_start = .;
         *(.bss.stack_aligned)
         *(.bss.page_aligned*)
-       *(.bss)
-       . = ALIGN(SMP_CACHE_BYTES);
+       . = ALIGN(PAGE_SIZE);
         __per_cpu_start = .;
+       *(.bss.percpu.page_aligned)
         *(.bss.percpu)
         . = ALIGN(SMP_CACHE_BYTES);
         *(.bss.percpu.read_mostly)
         . = ALIGN(SMP_CACHE_BYTES);
         __per_cpu_data_end = .;
+       *(.bss)
+       . = ALIGN(POINTER_ALIGN);
         __bss_end = .;
    } :text
    _end = . ;
--- a/xen/include/asm-arm/percpu.h
+++ b/xen/include/asm-arm/percpu.h
@@ -10,10 +10,8 @@ extern char __per_cpu_start[], __per_cpu
  extern unsigned long __per_cpu_offset[NR_CPUS];
  void percpu_init_areas(void);
  
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
  
  #define per_cpu(var, cpu)  \
      (*RELOC_HIDE(&per_cpu__##var, __per_cpu_offset[cpu]))
--- a/xen/include/asm-x86/percpu.h
+++ b/xen/include/asm-x86/percpu.h
@@ -7,10 +7,8 @@ extern unsigned long __per_cpu_offset[NR
  void percpu_init_areas(void);
  #endif
  
-/* Separate out the type, so (int[3], foo) works. */
-#define __DEFINE_PER_CPU(type, name, suffix)                    \
-    __section(".bss.percpu" #suffix)                            \
-    __typeof__(type) per_cpu_##name
+#define __DEFINE_PER_CPU(attr, type, name) \
+    attr __typeof__(type) per_cpu_ ## name
  
  /* var is in discarded region: offset to particular copy we want */
  #define per_cpu(var, cpu)  \
--- a/xen/include/xen/percpu.h
+++ b/xen/include/xen/percpu.h
@@ -9,9 +9,17 @@
   * The _##name concatenation is being used here to prevent 'name' from getting
   * macro expanded, while still allowing a per-architecture symbol name prefix.
   */
-#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
+#define DEFINE_PER_CPU(type, name) \
+    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
+
+#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
+    typedef char name ## _chk_t[BUILD_BUG_ON_ZERO(__alignof(type) & \
+                                                  (PAGE_SIZE - 1))]; \
+    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"), \
+                     type, _ ## name)
+
  #define DEFINE_PER_CPU_READ_MOSTLY(type, name) \
-	__DEFINE_PER_CPU(type, _##name, .read_mostly)
+    __DEFINE_PER_CPU(__section(".bss.percpu.read_mostly"), type, _ ## name)
  
  #define get_per_cpu_var(var)  (per_cpu__##var)
  

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

Re: [Xen-devel] [PATCH v4 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Andrew Cooper]

On 09/08/2019 13:32, Jan Beulich wrote:
> Insert explicit alignment such that the result is safe even with objects
> shorter than a page in length.  The POINTER_ALIGN for __bss_end is to
> cover
> the lack of SMP_CACHE_BYTES alignment, as the loops which zero the BSS
> use
> pointer-sized stores on all architectures.

...

> v4:
>  * Drop stray trailing ALIGN(). Make DEFINE_PER_CPU_PAGE_ALIGNED() verify
>    the alignment rather than specifying it.

My feelings about the stray-ness of ALIGN() notwithstanding, the commit
message now wrong and needs correcting.

> --- a/xen/include/xen/percpu.h
> +++ b/xen/include/xen/percpu.h
> @@ -9,9 +9,17 @@
>   * The _##name concatenation is being used here to prevent 'name'
> from getting
>   * macro expanded, while still allowing a per-architecture symbol
> name prefix.
>   */
> -#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
> +#define DEFINE_PER_CPU(type, name) \
> +    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
> +
> +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
> +    typedef char name ## _chk_t[BUILD_BUG_ON_ZERO(__alignof(type) & \
> +                                                  (PAGE_SIZE - 1))]; \
> +    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"), \
> +                     type, _ ## name)

I think this would be easier to read as:

#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name)                \
    typedef char name ## _chk_t[                               \
        BUILD_BUG_ON_ZERO(__alignof(type) & (PAGE_SIZE - 1))]; \
    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"),    \
                     type, _ ## name)


By not breaking important bit of logic across a newline.

Preferably with this changed, but definitely with the commit message
fixed, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

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

Re: [Xen-devel] [PATCH v4 1/2] xen/link: Introduce .bss.percpu.page_aligned

[Jan Beulich]

On 09.08.2019 15:42, Andrew Cooper wrote:
> On 09/08/2019 13:32, Jan Beulich wrote:
>> Insert explicit alignment such that the result is safe even with objects
>> shorter than a page in length.  The POINTER_ALIGN for __bss_end is to
>> cover
>> the lack of SMP_CACHE_BYTES alignment, as the loops which zero the BSS
>> use
>> pointer-sized stores on all architectures.
> 
> ...
> 
>> v4:
>>   * Drop stray trailing ALIGN(). Make DEFINE_PER_CPU_PAGE_ALIGNED() verify
>>     the alignment rather than specifying it.
> 
> My feelings about the stray-ness of ALIGN() notwithstanding, the commit
> message now wrong and needs correcting.

Oh, indeed, and not just for this aspect.

>> --- a/xen/include/xen/percpu.h
>> +++ b/xen/include/xen/percpu.h
>> @@ -9,9 +9,17 @@
>>    * The _##name concatenation is being used here to prevent 'name'
>> from getting
>>    * macro expanded, while still allowing a per-architecture symbol
>> name prefix.
>>    */
>> -#define DEFINE_PER_CPU(type, name) __DEFINE_PER_CPU(type, _##name, )
>> +#define DEFINE_PER_CPU(type, name) \
>> +    __DEFINE_PER_CPU(__section(".bss.percpu"), type, _ ## name)
>> +
>> +#define DEFINE_PER_CPU_PAGE_ALIGNED(type, name) \
>> +    typedef char name ## _chk_t[BUILD_BUG_ON_ZERO(__alignof(type) & \
>> +                                                  (PAGE_SIZE - 1))]; \
>> +    __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"), \
>> +                     type, _ ## name)
> 
> I think this would be easier to read as:
> 
> #define DEFINE_PER_CPU_PAGE_ALIGNED(type, name)                \
>      typedef char name ## _chk_t[                               \
>          BUILD_BUG_ON_ZERO(__alignof(type) & (PAGE_SIZE - 1))]; \
>      __DEFINE_PER_CPU(__section(".bss.percpu.page_aligned"),    \
>                       type, _ ## name)
> 
> 
> By not breaking important bit of logic across a newline.

Will do.

> Preferably with this changed, but definitely with the commit message
> fixed, Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks, Jan

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

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

[Jan Beulich]

From: Andrew Cooper <andrew.cooper3@citrix.com>

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.

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@citrix.com>

Introduce / use struct tss_page.

Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
TBD: Especially with how the previous patch now works I'm unconvinced of
      the utility of the linker script alignment check. It in particular
      doesn't check the property we're after in this patch, i.e. the fact
      that there's nothing else in the same page.
NB: Sadly get_per_cpu_var() can't also be used on the "left" side of a
     #define.
---
v4:
  * Introduce / use struct tss_page.

v3:
  * Drop the remark about CET.  It is no longer accurate in the latest version
    of the CET spec.

v2:
  * Rebase over changes to include __aligned() within
    DEFINE_PER_CPU_PAGE_ALIGNED()
  * Drop now-unused xen/percpu.h from setup.c

--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -16,7 +16,6 @@
  #include <xen/domain_page.h>
  #include <xen/version.h>
  #include <xen/gdbstub.h>
-#include <xen/percpu.h>
  #include <xen/hypercall.h>
  #include <xen/keyhandler.h>
  #include <xen/numa.h>
@@ -100,8 +99,6 @@ unsigned long __read_mostly xen_phys_sta
  
  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];
  
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -108,6 +108,12 @@ idt_entry_t __section(".bss.page_aligned
  /* 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 tss_page, init_tss_page);
+
  bool (*ioemul_handle_quirk)(
      u8 opcode, char *io_emul_stub, struct cpu_user_regs *regs);
  
--- a/xen/arch/x86/xen.lds.S
+++ b/xen/arch/x86/xen.lds.S
@@ -368,6 +368,8 @@ ASSERT(IS_ALIGNED(__2M_rwdata_end,   SEC
  
  ASSERT(IS_ALIGNED(cpu0_stack, STACK_SIZE), "cpu0_stack misaligned")
  
+ASSERT(IS_ALIGNED(per_cpu__init_tss_page, PAGE_SIZE), "per_cpu(init_tss) misaligned")
+
  ASSERT(IS_ALIGNED(__init_begin, PAGE_SIZE), "__init_begin misaligned")
  ASSERT(IS_ALIGNED(__init_end,   PAGE_SIZE), "__init_end misaligned")
  
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -411,7 +411,7 @@ static always_inline void __mwait(unsign
  #define IOBMP_BYTES             8192
  #define IOBMP_INVALID_OFFSET    0x8000
  
-struct __packed __cacheline_aligned tss_struct {
+struct __packed tss_struct {
      uint32_t :32;
      uint64_t rsp0, rsp1, rsp2;
      uint64_t :64;
@@ -425,6 +425,11 @@ struct __packed __cacheline_aligned tss_
      /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
      uint8_t __cacheline_filler[24];
  };
+struct tss_page {
+    struct tss_struct __aligned(PAGE_SIZE) tss;
+};
+DECLARE_PER_CPU(struct tss_page, init_tss_page);
+#define per_cpu__init_tss get_per_cpu_var(init_tss_page.tss)
  
  #define IST_NONE 0UL
  #define IST_DF   1UL
@@ -463,7 +468,6 @@ static inline void disable_each_ist(idt_
  extern idt_entry_t idt_table[];
  extern idt_entry_t *idt_tables[];
  
-DECLARE_PER_CPU(struct tss_struct, init_tss);
  DECLARE_PER_CPU(root_pgentry_t *, root_pgt);
  
  extern void write_ptbase(struct vcpu *v);

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

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

[Andrew Cooper]

On 09/08/2019 13:32, Jan Beulich wrote:
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> TBD: Especially with how the previous patch now works I'm unconvinced of
>      the utility of the linker script alignment check. It in particular
>      doesn't check the property we're after in this patch, i.e. the fact
>      that there's nothing else in the same page.

It should now probably be a BUILD_BUG_ON() checking sizeof(tss_page)
being exactly a page, given that there is also a compile time alignment
assertion.

> NB: Sadly get_per_cpu_var() can't also be used on the "left" side of a
>     #define.

I don't understand what you are trying to imply with this.  That said, ...

> --- a/xen/include/asm-x86/processor.h
> +++ b/xen/include/asm-x86/processor.h
> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsign
>  #define IOBMP_BYTES             8192
>  #define IOBMP_INVALID_OFFSET    0x8000
>  
> -struct __packed __cacheline_aligned tss_struct {
> +struct __packed tss_struct {
>      uint32_t :32;
>      uint64_t rsp0, rsp1, rsp2;
>      uint64_t :64;
> @@ -425,6 +425,11 @@ struct __packed __cacheline_aligned tss_
>      /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>      uint8_t __cacheline_filler[24];
>  };
> +struct tss_page {
> +    struct tss_struct __aligned(PAGE_SIZE) tss;
> +};
> +DECLARE_PER_CPU(struct tss_page, init_tss_page);
> +#define per_cpu__init_tss get_per_cpu_var(init_tss_page.tss)

... my first attempt started by renaming init_tss because the init part
is bogus.  I believe we inherited this nomenclature from Linux, but that
doesn't make it right.

I referred the renaming patch specifically to aid in backportability,
but given these changes to the type, leaving it alone isn't an option.

I'm not happy with introducing this diversion, because it means that all
users of per_cpu(init_tss) now have the wrong types in their hand from
the point of view of reading the code.

I'm still uncertain which is the least bad option between backporting
this version, and backporting the version which adjusts all users -
there aren't too many, and its definitely not the most awkward backport
we've ever had to do.

I could be persuaded into keeping this version for legacy trees, so long
as it doesn't propagate into master.  i.e. this patch drops the init_
prefix, and I'll rebase my renaming as a 3/2 again which gets committed
at around about the same time.

That way we retain a non-deceptive code in master.

Thoughts?

~Andrew

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

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

[Jan Beulich]

On 09.08.2019 16:59, Andrew Cooper wrote:
> On 09/08/2019 13:32, Jan Beulich wrote:
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> TBD: Especially with how the previous patch now works I'm unconvinced of
>>       the utility of the linker script alignment check. It in particular
>>       doesn't check the property we're after in this patch, i.e. the fact
>>       that there's nothing else in the same page.
> 
> It should now probably be a BUILD_BUG_ON() checking sizeof(tss_page)
> being exactly a page, given that there is also a compile time alignment
> assertion.

Will do.

>> NB: Sadly get_per_cpu_var() can't also be used on the "left" side of a
>>      #define.
> 
> I don't understand what you are trying to imply with this.

It would be nice to be able to say

#define get_per_cpu_var(init_tss) get_per_cpu_var(init_tss_page.tss)

>  That said, ...
> 
>> --- a/xen/include/asm-x86/processor.h
>> +++ b/xen/include/asm-x86/processor.h
>> @@ -411,7 +411,7 @@ static always_inline void __mwait(unsign
>>   #define IOBMP_BYTES             8192
>>   #define IOBMP_INVALID_OFFSET    0x8000
>>   
>> -struct __packed __cacheline_aligned tss_struct {
>> +struct __packed tss_struct {
>>       uint32_t :32;
>>       uint64_t rsp0, rsp1, rsp2;
>>       uint64_t :64;
>> @@ -425,6 +425,11 @@ struct __packed __cacheline_aligned tss_
>>       /* Pads the TSS to be cacheline-aligned (total size is 0x80). */
>>       uint8_t __cacheline_filler[24];
>>   };
>> +struct tss_page {
>> +    struct tss_struct __aligned(PAGE_SIZE) tss;
>> +};
>> +DECLARE_PER_CPU(struct tss_page, init_tss_page);
>> +#define per_cpu__init_tss get_per_cpu_var(init_tss_page.tss)
> 
> ... my first attempt started by renaming init_tss because the init part
> is bogus.  I believe we inherited this nomenclature from Linux, but that
> doesn't make it right.
> 
> I referred the renaming patch specifically to aid in backportability,
> but given these changes to the type, leaving it alone isn't an option.
> 
> I'm not happy with introducing this diversion, because it means that all
> users of per_cpu(init_tss) now have the wrong types in their hand from
> the point of view of reading the code.
> 
> I'm still uncertain which is the least bad option between backporting
> this version, and backporting the version which adjusts all users -
> there aren't too many, and its definitely not the most awkward backport
> we've ever had to do.
> 
> I could be persuaded into keeping this version for legacy trees, so long
> as it doesn't propagate into master.  i.e. this patch drops the init_
> prefix, and I'll rebase my renaming as a 3/2 again which gets committed
> at around about the same time.
> 
> That way we retain a non-deceptive code in master.
> 
> Thoughts?

I have no problem at all dropping the init_ here. It's not clear to me
though whether you saying "I'm not happy with introducing this diversion"
implies anything further that you'd want or even expect me to change.

Jan

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

[Xen-devel] [PATCH v4 3/2] x86: Clean up TSS types and variable names

[Andrew Cooper]

The _struct suffix on tss_struct is quite redundant.  Rename it to tss64 to
mirror the existing tss32 structure we have in HVM's Task Switch logic.

The per-cpu name having an init_ prefix is also wrong.  There is exactly one
TSS for each CPU, which is used for the lifetime of the system.  Drop the
redirection and update all callers to use tss_page properly.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Wei Liu <wl@xen.org>
CC: Roger Pau Monné <roger.pau@citrix.com>

RFC, and based on my requested changes for patch 2.
---
 xen/arch/x86/cpu/common.c       | 8 ++++----
 xen/arch/x86/hvm/vmx/vmcs.c     | 2 +-
 xen/arch/x86/smpboot.c          | 2 +-
 xen/arch/x86/traps.c            | 8 ++++----
 xen/include/asm-x86/processor.h | 5 ++---
 5 files changed, 12 insertions(+), 13 deletions(-)

diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c
index dc2dea4d6d..797d970d9a 100644
--- a/xen/arch/x86/cpu/common.c
+++ b/xen/arch/x86/cpu/common.c
@@ -707,7 +707,7 @@ void load_system_tables(void)
 	unsigned long stack_bottom = get_stack_bottom(),
 		stack_top = stack_bottom & ~(STACK_SIZE - 1);
 
-	struct tss_struct *tss = &this_cpu(init_tss);
+	struct tss64 *tss = &this_cpu(tss_page).tss;
 	seg_desc_t *gdt =
 		this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY;
 	seg_desc_t *compat_gdt =
@@ -722,7 +722,7 @@ void load_system_tables(void)
 		.limit = (IDT_ENTRIES * sizeof(idt_entry_t)) - 1,
 	};
 
-	*tss = (struct tss_struct){
+	*tss = (struct tss64){
 		/* Main stack for interrupts/exceptions. */
 		.rsp0 = stack_bottom,
 
@@ -750,12 +750,12 @@ void load_system_tables(void)
 	_set_tssldt_desc(
 		gdt + TSS_ENTRY,
 		(unsigned long)tss,
-		offsetof(struct tss_struct, __cacheline_filler) - 1,
+		offsetof(struct tss64, __cacheline_filler) - 1,
 		SYS_DESC_tss_avail);
 	_set_tssldt_desc(
 		compat_gdt + TSS_ENTRY,
 		(unsigned long)tss,
-		offsetof(struct tss_struct, __cacheline_filler) - 1,
+		offsetof(struct tss64, __cacheline_filler) - 1,
 		SYS_DESC_tss_busy);
 
 	per_cpu(full_gdt_loaded, cpu) = false;
diff --git a/xen/arch/x86/hvm/vmx/vmcs.c b/xen/arch/x86/hvm/vmx/vmcs.c
index 098613822a..c438befe35 100644
--- a/xen/arch/x86/hvm/vmx/vmcs.c
+++ b/xen/arch/x86/hvm/vmx/vmcs.c
@@ -796,7 +796,7 @@ static void vmx_set_host_env(struct vcpu *v)
               (unsigned long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY));
     __vmwrite(HOST_IDTR_BASE, (unsigned long)idt_tables[cpu]);
 
-    __vmwrite(HOST_TR_BASE, (unsigned long)&per_cpu(init_tss, cpu));
+    __vmwrite(HOST_TR_BASE, (unsigned long)&per_cpu(tss_page, cpu).tss);
 
     __vmwrite(HOST_SYSENTER_ESP, get_stack_bottom());
 
diff --git a/xen/arch/x86/smpboot.c b/xen/arch/x86/smpboot.c
index 8d5fef0012..4833891bda 100644
--- a/xen/arch/x86/smpboot.c
+++ b/xen/arch/x86/smpboot.c
@@ -825,7 +825,7 @@ static int setup_cpu_root_pgt(unsigned int cpu)
     if ( !rc )
         rc = clone_mapping(idt_tables[cpu], rpt);
     if ( !rc )
-        rc = clone_mapping(&per_cpu(init_tss, cpu), rpt);
+        rc = clone_mapping(&per_cpu(tss_page, cpu).tss, rpt);
     if ( !rc )
         rc = clone_mapping((void *)per_cpu(stubs.addr, cpu), rpt);
 
diff --git a/xen/arch/x86/traps.c b/xen/arch/x86/traps.c
index 252fe6d792..b143def51c 100644
--- a/xen/arch/x86/traps.c
+++ b/xen/arch/x86/traps.c
@@ -565,7 +565,7 @@ void show_stack_overflow(unsigned int cpu, const struct cpu_user_regs *regs)
 
     printk("Valid stack range: %p-%p, sp=%p, tss.rsp0=%p\n",
            (void *)esp_top, (void *)esp_bottom, (void *)esp,
-           (void *)per_cpu(init_tss, cpu).rsp0);
+           (void *)per_cpu(tss_page, cpu).tss.rsp0);
 
     /*
      * Trigger overflow trace if %esp is anywhere within the guard page, or
@@ -1903,7 +1903,7 @@ static void __init set_intr_gate(unsigned int n, void *addr)
 
 void load_TR(void)
 {
-    struct tss_struct *tss = &this_cpu(init_tss);
+    struct tss64 *tss = &this_cpu(tss_page).tss;
     struct desc_ptr old_gdt, tss_gdt = {
         .base = (long)(this_cpu(gdt) - FIRST_RESERVED_GDT_ENTRY),
         .limit = LAST_RESERVED_GDT_BYTE
@@ -1912,12 +1912,12 @@ void load_TR(void)
     _set_tssldt_desc(
         this_cpu(gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
-        offsetof(struct tss_struct, __cacheline_filler) - 1,
+        offsetof(struct tss64, __cacheline_filler) - 1,
         SYS_DESC_tss_avail);
     _set_tssldt_desc(
         this_cpu(compat_gdt) + TSS_ENTRY - FIRST_RESERVED_GDT_ENTRY,
         (unsigned long)tss,
-        offsetof(struct tss_struct, __cacheline_filler) - 1,
+        offsetof(struct tss64, __cacheline_filler) - 1,
         SYS_DESC_tss_busy);
 
     /* Switch to non-compat GDT (which has B bit clear) to execute LTR. */
diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h
index 6f180775f4..4c2710ca2b 100644
--- a/xen/include/asm-x86/processor.h
+++ b/xen/include/asm-x86/processor.h
@@ -411,7 +411,7 @@ static always_inline void __mwait(unsigned long eax, unsigned long ecx)
 #define IOBMP_BYTES             8192
 #define IOBMP_INVALID_OFFSET    0x8000
 
-struct __packed tss_struct {
+struct __packed tss64 {
     uint32_t :32;
     uint64_t rsp0, rsp1, rsp2;
     uint64_t :64;
@@ -426,10 +426,9 @@ struct __packed tss_struct {
     uint8_t __cacheline_filler[24];
 };
 struct tss_page {
-    struct tss_struct __aligned(PAGE_SIZE) tss;
+    struct tss64 __aligned(PAGE_SIZE) tss;
 };
 DECLARE_PER_CPU(struct tss_page, tss_page);
-#define per_cpu__init_tss get_per_cpu_var(tss_page.tss)
 
 #define IST_NONE 0UL
 #define IST_DF   1UL
-- 
2.11.0


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

Re: [Xen-devel] [PATCH v4 3/2] x86: Clean up TSS types and variable names

[Andrew Cooper]

On 09/08/2019 16:17, Andrew Cooper wrote:
> The _struct suffix on tss_struct is quite redundant.  Rename it to tss64 to
> mirror the existing tss32 structure we have in HVM's Task Switch logic.
>
> The per-cpu name having an init_ prefix is also wrong.  There is exactly one
> TSS for each CPU, which is used for the lifetime of the system.  Drop the
> redirection and update all callers to use tss_page properly.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Jan Beulich <JBeulich@suse.com>
> CC: Wei Liu <wl@xen.org>
> CC: Roger Pau Monné <roger.pau@citrix.com>
>
> RFC, and based on my requested changes for patch 2.

Actually, the more I think about this, the more I think this should be
folded into patch 2 and backported in this form.

~Andrew

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