Re: [PATCH 4/4] x86: prefer shadow stack for producing call traces

[Jan Beulich <jbeulich@xxxxxxxx>]

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