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.
Thanks for the report. I'm testing a patch.
(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.
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.
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.
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.
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)
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)
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)
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)
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)