From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from foss.arm.com (foss.arm.com [217.140.110.172]) by sourceware.org (Postfix) with ESMTP id 98572386001F; Mon, 31 Jan 2022 17:00:03 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 98572386001F 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 4116FED1; Mon, 31 Jan 2022 09:00:03 -0800 (PST) Received: from localhost (unknown [10.32.98.88]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id A94143F73B; Mon, 31 Jan 2022 09:00:01 -0800 (PST) From: Richard Sandiford To: Dan Li Mail-Followup-To: Dan Li , gcc-patches@gcc.gnu.org, richard.earnshaw@arm.com, marcus.shawcroft@arm.com, kyrylo.tkachov@arm.com, hp@gcc.gnu.org, ndesaulniers@google.com, nsz@gcc.gnu.org, pageexec@gmail.com, qinzhao@gcc.gnu.org, linux-hardening@vger.kernel.org, richard.sandiford@arm.com Cc: gcc-patches@gcc.gnu.org, richard.earnshaw@arm.com, marcus.shawcroft@arm.com, kyrylo.tkachov@arm.com, hp@gcc.gnu.org, ndesaulniers@google.com, nsz@gcc.gnu.org, pageexec@gmail.com, qinzhao@gcc.gnu.org, linux-hardening@vger.kernel.org Subject: Re: [PATCH] [PATCH, v3, 1/1, AARCH64][PR102768] aarch64: Add compiler support for Shadow Call Stack References: <20220129150035.114602-1-ashimida@linux.alibaba.com> Date: Mon, 31 Jan 2022 17:00:00 +0000 In-Reply-To: <20220129150035.114602-1-ashimida@linux.alibaba.com> (Dan Li's message of "Sat, 29 Jan 2022 07:00:35 -0800") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-12.4 required=5.0 tests=BAYES_00, GIT_PATCH_0, KAM_DMARC_STATUS, KAM_SHORT, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) 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: , X-List-Received-Date: Mon, 31 Jan 2022 17:00:09 -0000 Dan Li writes: > Shadow Call Stack can be used to protect the return address of a > function at runtime, and clang already supports this feature[1]. > > To enable SCS in user mode, in addition to compiler, other support > is also required (as discussed in [2]). This patch only adds basic > support for SCS from the compiler side, and provides convenience > for users to enable SCS. > > For linux kernel, only the support of the compiler is required. > > [1] https://clang.llvm.org/docs/ShadowCallStack.html > [2] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D102768 > > Signed-off-by: Dan Li > > gcc/ChangeLog: > > * config/aarch64/aarch64.c (aarch64_layout_frame): > Change callee_adjust when scs is enabled. > (aarch64_restore_callee_saves): Avoid pop x30 twice > when scs is enabled. > (aarch64_expand_prologue): Push x30 onto SCS before it's > pushed onto stack. > (aarch64_expand_epilogue): Pop x30 frome SCS, while > preventing it from being popped from the regular stack again. > (aarch64_override_options_internal): Add SCS compile option check. > (TARGET_HAVE_SHADOW_CALL_STACK): New hook. > * config/aarch64/aarch64.h (struct GTY): Add is_scs_enabled. > * config/aarch64/aarch64.md (scs_push): New template. > (scs_pop): Likewise. > * doc/invoke.texi: Document -fsanitize=3Dshadow-call-stack. > * doc/tm.texi: Regenerate. > * doc/tm.texi.in: Add hook have_shadow_call_stack. > * flag-types.h (enum sanitize_code): > Add SANITIZE_SHADOW_CALL_STACK. > * opts.c: Add shadow-call-stack. > * target.def: New hook. > * toplev.c (process_options): Add SCS compile option check. > > gcc/testsuite/ChangeLog: > > * gcc.target/aarch64/shadow_call_stack_1.c: New test. > * gcc.target/aarch64/shadow_call_stack_2.c: New test. > * gcc.target/aarch64/shadow_call_stack_3.c: New test. > * gcc.target/aarch64/shadow_call_stack_4.c: New test. > * gcc.target/aarch64/shadow_call_stack_5.c: New test. > * gcc.target/aarch64/shadow_call_stack_6.c: New test. > * gcc.target/aarch64/shadow_call_stack_7.c: New test. > * gcc.target/aarch64/shadow_call_stack_8.c: New test. > --- > V3: > - Change scs_push/pop to standard move patterns. > - Optimize scs_pop to avoid pop x30 twice when shadow stack is enabled. > > gcc/config/aarch64/aarch64.c | 66 +++++++++++++++++-- > gcc/config/aarch64/aarch64.h | 4 ++ > gcc/config/aarch64/aarch64.md | 10 +++ > gcc/doc/invoke.texi | 30 +++++++++ > gcc/doc/tm.texi | 5 ++ > gcc/doc/tm.texi.in | 2 + > gcc/flag-types.h | 2 + > gcc/opts.c | 1 + > gcc/target.def | 8 +++ > .../gcc.target/aarch64/shadow_call_stack_1.c | 6 ++ > .../gcc.target/aarch64/shadow_call_stack_2.c | 6 ++ > .../gcc.target/aarch64/shadow_call_stack_3.c | 45 +++++++++++++ > .../gcc.target/aarch64/shadow_call_stack_4.c | 20 ++++++ > .../gcc.target/aarch64/shadow_call_stack_5.c | 18 +++++ > .../gcc.target/aarch64/shadow_call_stack_6.c | 18 +++++ > .../gcc.target/aarch64/shadow_call_stack_7.c | 18 +++++ > .../gcc.target/aarch64/shadow_call_stack_8.c | 24 +++++++ > gcc/toplev.c | 10 +++ > 18 files changed, 289 insertions(+), 4 deletions(-) > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_1.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_2.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_3.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_4.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_5.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_6.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_7.c > create mode 100644 gcc/testsuite/gcc.target/aarch64/shadow_call_stack_8.c > > diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c > index 699c105a42a..461c205010e 100644 > --- a/gcc/config/aarch64/aarch64.c > +++ b/gcc/config/aarch64/aarch64.c > @@ -79,6 +79,7 @@ > #include "tree-ssa-loop-niter.h" > #include "fractional-cost.h" > #include "rtlanal.h" > +#include "asan.h" >=20=20 > /* This file should be included last. */ > #include "target-def.h" > @@ -7478,10 +7479,31 @@ aarch64_layout_frame (void) > frame.sve_callee_adjust =3D 0; > frame.callee_offset =3D 0; >=20=20 > + /* Shadow call stack only deal with functions where the LR is pushed Typo: s/deal/deals/ > + onto the stack and without specifying the "no_sanitize" attribute > + with the argument "shadow-call-stack". */ > + frame.is_scs_enabled > + =3D (!crtl->calls_eh_return > + && (sanitize_flags_p (SANITIZE_SHADOW_CALL_STACK) > + && known_ge (cfun->machine->frame.reg_offset[LR_REGNUM], 0))); Nit, but normal GCC style would be to use a single chain of &&s here: frame.is_scs_enabled =3D (!crtl->calls_eh_return && sanitize_flags_p (SANITIZE_SHADOW_CALL_STACK) && known_ge (cfun->machine->frame.reg_offset[LR_REGNUM], 0)); > + > + /* When shadow call stack is enabled, the scs_pop in the epilogue will > + restore x30, and we don't need to pop x30 again in the traditional > + way. At this time, if candidate2 is x30, we need to adjust > + max_push_offset to 256 to ensure that the offset meets the requirem= ents > + of emit_move_insn. Similarly, if candidate1 is x30, we need to set > + max_push_offset to 0, because x30 is not popped up at this time, so > + callee_adjust cannot be adjusted. */ > HOST_WIDE_INT max_push_offset =3D 0; > if (frame.wb_candidate2 !=3D INVALID_REGNUM) > - max_push_offset =3D 512; > - else if (frame.wb_candidate1 !=3D INVALID_REGNUM) > + { > + if (frame.is_scs_enabled && frame.wb_candidate2 =3D=3D R30_REGNUM) > + max_push_offset =3D 256; > + else > + max_push_offset =3D 512; > + } > + else if ((frame.wb_candidate1 !=3D INVALID_REGNUM) > + && !(frame.is_scs_enabled && frame.wb_candidate1 =3D=3D R30_REGNUM)) > max_push_offset =3D 256; > HOST_WIDE_INT const_size, const_outgoing_args_size, const_fp_offset; Maybe we should instead add separate fields for wb_push_candidate[12] and wb_pop_candidate[12]. The pop candidates would start out the same as the push candidates, but R30_REGNUM would get replaced with INVALID_REGNUM for SCS. Admittedly, suppressing the restore of x30 is turning out to be a bit more difficult than I'd realised :-/ > [=E2=80=A6] > diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h > index 2792bb29adb..1610a4fd74c 100644 > --- a/gcc/config/aarch64/aarch64.h > +++ b/gcc/config/aarch64/aarch64.h > @@ -916,6 +916,10 @@ struct GTY (()) aarch64_frame > unsigned spare_pred_reg; >=20=20 > bool laid_out; > + > + /* Nonzero if shadow call stack should be enabled for the current > + function, otherwise return FALSE. */ =E2=80=9CTrue=E2=80=9D seems better than =E2=80=9CNonzero=E2=80=9D given th= at this is a bool. (A lot of GCC bools were originally ints, which is why =E2=80=9Cnonzero=E2= =80=9D still appears in non-obvious places.) I think we can just drop =E2=80=9Cotherwise return FALSE=E2=80=9D: =E2=80= =9Creturn=E2=80=9D doesn't seem appropriate here, given that it's a variable. Looks great otherwise. Thanks especially for testing the corner cases. :-) One minor thing: > +/* { dg-final { scan-assembler-times "str\tx30, \\\[x18\\\], \[#|$\]?8" = 2 } } */ > +/* { dg-final { scan-assembler-times "ldr\tx30, \\\[x18, \[#|$\]?-8\\\]!= " 2 } } */ This sort of regexp can be easier to write if you quote them using {=E2=80= =A6} rather than "=E2=80=A6", since it reduces the number of backslashes needed.= E.g.: /* { dg-final { scan-assembler-times {str\tx30, \[x18\], [#|$]?8} 2 } } */ The current version is fine too though, and is widely used. Just mentioning it in case it's useful in future. Also, [#|$]? can be written #?. Thanks, Richard