diff mbox series

[v2,AArch64] PR92424: Fix -fpatchable-function-entry=N,M with BTI

Message ID 1f29f9cf-5072-8718-9bdf-2a07d6019442@arm.com
State New
Headers show
Series [v2,AArch64] PR92424: Fix -fpatchable-function-entry=N,M with BTI | expand

Commit Message

Szabolcs Nagy Jan. 21, 2020, 3:03 p.m. UTC 
v2:
- emit bti based on feedback from Richard Sandiford
  (dont copy varasm logic).
- add testcases.
- kept bti outside the patch area if possible, i.e. option (b)
  in earlier discussion.

This fix does not update the documentation of the generic
option, I think some text would be useful there about patch
area layout with indirect branch hardening (but i wanted
to keep this fix target specific).

gcc/ChangeLog:

2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
	cfun->machine->label_is_assembled.
	(aarch64_print_patchable_function_entry): New.
	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
	* config/aarch64/aarch64.h (struct machine_function): New field,
	label_is_assembled.

gcc/testsuite/ChangeLog:

2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>

	* gcc.target/aarch64/pr92424-1.c: New test.
	* gcc.target/aarch64/pr92424-2.c: New test.
	* gcc.target/aarch64/pr92424-3.c: New test.

Comments

Richard Sandiford Jan. 21, 2020, 3:34 p.m. UTC | #1 
Szabolcs Nagy <Szabolcs.Nagy@arm.com> writes:
> v2:
> - emit bti based on feedback from Richard Sandiford
>   (dont copy varasm logic).
> - add testcases.
> - kept bti outside the patch area if possible, i.e. option (b)
>   in earlier discussion.
>
> This fix does not update the documentation of the generic
> option, I think some text would be useful there about patch
> area layout with indirect branch hardening (but i wanted
> to keep this fix target specific).

Thanks for the update.  Looks great to me, and given Mark's response,
I agree we should go ahead with this as-is rather than try to change
the position of the BTI.

> gcc/ChangeLog:
>
> 2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
> 	cfun->machine->label_is_assembled.
> 	(aarch64_print_patchable_function_entry): New.
> 	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
> 	* config/aarch64/aarch64.h (struct machine_function): New field,
> 	label_is_assembled.
>
> gcc/testsuite/ChangeLog:
>
> 2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>
> 	* gcc.target/aarch64/pr92424-1.c: New test.
> 	* gcc.target/aarch64/pr92424-2.c: New test.
> 	* gcc.target/aarch64/pr92424-3.c: New test.

OK.  Same in principle for the backport too, but check-function-bodies is
only available on master.

Richard
Szabolcs Nagy Jan. 29, 2020, 2:40 p.m. UTC | #2 
On 21/01/2020 15:34, Richard Sandiford wrote:
> Szabolcs Nagy <Szabolcs.Nagy@arm.com> writes:
>> gcc/ChangeLog:
>>
>> 2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
>> 	cfun->machine->label_is_assembled.
>> 	(aarch64_print_patchable_function_entry): New.
>> 	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
>> 	* config/aarch64/aarch64.h (struct machine_function): New field,
>> 	label_is_assembled.
>>
>> gcc/testsuite/ChangeLog:
>>
>> 2020-01-21  Szabolcs Nagy  <szabolcs.nagy@arm.com>
>>
>> 	* gcc.target/aarch64/pr92424-1.c: New test.
>> 	* gcc.target/aarch64/pr92424-2.c: New test.
>> 	* gcc.target/aarch64/pr92424-3.c: New test.
> 
> OK.  Same in principle for the backport too, but check-function-bodies is
> only available on master.

backported to gcc-9 without the
gcc.target/aarch64/pr92424-1.c test.
diff mbox series

Patch

From 65a60c0dc4318a33e0a0e0a6573084d84bd18a88 Mon Sep 17 00:00:00 2001
From: Szabolcs Nagy <szabolcs.nagy@arm.com>
Date: Wed, 15 Jan 2020 12:23:40 +0000
Subject: [PATCH] [AArch64] PR92424: Fix -fpatchable-function-entry=N,M with
 BTI

This is a workaround that emits a BTI after the function label if that
is followed by a patch area. We try to remove the BTI that follows the
patch area (this may fail e.g. if the first instruction is a PACIASP).

So before this commit -fpatchable-function-entry=3,1 with bti generates

    .section __patchable_function_entries
    .8byte .LPFE
    .text
  .LPFE:
    nop
  foo:
    nop
    nop
    bti c // or paciasp
    ...

and after this commit

    .section __patchable_function_entries
    .8byte .LPFE
    .text
  .LPFE:
    nop
  foo:
    bti c
    nop
    nop
    // may be paciasp
    ...

and with -fpatchable-function-entry=1 (M=0) the code now is

  foo:
    bti c
    .section __patchable_function_entries
    .8byte .LPFE
    .text
  .LPFE:
    nop
    // may be paciasp
    ...

There is a new bti insn in the middle of the patchable area users need
to be aware of unless M=0 (patch area is after the new bti) or M=N
(patch area is before the label, no new bti). Note: bti is not added to
all functions consistently (it can be turned off per function using a
target attribute or the compiler may detect that the function is never
called indirectly), so if bti is inserted in the middle of a patch area
then user code needs to deal with detecting it.

Tested on aarch64-none-linux-gnu.

gcc/ChangeLog:

	* config/aarch64/aarch64.c (aarch64_declare_function_name): Set
	cfun->machine->label_is_assembled.
	(aarch64_print_patchable_function_entry): New.
	(TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY): Define.
	* config/aarch64/aarch64.h (struct machine_function): New field,
	label_is_assembled.

gcc/testsuite/ChangeLog:

	* gcc.target/aarch64/pr92424-1.c: New test.
	* gcc.target/aarch64/pr92424-2.c: New test.
	* gcc.target/aarch64/pr92424-3.c: New test.
---
 gcc/config/aarch64/aarch64.c                 |  31 +++++
 gcc/config/aarch64/aarch64.h                 |   1 +
 gcc/testsuite/gcc.target/aarch64/pr92424-1.c | 122 +++++++++++++++++++
 gcc/testsuite/gcc.target/aarch64/pr92424-2.c |  12 ++
 gcc/testsuite/gcc.target/aarch64/pr92424-3.c |  12 ++
 5 files changed, 178 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr92424-1.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr92424-2.c
 create mode 100644 gcc/testsuite/gcc.target/aarch64/pr92424-3.c

diff --git a/gcc/config/aarch64/aarch64.c b/gcc/config/aarch64/aarch64.c
index e40750380cc..ef037e226a7 100644
--- a/gcc/config/aarch64/aarch64.c
+++ b/gcc/config/aarch64/aarch64.c
@@ -18123,6 +18123,34 @@  aarch64_declare_function_name (FILE *stream, const char* name,
   /* Don't forget the type directive for ELF.  */
   ASM_OUTPUT_TYPE_DIRECTIVE (stream, name, "function");
   ASM_OUTPUT_LABEL (stream, name);
+
+  cfun->machine->label_is_assembled = true;
+}
+
+/* Implement PRINT_PATCHABLE_FUNCTION_ENTRY.  Check if the patch area is after
+   the function label and emit a BTI if necessary.  */
+
+void
+aarch64_print_patchable_function_entry (FILE *file,
+					unsigned HOST_WIDE_INT patch_area_size,
+					bool record_p)
+{
+  if (cfun->machine->label_is_assembled
+      && aarch64_bti_enabled ()
+      && !cgraph_node::get (cfun->decl)->only_called_directly_p ())
+    {
+      /* Remove the BTI that follows the patch area and insert a new BTI
+	 before the patch area right after the function label.  */
+      rtx_insn *insn = next_real_nondebug_insn (get_insns ());
+      if (insn
+	  && INSN_P (insn)
+	  && GET_CODE (PATTERN (insn)) == UNSPEC_VOLATILE
+	  && XINT (PATTERN (insn), 1) == UNSPECV_BTI_C)
+	delete_insn (insn);
+      asm_fprintf (file, "\thint\t34 // bti c\n");
+    }
+
+  default_print_patchable_function_entry (file, patch_area_size, record_p);
 }
 
 /* Implement ASM_OUTPUT_DEF_FROM_DECLS.  Output .variant_pcs for aliases.  */
@@ -21970,6 +21998,9 @@  aarch64_run_selftests (void)
 #undef TARGET_ASM_TRAMPOLINE_TEMPLATE
 #define TARGET_ASM_TRAMPOLINE_TEMPLATE aarch64_asm_trampoline_template
 
+#undef TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY
+#define TARGET_ASM_PRINT_PATCHABLE_FUNCTION_ENTRY aarch64_print_patchable_function_entry
+
 #undef TARGET_BUILD_BUILTIN_VA_LIST
 #define TARGET_BUILD_BUILTIN_VA_LIST aarch64_build_builtin_va_list
 
diff --git a/gcc/config/aarch64/aarch64.h b/gcc/config/aarch64/aarch64.h
index 342fe29bd91..297d197297e 100644
--- a/gcc/config/aarch64/aarch64.h
+++ b/gcc/config/aarch64/aarch64.h
@@ -842,6 +842,7 @@  typedef struct GTY (()) machine_function
   struct aarch64_frame frame;
   /* One entry for each hard register.  */
   bool reg_is_wrapped_separately[LAST_SAVED_REGNUM];
+  bool label_is_assembled;
 } machine_function;
 #endif
 
diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-1.c b/gcc/testsuite/gcc.target/aarch64/pr92424-1.c
new file mode 100644
index 00000000000..c413a2c306e
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr92424-1.c
@@ -0,0 +1,122 @@ 
+/* { dg-do "compile" } */
+/* { dg-options "-O1" } */
+/* { dg-final { check-function-bodies "**" "" } } */
+
+/* Note: this test only checks the instructions in the function bodies,
+   not the placement of the patch label or nops before the futncion.  */
+
+/*
+**f10_none:
+**	nop
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=none"),
+		patchable_function_entry (1, 0)))
+f10_none ()
+{
+}
+
+/*
+**f10_pac:
+**	hint	34 // bti c
+**	nop
+**	hint	25 // paciasp
+**	hint	29 // autiasp
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
+		patchable_function_entry (1, 0)))
+f10_pac ()
+{
+}
+
+/*
+**f10_bti:
+**	hint	34 // bti c
+**	nop
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti"),
+		patchable_function_entry (1, 0)))
+f10_bti ()
+{
+}
+
+/*
+**f11_none:
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=none"),
+		patchable_function_entry (1, 1)))
+f11_none ()
+{
+}
+
+/*
+**f11_pac:
+**	hint	25 // paciasp
+**	hint	29 // autiasp
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
+		patchable_function_entry (1, 1)))
+f11_pac ()
+{
+}
+
+/*
+**f11_bti:
+**	hint	34 // bti c
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti"),
+		patchable_function_entry (1, 1)))
+f11_bti ()
+{
+}
+
+/*
+**f21_none:
+**	nop
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=none"),
+		patchable_function_entry (2, 1)))
+f21_none ()
+{
+}
+
+/*
+**f21_pac:
+**	hint	34 // bti c
+**	nop
+**	hint	25 // paciasp
+**	hint	29 // autiasp
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
+		patchable_function_entry (2, 1)))
+f21_pac ()
+{
+}
+
+/*
+**f21_bti:
+**	hint	34 // bti c
+**	nop
+**	ret
+*/
+void
+__attribute__ ((target("branch-protection=bti"),
+		patchable_function_entry (2, 1)))
+f21_bti ()
+{
+}
diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-2.c b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
new file mode 100644
index 00000000000..0e75657a153
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr92424-2.c
@@ -0,0 +1,12 @@ 
+/* { dg-do "compile" } */
+/* { dg-options "-O1" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+__attribute__ ((target("branch-protection=bti"),
+		patchable_function_entry (1, 0)))
+f10_bti ()
+{
+}
+/* { dg-final { scan-assembler "f10_bti:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\tret\n" } } */
diff --git a/gcc/testsuite/gcc.target/aarch64/pr92424-3.c b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
new file mode 100644
index 00000000000..0a1f74d4096
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/pr92424-3.c
@@ -0,0 +1,12 @@ 
+/* { dg-do "compile" } */
+/* { dg-options "-O1" } */
+
+/* Test the placement of the .LPFE1 label.  */
+
+void
+__attribute__ ((target("branch-protection=bti+pac-ret+leaf"),
+		patchable_function_entry (1, 0)))
+f10_pac ()
+{
+}
+/* { dg-final { scan-assembler "f10_pac:\n\thint\t34 // bti c\n.*\.LPFE1:\n\tnop\n.*\thint\t25 // paciasp\n.*\thint\t29 // autiasp\n.*\tret\n" } } */
-- 
2.17.1