Re: [PATCH 1/2] x86/stackprotector/32: Make the canary into a regular percpu variable

[Sean Christopherson <sean.j.christopherson@intel.com>]

On Mon, Oct 05, 2020 at 12:30:03PM -0700, Andy Lutomirski wrote:
> On 32-bit kernels, the stackprotector canary is quite nasty -- it is
> stored at %gs:(20), which is nasty because 32-bit kernels use %fs for
> percpu storage.  It's even nastier because it means that whether %gs
> contains userspace state or kernel state while running kernel code
> sepends on whether stackprotector is enabled (this is

  depends

> CONFIG_X86_32_LAZY_GS), and this setting radically changes the way
> that segment selectors work.  Supporting both variants is a
> maintenance and testing mess.
> 
> Merely rearranging so that percpu and the stack canary
> share the same segment would be messy as the 32-bit percpu address
> layout isn't currently compatible with putting a variable at a fixed
> offset.
> 
> Fortunately, GCC 8.1 added options that allow the stack canary to be
> accessed as %fs:stack_canary, effectively turning it into an ordinary
> percpu variable.  This lets us get rid of all of the code to manage
> the stack canary GDT descriptor and the CONFIG_X86_32_LAZY_GS mess.
> 
> This patch forcibly disables stackprotector on older compilers that
> don't support the new options and makes the stack canary into a
> percpu variable.

It'd be helpful to explicitly state that the so called "lazy GS" approach is
now always used for i386.

> Signed-off-by: Andy Lutomirski <luto@kernel.org>
> ---

...

> diff --git a/arch/x86/include/asm/suspend_32.h b/arch/x86/include/asm/suspend_32.h
> index fdbd9d7b7bca..eb872363ca82 100644
> --- a/arch/x86/include/asm/suspend_32.h
> +++ b/arch/x86/include/asm/suspend_32.h
> @@ -16,9 +16,7 @@ struct saved_context {
>  	 * On x86_32, all segment registers, with the possible exception of

Is this still a "possible" exception, or is it now always an exception?

>  	 * gs, are saved at kernel entry in pt_regs.
>  	 */
> -#ifdef CONFIG_X86_32_LAZY_GS
>  	u16 gs;
> -#endif
>  	unsigned long cr0, cr2, cr3, cr4;
>  	u64 misc_enable;
>  	bool misc_enable_saved;

...

> diff --git a/arch/x86/kernel/tls.c b/arch/x86/kernel/tls.c
> index 64a496a0687f..3c883e064242 100644
> --- a/arch/x86/kernel/tls.c
> +++ b/arch/x86/kernel/tls.c
> @@ -164,17 +164,11 @@ int do_set_thread_area(struct task_struct *p, int idx,
>  		savesegment(fs, sel);
>  		if (sel == modified_sel)
>  			loadsegment(fs, sel);
> -
> -		savesegment(gs, sel);
> -		if (sel == modified_sel)
> -			load_gs_index(sel);
>  #endif
>  
> -#ifdef CONFIG_X86_32_LAZY_GS
>  		savesegment(gs, sel);
>  		if (sel == modified_sel)
> -			loadsegment(gs, sel);
> -#endif
> +			load_gs_index(sel);

Side topic, the "index" part of this is super confusing.  I had to reread
this entire patch after discovering load_gs_index is loadsegment on i386.

Maybe also worth a shout out in the changelog?

>  	} else {
>  #ifdef CONFIG_X86_64
>  		if (p->thread.fsindex == modified_sel)