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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

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