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

[Jim Wilson]

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