From: Richard Earnshaw <Richard.Earnshaw@foss.arm.com>
To: Andrea Corallo <andrea.corallo@arm.com>,
Andrea Corallo via Gcc-patches <gcc-patches@gcc.gnu.org>
Cc: Richard Earnshaw <Richard.Earnshaw@arm.com>, nd <nd@arm.com>
Subject: Re: [PATCH 10/12 V2] arm: Implement cortex-M return signing address codegen
Date: Fri, 1 Jul 2022 16:43:47 +0100 [thread overview]
Message-ID: <d1a465da-e5c5-2481-6b78-11c2d49edb23@foss.arm.com> (raw)
In-Reply-To: <gkrh7459mf4.fsf_-_@arm.com>
On 28/06/2022 10:17, Andrea Corallo via Gcc-patches wrote:
> Hi all,
>
> second version of this patch enabling address return signature and
> verification based on Armv8.1-M Pointer Authentication [1].
>
> To sign the return address, we use the PAC R12, LR, SP instruction
> upon function entry. This is signing LR using SP and storing the
> result in R12. R12 will be pushed into the stack.
>
> During function epilogue R12 will be popped and AUT R12, LR, SP will
> be used to verify that the content of LR is still valid before return.
>
> Here an example of PAC instrumented function prologue and epilogue:
>
> void foo (void);
>
> int main()
> {
> foo ();
> return 0;
> }
>
> Compiled with '-march=armv8.1-m.main -mbranch-protection=pac-ret
> -mthumb' translates into:
>
> main:
> pac ip, lr, sp
> push {r3, r7, ip, lr}
> add r7, sp, #0
> bl foo
> movs r3, #0
> mov r0, r3
> pop {r3, r7, ip, lr}
> aut ip, lr, sp
> bx lr
>
> The patch also takes care of generating a PACBTI instruction in place
> of the sequence BTI+PAC when Branch Target Identification is enabled
> contextually.
>
> Ex. the previous example compiled with '-march=armv8.1-m.main
> -mbranch-protection=pac-ret+bti -mthumb' translates into:
>
> main:
> pacbti ip, lr, sp
> push {r3, r7, ip, lr}
> add r7, sp, #0
> bl foo
> movs r3, #0
> mov r0, r3
> pop {r3, r7, ip, lr}
> aut ip, lr, sp
> bx lr
>
> As part of previous upstream suggestions a test for varargs has been
> added and '-mtpcs-frame' is deemed being incompatible with this return
> signing address feature being introduced.
>
> [1] <https://community.arm.com/developer/ip-products/processors/b/processors-ip-blog/posts/armv8-1-m-pointer-authentication-and-branch-target-identification-extension>
>
> gcc/Changelog
>
> * config/arm/arm.c: (arm_compute_frame_layout)
> (arm_expand_prologue, thumb2_expand_return, arm_expand_epilogue)
> (arm_conditional_register_usage): Update for pac codegen.
> (arm_current_function_pac_enabled_p): New function.
> * config/arm/arm.md (pac_ip_lr_sp, pacbti_ip_lr_sp, aut_ip_lr_sp):
> Add new patterns.
> * config/arm/unspecs.md (UNSPEC_PAC_IP_LR_SP)
> (UNSPEC_PACBTI_IP_LR_SP, UNSPEC_AUT_IP_LR_SP): Add unspecs.
>
> gcc/testsuite/Changelog
>
> * gcc.target/arm/pac.h : New file.
> * gcc.target/arm/pac-1.c : New test case.
> * gcc.target/arm/pac-2.c : Likewise.
> * gcc.target/arm/pac-3.c : Likewise.
> * gcc.target/arm/pac-4.c : Likewise.
> * gcc.target/arm/pac-5.c : Likewise.
> * gcc.target/arm/pac-6.c : Likewise.
> * gcc.target/arm/pac-7.c : Likewise.
> * gcc.target/arm/pac-8.c : Likewise.
>
@@ -21139,6 +21139,14 @@ arm_compute_save_core_reg_mask (void)
save_reg_mask |= arm_compute_save_reg0_reg12_mask ();
+ if (arm_current_function_pac_enabled_p ())
+ {
+ if (TARGET_TPCS_FRAME
+ || (TARGET_TPCS_LEAF_FRAME && crtl->is_leaf))
+ error ("TPCS incompatible with return address signing.");
+ save_reg_mask |= 1 << IP_REGNUM;
+ }
+
This is the wrong place for a test like this as it will be generated
every time this function is called (which might be more than once per
compiled function).
However, TPCS frames are only supported for 'thumb-1' code and PACBTI
needs armv8-m.main (ie Thumb-2), so the test is really pretty pointless
at the moment. I think we should just drop the error.
@@ -22302,7 +22310,7 @@ arm_emit_multi_reg_pop (unsigned long
saved_regs_mask)
par = emit_insn (par);
REG_NOTES (par) = dwarf;
- if (!return_in_pc)
+ if (!return_in_pc && !frame_pointer_needed)
arm_add_cfa_adjust_cfa_note (par, UNITS_PER_WORD * num_regs,
stack_pointer_rtx, stack_pointer_rtx);
}
What's this hunk for? It doesn't seem related to the PAC changes. Is
this some generic bug? If so, it should be pulled out into a separate
patch. If not, it needs some comment as to why we do it this way.
@@ -23352,6 +23360,11 @@ output_probe_stack_range (rtx reg1, rtx reg2)
return "";
}
+static bool aarch_bti_enabled ()
+{
+ return false;
+}
+
GNU style requires the function name to be in the first column:
static bool
aarch_bti_enabled ()
{
...
@@ -23431,11 +23444,12 @@ arm_expand_prologue (void)
/* The static chain register is the same as the IP register. If it is
clobbered when creating the frame, we need to save and restore
it. */
clobber_ip = IS_NESTED (func_type)
- && ((TARGET_APCS_FRAME && frame_pointer_needed && TARGET_ARM)
- || ((flag_stack_check == STATIC_BUILTIN_STACK_CHECK
- || flag_stack_clash_protection)
- && !df_regs_ever_live_p (LR_REGNUM)
- && arm_r3_live_at_start_p ()));
+ && (((TARGET_APCS_FRAME && frame_pointer_needed && TARGET_ARM)
+ || ((flag_stack_check == STATIC_BUILTIN_STACK_CHECK
+ || flag_stack_clash_protection)
+ && !df_regs_ever_live_p (LR_REGNUM)
+ && arm_r3_live_at_start_p ()))
+ || (arm_current_function_pac_enabled_p ()));
This whole statement needs parenthesis around it so that auto-indent
will work properly:
clobber_ip
= (IS_NESTED (func_type)
&& (....
|| arm_current_function_pac_enabled_p ()));
@@ -23511,6 +23525,14 @@ arm_expand_prologue (void)
}
}
+ if (arm_current_function_pac_enabled_p ())
+ {
+ if (aarch_bti_enabled ())
+ emit_insn (gen_pacbti_nop ());
+ else
+ emit_insn (gen_pac_nop ());
+ }
What about BTI enabled but PAC not?
@@ -27299,7 +27321,8 @@ thumb2_expand_return (bool simple_return)
to assert it for now to ensure that future code changes do not silently
change this behavior. */
gcc_assert (!IS_CMSE_ENTRY (arm_current_func_type ()));
- if (num_regs == 1)
+ if (num_regs == 1
+ && !(arm_current_function_pac_enabled_p ()))
Redundant parenthesis.
@@ -27314,10 +27337,20 @@ thumb2_expand_return (bool simple_return)
}
else
{
- saved_regs_mask &= ~ (1 << LR_REGNUM);
- saved_regs_mask |= (1 << PC_REGNUM);
- arm_emit_multi_reg_pop (saved_regs_mask);
- }
+ if (arm_current_function_pac_enabled_p ())
+ {
+ saved_regs_mask &= ~ (1 << PC_REGNUM);
Is that really needed? The other cases are changing LR to PC, but I
don't think PC should already be set as otherwise our calculation of the
frame size will be incorrect. Please try it as a gcc_assert() and
validate this assertion.
+ arm_emit_multi_reg_pop (saved_regs_mask);
+ emit_insn (gen_aut_nop ());
+ emit_jump_insn (simple_return_rtx);
@@ -30541,6 +30578,9 @@ arm_conditional_register_usage (void)
global_regs[ARM_HARD_FRAME_POINTER_REGNUM] = 1;
}
+ if (TARGET_HAVE_PACBTI)
+ call_used_regs[IP_REGNUM] = 1;
+
Why is this needed? CALL_USED_REGISTERS already defines IP (r12) as
call-used.
+++ b/gcc/config/arm/arm.md
@@ -11514,11 +11514,17 @@
(define_expand "prologue"
[(clobber (const_int 0))]
"TARGET_EITHER"
- "if (TARGET_32BIT)
+ "if (arm_current_function_pac_enabled_p () && !arm_arch8)
+ {
+ error (\"This architecture does not support branch protection
instructions\");
+ DONE;
+ }
No, this is the wrong place for a test like this. A check should be
placed in arm_option_override_internal instead (and normally we warn and
tweak the options to be something sensible if they conflict).
+(define_insn "pac_nop"
+ [(set (reg:SI IP_REGNUM)
+ (unspec:SI [(reg:SI SP_REGNUM) (reg:SI LR_REGNUM)]
+ UNSPEC_PAC_NOP))]
+ "TARGET_THUMB2"
+ "pac\t%|ip, %|lr, %|sp"
+ [(set_attr "length" "2")])
This pattern is missing a type. The length is also incorrect as the
instruction is 32-bits (4 bytes). Similarly for the other instructions
below. Also, you need to mark them as incompatible with conditional
execution (they're constrained-unpredictable in IT blocks).
All of the tests lack checks that the target board can run PAC/BTI.
R.
next prev parent reply other threads:[~2022-07-01 15:43 UTC|newest]
Thread overview: 53+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-04-28 8:39 [PATCH 0/12] arm: Enables return address verification and branch target identification on Cortex-M Andrea Corallo
2022-04-28 9:08 ` [PATCH 1/12] arm: Make mbranch-protection opts parsing common to AArch32/64 Andrea Corallo
2022-07-01 10:49 ` Richard Earnshaw
2022-04-28 9:37 ` [PATCH 2/12] arm: Add Armv8.1-M Mainline target feature +pacbti Andrea Corallo
2022-07-01 10:51 ` Richard Earnshaw
2022-04-28 9:38 ` [PATCH 3/12] arm: Add option -mbranch-protection Andrea Corallo
2022-07-01 10:59 ` Richard Earnshaw
2022-07-04 9:27 ` [PATCH 3/12 V2] " Andrea Corallo
2022-07-04 10:55 ` Richard Earnshaw
2022-04-28 9:40 ` [PATCH 4/12] arm: Add testsuite library support for PACBTI target Andrea Corallo
2022-07-01 13:03 ` Richard Earnshaw
2022-07-01 14:17 ` Richard Earnshaw
2022-07-04 14:47 ` Andrea Corallo
2022-07-05 10:05 ` Richard Earnshaw
2022-04-28 9:42 ` [PATCH 5/12] arm: Implement target feature macros for PACBTI Andrea Corallo
2022-07-01 14:26 ` Richard Earnshaw
2022-07-12 15:45 ` [PATCH 5/12 V2] " Andrea Corallo
2022-07-21 11:01 ` Richard Earnshaw
2022-07-22 10:35 ` [PATCH 5/12 V3] " Andrea Corallo
2022-07-22 14:34 ` [PATCH 5/12 V4] " Andrea Corallo
2022-04-28 9:44 ` [PATCH 6/12] arm: Add pointer authentication for stack-unwinding runtime Andrea Corallo
2022-07-01 14:41 ` Richard Earnshaw
2022-11-09 11:17 ` [PATCH 6/12 V2] " Andrea Corallo
2022-04-28 9:45 ` [PATCH 7/12] arm: Emit build attributes for PACBTI target feature Andrea Corallo
2022-07-01 14:49 ` Richard Earnshaw
2022-07-13 8:58 ` [PATCH 7/12 V2] " Andrea Corallo
2022-07-21 11:03 ` Richard Earnshaw
2022-07-22 14:57 ` Andrea Corallo
2022-04-28 9:46 ` [PATCH 8/12] arm: Introduce multilibs " Andrea Corallo
2022-06-01 12:32 ` [PATCH 8/12 V2] " Andrea Corallo
2022-07-01 14:54 ` Richard Earnshaw
2022-07-01 14:57 ` Richard Earnshaw
2022-07-21 9:04 ` [PATCH 8/12 V3] " Andrea Corallo
2022-07-21 11:09 ` Richard Earnshaw
2022-04-28 9:48 ` [PATCH 9/12] arm: Make libgcc bti compatible Andrea Corallo
2022-07-01 15:03 ` Richard Earnshaw
2022-07-21 9:17 ` [PATCH 9/12 V2] " Andrea Corallo
2022-07-21 11:41 ` Richard Earnshaw
2022-07-22 15:09 ` Andrea Corallo
2022-07-25 10:41 ` Richard Earnshaw
2022-12-12 14:54 ` Andrea Corallo
2022-04-28 9:50 ` [PATCH 10/12] arm: Implement cortex-M return signing address codegen Andrea Corallo
2022-06-28 9:17 ` [PATCH 10/12 V2] " Andrea Corallo
2022-07-01 15:43 ` Richard Earnshaw [this message]
2022-08-08 9:33 ` Andrea Corallo
2022-10-20 14:53 ` Kyrylo Tkachov
2022-04-28 9:51 ` [PATCH 11/12] aarch64: Make bti pass generic so it can be used by the arm backend Andrea Corallo
2022-05-06 8:23 ` Richard Sandiford
2022-07-01 15:53 ` Richard Earnshaw
2022-04-28 9:53 ` [PATCH 12/12] arm: implement bti injection Andrea Corallo
2022-06-28 9:21 ` [PATCH 12/12 V2] " Andrea Corallo
2022-07-01 16:04 ` Richard Earnshaw
2022-06-01 12:34 ` [PATCH 0/12] arm: Enables return address verification and branch target identification on Cortex-M Andrea Corallo
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d1a465da-e5c5-2481-6b78-11c2d49edb23@foss.arm.com \
--to=richard.earnshaw@foss.arm.com \
--cc=Richard.Earnshaw@arm.com \
--cc=andrea.corallo@arm.com \
--cc=gcc-patches@gcc.gnu.org \
--cc=nd@arm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).