[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [PATCH] x86/cet: Remove writeable mapping of the BSPs shadow stack
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
|
![]() |
Lists.xenproject.org is hosted with RackSpace, monitoring our |