Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[Richard Henderson]

> 	PR target/47766
> 	* config/i386/i386.md (PTR): New.
> 	(stack_protect_set: Check TARGET_LP64 instead of TARGET_64BIT.
> 	(stack_protect_test): Likewise.
> 	(stack_protect_set_<mode>): Replace ":P" with ":PTR".
> 	(stack_tls_protect_set_<mode>): Likewise.
> 	(stack_tls_protect_test_<mode>): Likewise.

Ok.


r~

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[H.J. Lu]

On Fri, Jul 29, 2011 at 3:14 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 1:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
>> On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>>
>>>>> This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?
>>>>
>>>> http://gcc.gnu.org/contribute.html#patches
>>>>
>>>
>>> Sorry. I should have mentioned testcase in:
>>>
>>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766
>>>
>>> Actually, they are in gcc testsuite.  I noticed them when
>>> I run gcc testsuite on x32.
>>
>> This looks like a middle-end problem to me.
>>
>> According to the documentation:
>>
>> --quote--
>> `stack_protect_set'
>>     This pattern, if defined, moves a `Pmode' value from the memory in
>>     operand 1 to the memory in operand 0 without leaving the value in
>>     a register afterward.  This is to avoid leaking the value some
>>     place that an attacker might use to rewrite the stack guard slot
>>     after having clobbered it.
>>
>>     If this pattern is not defined, then a plain move pattern is
>>     generated.
>>
>> `stack_protect_test'
>>     This pattern, if defined, compares a `Pmode' value from the memory
>>     in operand 1 with the memory in operand 0 without leaving the
>>     value in a register afterward and branches to operand 2 if the
>>     values weren't equal.
>>
>>     If this pattern is not defined, then a plain compare pattern and
>>     conditional branch pattern is used.
>> --quote--
>>
>> According to the documentation, x86 patterns are correct. However,
>> middle-end fails to extend ptr_mode value to Pmode, and in function.c,
>> stack_protect_prologue/stack_protect_epilogue, we already have
>> ptr_mode (SImode) operand:
>>
>> (mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars)
>>        (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32])
>>
>> (mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40]
>> <var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4
>> A32])
>>
>> An opinion of a RTL maintainer (CC'd) is needed here. Target
>> definition is OK in its current form.
>
> When -fstack-protector  was added, there are
>
> tree
> default_stack_protect_guard (void)
> {
>  tree t = stack_chk_guard_decl;
>
>  if (t == NULL)
>    {
>      rtx x;
>
>      t = build_decl (UNKNOWN_LOCATION,
>                      VAR_DECL, get_identifier ("__stack_chk_guard"),
>                      ptr_type_node);
>                      ^^^^^^^^^^^^^^^^^
>
> I think ptr_mode is intended and Pmode is just a typo.  Jakub, Richard,
> what are your thoughts on this?
>

Stack protector does use ptr_mode.  We have

/* i386 glibc provides __stack_chk_guard in %gs:0x14,
   x32 glibc provides it in %fs:0x18.
   x86_64 glibc provides it in %fs:0x28.  */
#define TARGET_THREAD_SSP_OFFSET \
  (TARGET_64BIT ? (TARGET_X32 ? 0x18 : 0x28) : 0x14)

in gnu-user64.h and

typedef struct
{
  void *tcb;            /* Pointer to the TCB.  Not necessarily the
                           thread descriptor used by libpthread.  */
  dtv_t *dtv;
  void *self;           /* Pointer to the thread descriptor.  */
  int multiple_threads;
  int gscope_flag;
  uintptr_t sysinfo;
  uintptr_t stack_guard;
  uintptr_t pointer_guard;

in sysdeps/x86_64/tls.h.  I believe it is a mistake to use Pmode in
the stack protector documentation.

-- 
H.J.

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[H.J. Lu]

On Thu, Jul 28, 2011 at 1:03 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:
>
>>>> This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?
>>>
>>> http://gcc.gnu.org/contribute.html#patches
>>>
>>
>> Sorry. I should have mentioned testcase in:
>>
>> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766
>>
>> Actually, they are in gcc testsuite.  I noticed them when
>> I run gcc testsuite on x32.
>
> This looks like a middle-end problem to me.
>
> According to the documentation:
>
> --quote--
> `stack_protect_set'
>     This pattern, if defined, moves a `Pmode' value from the memory in
>     operand 1 to the memory in operand 0 without leaving the value in
>     a register afterward.  This is to avoid leaking the value some
>     place that an attacker might use to rewrite the stack guard slot
>     after having clobbered it.
>
>     If this pattern is not defined, then a plain move pattern is
>     generated.
>
> `stack_protect_test'
>     This pattern, if defined, compares a `Pmode' value from the memory
>     in operand 1 with the memory in operand 0 without leaving the
>     value in a register afterward and branches to operand 2 if the
>     values weren't equal.
>
>     If this pattern is not defined, then a plain compare pattern and
>     conditional branch pattern is used.
> --quote--
>
> According to the documentation, x86 patterns are correct. However,
> middle-end fails to extend ptr_mode value to Pmode, and in function.c,
> stack_protect_prologue/stack_protect_epilogue, we already have
> ptr_mode (SImode) operand:
>
> (mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars)
>        (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32])
>
> (mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40]
> <var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4
> A32])
>
> An opinion of a RTL maintainer (CC'd) is needed here. Target
> definition is OK in its current form.

When -fstack-protector  was added, there are

tree
default_stack_protect_guard (void)
{
  tree t = stack_chk_guard_decl;

  if (t == NULL)
    {
      rtx x;

      t = build_decl (UNKNOWN_LOCATION,
                      VAR_DECL, get_identifier ("__stack_chk_guard"),
                      ptr_type_node);
                      ^^^^^^^^^^^^^^^^^

I think ptr_mode is intended and Pmode is just a typo.  Jakub, Richard,
what are your thoughts on this?

Thanks.


-- 
H.J.

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[Uros Bizjak]

On Thu, Jul 28, 2011 at 9:03 PM, H.J. Lu <hjl.tools@gmail.com> wrote:

>>> This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?
>>
>> http://gcc.gnu.org/contribute.html#patches
>>
>
> Sorry. I should have mentioned testcase in:
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766
>
> Actually, they are in gcc testsuite.  I noticed them when
> I run gcc testsuite on x32.

This looks like a middle-end problem to me.

According to the documentation:

--quote--
`stack_protect_set'
     This pattern, if defined, moves a `Pmode' value from the memory in
     operand 1 to the memory in operand 0 without leaving the value in
     a register afterward.  This is to avoid leaking the value some
     place that an attacker might use to rewrite the stack guard slot
     after having clobbered it.

     If this pattern is not defined, then a plain move pattern is
     generated.

`stack_protect_test'
     This pattern, if defined, compares a `Pmode' value from the memory
     in operand 1 with the memory in operand 0 without leaving the
     value in a register afterward and branches to operand 2 if the
     values weren't equal.

     If this pattern is not defined, then a plain compare pattern and
     conditional branch pattern is used.
--quote--

According to the documentation, x86 patterns are correct. However,
middle-end fails to extend ptr_mode value to Pmode, and in function.c,
stack_protect_prologue/stack_protect_epilogue, we already have
ptr_mode (SImode) operand:

(mem/v/f/c/i:SI (plus:DI (reg/f:DI 54 virtual-stack-vars)
        (const_int -4 [0xfffffffffffffffc])) [2 D.2704+0 S4 A32])

(mem/v/f/c/i:SI (symbol_ref:DI ("__stack_chk_guard") [flags 0x40]
<var_decl 0x7ffc35aa0be0 __stack_chk_guard>) [2 __stack_chk_guard+0 S4
A32])

An opinion of a RTL maintainer (CC'd) is needed here. Target
definition is OK in its current form.

Uros.

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[H.J. Lu]

On Thu, Jul 28, 2011 at 11:55 AM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Thu, Jul 28, 2011 at 8:13 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:
>
>> This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?
>
> http://gcc.gnu.org/contribute.html#patches
>

Sorry. I should have mentioned testcase in:

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=47766

Actually, they are in gcc testsuite.  I noticed them when
I run gcc testsuite on x32.

-- 
H.J.

Re: PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[Uros Bizjak]

On Thu, Jul 28, 2011 at 8:13 PM, H.J. Lu <hongjiu.lu@intel.com> wrote:

> This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?

http://gcc.gnu.org/contribute.html#patches

Uros.

PATCH: PR target/47766: [x32] -fstack-protector doesn't work

[H.J. Lu]

Hi,

This patch adds x32 support to UNSPEC_SP_XXX patterns.  OK for trunk?

Thanks.


H.J.
---
2011-07-28  H.J. Lu  <hongjiu.lu@intel.com>

	PR target/47766
	* config/i386/i386.md (PTR): New.
	(stack_protect_set: Check TARGET_LP64 instead of TARGET_64BIT.
	(stack_protect_test): Likewise.
	(stack_protect_set_<mode>): Replace ":P" with ":PTR".
	(stack_tls_protect_set_<mode>): Likewise.
	(stack_tls_protect_test_<mode>): Likewise.

diff --git a/gcc/config/i386/i386.md b/gcc/config/i386/i386.md
index f33b8a0..f4717b5 100644
--- a/gcc/config/i386/i386.md
+++ b/gcc/config/i386/i386.md
@@ -951,6 +951,11 @@
 ;; This mode iterator allows :P to be used for patterns that operate on
 ;; pointer-sized quantities.  Exactly one of the two alternatives will match.
 (define_mode_iterator P [(SI "Pmode == SImode") (DI "Pmode == DImode")])
+
+;; This mode iterator allows :PTR to be used for patterns that operate on
+;; ptr_mode sized quantities.
+(define_mode_iterator PTR
+  [(SI "ptr_mode == SImode") (DI "ptr_mode == DImode")])
 \f
 ;; Scheduling descriptions
 
@@ -17347,11 +17379,11 @@
 
 #ifdef TARGET_THREAD_SSP_OFFSET
   operands[1] = GEN_INT (TARGET_THREAD_SSP_OFFSET);
-  insn = (TARGET_64BIT
+  insn = (TARGET_LP64
 	  ? gen_stack_tls_protect_set_di
 	  : gen_stack_tls_protect_set_si);
 #else
-  insn = (TARGET_64BIT
+  insn = (TARGET_LP64
 	  ? gen_stack_protect_set_di
 	  : gen_stack_protect_set_si);
 #endif
@@ -17361,19 +17393,20 @@
 })
 
 (define_insn "stack_protect_set_<mode>"
-  [(set (match_operand:P 0 "memory_operand" "=m")
-	(unspec:P [(match_operand:P 1 "memory_operand" "m")] UNSPEC_SP_SET))
-   (set (match_scratch:P 2 "=&r") (const_int 0))
+  [(set (match_operand:PTR 0 "memory_operand" "=m")
+	(unspec:PTR [(match_operand:PTR 1 "memory_operand" "m")]
+		    UNSPEC_SP_SET))
+   (set (match_scratch:PTR 2 "=&r") (const_int 0))
    (clobber (reg:CC FLAGS_REG))]
   ""
   "mov{<imodesuffix>}\t{%1, %2|%2, %1}\;mov{<imodesuffix>}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2"
   [(set_attr "type" "multi")])
 
 (define_insn "stack_tls_protect_set_<mode>"
-  [(set (match_operand:P 0 "memory_operand" "=m")
-	(unspec:P [(match_operand:P 1 "const_int_operand" "i")]
-		  UNSPEC_SP_TLS_SET))
-   (set (match_scratch:P 2 "=&r") (const_int 0))
+  [(set (match_operand:PTR 0 "memory_operand" "=m")
+	(unspec:PTR [(match_operand:PTR 1 "const_int_operand" "i")]
+		    UNSPEC_SP_TLS_SET))
+   (set (match_scratch:PTR 2 "=&r") (const_int 0))
    (clobber (reg:CC FLAGS_REG))]
   ""
   "mov{<imodesuffix>}\t{%@:%P1, %2|%2, <iptrsize> PTR %@:%P1}\;mov{<imodesuffix>}\t{%2, %0|%0, %2}\;xor{l}\t%k2, %k2"
@@ -17391,11 +17424,11 @@
 
 #ifdef TARGET_THREAD_SSP_OFFSET
   operands[1] = GEN_INT (TARGET_THREAD_SSP_OFFSET);
-  insn = (TARGET_64BIT
+  insn = (TARGET_LP64
 	  ? gen_stack_tls_protect_test_di
 	  : gen_stack_tls_protect_test_si);
 #else
-  insn = (TARGET_64BIT
+  insn = (TARGET_LP64
 	  ? gen_stack_protect_test_di
 	  : gen_stack_protect_test_si);
 #endif
@@ -17409,20 +17442,20 @@
 
 (define_insn "stack_protect_test_<mode>"
   [(set (match_operand:CCZ 0 "flags_reg_operand" "")
-	(unspec:CCZ [(match_operand:P 1 "memory_operand" "m")
-		     (match_operand:P 2 "memory_operand" "m")]
+	(unspec:CCZ [(match_operand:PTR 1 "memory_operand" "m")
+		     (match_operand:PTR 2 "memory_operand" "m")]
 		    UNSPEC_SP_TEST))
-   (clobber (match_scratch:P 3 "=&r"))]
+   (clobber (match_scratch:PTR 3 "=&r"))]
   ""
   "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%2, %3|%3, %2}"
   [(set_attr "type" "multi")])
 
 (define_insn "stack_tls_protect_test_<mode>"
   [(set (match_operand:CCZ 0 "flags_reg_operand" "")
-	(unspec:CCZ [(match_operand:P 1 "memory_operand" "m")
-		     (match_operand:P 2 "const_int_operand" "i")]
+	(unspec:CCZ [(match_operand:PTR 1 "memory_operand" "m")
+		     (match_operand:PTR 2 "const_int_operand" "i")]
 		    UNSPEC_SP_TLS_TEST))
-   (clobber (match_scratch:P 3 "=r"))]
+   (clobber (match_scratch:PTR 3 "=r"))]
   ""
   "mov{<imodesuffix>}\t{%1, %3|%3, %1}\;xor{<imodesuffix>}\t{%@:%P2, %3|%3, <iptrsize> PTR %@:%P2}"
   [(set_attr "type" "multi")])