Bug 96191 - aarch64 stack_protect_test canary leak
Summary: aarch64 stack_protect_test canary leak
Status: ASSIGNED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 11.0
: P3 normal
Target Milestone: ---
Assignee: rsandifo@gcc.gnu.org
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-07-14 02:49 UTC by Jim Wilson
Modified: 2020-08-07 11:18 UTC (History)
3 users (show)

See Also:
Host:
Target: aarch64
Build:
Known to work:
Known to fail:
Last reconfirmed: 2020-07-14 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Jim Wilson 2020-07-14 02:49:26 UTC
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.
Comment 1 rsandifo@gcc.gnu.org 2020-07-14 08:22:23 UTC
Thanks for the report.  I'm testing a patch.
Comment 2 Wilco 2020-07-14 18:58:38 UTC
(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.
Comment 3 Jim Wilson 2020-07-14 22:30:21 UTC
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.
Comment 4 CVS Commits 2020-08-05 14:19:20 UTC
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.
Comment 5 CVS Commits 2020-08-06 18:20:05 UTC
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.
Comment 6 CVS Commits 2020-08-07 11:15:33 UTC
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)
Comment 7 CVS Commits 2020-08-07 11:15:39 UTC
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)
Comment 8 CVS Commits 2020-08-07 11:18:01 UTC
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)
Comment 9 CVS Commits 2020-08-07 11:18:06 UTC
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)