| Message ID | 20200501225838.9866-9-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | x86: Support for CET Supervisor Shadow Stacks | expand |
On 02.05.2020 00:58, Andrew Cooper wrote: > --- a/xen/arch/x86/cpu/common.c > +++ b/xen/arch/x86/cpu/common.c > @@ -748,6 +748,25 @@ void load_system_tables(void) > .bitmap = IOBMP_INVALID_OFFSET, > }; > > + /* Set up the shadow stack IST. */ > + if ( cpu_has_xen_shstk ) { This being a Linux style function, you want to omit the blanks immediately inside the parentheses bother here and in the for() below. > + unsigned int i; > + uint64_t *ist_ssp = this_cpu(tss_page).ist_ssp; > + > + /* Must point at the supervisor stack token. */ > + ist_ssp[IST_MCE] = stack_top + (IST_MCE * 0x400) - 8; > + ist_ssp[IST_NMI] = stack_top + (IST_NMI * 0x400) - 8; > + ist_ssp[IST_DB] = stack_top + (IST_DB * 0x400) - 8; > + ist_ssp[IST_DF] = stack_top + (IST_DF * 0x400) - 8; Introduce a constant for 0x400, to then also be used in the invocations of write_sss_token()? > + /* Poision unused entries. */ > + for ( i = IST_MAX; > + i < ARRAY_SIZE(this_cpu(tss_page).ist_ssp); ++i ) > + ist_ssp[i] = 0x8600111111111111ul; IST_MAX == IST_DF, so you're overwriting one token here. > --- a/xen/include/asm-x86/processor.h > +++ b/xen/include/asm-x86/processor.h > @@ -434,7 +434,8 @@ struct __packed tss64 { > uint16_t :16, bitmap; > }; > struct tss_page { > - struct tss64 __aligned(PAGE_SIZE) tss; > + uint64_t __aligned(PAGE_SIZE) ist_ssp[8]; > + struct tss64 tss; > }; Just curious - any particular reason you put this ahead of the TSS? Jan
On 04/05/2020 15:55, Jan Beulich wrote: >> + /* Poision unused entries. */ >> + for ( i = IST_MAX; >> + i < ARRAY_SIZE(this_cpu(tss_page).ist_ssp); ++i ) >> + ist_ssp[i] = 0x8600111111111111ul; > IST_MAX == IST_DF, so you're overwriting one token here. And failing to poison entry 0. This was a bad rearrangement when tidying the series up. Unfortunately, testing the #DF path isn't terribly easy. >> --- a/xen/include/asm-x86/processor.h >> +++ b/xen/include/asm-x86/processor.h >> @@ -434,7 +434,8 @@ struct __packed tss64 { >> uint16_t :16, bitmap; >> }; >> struct tss_page { >> - struct tss64 __aligned(PAGE_SIZE) tss; >> + uint64_t __aligned(PAGE_SIZE) ist_ssp[8]; >> + struct tss64 tss; >> }; > Just curious - any particular reason you put this ahead of the TSS? Yes. Reduced chance of interacting with a buggy IO bitmap offset. Furthermore, we could do away most of the IO emulation quirking, and the #GP path overhead, if we actually constructed a real IO bitmap for dom0. That would require using the 8k following the TSS. ~Andrew
diff --git a/xen/arch/x86/cpu/common.c b/xen/arch/x86/cpu/common.c index 290f9f1c30..3962717aa5 100644 --- a/xen/arch/x86/cpu/common.c +++ b/xen/arch/x86/cpu/common.c @@ -748,6 +748,25 @@ void load_system_tables(void) .bitmap = IOBMP_INVALID_OFFSET, }; + /* Set up the shadow stack IST. */ + if ( cpu_has_xen_shstk ) { + unsigned int i; + uint64_t *ist_ssp = this_cpu(tss_page).ist_ssp; + + /* Must point at the supervisor stack token. */ + ist_ssp[IST_MCE] = stack_top + (IST_MCE * 0x400) - 8; + ist_ssp[IST_NMI] = stack_top + (IST_NMI * 0x400) - 8; + ist_ssp[IST_DB] = stack_top + (IST_DB * 0x400) - 8; + ist_ssp[IST_DF] = stack_top + (IST_DF * 0x400) - 8; + + /* Poision unused entries. */ + for ( i = IST_MAX; + i < ARRAY_SIZE(this_cpu(tss_page).ist_ssp); ++i ) + ist_ssp[i] = 0x8600111111111111ul; + + wrmsrl(MSR_INTERRUPT_SSP_TABLE, (unsigned long)ist_ssp); + } + BUILD_BUG_ON(sizeof(*tss) <= 0x67); /* Mandated by the architecture. */ _set_tssldt_desc(gdt + TSS_ENTRY, (unsigned long)tss, diff --git a/xen/arch/x86/mm.c b/xen/arch/x86/mm.c index bc44d865ef..4e2c3c9735 100644 --- a/xen/arch/x86/mm.c +++ b/xen/arch/x86/mm.c @@ -6000,12 +6000,28 @@ void memguard_unguard_range(void *p, unsigned long l) #endif -void memguard_guard_stack(void *p) +static void write_sss_token(unsigned long *ptr) { - map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, _PAGE_NONE); + /* + * A supervisor shadow stack token is its own linear address, with the + * busy bit (0) clear. + */ + *ptr = (unsigned long)ptr; +} +void memguard_guard_stack(void *p) +{ + /* IST Shadow stacks. 4x 1k in stack page 0. */ + write_sss_token(p + 0x3f8); + write_sss_token(p + 0x7f8); + write_sss_token(p + 0xbf8); + write_sss_token(p + 0xff8); + map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, PAGE_HYPERVISOR_SHSTK); + + /* Primary Shadow Stack. 1x 4k in stack page 5. */ p += 5 * PAGE_SIZE; - map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, _PAGE_NONE); + write_sss_token(p + 0xff8); + map_pages_to_xen((unsigned long)p, virt_to_mfn(p), 1, PAGE_HYPERVISOR_SHSTK); } void memguard_unguard_stack(void *p) diff --git a/xen/include/asm-x86/page.h b/xen/include/asm-x86/page.h index 5acf3d3d5a..f632affaef 100644 --- a/xen/include/asm-x86/page.h +++ b/xen/include/asm-x86/page.h @@ -364,6 +364,7 @@ void efi_update_l4_pgtable(unsigned int l4idx, l4_pgentry_t); _PAGE_DIRTY | _PAGE_RW) #define __PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR | _PAGE_PCD) #define __PAGE_HYPERVISOR_UC (__PAGE_HYPERVISOR | _PAGE_PCD | _PAGE_PWT) +#define __PAGE_HYPERVISOR_SHSTK (__PAGE_HYPERVISOR_RO | _PAGE_DIRTY) #define MAP_SMALL_PAGES _PAGE_AVAIL0 /* don't use superpages mappings */ diff --git a/xen/include/asm-x86/processor.h b/xen/include/asm-x86/processor.h index f7e80d12e4..54e1a8b605 100644 --- a/xen/include/asm-x86/processor.h +++ b/xen/include/asm-x86/processor.h @@ -434,7 +434,8 @@ struct __packed tss64 { uint16_t :16, bitmap; }; struct tss_page { - struct tss64 __aligned(PAGE_SIZE) tss; + uint64_t __aligned(PAGE_SIZE) ist_ssp[8]; + struct tss64 tss; }; DECLARE_PER_CPU(struct tss_page, tss_page); diff --git a/xen/include/asm-x86/x86_64/page.h b/xen/include/asm-x86/x86_64/page.h index 9876634881..26621f9519 100644 --- a/xen/include/asm-x86/x86_64/page.h +++ b/xen/include/asm-x86/x86_64/page.h @@ -171,6 +171,7 @@ static inline intpte_t put_pte_flags(unsigned int x) #define PAGE_HYPERVISOR_RW (__PAGE_HYPERVISOR_RW | _PAGE_GLOBAL) #define PAGE_HYPERVISOR_RX (__PAGE_HYPERVISOR_RX | _PAGE_GLOBAL) #define PAGE_HYPERVISOR_RWX (__PAGE_HYPERVISOR | _PAGE_GLOBAL) +#define PAGE_HYPERVISOR_SHSTK (__PAGE_HYPERVISOR_SHSTK | _PAGE_GLOBAL) #define PAGE_HYPERVISOR PAGE_HYPERVISOR_RW #define PAGE_HYPERVISOR_UCMINUS (__PAGE_HYPERVISOR_UCMINUS | \
Introduce HYPERVISOR_SHSTK pagetable constants, which are Read-Only + Dirty. Use these in place of _PAGE_NONE for memguard_guard_stack(). Supervisor shadow stacks need a token written at the top, which is most easily done before making the frame read only. Allocate the shadow IST stack block in struct tss_page. It doesn't strictly need to live here, but it is a convenient location (and XPTI-safe, for testing purposes). Have load_system_tables() set up the shadow IST stack table when setting up the regular IST in the TSS. 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> --- xen/arch/x86/cpu/common.c | 19 +++++++++++++++++++ xen/arch/x86/mm.c | 22 +++++++++++++++++++--- xen/include/asm-x86/page.h | 1 + xen/include/asm-x86/processor.h | 3 ++- xen/include/asm-x86/x86_64/page.h | 1 + 5 files changed, 42 insertions(+), 4 deletions(-)