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

[Andrew Cooper <Andrew.Cooper3@xxxxxxxxxx>]

On 16/03/2022 08:33, Jan Beulich wrote:
> 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.)

This is practically a textbook definition of a latent bug.  Using one of
a number of functions in Xen will either read utter garbage off the
stack, or clobber the stack frame and most likely a return address, and
the reason this hasn't exploded is luck, not design.

>
>> --- 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 ...

It does, now that I've rebased on staging.


>> --- 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 already occupies space, because of mkelf32.

>  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.

Living in .bss would prevent it from being reclaimed.  We've got several
nasty bugs from shooting holes in the Xen image, and too many special
cases already.

Furthermore, we're talking about initdata here.  Size is not a concern,
especially when its 7-9 orders of magnitude smaller than typical systems.

The choice here is between between (theoretically but not in practice)
extra space on disk, and not reclaiming 32k of init data after boot.

>> -       . = 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.

It would actually be a severe error for there to be anything in here
with less than pointer alignment, because of how the section gets
walked.  But I'll keep the ALIGN() in.

~Andrew