Re: [PATCH] Fix -fstack-protector* on darwin/mingw etc. (PR target/85644)

[Uros Bizjak <ubizjak at gmail dot com>]

On Wed, Nov 21, 2018 at 11:21 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> Hi!
>
> As I wrote in the PR, before PR81708 commits,
> while i386 defaulted to SSP_TLS rather than SSP_GLOBAL on everything but
> Android, the -mstack-protector-guard= switch controlled pretty much
> whether the i386.md special stack protector patterns are used (if tls)
> or whether generic code is used (global).  These special stack protector
> patterns did one thing if TARGET_THREAD_SSP_OFFSET macro was defined
> (only defined on glibc targets) - code like:
>         movq    %fs:40, %rax
>         movq    %rax, -8(%rbp)
>         xorl    %eax, %eax
> in the prologue and
>         movq    -8(%rbp), %rdx
>         xorq    %fs:40, %rdx
>         je      .L4
> in the epilogue.  If TARGET_THREAD_SSP_OFFSET macro wasn't defined, it would
> do instead:
>         movq    .refptr.__stack_chk_guard(%rip), %rax
>         movq    (%rax), %rcx
>         movq    %rcx, -8(%rbp)
>         xorl    %ecx, %ecx
> and
>         movq    .refptr.__stack_chk_guard(%rip), %rdx
>         movq    -8(%rbp), %rcx
>         xorq    (%rdx), %rcx
>         je      .L4
> (this is taken from 7.x cross to mingw).
> Finally, for Android or when -mstack-protector-guard=global was used, it
> emitted:
>         movq    __stack_chk_guard(%rip), %rax
>         movq    %rax, -8(%rbp)
> and
>         movq    __stack_chk_guard(%rip), %rdx
>         cmpq    %rdx, %rcx
>         je      .L4
> Note, apart from OS specific details, those =global sequences are similar
> to the =tls ones when TARGET_THREAD_SSP_OFFSET is not defined, the main
> difference is that the =tls ones are more secure as they clear registers
> containing the guard as quickly as possible.  The PR81708 changes dropped
> the non-tls special stack_protector_* patterns from i386.md and now =tls
> implies really tls, but the default remained, so mingw32 or darwin still
> default to tls and just use 0 offset by default.
>
> So, this patch changes the default for mingw32, darwin and everything else
> except gnu-user*.h to be =global, and just forces those special i386.md
> more secure patterns unconditionally (slightly changing the generated code
> on Android, but it is one extra insn in prologue and one fewer in the
> epilogue).
>
> With this patch -mstack-protector-guard=tls is really for tls and =global
> for pure var access and user can override the defaults on non-glibc targets,
> but they should get a default that works there.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, plus tested with a
> cross to mingw, ok for trunk?
>
> 2018-11-21  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/85644
>         PR target/86832
>         * config/i386/i386.c (ix86_option_override_internal): Default
>         ix86_stack_protector_guard to SSP_TLS only if TARGET_THREAD_SSP_OFFSET
>         is defined.
>         * config/i386/i386.md (stack_protect_set, stack_protect_set_<mode>,
>         stack_protect_test, stack_protect_test_<mode>): Use empty condition
>         instead of TARGET_SSP_TLS_GUARD.

OK for mainline and backports, with a bit of bikeshedding below.

Thanks,
Uros.

> --- gcc/config/i386/i386.c.jj   2018-11-20 21:39:00.905577452 +0100
> +++ gcc/config/i386/i386.c      2018-11-21 18:02:49.448049161 +0100
> @@ -4557,8 +4557,13 @@ ix86_option_override_internal (bool main
>
>    /* Handle stack protector */
>    if (!opts_set->x_ix86_stack_protector_guard)
> -    opts->x_ix86_stack_protector_guard
> -      = TARGET_HAS_BIONIC ? SSP_GLOBAL : SSP_TLS;
> +    {
> +      opts->x_ix86_stack_protector_guard = SSP_GLOBAL;
> +#ifdef TARGET_THREAD_SSP_OFFSET
> +      if (!TARGET_HAS_BIONIC)
> +       opts->x_ix86_stack_protector_guard = SSP_TLS;
> +#endif
> +    }

How about:

--cut here--
  /* Handle stack protector */
  if (!opts_set->x_ix86_stack_protector_guard)
    {
#ifdef TARGET_THREAD_SSP_OFFSET
      if (!TARGET_HAS_BIONIC)
    opts->x_ix86_stack_protector_guard = SSP_TLS;
      else
#endif
    opts->x_ix86_stack_protector_guard = SSP_GLOBAL;
    }
--cut here--

>  #ifdef TARGET_THREAD_SSP_OFFSET
>    ix86_stack_protector_guard_offset = TARGET_THREAD_SSP_OFFSET;
> --- gcc/config/i386/i386.md.jj  2018-11-21 11:45:12.090721862 +0100
> +++ gcc/config/i386/i386.md     2018-11-21 18:03:46.166119350 +0100
> @@ -19010,7 +19010,7 @@ (define_insn "*prefetch_prefetchwt1"
>  (define_expand "stack_protect_set"
>    [(match_operand 0 "memory_operand")
>     (match_operand 1 "memory_operand")]
> -  "TARGET_SSP_TLS_GUARD"
> +  ""
>  {
>    rtx (*insn)(rtx, rtx);
>
> @@ -19028,7 +19028,7 @@ (define_insn "stack_protect_set_<mode>"
>                     UNSPEC_SP_SET))
>     (set (match_scratch:PTR 2 "=&r") (const_int 0))
>     (clobber (reg:CC FLAGS_REG))]
> -  "TARGET_SSP_TLS_GUARD"
> +  ""
>    "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2"
>    [(set_attr "type" "multi")])
>
> @@ -19036,7 +19036,7 @@ (define_expand "stack_protect_test"
>    [(match_operand 0 "memory_operand")
>     (match_operand 1 "memory_operand")
>     (match_operand 2)]
> -  "TARGET_SSP_TLS_GUARD"
> +  ""
>  {
>    rtx flags = gen_rtx_REG (CCZmode, FLAGS_REG);
>
> @@ -19059,7 +19059,7 @@ (define_insn "stack_protect_test_<mode>"
>                      (match_operand:PTR 2 "memory_operand" "m")]
>                     UNSPEC_SP_TEST))
>     (clobber (match_scratch:PTR 3 "=&r"))]
> -  "TARGET_SSP_TLS_GUARD"
> +  ""
>    "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%2, %3|%3, %2}"
>    [(set_attr "type" "multi")])
>
>
>         Jakub