From patchwork Wed Aug 5 14:19:24 2020 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 1341251 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=gcc.gnu.org (client-ip=8.43.85.97; helo=sourceware.org; envelope-from=gcc-patches-bounces@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=arm.com Received: from sourceware.org (server2.sourceware.org [8.43.85.97]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 4BMDK20qMhz9s1x for ; Thu, 6 Aug 2020 00:19:32 +1000 (AEST) Received: from server2.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 557113851C17; Wed, 5 Aug 2020 14:19:30 +0000 (GMT) X-Original-To: gcc-patches@gcc.gnu.org Delivered-To: gcc-patches@gcc.gnu.org Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id EB3943857008 for ; Wed, 5 Aug 2020 14:19:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.3.2 sourceware.org EB3943857008 Authentication-Results: sourceware.org; dmarc=none (p=none dis=none) header.from=arm.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=richard.sandiford@arm.com Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.121.207.14]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 99E0E1424 for ; Wed, 5 Aug 2020 07:19:26 -0700 (PDT) Received: from localhost (e121540-lin.manchester.arm.com [10.32.98.126]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 41B893F7BB for ; Wed, 5 Aug 2020 07:19:26 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, richard.sandiford@arm.com Subject: [committed] aarch64: Clear canary value after stack_protect_test [PR96191] Date: Wed, 05 Aug 2020 15:19:24 +0100 Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 X-Spam-Status: No, score=-13.0 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.2 X-Spam-Checker-Version: SpamAssassin 3.4.2 (2018-09-13) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gcc-patches-bounces@gcc.gnu.org Sender: "Gcc-patches" 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. Tested on aarch64-linux-gnu and aarch64_be-elf, committed. I'll backport to branches over the next few days. I'm about to post a patch for arm too. This is needed to make aarch64 conform to GCC's current -fstack-protector implementation. However, I think we should reconsider whether the zeroing is actually necessary and what it's actually protecting against. I'll send a separate message about that to gcc@. Richard gcc/ PR target/96191 * config/aarch64/aarch64.md (stack_protect_test_): 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. --- gcc/config/aarch64/aarch64.md | 34 ++++--- .../gcc.target/aarch64/stack-protector-1.c | 89 +++++++++++++++++++ .../gcc.target/aarch64/stack-protector-2.c | 6 ++ 3 files changed, 110 insertions(+), 19 deletions(-) create mode 100644 gcc/testsuite/gcc.target/aarch64/stack-protector-1.c create mode 100644 gcc/testsuite/gcc.target/aarch64/stack-protector-2.c diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md index 25d77256b96..9b20dd0b1a0 100644 --- a/gcc/config/aarch64/aarch64.md +++ b/gcc/config/aarch64/aarch64.md @@ -7217,10 +7217,8 @@ (define_expand "stack_protect_test" (match_operand 2)] "" { - rtx result; machine_mode mode = GET_MODE (operands[0]); - result = gen_reg_rtx(mode); if (aarch64_stack_protector_guard != SSP_GLOBAL) { /* Generate access through the system register. The @@ -7245,29 +7243,27 @@ (define_expand "stack_protect_test" operands[1] = gen_rtx_MEM (mode, tmp_reg); } emit_insn ((mode == DImode - ? gen_stack_protect_test_di - : gen_stack_protect_test_si) (result, - operands[0], - operands[1])); - - if (mode == DImode) - emit_jump_insn (gen_cbranchdi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx), - result, const0_rtx, operands[2])); - else - emit_jump_insn (gen_cbranchsi4 (gen_rtx_EQ (VOIDmode, result, const0_rtx), - result, const0_rtx, operands[2])); + ? gen_stack_protect_test_di + : gen_stack_protect_test_si) (operands[0], operands[1])); + + rtx cc_reg = gen_rtx_REG (CCmode, CC_REGNUM); + emit_jump_insn (gen_condjump (gen_rtx_EQ (VOIDmode, cc_reg, const0_rtx), + cc_reg, operands[2])); DONE; }) +;; DO NOT SPLIT THIS PATTERN. It is important for security reasons that the +;; canary value does not live beyond the end of this sequence. (define_insn "stack_protect_test_" - [(set (match_operand:PTR 0 "register_operand" "=r") - (unspec:PTR [(match_operand:PTR 1 "memory_operand" "m") - (match_operand:PTR 2 "memory_operand" "m")] - UNSPEC_SP_TEST)) + [(set (reg:CC CC_REGNUM) + (unspec:CC [(match_operand:PTR 0 "memory_operand" "m") + (match_operand:PTR 1 "memory_operand" "m")] + UNSPEC_SP_TEST)) + (clobber (match_scratch:PTR 2 "=&r")) (clobber (match_scratch:PTR 3 "=&r"))] "" - "ldr\t%3, %1\;ldr\t%0, %2\;eor\t%0, %3, %0" - [(set_attr "length" "12") + "ldr\t%2, %0\;ldr\t%3, %1\;subs\t%2, %2, %3\;mov\t%3, 0" + [(set_attr "length" "16") (set_attr "type" "multiple")]) ;; Write into the Floating-point Status or Control Register. diff --git a/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c b/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c new file mode 100644 index 00000000000..73e83bc413f --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/stack-protector-1.c @@ -0,0 +1,89 @@ +/* { dg-do run } */ +/* { dg-require-effective-target fstack_protector } */ +/* { dg-options "-fstack-protector-all -O2" } */ + +extern volatile long *stack_chk_guard_ptr; + +volatile long * +get_ptr (void) +{ + return stack_chk_guard_ptr; +} + +void __attribute__ ((noipa)) +f (void) +{ + volatile int x; + x = 1; + x += 1; +} + +#define CHECK(REG) "\tcmp\tx0, " #REG "\n\tbeq\t1f\n" + +asm ( +" .pushsection .data\n" +" .align 3\n" +" .globl stack_chk_guard_ptr\n" +"stack_chk_guard_ptr:\n" +#if __ILP32__ +" .word __stack_chk_guard\n" +#else +" .xword __stack_chk_guard\n" +#endif +" .weak __stack_chk_guard\n" +"__stack_chk_guard:\n" +" .word 0xdead4321\n" +" .word 0xbeef8765\n" +" .text\n" +" .globl main\n" +" .type main, %function\n" +"main:\n" +" bl get_ptr\n" +" str x0, [sp, #-16]!\n" +" bl f\n" +" str x0, [sp, #8]\n" +" ldr x0, [sp]\n" +#if __ILP32__ +" ldr w0, [x0]\n" +#else +" ldr x0, [x0]\n" +#endif + CHECK (x1) + CHECK (x2) + CHECK (x3) + CHECK (x4) + CHECK (x5) + CHECK (x6) + CHECK (x7) + CHECK (x8) + CHECK (x9) + CHECK (x10) + CHECK (x11) + CHECK (x12) + CHECK (x13) + CHECK (x14) + CHECK (x15) + CHECK (x16) + CHECK (x17) + CHECK (x18) + CHECK (x19) + CHECK (x20) + CHECK (x21) + CHECK (x22) + CHECK (x23) + CHECK (x24) + CHECK (x25) + CHECK (x26) + CHECK (x27) + CHECK (x28) + CHECK (x29) + CHECK (x30) +" ldr x1, [sp]\n" + CHECK (x1) +" mov x0, #0\n" +" b exit\n" +"1:\n" +" b abort\n" +" .size main, .-main\n" +" .popsection" +); diff --git a/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c b/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c new file mode 100644 index 00000000000..266c36fdbc6 --- /dev/null +++ b/gcc/testsuite/gcc.target/aarch64/stack-protector-2.c @@ -0,0 +1,6 @@ +/* { dg-do run } */ +/* { dg-require-effective-target fstack_protector } */ +/* { dg-require-effective-target fpic } */ +/* { dg-options "-fstack-protector-all -O2 -fpic" } */ + +#include "stack-protector-1.c"