[Bug target/102926] New: TLS register value is spilled to the stack instead of reloaded from the system register

[Bug target/102926] New: TLS register value is spilled to the stack instead of reloaded from the system register

[ardb at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

            Bug ID: 102926
           Summary: TLS register value is spilled to the stack instead of
                    reloaded from the system register
           Product: gcc
           Version: 12.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: ardb at kernel dot org
  Target Milestone: ---
            Target: ARM

The code below uses the hardware TLS register on ARM, and instead of reloading
its value directly from the system register, it spills its value to the stack.
This is suboptimal, and given that the TLS register is being proposed as an
alternative reference for the stack protector canary, it is also a security
concern, as an attacker that controls the stack may be able to control both
sides of the equation in the stack protector check occurring at the end of the
function.

Instead, I would expect any subsequent uses of the thread pointer to simply
issue the MRC again, which doesn't touch memory.

$ cat /tmp/spill.c 

    int foo(int);
    int bar(void)
    {
        int *l = __builtin_thread_pointer();

        return foo(l[0]) + l[1];
    }

$ arm-linux-gnueabihf-gcc -o - -S /tmp/spill.c -O3 -mtp=cp15 -ffixed-r4
-ffixed-r5 -ffixed-r6 -ffixed-r7 -ffixed-r8 -ffixed-r9 -ffixed-r10
-fno-omit-frame-pointer
        .cpu arm10tdmi
        .arch armv5t
        .fpu softvfp
        .eabi_attribute 20, 1
        .eabi_attribute 21, 1
        .eabi_attribute 23, 3
        .eabi_attribute 24, 1
        .eabi_attribute 25, 1
        .eabi_attribute 26, 2
        .eabi_attribute 30, 2
        .eabi_attribute 34, 0
        .eabi_attribute 18, 4
        .file   "spill.c"
        .text
        .align  2
        .global bar
        .syntax unified
        .arm
        .type   bar, %function
bar:
        @ args = 0, pretend = 0, frame = 8
        @ frame_needed = 1, uses_anonymous_args = 0
        push    {fp, lr}
        add     fp, sp, #4
        sub     sp, sp, #8
        mrc     p15, 0, r3, c13, c0, 3  @ load_tp_hard
        ldr     r0, [r3]
        str     r3, [fp, #-8]
        bl      foo
        ldr     r3, [fp, #-8]
        ldr     r3, [r3, #4]
        add     r0, r0, r3
        sub     sp, fp, #4
        @ sp needed
        pop     {fp, pc}
        .size   bar, .-bar
        .ident  "GCC: (GNU) 12.0.0 20211024 (experimental)"
        .section        .note.GNU-stack,"",%progbits

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |missed-optimization

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
I don't think it is a security risk really because all spills then can be
considered security risks.

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

--- Comment #2 from Richard Biener <rguenth at gcc dot gnu.org> ---
I would also guess that load_tp_hard is slower and the large mnemonic suggests
a larger instruction (ok, but we're risc and thus fixed size instructions?)

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[ardb at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

--- Comment #3 from Ard Biesheuvel <ardb at kernel dot org> ---
(In reply to Andrew Pinski from comment #1)
> I don't think it is a security risk really because all spills then can be
> considered security risks.

I would argue that there is no consensus on that. In fact, there is elaborate
handling of the stack canary in the ARM backend, not only to prevent spilling
of the stack canary address or value, but even to ensure that the value is
cleared from all registers that carried it while performing the test or the
set.

In any case, regardless of whether this is a security issue or not (when/if my
TLS stack protector changes get accepted), spilling the TLS register value is
definitely unnecessary.

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

--- Comment #4 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
If anything it is a cost model issue in the back-end.

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[ardb at kernel dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

--- Comment #5 from Ard Biesheuvel <ardb at kernel dot org> ---
Actually, I can reproduce the same behavior without the TLS register, where the
result of a MOVW/MOVT immediate assignment is also spilled to the stack.

    int ll[2];
    int foo(int);
    int bar(void)
    {
        int *l = ll;

        return foo(l[0]) + l[1];
    }

will produce this for armv7-a (with [r4-r11] fixed)

        push    {fp, lr}
        movw    r3, #:lower16:.LANCHOR0
        movt    r3, #:upper16:.LANCHOR0
        add     fp, sp, #4
        sub     sp, sp, #8
        ldr     r0, [r3]
        str     r3, [fp, #-8]
        bl      foo
        ldr     r3, [fp, #-8]
        ldr     r3, [r3, #4]
        add     r0, r0, r3
        sub     sp, fp, #4
        @ sp needed
        pop     {fp, pc}

whereas pre-v7 gives me

        ldr     r3, .L4
        push    {fp, lr}
        add     fp, sp, #4
        ldr     r0, [r3]
        bl      foo
        ldr     r3, .L4
        ldr     r3, [r3, #4]
        add     r0, r0, r3
        pop     {fp, pc}

i.e., the address is simply reloaded from the literal pool rather than spilled.

[Bug target/102926] TLS register value is spilled to the stack instead of reloaded from the system register

[rsandifo at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=102926

rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2021-10-25
                 CC|                            |rsandifo at gcc dot gnu.org

--- Comment #6 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
I guess the first step would be to wrap the rhs of:

(define_insn "load_tp_hard"
  [(set (match_operand:SI 0 "register_operand" "=r")
        (unspec:SI [(const_int 0)] UNSPEC_TLS))]
  "TARGET_HARD_TP"
  "mrc%?\\tp15, 0, %0, c13, c0, 3\\t@ load_tp_hard"
  [(set_attr "predicable" "yes")
   (set_attr "type" "mrs")]
)

in (const …), so that it can be rematerialised.