[Bug target/96191] New: aarch64 stack_protect_test canary leak

[Bug target/96191] New: aarch64 stack_protect_test canary leak

[wilson at gcc dot gnu.org]

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

            Bug ID: 96191
           Summary: aarch64 stack_protect_test canary leak
           Product: gcc
           Version: 11.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: wilson at gcc dot gnu.org
  Target Milestone: ---

Given a simple testcase
extern int sub (int);

int
main (void)
{
  sub (10);
  return 0;
}
commpiling with -O -S -fstack-protector-all -mstack-protector-guard=global
in the epilogue for the canary check I see
        ldr     x1, [sp, 40]
        ldr     x0, [x19, #:lo12:__stack_chk_guard]
        eor     x0, x1, x0
        cbnz    x0, .L4
Both x0 and x1 have the stack protector canary loaded into them, and the eor
clobbers x0, but x1 is left alone.  This means the value of the canary is
leaking from the epilogue.  The canary value is never supposed to survive in a
register outside the stack protector patterns.

A powerpc64-linux toolchain build with the same testcase and options generates
        lwz 9,28(1)
        lwz 10,0(31)
        xor. 9,9,10
        li 10,0
        bne- 0,.L4
and note that it clears the second register after the xor to prevent the canary
leak.  The aarch64 stack_protect_test pattern should do the same thing.

[Bug target/96191] aarch64 stack_protect_test canary leak

[rsandifo at gcc dot gnu.org]

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

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

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Assignee|unassigned at gcc dot gnu.org      |rsandifo at gcc dot gnu.org
     Ever confirmed|0                           |1
             Status|UNCONFIRMED                 |ASSIGNED
                 CC|                            |rsandifo at gcc dot gnu.org
   Last reconfirmed|                            |2020-07-14

--- Comment #1 from rsandifo at gcc dot gnu.org <rsandifo at gcc dot gnu.org> ---
Thanks for the report.  I'm testing a patch.

[Bug target/96191] aarch64 stack_protect_test canary leak

[wilco at gcc dot gnu.org]

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

Wilco <wilco at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |wilco at gcc dot gnu.org

--- Comment #2 from Wilco <wilco at gcc dot gnu.org> ---
(In reply to Jim Wilson from comment #0)
> Given a simple testcase
> extern int sub (int);
> 
> int
> main (void)
> {
>   sub (10);
>   return 0;
> }
> commpiling with -O -S -fstack-protector-all -mstack-protector-guard=global
> in the epilogue for the canary check I see
> 	ldr	x1, [sp, 40]
> 	ldr	x0, [x19, #:lo12:__stack_chk_guard]
> 	eor	x0, x1, x0
> 	cbnz	x0, .L4
> Both x0 and x1 have the stack protector canary loaded into them, and the eor
> clobbers x0, but x1 is left alone.  This means the value of the canary is
> leaking from the epilogue.  The canary value is never supposed to survive in
> a register outside the stack protector patterns.
> 
> A powerpc64-linux toolchain build with the same testcase and options
> generates
> 	lwz 9,28(1)
> 	lwz 10,0(31)
> 	xor. 9,9,10
> 	li 10,0
> 	bne- 0,.L4
> and note that it clears the second register after the xor to prevent the
> canary leak.  The aarch64 stack_protect_test pattern should do the same
> thing.

The canary value is not a secret. What would the purpose of clearing the
register be given the stack slot containing the canary is not cleared as well?
And register could potentially contain the address of the canary or that of a
global nearby, making reading the canary value really easy.

[Bug target/96191] aarch64 stack_protect_test canary leak

[wilson at gcc dot gnu.org]

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

--- Comment #3 from Jim Wilson <wilson at gcc dot gnu.org> ---
The location of the canary is not known to the attacker.  You are not supposed
to leak the address of the canary or the value of the canary.  If you leak
either, then an attacker has a chance to restore the canary after clobbering
it.

See the descriptions of the stack_protect_set and stack_protect_test patterns
in gcc/doc/md.texi which make clear that no intermediate values should be
allowed to survive past the end of the pattern.

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #4 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:fe1a26429038d7cd17abc53f96a6f3e2639b605f

commit r11-2576-gfe1a26429038d7cd17abc53f96a6f3e2639b605f
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Wed Aug 5 15:18:36 2020 +0100

    aarch64: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/aarch64/aarch64.md (stack_protect_test_<mode>): Set the
            CC register directly, instead of a GPR.  Replace the original GPR
            destination with an extra scratch register.  Zero out operand 3
            after use.
            (stack_protect_test): Update accordingly.

    gcc/testsuite/
            PR target/96191
            * gcc.target/aarch64/stack-protector-1.c: New test.
            * gcc.target/aarch64/stack-protector-2.c: Likewise.

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #5 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Richard Sandiford <rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:6a3f3e08723063ea2dadb7ddf503f02972a724e2

commit r11-2598-g6a3f3e08723063ea2dadb7ddf503f02972a724e2
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Thu Aug 6 19:19:41 2020 +0100

    arm: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/arm/arm.md (arm_stack_protect_test_insn): Zero out
            operand 2 after use.
            * config/arm/thumb1.md (thumb1_stack_protect_test_insn): Likewise.

    gcc/testsuite/
            * gcc.target/arm/stack-protector-1.c: New test.
            * gcc.target/arm/stack-protector-2.c: Likewise.

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:bab5fdaf9abb1236a7a56258d1d36265068b4827

commit r10-8583-gbab5fdaf9abb1236a7a56258d1d36265068b4827
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Aug 7 12:15:01 2020 +0100

    aarch64: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/aarch64/aarch64.md (stack_protect_test_<mode>): Set the
            CC register directly, instead of a GPR.  Replace the original GPR
            destination with an extra scratch register.  Zero out operand 3
            after use.
            (stack_protect_test): Update accordingly.

    gcc/testsuite/
            PR target/96191
            * gcc.target/aarch64/stack-protector-1.c: New test.
            * gcc.target/aarch64/stack-protector-2.c: Likewise.

    (cherry picked from commit fe1a26429038d7cd17abc53f96a6f3e2639b605f)

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #7 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-10 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:8f6b7c9796035ad8b2cdfbce5d3d11dd4b81fad7

commit r10-8584-g8f6b7c9796035ad8b2cdfbce5d3d11dd4b81fad7
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Aug 7 12:15:02 2020 +0100

    arm: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/arm/arm.md (arm_stack_protect_test_insn): Zero out
            operand 2 after use.
            * config/arm/thumb1.md (thumb1_stack_protect_test_insn): Likewise.

    gcc/testsuite/
            * gcc.target/arm/stack-protector-1.c: New test.
            * gcc.target/arm/stack-protector-2.c: Likewise.

    (cherry picked from commit 6a3f3e08723063ea2dadb7ddf503f02972a724e2)

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #8 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:5380912a17ea09a8996720fb62b1a70c16c8f9f2

commit r9-8794-g5380912a17ea09a8996720fb62b1a70c16c8f9f2
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Aug 7 12:17:37 2020 +0100

    aarch64: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/aarch64/aarch64.md (stack_protect_test_<mode>): Set the
            CC register directly, instead of a GPR.  Replace the original GPR
            destination with an extra scratch register.  Zero out operand 3
            after use.
            (stack_protect_test): Update accordingly.

    gcc/testsuite/
            PR target/96191
            * gcc.target/aarch64/stack-protector-1.c: New test.
            * gcc.target/aarch64/stack-protector-2.c: Likewise.

    (cherry picked from commit fe1a26429038d7cd17abc53f96a6f3e2639b605f)

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-9 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:3e40be9cc92d3fa117be7f4fab07cedeed8361a2

commit r9-8795-g3e40be9cc92d3fa117be7f4fab07cedeed8361a2
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Fri Aug 7 12:17:37 2020 +0100

    arm: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/arm/arm.md (arm_stack_protect_test_insn): Zero out
            operand 2 after use.
            * config/arm/thumb1.md (thumb1_stack_protect_test_insn): Likewise.

    gcc/testsuite/
            * gcc.target/arm/stack-protector-1.c: New test.
            * gcc.target/arm/stack-protector-2.c: Likewise.

    (cherry picked from commit 6a3f3e08723063ea2dadb7ddf503f02972a724e2)

[Bug target/96191] aarch64 stack_protect_test canary leak

[cvs-commit at gcc dot gnu.org]

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

--- Comment #10 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The releases/gcc-8 branch has been updated by Richard Sandiford
<rsandifo@gcc.gnu.org>:

https://gcc.gnu.org/g:3ee527923b1ce92c6b16c587d072720a6c813c95

commit r8-10627-g3ee527923b1ce92c6b16c587d072720a6c813c95
Author: Richard Sandiford <richard.sandiford@arm.com>
Date:   Tue Nov 17 18:16:45 2020 +0000

    aarch64: Clear canary value after stack_protect_test [PR96191]

    The stack_protect_test patterns were leaving the canary value in the
    temporary register, meaning that it was often still in registers on
    return from the function.  An attacker might therefore have been
    able to use it to defeat stack-smash protection for a later function.

    gcc/
            PR target/96191
            * config/aarch64/aarch64.md (stack_protect_test_<mode>): Set the
            CC register directly, instead of a GPR.  Replace the original GPR
            destination with an extra scratch register.  Zero out operand 3
            after use.
            (stack_protect_test): Update accordingly.

    gcc/testsuite/
            PR target/96191
            * gcc.target/aarch64/stack-protector-1.c: New test.
            * gcc.target/aarch64/stack-protector-2.c: Likewise.

    (cherry picked from commit fe1a26429038d7cd17abc53f96a6f3e2639b605f)