On 07/05/2020 14:17, Jan Beulich wrote: > [CAUTION - EXTERNAL EMAIL] DO NOT reply, click links, or open attachments > unless you have verified the sender and know the content is safe. > > On 02.05.2020 00:58, Andrew Cooper wrote: >> We need to unwind up to the supervisor token. See the comment for details. >> >> Signed-off-by: Andrew Cooper <andrew.coop...@citrix.com> >> --- >> CC: Jan Beulich <jbeul...@suse.com> >> CC: Wei Liu <w...@xen.org> >> CC: Roger Pau Monné <roger....@citrix.com> >> --- >> xen/include/asm-x86/current.h | 42 >> +++++++++++++++++++++++++++++++++++++++--- >> 1 file changed, 39 insertions(+), 3 deletions(-) >> >> diff --git a/xen/include/asm-x86/current.h b/xen/include/asm-x86/current.h >> index 99b66a0087..2a7b728b1e 100644 >> --- a/xen/include/asm-x86/current.h >> +++ b/xen/include/asm-x86/current.h >> @@ -124,13 +124,49 @@ unsigned long get_stack_dump_bottom (unsigned long sp); >> # define CHECK_FOR_LIVEPATCH_WORK "" >> #endif >> >> +#ifdef CONFIG_XEN_SHSTK >> +/* >> + * We need to unwind the primary shadow stack to its supervisor token, >> located >> + * at 0x5ff8 from the base of the stack blocks. >> + * >> + * Read the shadow stack pointer, subtract it from 0x5ff8, divide by 8 to >> get >> + * the number of slots needing popping. >> + * >> + * INCSSPQ can't pop more than 255 entries. We shouldn't ever need to pop >> + * that many entries, and getting this wrong will cause us to #DF later. >> + */ >> +# define SHADOW_STACK_WORK \ >> + "mov $1, %[ssp];" \ >> + "rdsspd %[ssp];" \ >> + "cmp $1, %[ssp];" \ >> + "je 1f;" /* CET not active? Skip. */ \ >> + "mov $"STR(0x5ff8)", %[val];" \ > As per comments on earlier patches, I think it would be nice if > this wasn't a literal number here, but tied to actual stack > layout via some suitable expression. An option might be to use > 0xff8 (or the constant to be introduced for it in the earlier > patch) here and ... > >> + "and $"STR(STACK_SIZE - 1)", %[ssp];" \ > ... PAGE_SIZE here.
It is important to use STACK_SIZE here and not PAGE_SIZE to trigger... >> + "sub %[ssp], %[val];" \ >> + "shr $3, %[val];" \ >> + "cmp $255, %[val];" \ >> + "jle 2f;" \ ... this condition if we try to reset&jump from more than 4k away from 0x5ff8, e.g. from a IST stack. Whatever happens we're going to crash, but given that we're talking about imm32's here, > Perhaps better "jbe", treating the unsigned values as such? What I really want is actually to opencode an UNLIKLEY() region seeing none of our infrastructure works inside inline asm. Same for... > >> + "ud2a;" \ ... this to turn into a real BUG. ~Andrew
