[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v2 08/14] x86/cpu: Adjust reset_stack_and_jump() to be shadow stack compatible



On 27.05.2020 21:18, Andrew Cooper wrote:
> We need to unwind up to the supervisor token.  See the comment for details.
> 
> The use of UNLIKELY_END_SECTION in this case highlights that it isn't safe
> when it isn't the final statement of an asm().  Adjust all declarations with a
> newline.

That's only one perspective to take. I'd appreciate if you undid this.
The intention has always been ...

> --- a/xen/include/asm-x86/current.h
> +++ b/xen/include/asm-x86/current.h
> @@ -124,13 +124,55 @@ 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.  
> Turn
> + * it into a BUG() now for fractionally easier debugging.
> + */
> +# define SHADOW_STACK_WORK                                      \
> +    "mov $1, %[ssp];"                                           \
> +    "rdsspd %[ssp];"                                            \
> +    "cmp $1, %[ssp];"                                           \
> +    "je .L_shstk_done.%=;" /* CET not active?  Skip. */         \
> +    "mov $%c[skstk_base], %[val];"                              \
> +    "and $%c[stack_mask], %[ssp];"                              \
> +    "sub %[ssp], %[val];"                                       \
> +    "shr $3, %[val];"                                           \
> +    "cmp $255, %[val];" /* More than 255 entries?  Crash. */    \
> +    UNLIKELY_START(a, shstk_adjust)                             \

... to put suitable separators at the use sites (which, seeing all
the other semicolons here, would be a semicolon then, not a
newline/tab combination. If a tab was to be put anywhere, then like
this:

#define UNLIKELY_START_SECTION "\t.pushsection .text.unlikely,\"ax\""
#define UNLIKELY_END_SECTION   "\t.popsection"

as directives (from a cross arch perspective) are supposed to be
indented; it's just that on most arch-es it doesn't matter (and
hence is irrelevant here).

Preferably with this aspect undone
Reviewed-by: Jan Beulich <jbeulich@xxxxxxxx>

As to the comment, could I talk you into replacing the two 0x5ff8
instances by something like "last word of the shadow stack"?

Jan



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.