On Mon, Oct 5, 2020 at 7:29 PM Sean Christopherson
<sean.j.christopher...@intel.com> wrote:
>
> 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 <l...@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?
Good catch.
>
> > * 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?
Sure.
load_gs_index() makes perfect sense to me because I've been drinking
the kool-aid for too long. Maybe some day we should rename it, but
I"m not sure what the best name would be. set_gs_update_user_base()?
The semantics are that it loads GS except that it changes the user
GSBASE instead of the kernel GSBASE. Thanks, AMD.
--Andy
