| Message ID | fdacbb8b-5a58-4d6b-89b2-ea98b4fed895@suse.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | x86: CET-SS related adjustments | expand |
On 28/02/2024 1:53 pm, Jan Beulich wrote: > --- a/xen/arch/x86/traps.c > +++ b/xen/arch/x86/traps.c > @@ -539,6 +544,50 @@ static void show_trace(const struct cpu_ > !is_active_kernel_text(tos) ) > printk(" [<%p>] R %pS\n", _p(regs->rip), _p(regs->rip)); > > + if ( IS_ENABLED(CONFIG_XEN_SHSTK) && rdssp() != SSP_NO_SHSTK ) > + { > + const unsigned long *ptr = _p(regs->entry_ssp); > + unsigned int n; > + > + for ( n = 0; (unsigned long)ptr & (PAGE_SIZE - sizeof(*ptr)); ++n ) > + { > + unsigned long val = *ptr; > + > + if ( is_active_kernel_text(val) || in_stub(val) ) > + { > + /* Normal return address entry. */ > + printk(" [<%p>] C %pS\n", _p(val), _p(val)); > + ++ptr; > + } > + else if ( !((val ^ *ptr) >> (PAGE_SHIFT + STACK_ORDER)) ) > + { > + if ( val & (sizeof(val) - 1) ) > + { > + /* Most likely a supervisor token. */ > + break; > + } Tokens are their own linear address, with metadata in the bottom two bits. I think it would be better to check that explicitly, rather than assuming anything nonzero in the upper bits is a token. > + > + /* > + * Ought to be a hypervisor interruption frame. But don't > + * (re)log the current frame's %rip. > + */ > + if ( n || ptr[1] != regs->rip ) > + printk(" [<%p>] E %pS\n", _p(ptr[1]), _p(ptr[1])); > + ptr = _p(val); > + } > + else > + { > + /* Ought to be a PV guest hypercall/interruption frame. */ > + printk(" %04lx:[<%p>] E\n", ptr[2], _p(ptr[1])); > + ptr = 0; On a CPL3 -> CPL0 transition, the guest's SSP is written back into MSR_PL3_SSP. The supervisor token on MSR_PL0_SSP is marked busy (either automatically, or by SETSSBY), but nothing pertaining to CPL3 is pushed onto the supervisor shadow stack. This is why we can move off an IST stack onto the primary stack when interrupting CPL3 with only a CLEARSSBSY/SETSSBSY pair, and no memmove() loop of WRSS's. In other words, I'm pretty sure this is a dead codeapth. (Or worse, if it happens not to be dead, then the comment is misleading.) A CPL1 -> CPL0 transition does push an shstk interrupt frame, and not wanting to memmove() the shstk by 3 slots on a context switch is part of why I just disallowed PV32 guests when CET was active. ~Andrew
On 28.02.2024 17:15, Andrew Cooper wrote: > On 28/02/2024 1:53 pm, Jan Beulich wrote: >> --- a/xen/arch/x86/traps.c >> +++ b/xen/arch/x86/traps.c >> @@ -539,6 +544,50 @@ static void show_trace(const struct cpu_ >> !is_active_kernel_text(tos) ) >> printk(" [<%p>] R %pS\n", _p(regs->rip), _p(regs->rip)); >> >> + if ( IS_ENABLED(CONFIG_XEN_SHSTK) && rdssp() != SSP_NO_SHSTK ) >> + { >> + const unsigned long *ptr = _p(regs->entry_ssp); >> + unsigned int n; >> + >> + for ( n = 0; (unsigned long)ptr & (PAGE_SIZE - sizeof(*ptr)); ++n ) >> + { >> + unsigned long val = *ptr; >> + >> + if ( is_active_kernel_text(val) || in_stub(val) ) >> + { >> + /* Normal return address entry. */ >> + printk(" [<%p>] C %pS\n", _p(val), _p(val)); >> + ++ptr; >> + } >> + else if ( !((val ^ *ptr) >> (PAGE_SHIFT + STACK_ORDER)) ) >> + { >> + if ( val & (sizeof(val) - 1) ) >> + { >> + /* Most likely a supervisor token. */ >> + break; >> + } > > Tokens are their own linear address, with metadata in the bottom two > bits. I think it would be better to check that explicitly, rather than > assuming anything nonzero in the upper bits is a token. Well, yes. What you don't say though is what to do in such an event (other than simply breaking out of the loop). The lack of a clear route here is why I've done it the "wider" way. And why hence the comment says "likely". >> + >> + /* >> + * Ought to be a hypervisor interruption frame. But don't >> + * (re)log the current frame's %rip. >> + */ >> + if ( n || ptr[1] != regs->rip ) >> + printk(" [<%p>] E %pS\n", _p(ptr[1]), _p(ptr[1])); >> + ptr = _p(val); >> + } >> + else >> + { >> + /* Ought to be a PV guest hypercall/interruption frame. */ >> + printk(" %04lx:[<%p>] E\n", ptr[2], _p(ptr[1])); >> + ptr = 0; > > On a CPL3 -> CPL0 transition, the guest's SSP is written back into > MSR_PL3_SSP. The supervisor token on MSR_PL0_SSP is marked busy (either > automatically, or by SETSSBY), but nothing pertaining to CPL3 is pushed > onto the supervisor shadow stack. > > This is why we can move off an IST stack onto the primary stack when > interrupting CPL3 with only a CLEARSSBSY/SETSSBSY pair, and no memmove() > loop of WRSS's. > > In other words, I'm pretty sure this is a dead codeapth. (Or worse, if > it happens not to be dead, then the comment is misleading.) IOW you're confirming the respective post-commit-message remark. Then, like above, the question here similarly is: What's the most reasonable thing to do in the final "else"? > A CPL1 -> CPL0 transition does push an shstk interrupt frame, and not > wanting to memmove() the shstk by 3 slots on a context switch is part of > why I just disallowed PV32 guests when CET was active. Hmm, yes, such would be needed when switching to PV32. When switching from PV32, such moving wouldn't be necessary, would it? Upon exiting to ring3 the ring0 shadow stack isn't consulted (beyond an alignment check on SSP), and upon next entry (from ring3) the proper base ring0 would be used again. For VMX it would apparently be a matter of using the "load CET" VM-exit control to get SSP similarly reset to its base value. For SVM I'm afraid I can't find any explicit information in the PM as to what happens during #VMEXIT (nor what, if anything, is saved by VMRUN). Jan
--- a/xen/arch/x86/traps.c +++ b/xen/arch/x86/traps.c @@ -449,6 +449,11 @@ unsigned long get_stack_dump_bottom(unsi } } +static bool in_stub(unsigned long addr) +{ + return !((this_cpu(stubs.addr) ^ addr) >> STUB_BUF_SHIFT); +} + #if !defined(CONFIG_FRAME_POINTER) /* @@ -539,6 +544,50 @@ static void show_trace(const struct cpu_ !is_active_kernel_text(tos) ) printk(" [<%p>] R %pS\n", _p(regs->rip), _p(regs->rip)); + if ( IS_ENABLED(CONFIG_XEN_SHSTK) && rdssp() != SSP_NO_SHSTK ) + { + const unsigned long *ptr = _p(regs->entry_ssp); + unsigned int n; + + for ( n = 0; (unsigned long)ptr & (PAGE_SIZE - sizeof(*ptr)); ++n ) + { + unsigned long val = *ptr; + + if ( is_active_kernel_text(val) || in_stub(val) ) + { + /* Normal return address entry. */ + printk(" [<%p>] C %pS\n", _p(val), _p(val)); + ++ptr; + } + else if ( !((val ^ *ptr) >> (PAGE_SHIFT + STACK_ORDER)) ) + { + if ( val & (sizeof(val) - 1) ) + { + /* Most likely a supervisor token. */ + break; + } + + /* + * Ought to be a hypervisor interruption frame. But don't + * (re)log the current frame's %rip. + */ + if ( n || ptr[1] != regs->rip ) + printk(" [<%p>] E %pS\n", _p(ptr[1]), _p(ptr[1])); + ptr = _p(val); + } + else + { + /* Ought to be a PV guest hypercall/interruption frame. */ + printk(" %04lx:[<%p>] E\n", ptr[2], _p(ptr[1])); + ptr = 0; + } + } + + /* Fall back to legacy stack trace if nothing was logged at all. */ + if ( n ) + return; + } + if ( fault ) { printk(" [Fault on access]\n");
Shadow stacks contain little more than return addresses, and they in particular allow precise call traces also without FRAME_POINTER. Signed-off-by: Jan Beulich <jbeulich@suse.com> --- While the 'E' for exception frames is probably okay, I'm not overly happy with the 'C' (for CET). I would have preferred 'S' (for shadow), but we use that character already. As an alternative to suppressing output for the top level exception frame, adding the new code ahead of the 'R' output line (and then also ahead of the stack top read) could be considered. Perhaps having a printk() for the PV entry case is meaningless, for - no frame being pushed when entered from CPL=3 (64-bit PV), - no entry possible from CPL<3 (32-bit PV disabled when CET is active)? In which case the comment probably should just be "Bogus." and the code merely be "break;". Quite likely a number of other uses of is_active_kernel_text() also want amending with in_stub().