Re: [PATCH] x86/cet: Remove writeable mapping of the BSPs shadow stack

[Jan Beulich <jbeulich@xxxxxxxx>]

On 15.03.2022 17:53, Andrew Cooper wrote:
> An unintended consequence of the BSP using cpu0_stack[] is that writeable
> mappings to the BSPs shadow stacks are retained in the bss.  This renders
> CET-SS almost useless, as an attacker can update both return addresses and the
> ret will not fault.
> 
> We specifically don't want the shatter the superpage mapping .data/.bss, so
> the only way to fix this is to not have the BSP stack in the main Xen image.
> 
> Break cpu_alloc_stack() out of cpu_smpboot_alloc(), and dynamically allocate
> the BSP stack as early as reasonable in __start_xen().  As a consequence,
> there is no need to delay the BSP's memguard_guard_stack() call.
> 
> Copy the top of cpu info block just before switching to use the new stack.
> Fix a latent bug by setting %rsp to info->guest_cpu_user_regs rather than
> ->es; this would be buggy if reinit_bsp_stack() called schedule() (which
> rewrites the GPR block) directly, but luckily it doesn't.

While I don't mind the change, I also don't view the original code as
latently buggy. (Just a remark, not a request to change anything.)

> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -148,7 +148,7 @@ cpumask_t __read_mostly cpu_present_map;
>  
>  unsigned long __read_mostly xen_phys_start;
>  
> -char __section(".bss.stack_aligned") __aligned(STACK_SIZE)
> +char __section("init.bss.stack_aligned") __aligned(STACK_SIZE)
>      cpu0_stack[STACK_SIZE];

I guess the section name was meant to start with a dot, matching
the linker script change? You should actually have seen
--orphan-handling in action here ...

> --- a/xen/arch/x86/xen.lds.S
> +++ b/xen/arch/x86/xen.lds.S
> @@ -215,8 +215,9 @@ SECTIONS
>    } PHDR(text)
>    DECL_SECTION(.init.data) {
>  #endif
> +       . = ALIGN(STACK_SIZE);
> +       *(.init.bss.stack_aligned)

No real need for the ALIGN() here - it's the contributions to the
section which ought to come with proper alignment. Imo ALIGN()
should only ever be there ahead of a symbol definition, as otherwise
the symbol might not mark what it is intended to mark due to padding
which might be inserted. See also 01fe4da6243b ("x86: force suitable
alignment in sources rather than in linker script").

Really we should consider using

    *(SORT_BY_ALIGNMENT(.init.data .init.data.* .init.bss.*))

While I can see your point against forcing sorting by alignment
globally, this very argument doesn't apply here (at least until
there appeared a way for the section attribute and -fdata-sections
to actually interact, such that .init.* could also become per-
function/object).

Then again - this block of zeroes doesn't need to occupy space in
the binary. It could very well live in a @nobits .init.bss in the
final ELF binary. But sadly the section isn't @nobits in the object
file, and with that there would need to be a way to make the linker
convert it to @nobits (and I'm unaware of such). What would work is
naming the section .bss.init.stack_aligned (or e.g.
.bss..init.stack_aligned to make it easier to separate it from
.bss.* in the linker script) - that'll make gcc mark it @nobits.

> -       . = ALIGN(POINTER_ALIGN);
>         __initdata_cf_clobber_start = .;

As a consequence, this ALIGN() shouldn't go away. The only present
contribution to the section is as large as its alignment, but that's
not generally a requirement.

Jan