Bug 81708 - The x86 stack canary location should be customizable
Summary: The x86 stack canary location should be customizable
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: unknown
: P3 normal
Target Milestone: 8.0
Assignee: Uroš Bizjak
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-08-04 00:31 UTC by Andy Lutomirski
Modified: 2019-06-15 00:46 UTC (History)
5 users (show)

See Also:
Host:
Target: x86_64-*-*, i?86-*-*
Build:
Known to work:
Known to fail:
Last reconfirmed: 2017-08-08 00:00:00


Attachments
Patch that introduces mstack-protector-guard-offset= and mstack-protector-guard-reg= (1.66 KB, patch)
2017-08-08 09:39 UTC, Uroš Bizjak
Details | Diff
patch that introduces mstack-protector-guard-symbol= (1000 bytes, patch)
2017-08-09 08:50 UTC, Uroš Bizjak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andy Lutomirski 2017-08-04 00:31:46 UTC
There are four credible ways to find the stack canary:

1. %fs:symbol
2. %fs:symbol(%rip) [with a PCREL relocation]
3. %gs:symbol
4. %gs:symbol(%rip)

(Obviously the %rip variants only work on x86_64.)

The current code is roughly equivalent to (1) or (3) where symbol is an absolute symbol equal to 0x28 or similar.

Please give a command line option to choose any of the four modes and specify the symbol name.  (Or just hardcode the symbol name to __gcc_stack_canary or whatever if the option is set.)

My perferred solution would be -mstack-protector-cookie=gs:symname or -mstack-protector-cookie=gs:symname(%rip) or -mstack-protector-cookie=gs:0x28 depending on what's desired.

I personally consider it to have been a mistake for Linux to support a stack canary without insisting that GCC fix this issue first.  The x86_32 case, in particular, is a collossal mess in the kernel that slows kernel entries down and seriously overcomplicates the kernel code because the stack canary addressing mode that GCC chooses is nonsensical.
Comment 1 H. Peter Anvin 2017-08-04 00:39:46 UTC
Well, you can choose between "__stack_chk_guard(%rip)" and "%gs:40"... :)
Comment 2 Andy Lutomirski 2017-08-04 00:47:12 UTC
(In reply to H. Peter Anvin from comment #1)
> Well, you can choose between "__stack_chk_guard(%rip)" and "%gs:40"... :)

Wow, I guess I didn't even consider the former.  That would be option 5: symbol(%rip).  Let's not use it for the kernel.

For x86_32, we want %fs:symbol.  %fs:constant would be tolarable, too.  For x86_64, it depends on the end game for percpu access, I suppose.
Comment 3 Uroš Bizjak 2017-08-08 09:39:37 UTC
Created attachment 41949 [details]
Patch that introduces mstack-protector-guard-offset= and mstack-protector-guard-reg=

Prototype patch that introduces mstack-protector-guard-offset= and mstack-protector-guard-reg= options.  The later can be used to override default TLS reg, so compilation becomes independent of -mcmodel=kernel.
Comment 4 Uroš Bizjak 2017-08-08 09:44:53 UTC
(In reply to Uroš Bizjak from comment #3)
> Created attachment 41949 [details]
> Patch that introduces mstack-protector-guard-offset= and
> mstack-protector-guard-reg=

e.g.:

-mstack-protector-guard-reg=%fs -mstack-protector-guard-offset=0x200
Comment 5 uros 2017-08-08 16:49:19 UTC
Author: uros
Date: Tue Aug  8 16:48:46 2017
New Revision: 250965

URL: https://gcc.gnu.org/viewcvs?rev=250965&root=gcc&view=rev
Log:
	PR target/81708
	* config/i386/i386.opt (mstack-protector-guard-reg=): New option
	(mstack-protector-guard-offset=): Ditto.
	* config/i386/i386.c (ix86_option_override): Handle
	-mstack-protector-guard-reg= and -mstack-protector-guard-offset=
	options.
	(ix86_stack_protect_guard): Use ix86_stack_protect_guard_reg and
	ix86_stack_protect_guard_offset variables.
	(TARGET_STACK_PROTECT_GUARD): Always define.
	* doc/invoke.texi (x86 Options): Document -mstack-protector-guard-reg=
	and -mstack-protector-guard-offset= options.

testsuite/ChangeLog:

	PR target/81708
	* gcc.target/i386/stack-prot-guard.c: New test.


Added:
    trunk/gcc/testsuite/gcc.target/i386/stack-prot-guard.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/config/i386/i386.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
Comment 6 Uroš Bizjak 2017-08-08 16:54:50 UTC
Implemented for gcc-8.
Comment 7 Andy Lutomirski 2017-08-08 17:19:53 UTC
Hmm.  This is a big improvement, but it's still going to be awkward to use -- if we want to use a normal Linux percpu variable, we're stuck putting it in a fixed location that's known at compile time as opposed to just at link time.  It also still prevents Linux from switching to PC-relative percpu addressing to reduce text size.

Can we do -mstack-protector-offset=[symbol name]?  And could it be extended to ask for PC-relative addressing?
Comment 8 H. Peter Anvin 2017-08-08 17:24:52 UTC
How about simply letting the user enter an assembly expression of neither of the standard ABI options are suitable?  Also, shouldn't the user space default on 64 bits be an offset into the TLS using %fs, or is there something magic about how the kernel is compiled that changes it to %gs:?
Comment 9 H. Peter Anvin 2017-08-08 17:27:35 UTC
In some applications it might even be appropriate to use the RDPID instruction and store the canary in the IA32_TSC_AUX MSR.
Comment 10 Uroš Bizjak 2017-08-09 08:50:33 UTC
Created attachment 41955 [details]
patch that introduces mstack-protector-guard-symbol=

This patch can be used to override TLS offset with a symbol.

gcc.target/i386/stack-prot-guard.c compiles to (-O2 -fstack-protector-all -mstack-protector-guard-symbol=symbol_name):

f:
        subq    $24, %rsp
        movq    %fs:symbol_name(%rip), %rax
        movq    %rax, 8(%rsp)
        xorl    %eax, %eax
        movq    8(%rsp), %rax
        xorq    %fs:symbol_name(%rip), %rax
        jne     .L5
        addq    $24, %rsp
        ret
Comment 11 Uroš Bizjak 2017-08-09 08:54:23 UTC
(In reply to H. Peter Anvin from comment #8)
> How about simply letting the user enter an assembly expression of neither of
> the standard ABI options are suitable?  Also, shouldn't the user space
> default on 64 bits be an offset into the TLS using %fs, or is there
> something magic about how the kernel is compiled that changes it to %gs:?

-mcmodel=kernel switches TLS reg to %gs.

You can use mstack-protector-guard-reg=%gs with other code models.
Comment 12 uros 2017-08-10 20:59:42 UTC
Author: uros
Date: Thu Aug 10 20:59:10 2017
New Revision: 251040

URL: https://gcc.gnu.org/viewcvs?rev=251040&root=gcc&view=rev
Log:
	PR target/81708
	* config/i386/i386.opt (mstack-protector-guard-symbol=): New option
	* config/i386/i386.c (ix86_stack_protect_guard): Use
	ix86_stack_protect_guard_symbol_str to generate varible declaration.
	* doc/invoke.texi (x86 Options): Document
	-mstack-protector-guard-symbol= option.

testsuite/ChangeLog:

	PR target/81708
	* gcc.target/i386/stack-prot-sym.c: New test.


Added:
    trunk/gcc/testsuite/gcc.target/i386/stack-prot-sym.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/config/i386/i386.c
    trunk/gcc/config/i386/i386.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/testsuite/ChangeLog
Comment 13 Aldy Hernandez 2017-09-13 16:41:59 UTC
Author: aldyh
Date: Wed Sep 13 16:41:28 2017
New Revision: 252350

URL: https://gcc.gnu.org/viewcvs?rev=252350&root=gcc&view=rev
Log:
	PR target/81708
	* config/i386/i386.opt (mstack-protector-guard-reg=): New option
	(mstack-protector-guard-offset=): Ditto.
	* config/i386/i386.c (ix86_option_override): Handle
	-mstack-protector-guard-reg= and -mstack-protector-guard-offset=
	options.
	(ix86_stack_protect_guard): Use ix86_stack_protect_guard_reg and
	ix86_stack_protect_guard_offset variables.
	(TARGET_STACK_PROTECT_GUARD): Always define.
	* doc/invoke.texi (x86 Options): Document -mstack-protector-guard-reg=
	and -mstack-protector-guard-offset= options.

testsuite/ChangeLog:

	PR target/81708
	* gcc.target/i386/stack-prot-guard.c: New test.

Added:
    branches/range-gen2/gcc/testsuite/gcc.target/i386/stack-prot-guard.c
Modified:
    branches/range-gen2/gcc/ChangeLog
    branches/range-gen2/gcc/config/i386/i386.c
    branches/range-gen2/gcc/config/i386/i386.opt
    branches/range-gen2/gcc/doc/invoke.texi
    branches/range-gen2/gcc/testsuite/ChangeLog
Comment 14 Aldy Hernandez 2017-09-13 16:52:09 UTC
Author: aldyh
Date: Wed Sep 13 16:51:37 2017
New Revision: 252397

URL: https://gcc.gnu.org/viewcvs?rev=252397&root=gcc&view=rev
Log:
	PR target/81708
	* config/i386/i386.opt (mstack-protector-guard-symbol=): New option
	* config/i386/i386.c (ix86_stack_protect_guard): Use
	ix86_stack_protect_guard_symbol_str to generate varible declaration.
	* doc/invoke.texi (x86 Options): Document
	-mstack-protector-guard-symbol= option.

testsuite/ChangeLog:

	PR target/81708
	* gcc.target/i386/stack-prot-sym.c: New test.

Added:
    branches/range-gen2/gcc/testsuite/gcc.target/i386/stack-prot-sym.c
Modified:
    branches/range-gen2/gcc/ChangeLog
    branches/range-gen2/gcc/config/i386/i386.c
    branches/range-gen2/gcc/config/i386/i386.opt
    branches/range-gen2/gcc/doc/invoke.texi
    branches/range-gen2/gcc/testsuite/ChangeLog