[RISC-V] Add support for TLS stack protector canary access

The linux kernel guys are discussing about supporting TLS register based
stack proctector canary, the link is as follows:
	https://lore.kernel.org/linux-riscv/202007051820.DABE7F87D7@keescook/T/#t
I implemented register based stack protector canary with reference to
aarch64 and x86. When adding -mstack-protector-guard=tls,
use -mstack-protector-guard= to specify a register such as tp
and mstack-protector-guard-offset= to specify the offset, then
the TLS stack protector canary code will be generated.

gcc/
	* config/riscv/riscv-opts.h (stack_protector_guard): New enum.
	* config/riscv/riscv.c (riscv_option_override): Handle
	the new options.
	* config/riscv/riscv.md (stack_protect_set): New pattern to handle
	flexible stack protector guard settings.
	(stack_protect_set_<mode>): Ditto.
	(stack_protect_test): Ditto.
	(stack_protect_test_<mode>): Ditto.
	* config/riscv/riscv.opt (mstack-protector-guard=,
	mstack-protector-guard-reg=, mstack-protector-guard-offset=): New
	options.
	* doc/invoke.texi (Option Summary) [RISC-V Options]:
	Add -mstack-protector-guard=, -mstack-protector-guard-reg=, and
	-mstack-protector-guard-offset=.
	(RISC-V Options): Ditto.

---
 gcc/ChangeLog                 | 18 ++++++++
 gcc/config/riscv/riscv-opts.h |  6 +++
 gcc/config/riscv/riscv.c      | 41 ++++++++++++++++++
 gcc/config/riscv/riscv.md     | 80 +++++++++++++++++++++++++++++++++++
 gcc/config/riscv/riscv.opt    | 28 ++++++++++++
 gcc/doc/invoke.texi           | 22 +++++++++-
 6 files changed, 194 insertions(+), 1 deletion(-)

Hi Cooper,

Great Job!

Tested-by: Guo Ren <guoren@kernel.org>

Here is kernel related patch with tested result:

https://lore.kernel.org/linux-riscv/1594279697-72511-2-git-send-email-guoren@kernel.org/T/#u 


Best Regards
  Guo Ren

On 2020/7/8 上午10:51, cooper wrote:
> The linux kernel guys are discussing about supporting TLS register based
> stack proctector canary, the link is as follows:
> 	https://lore.kernel.org/linux-riscv/202007051820.DABE7F87D7@keescook/T/#t
> I implemented register based stack protector canary with reference to
> aarch64 and x86. When adding -mstack-protector-guard=tls,
> use -mstack-protector-guard= to specify a register such as tp
> and mstack-protector-guard-offset= to specify the offset, then
> the TLS stack protector canary code will be generated.
>
> gcc/
> 	* config/riscv/riscv-opts.h (stack_protector_guard): New enum.
> 	* config/riscv/riscv.c (riscv_option_override): Handle
> 	the new options.
> 	* config/riscv/riscv.md (stack_protect_set): New pattern to handle
> 	flexible stack protector guard settings.
> 	(stack_protect_set_<mode>): Ditto.
> 	(stack_protect_test): Ditto.
> 	(stack_protect_test_<mode>): Ditto.
> 	* config/riscv/riscv.opt (mstack-protector-guard=,
> 	mstack-protector-guard-reg=, mstack-protector-guard-offset=): New
> 	options.
> 	* doc/invoke.texi (Option Summary) [RISC-V Options]:
> 	Add -mstack-protector-guard=, -mstack-protector-guard-reg=, and
> 	-mstack-protector-guard-offset=.
> 	(RISC-V Options): Ditto.
>
> ---
>   gcc/ChangeLog                 | 18 ++++++++
>   gcc/config/riscv/riscv-opts.h |  6 +++
>   gcc/config/riscv/riscv.c      | 41 ++++++++++++++++++
>   gcc/config/riscv/riscv.md     | 80 +++++++++++++++++++++++++++++++++++
>   gcc/config/riscv/riscv.opt    | 28 ++++++++++++
>   gcc/doc/invoke.texi           | 22 +++++++++-
>   6 files changed, 194 insertions(+), 1 deletion(-)
>
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index ea2f78df22e..98745f9f946 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,21 @@
> +2020-07-07  Cooper Qu  <cooper.qu@linux.alibaba.com>
> +
> +	* config/riscv/riscv-opts.h (stack_protector_guard): New enum.
> +	* config/riscv/riscv.c (riscv_option_override): Handle
> +	the new options.
> +	* config/riscv/riscv.md (stack_protect_set): New pattern to handle
> +	flexible stack protector guard settings.
> +	(stack_protect_set_<mode>): Ditto.
> +	(stack_protect_test): Ditto.
> +	(stack_protect_test_<mode>): Ditto.
> +	* config/riscv/riscv.opt (mstack-protector-guard=,
> +	mstack-protector-guard-reg=, mstack-protector-guard-offset=): New
> +	options.
> +	* doc/invoke.texi (Option Summary) [RISC-V Options]:
> +	Add -mstack-protector-guard=, -mstack-protector-guard-reg=, and
> +	-mstack-protector-guard-offset=.
> +	(RISC-V Options): Ditto.
> +
>   2020-07-06  Richard Biener  <rguenther@suse.de>
>   
>   	PR tree-optimization/96075
> diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
> index 8f12e50b9f1..2a3f9d9eef5 100644
> --- a/gcc/config/riscv/riscv-opts.h
> +++ b/gcc/config/riscv/riscv-opts.h
> @@ -51,4 +51,10 @@ enum riscv_align_data {
>     riscv_align_data_type_natural
>   };
>   
> +/* Where to get the canary for the stack protector.  */
> +enum stack_protector_guard {
> +  SSP_TLS,			/* per-thread canary in TLS block */
> +  SSP_GLOBAL			/* global canary */
> +};
> +
>   #endif /* ! GCC_RISCV_OPTS_H */
> diff --git a/gcc/config/riscv/riscv.c b/gcc/config/riscv/riscv.c
> index bfb3885ed08..e606f24fa74 100644
> --- a/gcc/config/riscv/riscv.c
> +++ b/gcc/config/riscv/riscv.c
> @@ -4775,6 +4775,47 @@ riscv_option_override (void)
>   	   " [%<-mriscv-attribute%>]");
>   #endif
>   
> +  if (riscv_stack_protector_guard == SSP_GLOBAL
> +      && global_options_set.x_riscv_stack_protector_guard_offset_str)
> +    {
> +      error ("incompatible options %<-mstack-protector-guard=global%> and "
> +	     "%<-mstack-protector-guard-offset=%s%>",
> +	     riscv_stack_protector_guard_offset_str);
> +    }
> +
> +  if (riscv_stack_protector_guard == SSP_TLS
> +      && !(global_options_set.x_riscv_stack_protector_guard_offset_str
> +	   && global_options_set.x_riscv_stack_protector_guard_reg_str))
> +    {
> +      error ("both %<-mstack-protector-guard-offset%> and "
> +	     "%<-mstack-protector-guard-reg%> must be used "
> +	     "with %<-mstack-protector-guard=sysreg%>");
> +    }
> +
> +  if (global_options_set.x_riscv_stack_protector_guard_reg_str)
> +    {
> +      const char *str = riscv_stack_protector_guard_reg_str;
> +      int reg = decode_reg_name (str);
> +
> +      if (!IN_RANGE (reg, 1, 31))
> +	error ("%qs is not a valid base register in %qs", str,
> +	       "-mstack-protector-guard-reg=");
> +
> +      riscv_stack_protector_guard_reg = reg;
> +    }
> +
> +  if (global_options_set.x_riscv_stack_protector_guard_offset_str)
> +    {
> +      char *end;
> +      const char *str = riscv_stack_protector_guard_offset_str;
> +      errno = 0;
> +      long offs = strtol (riscv_stack_protector_guard_offset_str, &end, 0);
> +      if (!*str || *end || errno)
> +	error ("%qs is not a valid offset in %qs", str,
> +	       "-mstack-protector-guard-offset=");
> +      riscv_stack_protector_guard_offset = offs;
> +    }
> +
>   }
>   
>   /* Implement TARGET_CONDITIONAL_REGISTER_USAGE.  */
> diff --git a/gcc/config/riscv/riscv.md b/gcc/config/riscv/riscv.md
> index 36012ad1f77..9e67271f29e 100644
> --- a/gcc/config/riscv/riscv.md
> +++ b/gcc/config/riscv/riscv.md
> @@ -65,6 +65,10 @@
>     UNSPECV_BLOCKAGE
>     UNSPECV_FENCE
>     UNSPECV_FENCE_I
> +
> +  ;; Stack Smash Protector
> +  UNSPEC_SSP_SET
> +  UNSPEC_SSP_TEST
>   ])
>   
>   (define_constants
> @@ -2515,6 +2519,82 @@
>     DONE;
>   })
>   
> +;; Named patterns for stack smashing protection.
> +
> +(define_expand "stack_protect_set"
> +  [(match_operand 0 "memory_operand")
> +   (match_operand 1 "memory_operand")]
> +  ""
> +{
> +  machine_mode mode = GET_MODE (operands[0]);
> +  if (riscv_stack_protector_guard == SSP_TLS)
> +  {
> +    rtx reg = gen_rtx_REG (Pmode, riscv_stack_protector_guard_reg);
> +    rtx offset = GEN_INT (riscv_stack_protector_guard_offset);
> +    rtx addr = gen_rtx_PLUS (Pmode, reg, offset);
> +    operands[1] = gen_rtx_MEM (Pmode, addr);
> +  }
> +
> +  emit_insn ((mode == DImode
> +	      ? gen_stack_protect_set_di
> +	      : gen_stack_protect_set_si) (operands[0], operands[1]));
> +  DONE;
> +})
> +
> +;; DO NOT SPLIT THIS PATTERN.  It is important for security reasons that the
> +;; canary value does not live beyond the life of this sequence.
> +(define_insn "stack_protect_set_<mode>"
> +  [(set (match_operand:GPR 0 "memory_operand" "=m")
> +	(unspec:GPR [(match_operand:GPR 1 "memory_operand" "m")]
> +	 UNSPEC_SSP_SET))
> +   (set (match_scratch:GPR 2 "=&r") (const_int 0))]
> +  ""
> +  "<load>\\t%2, %1\;<store>\\t%2, %0\;li\t%2, 0"
> +  [(set_attr "length" "12")])
> +
> +(define_expand "stack_protect_test"
> +  [(match_operand 0 "memory_operand")
> +   (match_operand 1 "memory_operand")
> +   (match_operand 2)]
> +  ""
> +{
> +  rtx result;
> +  machine_mode mode = GET_MODE (operands[0]);
> +
> +  result = gen_reg_rtx(mode);
> +  if (riscv_stack_protector_guard == SSP_TLS)
> +  {
> +      rtx reg = gen_rtx_REG (Pmode, riscv_stack_protector_guard_reg);
> +      rtx offset = GEN_INT (riscv_stack_protector_guard_offset);
> +      rtx addr = gen_rtx_PLUS (Pmode, reg, offset);
> +      operands[1] = gen_rtx_MEM (Pmode, addr);
> +  }
> +  emit_insn ((mode == DImode
> +		  ? gen_stack_protect_test_di
> +		  : gen_stack_protect_test_si) (result,
> +					        operands[0],
> +					        operands[1]));
> +
> +  if (mode == DImode)
> +    emit_jump_insn (gen_cbranchdi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
> +				    result, const0_rtx, operands[2]));
> +  else
> +    emit_jump_insn (gen_cbranchsi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx),
> +				    result, const0_rtx, operands[2]));
> +
> +  DONE;
> +})
> +
> +(define_insn "stack_protect_test_<mode>"
> +  [(set (match_operand:GPR 0 "register_operand" "=r")
> +	(unspec:GPR [(match_operand:GPR 1 "memory_operand" "m")
> +		     (match_operand:GPR 2 "memory_operand" "m")]
> +	 UNSPEC_SSP_TEST))
> +   (clobber (match_scratch:GPR 3 "=&r"))]
> +  ""
> +  "<load>\t%3, %1\;<load>\t%0, %2\;xor\t%0, %3, %0"
> +  [(set_attr "length" "12")])
> +
>   (include "sync.md")
>   (include "peephole.md")
>   (include "pic.md")
> diff --git a/gcc/config/riscv/riscv.opt b/gcc/config/riscv/riscv.opt
> index e4bfcb86f51..f01d3ab79c3 100644
> --- a/gcc/config/riscv/riscv.opt
> +++ b/gcc/config/riscv/riscv.opt
> @@ -151,3 +151,31 @@ Enum(riscv_align_data) String(xlen) Value(riscv_align_data_type_xlen)
>   
>   EnumValue
>   Enum(riscv_align_data) String(natural) Value(riscv_align_data_type_natural)
> +
> +mstack-protector-guard=
> +Target RejectNegative Joined Enum(stack_protector_guard) Var(riscv_stack_protector_guard) Init(SSP_GLOBAL)
> +Use given stack-protector guard.
> +
> +Enum
> +Name(stack_protector_guard) Type(enum stack_protector_guard)
> +Valid arguments to -mstack-protector-guard=:
> +
> +EnumValue
> +Enum(stack_protector_guard) String(tls) Value(SSP_TLS)
> +
> +EnumValue
> +Enum(stack_protector_guard) String(global) Value(SSP_GLOBAL)
> +
> +mstack-protector-guard-reg=
> +Target RejectNegative Joined Var(riscv_stack_protector_guard_reg_str)
> +Use the given base register for addressing the stack-protector guard.
> +
> +TargetVariable
> +int riscv_stack_protector_guard_reg = 0
> +
> +mstack-protector-guard-offset=
> +Target RejectNegative Joined Integer Var(riscv_stack_protector_guard_offset_str)
> +Use the given offset for addressing the stack-protector guard.
> +
> +TargetVariable
> +long riscv_stack_protector_guard_offset = 0
> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index e21d8a5217b..e17b9e4e52d 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1137,7 +1137,9 @@ See RS/6000 and PowerPC Options.
>   -mexplicit-relocs  -mno-explicit-relocs @gol
>   -mrelax  -mno-relax @gol
>   -mriscv-attribute  -mmo-riscv-attribute @gol
> --malign-data=@var{type}}
> +-malign-data=@var{type} @gol
> ++-mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{reg} @gol
> ++-mstack-protector-guard-offset=@var{offset}}
>   
>   @emph{RL78 Options}
>   @gccoptlist{-msim  -mmul=none  -mmul=g13  -mmul=g14  -mallregs @gol
> @@ -25711,6 +25713,24 @@ Control how GCC aligns variables and constants of array, structure, or union
>   types.  Supported values for @var{type} are @samp{xlen} which uses x register
>   width as the alignment value, and @samp{natural} which uses natural alignment.
>   @samp{xlen} is the default.
> +
> +@item -mstack-protector-guard=@var{guard}
> +@itemx -mstack-protector-guard-reg=@var{reg}
> +@itemx -mstack-protector-guard-offset=@var{offset}
> +@opindex mstack-protector-guard
> +@opindex mstack-protector-guard-reg
> +@opindex mstack-protector-guard-offset
> +Generate stack protection code using canary at @var{guard}.  Supported
> +locations are @samp{global} for a global canary or @samp{tls} for per-thread
> +canary in the TLS block.
> +
> +With the latter choice the options
> +@option{-mstack-protector-guard-reg=@var{reg}} and
> +@option{-mstack-protector-guard-offset=@var{offset}} furthermore specify
> +which register to use as base register for reading the canary,
> +and from what offset from that base register. There is no default
> +register or offset as this is entirely for use within the Linux
> +kernel.
>   @end table
>   
>   @node RL78 Options

On Tue, Jul 7, 2020 at 7:51 PM cooper <cooper.qu@linux.alibaba.com> wrote:
> gcc/
>         * config/riscv/riscv-opts.h (stack_protector_guard): New enum.
>         * config/riscv/riscv.c (riscv_option_override): Handle
>         the new options.
>         * config/riscv/riscv.md (stack_protect_set): New pattern to handle
>         flexible stack protector guard settings.
>         (stack_protect_set_<mode>): Ditto.
>         (stack_protect_test): Ditto.
>         (stack_protect_test_<mode>): Ditto.
>         * config/riscv/riscv.opt (mstack-protector-guard=,
>         mstack-protector-guard-reg=, mstack-protector-guard-offset=): New
>         options.
>         * doc/invoke.texi (Option Summary) [RISC-V Options]:
>         Add -mstack-protector-guard=, -mstack-protector-guard-reg=, and
>         -mstack-protector-guard-offset=.
>         (RISC-V Options): Ditto.

Overall this looks OK.

Please don't include the ChangeLog in the diff.  This is autogenerated
from the git log above now.

I notice in the epilogue I get
    ld a4, 8(sp)
    ld a5, 100(t6)
    xor a5, a4, a5
    bne a5,zero,.L4
This looks like a security leak that the canary value is left in a4.
The i386 implementation operates directly on memory without loading
into registers.  The rs6000 implementation is careful to load 0 into
the other register in the stack_protector_test code after the xor.  I
think this is a bug in the aarch64 code that it isn't clearing the
other register.  And I think it is a bug in your code too.  If we
don't need to clear the canary from the two registers, then you should
eliminate the xor and just use "bne a5,a4,.L4".  But I think the way
you have it is right, you just need to clear the a4 register after the
xor.

If I use an offset outside the const immediate range of -2048 to 2047
then I get an ICE.  You either need to error on invalid offset, or do
some extra work to get valid addresses regardless of the offset.  And
that maybe also requires clearing the extra registers after the load
to avoid leaving the address of the canary in a register after the
sequence depending on how secure this needs to be.

rohan:2457$ ./xgcc -B./ -O -mstack-protector-guard=tls
-mstack-protector-guard-reg=x31 -mstack-protector-guard-offset=2048 -S
tmp.c -fstack-protector-all
tmp.c: In function ‘main’:
tmp.c:8:1: error: unrecognizable insn:
    8 | }
      | ^
(insn 4 3 7 2 (parallel [
            (set (mem/v/f/c:DI (plus:DI (reg/f:DI 67 virtual-stack-vars)
                        (const_int -8 [0xfffffffffffffff8])) [1
D.1495+0 S8 A64])
                (unspec:DI [
                        (mem:DI (plus:DI (reg:DI 31 t6)
                                (const_int 2048 [0x800])) [0  S8 A64])
                    ] UNSPEC_FLE_QUIET))
            (set (scratch:DI)
                (const_int 0 [0]))
        ]) "tmp.c":5:1 -1
     (nil))

Jim

On 2020/7/13 上午8:32, Jim Wilson wrote:
> This looks like a security leak that the canary value is left in a4.
> The i386 implementation operates directly on memory without loading
> into registers.  The rs6000 implementation is careful to load 0 into
> the other register in the stack_protector_test code after the xor.  I
> think this is a bug in the aarch64 code that it isn't clearing the
> other register.  And I think it is a bug in your code too.  If we
> don't need to clear the canary from the two registers, then you should
> eliminate the xor and just use "bne a5,a4,.L4".  But I think the way
> you have it is right, you just need to clear the a4 register after the
> xor.
>
> If I use an offset outside the const immediate range of -2048 to 2047
> then I get an ICE.  You either need to error on invalid offset, or do
> some extra work to get valid addresses regardless of the offset.  And
> that maybe also requires clearing the extra registers after the load
> to avoid leaving the address of the canary in a register after the
> sequence depending on how secure this needs to be.
>
Thanks Jim. I will clear the register after the xor and error on the 
offset outside the const immediate range

to fix these two bugs.


Best regards,

Cooper