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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <CALCETrXcFDZ4U_nkDK=r4WBmWtM954roGzNY_vBxippUiBTjTg@mail.gmail.com>
Date:   Tue, 6 Oct 2020 18:16:07 -0700
From:   Andy Lutomirski <luto@...nel.org>
To:     Brian Gerst <brgerst@...il.com>
Cc:     Andy Lutomirski <luto@...nel.org>,
        "the arch/x86 maintainers" <x86@...nel.org>,
        LKML <linux-kernel@...r.kernel.org>,
        "Christopherson, Sean J" <sean.j.christopherson@...el.com>
Subject: Re: [PATCH 1/2] x86/stackprotector/32: Make the canary into a regular
 percpu variable

On Tue, Oct 6, 2020 at 10:14 AM Brian Gerst <brgerst@...il.com> wrote:
>
> On Mon, Oct 5, 2020 at 3:31 PM Andy Lutomirski <luto@...nel.org> 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
> > 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.
>
> This doesn't consider !SMP builds, where per-cpu variable are just
> normal variables, and the FS segment is disabled.  Unfortunately,
> -mstack-protector-guard-reg=ds is not accepted.  The simplest fix
> would be to define  __KERNEL_PERCPU when either SMP or STACKPROTECTOR
> are enabled.

I don't love this because it breaks the "stack canary is just a
regular PERCPU variable" idea.  GCC accepts
-mstack-protector-guard=global to get rid of the segment override, but
then it ignores -mstack-protector-guard-offset=stack_canary.  So I
could do this:

#ifdef CONFIG_SMP
EXPORT_PER_CPU_SYMBOL(stack_canary);
#else
/*
 * GCC can't use a symbol called 'stack_canary' as the stack canary with
 * an FS or GS segment override, and SMP=n percpu variables are just normal
 * variables.  But GCC can use '__stack_chk_guard'.
 */
extern unsigned long __attribute__((alias("stack_canary"))) __stack_chk_guard;
EXPORT_SYMBOL(__stack_chk_guard);
#endif

Or I suppose I could just rename the thing __stack_chk_guard.  The
only downside of the latter seems to be that an accidental mix of SMP
and !SMP object files (or modules!) would crash, but I suppose they
likely crash anyway.