[PATCH 0/4] [rs6000] ROP support

[PATCH 0/4] [rs6000] ROP support

[Bill Schmidt]

Add POWER10 support for hashst[p] and hashchk[p] operations.  When
the -mrop-protect option is selected, any function that loads the link
register from memory before returning must have protection in the
prologue and epilogue to ensure the link register save location has
not been compromised.  If -mprivileged is also specified, the
protection instructions generated require supervisor privilege.

The patches are broken up into logical chunks:
 - Option handling
 - Instruction generation
 - Predefined macro handling
 - Test cases

Bootstrapped and tested on a POWER10 system with no regressions.
Tests on a kernel that enables user-space ROP mitigation were
successful.  Is this series ok for trunk?  I would also like to later
backport these patches to GCC for the 11.2 release.

Thanks!
Bill

Bill Schmidt (4):
  rs6000: Add -mrop-protect and -mprivileged flags
  rs6000: Emit ROP-protect instructions in prologue and epilogue
  rs6000: Conditionally define __ROP_PROTECT__
  rs6000: Add ROP tests

 gcc/config/rs6000/rs6000-c.c             |  3 +
 gcc/config/rs6000/rs6000-internal.h      |  2 +
 gcc/config/rs6000/rs6000-logue.c         | 86 +++++++++++++++++++++---
 gcc/config/rs6000/rs6000.c               |  7 ++
 gcc/config/rs6000/rs6000.md              | 39 +++++++++++
 gcc/config/rs6000/rs6000.opt             |  6 ++
 gcc/doc/invoke.texi                      | 19 +++++-
 gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 +++++
 gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 +++++
 gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 ++++++
 gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++
 gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++
 12 files changed, 231 insertions(+), 13 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c

-- 
2.27.0

[PATCH 1/4] rs6000: Add -mrop-protect and -mprivileged flags

[Bill Schmidt]

2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	* config/rs6000/rs6000.c (rs6000_option_override_internal):
	Disable shrink wrap when inserting ROP-protect instructions.
	* config/rs6000/rs6000.opt (mrop-protect): New option.
	(mprivileged): Likewise.
	* doc/invoke.texi: Document mrop-protect and mprivileged.
---
 gcc/config/rs6000/rs6000.c   |  7 +++++++
 gcc/config/rs6000/rs6000.opt |  6 ++++++
 gcc/doc/invoke.texi          | 19 +++++++++++++++++--
 3 files changed, 30 insertions(+), 2 deletions(-)

diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 844fee88cf3..d13ed6e7ff4 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -4036,6 +4036,13 @@ rs6000_option_override_internal (bool global_init_p)
       && ((rs6000_isa_flags_explicit & OPTION_MASK_QUAD_MEMORY_ATOMIC) == 0))
     rs6000_isa_flags |= OPTION_MASK_QUAD_MEMORY_ATOMIC;
 
+  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
+  if (rs6000_rop_protect)
+    {
+      flag_shrink_wrap = 0;
+      flag_shrink_wrap_separate = 0;
+    }
+
   /* If we can shrink-wrap the TOC register save separately, then use
      -msave-toc-indirect unless explicitly disabled.  */
   if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
index 0dbdf753673..d116fd12f7e 100644
--- a/gcc/config/rs6000/rs6000.opt
+++ b/gcc/config/rs6000/rs6000.opt
@@ -619,3 +619,9 @@ Generate (do not generate) MMA instructions.
 
 mrelative-jumptables
 Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save
+
+mrop-protect
+Target Var(rs6000_rop_protect) Init(0)
+
+mprivileged
+Target Var(rs6000_privileged) Init(0)
diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
index e98b0962b9f..36bd0bf9b3b 100644
--- a/gcc/doc/invoke.texi
+++ b/gcc/doc/invoke.texi
@@ -1238,7 +1238,8 @@ See RS/6000 and PowerPC Options.
 -mgnu-attribute  -mno-gnu-attribute @gol
 -mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{reg} @gol
 -mstack-protector-guard-offset=@var{offset} -mprefixed -mno-prefixed @gol
--mpcrel -mno-pcrel -mmma -mno-mmma}
+-mpcrel -mno-pcrel -mmma -mno-mmma -mrop-protect -mno-rop-protect @gol
+-mprivileged -mno-privileged}
 
 @emph{RX Options}
 @gccoptlist{-m64bit-doubles  -m32bit-doubles  -fpu  -nofpu@gol
@@ -27019,7 +27020,8 @@ following options:
 -mmulhw  -mdlmzb  -mmfpgpr  -mvsx @gol
 -mcrypto  -mhtm  -mpower8-fusion  -mpower8-vector @gol
 -mquad-memory  -mquad-memory-atomic  -mfloat128 @gol
--mfloat128-hardware -mprefixed -mpcrel -mmma}
+-mfloat128-hardware -mprefixed -mpcrel -mmma @gol
+-mrop-protect -mprivileged}
 
 The particular options set for any particular CPU varies between
 compiler versions, depending on what setting seems to produce optimal
@@ -28024,6 +28026,19 @@ store instructions when the option @option{-mcpu=future} is used.
 Generate (do not generate) the MMA instructions when the option
 @option{-mcpu=future} is used.
 
+@item -mrop-protect
+@itemx -mno-rop-protect
+@opindex mrop-protect
+@opindex mno-rop-protect
+Generate (do not generate) ROP protection instructions when the option
+@option{-mcpu=power10} is used.
+
+@item -mprivileged
+@itemx -mno-privileged
+@opindex mprivileged
+@opindex mno-privileged
+Generate (do not generate) instructions for privileged state.
+
 @item -mblock-ops-unaligned-vsx
 @itemx -mno-block-ops-unaligned-vsx
 @opindex block-ops-unaligned-vsx
-- 
2.27.0

[PATCH 2/4] rs6000: Emit ROP-protect instructions in prologue and epilogue

[Bill Schmidt]

Insert the hashst and hashchk instructions when -mrop-protect has been
selected.  The encrypted save slot for ROP mitigation is placed
between the parameter save area and the alloca space (if any;
otherwise the local variable space).

Note that ROP-mitigation instructions are currently only provided for
the ELFv2 ABI.

2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	* config/rs6000/rs6000-internal.h (rs6000_stack): Add
	rop_check_save_offset and rop_check_size.
	* config/rs6000/rs6000-logue.c (rs6000_stack_info): Compute
	rop_check_size and rop_check_save_offset.
	(debug_stack_info): Dump rop_save_offset and rop_check_size.
	(rs6000_emit_prologue): Assert if WORLD_SAVE used with
	-mrop-protect; emit hashst[p] in prologue; emit hashchk[p] in
	epilogue.
	* config/rs6000/rs6000.md (unspec): Add UNSPEC_HASHST[P] and
	UNSPEC_HASHCHK[P].
	(hashst): New define_insn.
	(hashstp): Likewise.
	(hashchk): Likewise.
	(hashchkp): Likewise.
---
 gcc/config/rs6000/rs6000-internal.h |  2 +
 gcc/config/rs6000/rs6000-logue.c    | 86 +++++++++++++++++++++++++----
 gcc/config/rs6000/rs6000.md         | 39 +++++++++++++
 3 files changed, 116 insertions(+), 11 deletions(-)

diff --git a/gcc/config/rs6000/rs6000-internal.h b/gcc/config/rs6000/rs6000-internal.h
index 428a7861a98..8fc77ba6138 100644
--- a/gcc/config/rs6000/rs6000-internal.h
+++ b/gcc/config/rs6000/rs6000-internal.h
@@ -39,6 +39,7 @@ typedef struct rs6000_stack {
   int gp_save_offset;		/* offset to save GP regs from initial SP */
   int fp_save_offset;		/* offset to save FP regs from initial SP */
   int altivec_save_offset;	/* offset to save AltiVec regs from initial SP */
+  int rop_check_save_offset;	/* offset to save ROP check from initial SP */
   int lr_save_offset;		/* offset to save LR from initial SP */
   int cr_save_offset;		/* offset to save CR from initial SP */
   int vrsave_save_offset;	/* offset to save VRSAVE from initial SP */
@@ -53,6 +54,7 @@ typedef struct rs6000_stack {
   int gp_size;			/* size of saved GP registers */
   int fp_size;			/* size of saved FP registers */
   int altivec_size;		/* size of saved AltiVec registers */
+  int rop_check_size;		/* size of ROP check slot */
   int cr_size;			/* size to hold CR if not in fixed area */
   int vrsave_size;		/* size to hold VRSAVE */
   int altivec_padding_size;	/* size of altivec alignment padding */
diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
index b0ac183ceff..10cf7a2de93 100644
--- a/gcc/config/rs6000/rs6000-logue.c
+++ b/gcc/config/rs6000/rs6000-logue.c
@@ -595,19 +595,21 @@ rs6000_savres_strategy (rs6000_stack_t *info,
 		+---------------------------------------+
 		| Parameter save area (+padding*) (P)	|  32
 		+---------------------------------------+
-		| Alloca space (A)			|  32+P
+		| Optional ROP check slot (R)		|  32+P
 		+---------------------------------------+
-		| Local variable space (L)		|  32+P+A
+		| Alloca space (A)			|  32+P+R
 		+---------------------------------------+
-		| Save area for AltiVec registers (W)	|  32+P+A+L
+		| Local variable space (L)		|  32+P+R+A
 		+---------------------------------------+
-		| AltiVec alignment padding (Y)		|  32+P+A+L+W
+		| Save area for AltiVec registers (W)	|  32+P+R+A+L
 		+---------------------------------------+
-		| Save area for GP registers (G)	|  32+P+A+L+W+Y
+		| AltiVec alignment padding (Y)		|  32+P+R+A+L+W
 		+---------------------------------------+
-		| Save area for FP registers (F)	|  32+P+A+L+W+Y+G
+		| Save area for GP registers (G)	|  32+P+R+A+L+W+Y
 		+---------------------------------------+
-	old SP->| back chain to caller's caller		|  32+P+A+L+W+Y+G+F
+		| Save area for FP registers (F)	|  32+P+R+A+L+W+Y+G
+		+---------------------------------------+
+	old SP->| back chain to caller's caller		|  32+P+R+A+L+W+Y+G+F
 		+---------------------------------------+
 
      * If the alloca area is present, the parameter save area is
@@ -717,6 +719,19 @@ rs6000_stack_info (void)
   /* Does this function call anything (apart from sibling calls)?  */
   info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame);
 
+  if (TARGET_POWER10 && info->calls_p
+      && DEFAULT_ABI == ABI_ELFv2 && rs6000_rop_protect)
+    info->rop_check_size = 8;
+  else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2)
+    {
+      /* We can't check this in rs6000_option_override_internal since
+	 DEFAULT_ABI isn't established yet.  */
+      error ("%qs requires the ELFv2 ABI", "-mrop-protect");
+      info->rop_check_size = 0;
+    }
+  else
+    info->rop_check_size = 0;
+
   /* Determine if we need to save the condition code registers.  */
   if (save_reg_p (CR2_REGNO)
       || save_reg_p (CR3_REGNO)
@@ -806,8 +821,14 @@ rs6000_stack_info (void)
 	  gcc_assert (info->altivec_size == 0
 		      || info->altivec_save_offset % 16 == 0);
 
-	  /* Adjust for AltiVec case.  */
-	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;
+	  if (info->rop_check_size != 0)
+	    {
+	      info->rop_check_save_offset
+		= info->altivec_save_offset - info->rop_check_size;
+	      info->ehrd_offset = info->rop_check_save_offset - ehrd_size;
+	    }
+	  else
+	    info->ehrd_offset = info->altivec_save_offset - ehrd_size;
 	}
       else
 	info->ehrd_offset = info->gp_save_offset - ehrd_size;
@@ -849,6 +870,7 @@ rs6000_stack_info (void)
 				  + info->gp_size
 				  + info->altivec_size
 				  + info->altivec_padding_size
+				  + info->rop_check_size
 				  + ehrd_size
 				  + ehcr_size
 				  + info->cr_size
@@ -987,6 +1009,10 @@ debug_stack_info (rs6000_stack_t *info)
     fprintf (stderr, "\tvrsave_save_offset  = %5d\n",
 	     info->vrsave_save_offset);
 
+  if (info->rop_check_size)
+    fprintf (stderr, "\trop_check_save_offset = %5d\n",
+	     info->rop_check_save_offset);
+
   if (info->lr_save_p)
     fprintf (stderr, "\tlr_save_offset      = %5d\n", info->lr_save_offset);
 
@@ -1026,6 +1052,9 @@ debug_stack_info (rs6000_stack_t *info)
     fprintf (stderr, "\taltivec_padding_size= %5d\n",
 	     info->altivec_padding_size);
 
+  if (info->rop_check_size)
+    fprintf (stderr, "\trop_check_size      = %5d\n", info->rop_check_size);
+
   if (info->cr_size)
     fprintf (stderr, "\tcr_size             = %5d\n", info->cr_size);
 
@@ -3044,7 +3073,6 @@ rs6000_emit_prologue (void)
 	cfun->machine->r2_setup_needed = true;
     }
 
-
   if (flag_stack_usage_info)
     current_function_static_stack_size = info->total_size;
 
@@ -3105,7 +3133,8 @@ rs6000_emit_prologue (void)
 		  && (!crtl->calls_eh_return
 		      || info->ehrd_offset == -432)
 		  && info->vrsave_save_offset == -224
-		  && info->altivec_save_offset == -416);
+		  && info->altivec_save_offset == -416
+		  && !rs6000_rop_protect);
 
       treg = gen_rtx_REG (SImode, 11);
       emit_move_insn (treg, GEN_INT (-info->total_size));
@@ -3252,6 +3281,24 @@ rs6000_emit_prologue (void)
 	}
     }
 
+  /* The ROP hash store must occur before a stack frame is created.  */
+  /* NOTE: The hashst isn't needed if we're going to do a sibcall,
+     but there's no way to know that here.  Harmless except for
+     performance, of course.  */
+  if (TARGET_POWER10 && rs6000_rop_protect && info->rop_check_size != 0)
+    {
+      gcc_assert (DEFAULT_ABI == ABI_ELFv2);
+      rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
+      rtx addr = gen_rtx_PLUS (Pmode, stack_ptr,
+			       GEN_INT (info->rop_check_save_offset));
+      rtx mem = gen_rtx_MEM (Pmode, addr);
+      rtx reg0 = gen_rtx_REG (Pmode, 0);
+      if (rs6000_privileged)
+	emit_insn (gen_hashstp (mem, reg0));
+      else
+	emit_insn (gen_hashst (mem, reg0));
+    }
+
   /* If we need to save CR, put it into r12 or r11.  Choose r12 except when
      r12 will be needed by out-of-line gpr save.  */
   cr_save_regno = ((DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
@@ -4980,6 +5027,23 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
       emit_insn (gen_add3_insn (sp_reg_rtx, sp_reg_rtx, sa));
     }
 
+  /* The ROP hash check must occur after the stack pointer is restored,
+     and is not performed for a sibcall.  */
+  if (TARGET_POWER10 && rs6000_rop_protect && info->rop_check_size != 0
+      && epilogue_type != EPILOGUE_TYPE_SIBCALL)
+    {
+      gcc_assert (DEFAULT_ABI == ABI_ELFv2);
+      rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
+      rtx addr = gen_rtx_PLUS (Pmode, stack_ptr,
+			       GEN_INT (info->rop_check_save_offset));
+      rtx mem = gen_rtx_MEM (Pmode, addr);
+      rtx reg0 = gen_rtx_REG (Pmode, 0);
+      if (rs6000_privileged)
+	emit_insn (gen_hashchkp (reg0, mem));
+      else
+	emit_insn (gen_hashchk (reg0, mem));
+    }
+
   if (epilogue_type != EPILOGUE_TYPE_SIBCALL && restoring_FPRs_inline)
     {
       if (cfa_restores)
diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
index c8cdc42533c..eec2e7532e1 100644
--- a/gcc/config/rs6000/rs6000.md
+++ b/gcc/config/rs6000/rs6000.md
@@ -154,6 +154,10 @@ (define_c_enum "unspec"
    UNSPEC_CNTTZDM
    UNSPEC_PDEPD
    UNSPEC_PEXTD
+   UNSPEC_HASHST
+   UNSPEC_HASHSTP
+   UNSPEC_HASHCHK
+   UNSPEC_HASHCHKP
   ])
 
 ;;
@@ -14948,6 +14952,41 @@ (define_insn "*cmpeqb_internal"
   "TARGET_P9_MISC && TARGET_64BIT"
   "cmpeqb %0,%1,%2"
   [(set_attr "type" "logical")])
+
+
+;; ROP mitigation instructions.
+
+(define_insn "hashst"
+  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
+        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
+	           UNSPEC_HASHST))]
+  "TARGET_POWER10 && rs6000_rop_protect"
+  "hashst %1,%0"
+  [(set_attr "type" "store")])
+
+(define_insn "hashstp"
+  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
+        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
+	           UNSPEC_HASHSTP))]
+  "TARGET_POWER10 && rs6000_rop_protect && rs6000_privileged"
+  "hashstp %1,%0"
+  [(set_attr "type" "store")])
+
+(define_insn "hashchk"
+  [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r")
+		     (match_operand:DI 1 "simple_offsettable_mem_operand" "m")]
+		    UNSPEC_HASHCHK)]
+  "TARGET_POWER10 && rs6000_rop_protect"
+  "hashchk %0,%1"
+  [(set_attr "type" "load")])
+
+(define_insn "hashchkp"
+  [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r")
+		     (match_operand:DI 1 "simple_offsettable_mem_operand" "m")]
+		    UNSPEC_HASHCHKP)]
+  "TARGET_POWER10 && rs6000_rop_protect && rs6000_privileged"
+  "hashchkp %0,%1"
+  [(set_attr "type" "load")])
 \f
 
 (include "sync.md")
-- 
2.27.0

[PATCH 3/4] rs6000: Conditionally define __ROP_PROTECT__

[Bill Schmidt]

2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/
	* config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Define
	__ROP_PROTECT__ if -mrop-protect is selected.
---
 gcc/config/rs6000/rs6000-c.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
index 0f8a629ff5a..afcb5bb6e39 100644
--- a/gcc/config/rs6000/rs6000-c.c
+++ b/gcc/config/rs6000/rs6000-c.c
@@ -602,6 +602,9 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
   /* Whether pc-relative code is being generated.  */
   if ((flags & OPTION_MASK_PCREL) != 0)
     rs6000_define_or_undefine_macro (define_p, "__PCREL__");
+  /* Tell the user -mrop-protect is in play.  */
+  if (rs6000_rop_protect)
+    rs6000_define_or_undefine_macro (define_p, "__ROP_PROTECT__");
 }
 
 void
-- 
2.27.0

[PATCH 4/4] rs6000: Add ROP tests

[Bill Schmidt]

2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>

gcc/testsuite/
	* gcc.target/powerpc/rop-1.c: New.
	* gcc.target/powerpc/rop-2.c: New.
	* gcc.target/powerpc/rop-3.c: New.
	* gcc.target/powerpc/rop-4.c: New.
	* gcc.target/powerpc/rop-5.c: New.
---
 gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 ++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 ++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 +++++++++++++++++++
 gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++++++++++++
 gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++++++++++++++
 5 files changed, 82 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
 create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c

diff --git a/gcc/testsuite/gcc.target/powerpc/rop-1.c b/gcc/testsuite/gcc.target/powerpc/rop-1.c
new file mode 100644
index 00000000000..cf8e2b01dda
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/rop-1.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
+
+/* Verify that ROP-protect instructions are inserted when a
+   call is present.  */
+
+extern void foo (void);
+
+int bar ()
+{
+  foo ();
+  return 5;
+}
+
+/* { dg-final { scan-assembler {\mhashst\M} } } */
+/* { dg-final { scan-assembler {\mhashchk\M} } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/rop-2.c b/gcc/testsuite/gcc.target/powerpc/rop-2.c
new file mode 100644
index 00000000000..dde403b0ef5
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/rop-2.c
@@ -0,0 +1,16 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect -mprivileged" } */
+
+/* Verify that privileged ROP-protect instructions are inserted when a
+   call is present.  */
+
+extern void foo (void);
+
+int bar ()
+{
+  foo ();
+  return 5;
+}
+
+/* { dg-final { scan-assembler {\mhashstp\M} } } */
+/* { dg-final { scan-assembler {\mhashchkp\M} } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/rop-3.c b/gcc/testsuite/gcc.target/powerpc/rop-3.c
new file mode 100644
index 00000000000..054f94fda99
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/rop-3.c
@@ -0,0 +1,19 @@
+/* { dg-do run { target { power10_hw } } } */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-require-effective-target powerpc_elfv2 } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
+
+/* Verify that ROP-protect instructions execute correctly when a
+   call is present.  */
+
+void __attribute__((noinline)) foo ()
+{
+  asm ("");
+}
+
+int main ()
+{
+  foo ();
+  return 0;
+}
+
diff --git a/gcc/testsuite/gcc.target/powerpc/rop-4.c b/gcc/testsuite/gcc.target/powerpc/rop-4.c
new file mode 100644
index 00000000000..e2be8b2c035
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/rop-4.c
@@ -0,0 +1,14 @@
+/* { dg-do compile } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
+
+/* Verify that no ROP-protect instructions are inserted when no
+   call is present.  */
+
+
+int bar ()
+{
+  return 5;
+}
+
+/* { dg-final { scan-assembler-not {\mhashst\M} } } */
+/* { dg-final { scan-assembler-not {\mhashchk\M} } } */
diff --git a/gcc/testsuite/gcc.target/powerpc/rop-5.c b/gcc/testsuite/gcc.target/powerpc/rop-5.c
new file mode 100644
index 00000000000..b759fa59979
--- /dev/null
+++ b/gcc/testsuite/gcc.target/powerpc/rop-5.c
@@ -0,0 +1,17 @@
+/* { dg-do run { target { power10_hw } } } */
+/* { dg-require-effective-target power10_ok } */
+/* { dg-require-effective-target powerpc_elfv2 } */
+/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
+
+/* Verify that __ROP_PROTECT__ is predefined for -mrop-protect.  */
+
+extern void abort (void);
+
+int main ()
+{
+#ifndef __ROP_PROTECT__
+  abort ();
+#endif
+  return 0;
+}
+
-- 
2.27.0

Re: [PATCH 0/4] [rs6000] ROP support

[will schmidt]

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> Add POWER10 support for hashst[p] and hashchk[p] operations.  When
> the -mrop-protect option is selected, any function that loads the
> link
> register from memory before returning must have protection in the
> prologue and epilogue to ensure the link register save location has
> not been compromised.  If -mprivileged is also specified, the
> protection instructions generated require supervisor privilege.

Hi,

Is -mprivileged tied directly to ROP, or is it a 'generic' option?

As
is, it looks like it can be considered generic, so could be also used
for other cases where we would want to generate instructions that
require supervisor privilege.

Additional comments on the subsequent patches.. 
thanks
-Will

> 
> The patches are broken up into logical chunks:
>  - Option handling
>  - Instruction generation
>  - Predefined macro handling
>  - Test cases
> 
> Bootstrapped and tested on a POWER10 system with no regressions.
> Tests on a kernel that enables user-space ROP mitigation were
> successful.  Is this series ok for trunk?  I would also like to later
> backport these patches to GCC for the 11.2 release.
> 
> Thanks!
> Bill
> 
> Bill Schmidt (4):
>   rs6000: Add -mrop-protect and -mprivileged flags
>   rs6000: Emit ROP-protect instructions in prologue and epilogue
>   rs6000: Conditionally define __ROP_PROTECT__
>   rs6000: Add ROP tests
> 
>  gcc/config/rs6000/rs6000-c.c             |  3 +
>  gcc/config/rs6000/rs6000-internal.h      |  2 +
>  gcc/config/rs6000/rs6000-logue.c         | 86 +++++++++++++++++++++-
> --
>  gcc/config/rs6000/rs6000.c               |  7 ++
>  gcc/config/rs6000/rs6000.md              | 39 +++++++++++
>  gcc/config/rs6000/rs6000.opt             |  6 ++
>  gcc/doc/invoke.texi                      | 19 +++++-
>  gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 +++++
>  gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 +++++
>  gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 ++++++
>  gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++
>  gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++
>  12 files changed, 231 insertions(+), 13 deletions(-)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c
> 

Re: [PATCH 1/4] rs6000: Add -mrop-protect and -mprivileged flags

[will schmidt]

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/rs6000.c (rs6000_option_override_internal):
> 	Disable shrink wrap when inserting ROP-protect instructions.
> 	* config/rs6000/rs6000.opt (mrop-protect): New option.
> 	(mprivileged): Likewise.
> 	* doc/invoke.texi: Document mrop-protect and mprivileged.

Hi, 


> ---
>  gcc/config/rs6000/rs6000.c   |  7 +++++++
>  gcc/config/rs6000/rs6000.opt |  6 ++++++
>  gcc/doc/invoke.texi          | 19 +++++++++++++++++--
>  3 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
> index 844fee88cf3..d13ed6e7ff4 100644
> --- a/gcc/config/rs6000/rs6000.c
> +++ b/gcc/config/rs6000/rs6000.c
> @@ -4036,6 +4036,13 @@ rs6000_option_override_internal (bool global_init_p)
>        && ((rs6000_isa_flags_explicit & OPTION_MASK_QUAD_MEMORY_ATOMIC) == 0))
>      rs6000_isa_flags |= OPTION_MASK_QUAD_MEMORY_ATOMIC;
> 
> +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> +  if (rs6000_rop_protect)
> +    {
> +      flag_shrink_wrap = 0;
> +      flag_shrink_wrap_separate = 0;
> +    }

Does this (shrink-wrap is disabled if/when ROP-protect is enabled) need
additional commentary somewhere?  


> +
>    /* If we can shrink-wrap the TOC register save separately, then use
>       -msave-toc-indirect unless explicitly disabled.  */
>    if ((rs6000_isa_flags_explicit & OPTION_MASK_SAVE_TOC_INDIRECT) == 0
> diff --git a/gcc/config/rs6000/rs6000.opt b/gcc/config/rs6000/rs6000.opt
> index 0dbdf753673..d116fd12f7e 100644
> --- a/gcc/config/rs6000/rs6000.opt
> +++ b/gcc/config/rs6000/rs6000.opt
> @@ -619,3 +619,9 @@ Generate (do not generate) MMA instructions.
> 
>  mrelative-jumptables
>  Target Undocumented Var(rs6000_relative_jumptables) Init(1) Save
> +
> +mrop-protect
> +Target Var(rs6000_rop_protect) Init(0)
> +
> +mprivileged
> +Target Var(rs6000_privileged) Init(0)

Most but not all of the entries in rs6000.opt have an additional
description line.  I'd wonder about updating this to be stl

> +mrop-protect
> +Target Var(rs6000_rop_protect) Init(0)

Enable ROP protection 

> +
> +mprivileged
> +Target Var(rs6000_privileged) Init(0)

Enable privileged instructions for ROP protection.


OK with me either way.  :-)




> diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi
> index e98b0962b9f..36bd0bf9b3b 100644
> --- a/gcc/doc/invoke.texi
> +++ b/gcc/doc/invoke.texi
> @@ -1238,7 +1238,8 @@ See RS/6000 and PowerPC Options.
>  -mgnu-attribute  -mno-gnu-attribute @gol
>  -mstack-protector-guard=@var{guard} -mstack-protector-guard-reg=@var{reg} @gol
>  -mstack-protector-guard-offset=@var{offset} -mprefixed -mno-prefixed @gol
> --mpcrel -mno-pcrel -mmma -mno-mmma}
> +-mpcrel -mno-pcrel -mmma -mno-mmma -mrop-protect -mno-rop-protect @gol
> +-mprivileged -mno-privileged}
> 
>  @emph{RX Options}
>  @gccoptlist{-m64bit-doubles  -m32bit-doubles  -fpu  -nofpu@gol
> @@ -27019,7 +27020,8 @@ following options:
>  -mmulhw  -mdlmzb  -mmfpgpr  -mvsx @gol
>  -mcrypto  -mhtm  -mpower8-fusion  -mpower8-vector @gol
>  -mquad-memory  -mquad-memory-atomic  -mfloat128 @gol
> --mfloat128-hardware -mprefixed -mpcrel -mmma}
> +-mfloat128-hardware -mprefixed -mpcrel -mmma @gol
> +-mrop-protect -mprivileged}
> 
>  The particular options set for any particular CPU varies between
>  compiler versions, depending on what setting seems to produce optimal
> @@ -28024,6 +28026,19 @@ store instructions when the option @option{-mcpu=future} is used.
>  Generate (do not generate) the MMA instructions when the option
>  @option{-mcpu=future} is used.
> 
> +@item -mrop-protect
> +@itemx -mno-rop-protect
> +@opindex mrop-protect
> +@opindex mno-rop-protect
> +Generate (do not generate) ROP protection instructions when the option
> +@option{-mcpu=power10} is used.

Is the option on by default?  if so, may want another testcase to
verify ROP instructions are generated with just -mcpu=power10.
if not,
perhaps the "-mcpu=power10" reference here instead be "-mrop-protect".


> +
> +@item -mprivileged
> +@itemx -mno-privileged
> +@opindex mprivileged
> +@opindex mno-privileged
> +Generate (do not generate) instructions for privileged state.
> +
>  @item -mblock-ops-unaligned-vsx
>  @itemx -mno-block-ops-unaligned-vsx
>  @opindex block-ops-unaligned-vsx


lgtm
thanks,
-Will

Re: [PATCH 2/4] rs6000: Emit ROP-protect instructions in prologue and epilogue

[will schmidt]

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> Insert the hashst and hashchk instructions when -mrop-protect has been
> selected.  The encrypted save slot for ROP mitigation is placed
> between the parameter save area and the alloca space (if any;
> otherwise the local variable space).
> 
> Note that ROP-mitigation instructions are currently only provided for
> the ELFv2 ABI.
> 
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/rs6000-internal.h (rs6000_stack): Add
> 	rop_check_save_offset and rop_check_size.
> 	* config/rs6000/rs6000-logue.c (rs6000_stack_info): Compute
> 	rop_check_size and rop_check_save_offset.
> 	(debug_stack_info): Dump rop_save_offset and rop_check_size.
> 	(rs6000_emit_prologue): Assert if WORLD_SAVE used with
> 	-mrop-protect; emit hashst[p] in prologue; emit hashchk[p] in
> 	epilogue.
> 	* config/rs6000/rs6000.md (unspec): Add UNSPEC_HASHST[P] and
> 	UNSPEC_HASHCHK[P].
> 	(hashst): New define_insn.
> 	(hashstp): Likewise.
> 	(hashchk): Likewise.
> 	(hashchkp): Likewise.

ok

> ---
>  gcc/config/rs6000/rs6000-internal.h |  2 +
>  gcc/config/rs6000/rs6000-logue.c    | 86 +++++++++++++++++++++++++----
>  gcc/config/rs6000/rs6000.md         | 39 +++++++++++++
>  3 files changed, 116 insertions(+), 11 deletions(-)
> 
> diff --git a/gcc/config/rs6000/rs6000-internal.h b/gcc/config/rs6000/rs6000-internal.h
> index 428a7861a98..8fc77ba6138 100644
> --- a/gcc/config/rs6000/rs6000-internal.h
> +++ b/gcc/config/rs6000/rs6000-internal.h
> @@ -39,6 +39,7 @@ typedef struct rs6000_stack {
>    int gp_save_offset;		/* offset to save GP regs from initial SP */
>    int fp_save_offset;		/* offset to save FP regs from initial SP */
>    int altivec_save_offset;	/* offset to save AltiVec regs from initial SP */
> +  int rop_check_save_offset;	/* offset to save ROP check from initial SP */
>    int lr_save_offset;		/* offset to save LR from initial SP */
>    int cr_save_offset;		/* offset to save CR from initial SP */
>    int vrsave_save_offset;	/* offset to save VRSAVE from initial SP */
> @@ -53,6 +54,7 @@ typedef struct rs6000_stack {
>    int gp_size;			/* size of saved GP registers */
>    int fp_size;			/* size of saved FP registers */
>    int altivec_size;		/* size of saved AltiVec registers */
> +  int rop_check_size;		/* size of ROP check slot */
>    int cr_size;			/* size to hold CR if not in fixed area */
>    int vrsave_size;		/* size to hold VRSAVE */
>    int altivec_padding_size;	/* size of altivec alignment padding */

ok

> diff --git a/gcc/config/rs6000/rs6000-logue.c b/gcc/config/rs6000/rs6000-logue.c
> index b0ac183ceff..10cf7a2de93 100644
> --- a/gcc/config/rs6000/rs6000-logue.c
> +++ b/gcc/config/rs6000/rs6000-logue.c
> @@ -595,19 +595,21 @@ rs6000_savres_strategy (rs6000_stack_t *info,
>  		+---------------------------------------+
>  		| Parameter save area (+padding*) (P)	|  32
>  		+---------------------------------------+
> -		| Alloca space (A)			|  32+P
> +		| Optional ROP check slot (R)		|  32+P
>  		+---------------------------------------+
> -		| Local variable space (L)		|  32+P+A
> +		| Alloca space (A)			|  32+P+R
>  		+---------------------------------------+
> -		| Save area for AltiVec registers (W)	|  32+P+A+L
> +		| Local variable space (L)		|  32+P+R+A
>  		+---------------------------------------+
> -		| AltiVec alignment padding (Y)		|  32+P+A+L+W
> +		| Save area for AltiVec registers (W)	|  32+P+R+A+L
>  		+---------------------------------------+
> -		| Save area for GP registers (G)	|  32+P+A+L+W+Y
> +		| AltiVec alignment padding (Y)		|  32+P+R+A+L+W
>  		+---------------------------------------+
> -		| Save area for FP registers (F)	|  32+P+A+L+W+Y+G
> +		| Save area for GP registers (G)	|  32+P+R+A+L+W+Y
>  		+---------------------------------------+
> -	old SP->| back chain to caller's caller		|  32+P+A+L+W+Y+G+F
> +		| Save area for FP registers (F)	|  32+P+R+A+L+W+Y+G
> +		+---------------------------------------+
> +	old SP->| back chain to caller's caller		|  32+P+R+A+L+W+Y+G+F
>  		+---------------------------------------+
> 
>       * If the alloca area is present, the parameter save area is

ok

> @@ -717,6 +719,19 @@ rs6000_stack_info (void)
>    /* Does this function call anything (apart from sibling calls)?  */
>    info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame);
> 
> +  if (TARGET_POWER10 && info->calls_p
> +      && DEFAULT_ABI == ABI_ELFv2 && rs6000_rop_protect)
> +    info->rop_check_size = 8;
> +  else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2)
> +    {
> +      /* We can't check this in rs6000_option_override_internal since
> +	 DEFAULT_ABI isn't established yet.  */
> +      error ("%qs requires the ELFv2 ABI", "-mrop-protect");
> +      info->rop_check_size = 0;
ok
> +    }
> +  else
> +    info->rop_check_size = 0;
> +
ok

>    /* Determine if we need to save the condition code registers.  */
>    if (save_reg_p (CR2_REGNO)
>        || save_reg_p (CR3_REGNO)
> @@ -806,8 +821,14 @@ rs6000_stack_info (void)
>  	  gcc_assert (info->altivec_size == 0
>  		      || info->altivec_save_offset % 16 == 0);
> 
> -	  /* Adjust for AltiVec case.  */
> -	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;
> +	  if (info->rop_check_size != 0)
> +	    {
> +	      info->rop_check_save_offset
> +		= info->altivec_save_offset - info->rop_check_size;
> +	      info->ehrd_offset = info->rop_check_save_offset - ehrd_size;
> +	    }
> +	  else
> +	    info->ehrd_offset = info->altivec_save_offset - ehrd_size;
ok

>  	}
>        else
>  	info->ehrd_offset = info->gp_save_offset - ehrd_size;
> @@ -849,6 +870,7 @@ rs6000_stack_info (void)
>  				  + info->gp_size
>  				  + info->altivec_size
>  				  + info->altivec_padding_size
> +				  + info->rop_check_size
>  				  + ehrd_size
>  				  + ehcr_size
>  				  + info->cr_size
> @@ -987,6 +1009,10 @@ debug_stack_info (rs6000_stack_t *info)
>      fprintf (stderr, "\tvrsave_save_offset  = %5d\n",
>  	     info->vrsave_save_offset);
> 
> +  if (info->rop_check_size)
> +    fprintf (stderr, "\trop_check_save_offset = %5d\n",
> +	     info->rop_check_save_offset);
> +

ok

>    if (info->lr_save_p)
>      fprintf (stderr, "\tlr_save_offset      = %5d\n", info->lr_save_offset);
> 
> @@ -1026,6 +1052,9 @@ debug_stack_info (rs6000_stack_t *info)
>      fprintf (stderr, "\taltivec_padding_size= %5d\n",
>  	     info->altivec_padding_size);
> 
> +  if (info->rop_check_size)
> +    fprintf (stderr, "\trop_check_size      = %5d\n", info->rop_check_size);
> +
>    if (info->cr_size)
>      fprintf (stderr, "\tcr_size             = %5d\n", info->cr_size);
> 
> @@ -3044,7 +3073,6 @@ rs6000_emit_prologue (void)
>  	cfun->machine->r2_setup_needed = true;
>      }
> 
> -
>    if (flag_stack_usage_info)
>      current_function_static_stack_size = info->total_size;
> 
> @@ -3105,7 +3133,8 @@ rs6000_emit_prologue (void)
>  		  && (!crtl->calls_eh_return
>  		      || info->ehrd_offset == -432)
>  		  && info->vrsave_save_offset == -224
> -		  && info->altivec_save_offset == -416);
> +		  && info->altivec_save_offset == -416
> +		  && !rs6000_rop_protect);
> 

ok


>        treg = gen_rtx_REG (SImode, 11);
>        emit_move_insn (treg, GEN_INT (-info->total_size));
> @@ -3252,6 +3281,24 @@ rs6000_emit_prologue (void)
>  	}
>      }
> 
> +  /* The ROP hash store must occur before a stack frame is created.  */
> +  /* NOTE: The hashst isn't needed if we're going to do a sibcall,
> +     but there's no way to know that here.  Harmless except for
> +     performance, of course.  */
> +  if (TARGET_POWER10 && rs6000_rop_protect && info->rop_check_size != 0)
> +    {
> +      gcc_assert (DEFAULT_ABI == ABI_ELFv2);
> +      rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
> +      rtx addr = gen_rtx_PLUS (Pmode, stack_ptr,
> +			       GEN_INT (info->rop_check_save_offset));
> +      rtx mem = gen_rtx_MEM (Pmode, addr);
> +      rtx reg0 = gen_rtx_REG (Pmode, 0);
> +      if (rs6000_privileged)
> +	emit_insn (gen_hashstp (mem, reg0));
> +      else
> +	emit_insn (gen_hashst (mem, reg0));
> +    }
> +

ok


>    /* If we need to save CR, put it into r12 or r11.  Choose r12 except when
>       r12 will be needed by out-of-line gpr save.  */
>    cr_save_regno = ((DEFAULT_ABI == ABI_AIX || DEFAULT_ABI == ABI_ELFv2)
> @@ -4980,6 +5027,23 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
>        emit_insn (gen_add3_insn (sp_reg_rtx, sp_reg_rtx, sa));
>      }
> 
> +  /* The ROP hash check must occur after the stack pointer is restored,
> +     and is not performed for a sibcall.  */
> +  if (TARGET_POWER10 && rs6000_rop_protect && info->rop_check_size != 0
> +      && epilogue_type != EPILOGUE_TYPE_SIBCALL)
> +    {
> +      gcc_assert (DEFAULT_ABI == ABI_ELFv2);
> +      rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
> +      rtx addr = gen_rtx_PLUS (Pmode, stack_ptr,
> +			       GEN_INT (info->rop_check_save_offset));
> +      rtx mem = gen_rtx_MEM (Pmode, addr);
> +      rtx reg0 = gen_rtx_REG (Pmode, 0);
> +      if (rs6000_privileged)
> +	emit_insn (gen_hashchkp (reg0, mem));
> +      else
> +	emit_insn (gen_hashchk (reg0, mem));
> +    }
> +

ok

>    if (epilogue_type != EPILOGUE_TYPE_SIBCALL && restoring_FPRs_inline)
>      {
>        if (cfa_restores)
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index c8cdc42533c..eec2e7532e1 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -154,6 +154,10 @@ (define_c_enum "unspec"
>     UNSPEC_CNTTZDM
>     UNSPEC_PDEPD
>     UNSPEC_PEXTD
> +   UNSPEC_HASHST
> +   UNSPEC_HASHSTP
> +   UNSPEC_HASHCHK
> +   UNSPEC_HASHCHKP
>    ])
> 
>  ;;
> @@ -14948,6 +14952,41 @@ (define_insn "*cmpeqb_internal"
>    "TARGET_P9_MISC && TARGET_64BIT"
>    "cmpeqb %0,%1,%2"
>    [(set_attr "type" "logical")])
> +
> +
> +;; ROP mitigation instructions.
> +
> +(define_insn "hashst"
> +  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
> +        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> +	           UNSPEC_HASHST))]
> +  "TARGET_POWER10 && rs6000_rop_protect"
> +  "hashst %1,%0"
> +  [(set_attr "type" "store")])
> +
> +(define_insn "hashstp"
> +  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
> +        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> +	           UNSPEC_HASHSTP))]
> +  "TARGET_POWER10 && rs6000_rop_protect && rs6000_privileged"
> +  "hashstp %1,%0"
> +  [(set_attr "type" "store")])
> +
> +(define_insn "hashchk"
> +  [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r")
> +		     (match_operand:DI 1 "simple_offsettable_mem_operand" "m")]
> +		    UNSPEC_HASHCHK)]
> +  "TARGET_POWER10 && rs6000_rop_protect"
> +  "hashchk %0,%1"
> +  [(set_attr "type" "load")])
> +
> +(define_insn "hashchkp"
> +  [(unspec_volatile [(match_operand:DI 0 "int_reg_operand" "r")
> +		     (match_operand:DI 1 "simple_offsettable_mem_operand" "m")]
> +		    UNSPEC_HASHCHKP)]
> +  "TARGET_POWER10 && rs6000_rop_protect && rs6000_privileged"
> +  "hashchkp %0,%1"
> +  [(set_attr "type" "load")])

ok


lgtm, 
thanks,
-Will

>  
> 
>  (include "sync.md")

Re: [PATCH 3/4] rs6000: Conditionally define __ROP_PROTECT__

[will schmidt]

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Define
> 	__ROP_PROTECT__ if -mrop-protect is selected.


ok

> ---
>  gcc/config/rs6000/rs6000-c.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gcc/config/rs6000/rs6000-c.c b/gcc/config/rs6000/rs6000-c.c
> index 0f8a629ff5a..afcb5bb6e39 100644
> --- a/gcc/config/rs6000/rs6000-c.c
> +++ b/gcc/config/rs6000/rs6000-c.c
> @@ -602,6 +602,9 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
>    /* Whether pc-relative code is being generated.  */
>    if ((flags & OPTION_MASK_PCREL) != 0)
>      rs6000_define_or_undefine_macro (define_p, "__PCREL__");
> +  /* Tell the user -mrop-protect is in play.  */
> +  if (rs6000_rop_protect)
> +    rs6000_define_or_undefine_macro (define_p, "__ROP_PROTECT__");
> 

I notice that almost all of the other defines are controled by an (if
(flags & OPTION) logic block.. but this seems OK.

lgtm, 
thanks,
-WIll


>  }
> 
>  void

Re: [PATCH 4/4] rs6000: Add ROP tests

[will schmidt]

On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/testsuite/
> 	* gcc.target/powerpc/rop-1.c: New.
> 	* gcc.target/powerpc/rop-2.c: New.
> 	* gcc.target/powerpc/rop-3.c: New.
> 	* gcc.target/powerpc/rop-4.c: New.
> 	* gcc.target/powerpc/rop-5.c: New.

ok

> ---
>  gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 ++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 ++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 +++++++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++++++++++++
>  gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++++++++++++++
>  5 files changed, 82 insertions(+)
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
>  create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c
> 
> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-1.c b/gcc/testsuite/gcc.target/powerpc/rop-1.c
> new file mode 100644
> index 00000000000..cf8e2b01dda
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-1.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
> +
> +/* Verify that ROP-protect instructions are inserted when a
> +   call is present.  */
> +
> +extern void foo (void);
> +
> +int bar ()
> +{
> +  foo ();
> +  return 5;
> +}
> +
> +/* { dg-final { scan-assembler {\mhashst\M} } } */
> +/* { dg-final { scan-assembler {\mhashchk\M} } } */

ok


> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-2.c b/gcc/testsuite/gcc.target/powerpc/rop-2.c
> new file mode 100644
> index 00000000000..dde403b0ef5
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-2.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect -mprivileged" } */
> +
> +/* Verify that privileged ROP-protect instructions are inserted when a
> +   call is present.  */
> +
> +extern void foo (void);
> +
> +int bar ()
> +{
> +  foo ();
> +  return 5;
> +}
> +
> +/* { dg-final { scan-assembler {\mhashstp\M} } } */
> +/* { dg-final { scan-assembler {\mhashchkp\M} } } */

ok


> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-3.c b/gcc/testsuite/gcc.target/powerpc/rop-3.c
> new file mode 100644
> index 00000000000..054f94fda99
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-3.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
> +
> +/* Verify that ROP-protect instructions execute correctly when a
> +   call is present.  */
> +
> +void __attribute__((noinline)) foo ()
> +{
> +  asm ("");
> +}
> +
> +int main ()
> +{
> +  foo ();
> +  return 0;
> +}
> +

ok


> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-4.c b/gcc/testsuite/gcc.target/powerpc/rop-4.c
> new file mode 100644
> index 00000000000..e2be8b2c035
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-4.c
> @@ -0,0 +1,14 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
> +
> +/* Verify that no ROP-protect instructions are inserted when no
> +   call is present.  */
> +
> +
> +int bar ()
> +{
> +  return 5;
> +}
> +
> +/* { dg-final { scan-assembler-not {\mhashst\M} } } */
> +/* { dg-final { scan-assembler-not {\mhashchk\M} } } */


ok

> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-5.c b/gcc/testsuite/gcc.target/powerpc/rop-5.c
> new file mode 100644
> index 00000000000..b759fa59979
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-5.c
> @@ -0,0 +1,17 @@
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
> +
> +/* Verify that __ROP_PROTECT__ is predefined for -mrop-protect.  */
> +
> +extern void abort (void);
> +
> +int main ()
> +{
> +#ifndef __ROP_PROTECT__
> +  abort ();
> +#endif
> +  return 0;
> +}
> +

ok.


Does there need to be another test to verify if -mrop-protect is on by
default without specifying -mrop-protect?   (or is it?)  Question on
0/4.

with that noted,
lgtm, 
thanks,
-will

Re: [PATCH 0/4] [rs6000] ROP support

[Bill Schmidt]

On 4/26/21 11:01 AM, will schmidt wrote:
> On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
>> Add POWER10 support for hashst[p] and hashchk[p] operations.  When
>> the -mrop-protect option is selected, any function that loads the
>> link
>> register from memory before returning must have protection in the
>> prologue and epilogue to ensure the link register save location has
>> not been compromised.  If -mprivileged is also specified, the
>> protection instructions generated require supervisor privilege.
> Hi,
>
> Is -mprivileged tied directly to ROP, or is it a 'generic' option?
>
> As
> is, it looks like it can be considered generic, so could be also used
> for other cases where we would want to generate instructions that
> require supervisor privilege.

Yes, this is deliberately designed to be orthogonal from the specific 
ROP support.  That is, ROP is the first use, but other future uses are 
anticipated.

Bill

>
> Additional comments on the subsequent patches..
> thanks
> -Will
>
>> The patches are broken up into logical chunks:
>>   - Option handling
>>   - Instruction generation
>>   - Predefined macro handling
>>   - Test cases
>>
>> Bootstrapped and tested on a POWER10 system with no regressions.
>> Tests on a kernel that enables user-space ROP mitigation were
>> successful.  Is this series ok for trunk?  I would also like to later
>> backport these patches to GCC for the 11.2 release.
>>
>> Thanks!
>> Bill
>>
>> Bill Schmidt (4):
>>    rs6000: Add -mrop-protect and -mprivileged flags
>>    rs6000: Emit ROP-protect instructions in prologue and epilogue
>>    rs6000: Conditionally define __ROP_PROTECT__
>>    rs6000: Add ROP tests
>>
>>   gcc/config/rs6000/rs6000-c.c             |  3 +
>>   gcc/config/rs6000/rs6000-internal.h      |  2 +
>>   gcc/config/rs6000/rs6000-logue.c         | 86 +++++++++++++++++++++-
>> --
>>   gcc/config/rs6000/rs6000.c               |  7 ++
>>   gcc/config/rs6000/rs6000.md              | 39 +++++++++++
>>   gcc/config/rs6000/rs6000.opt             |  6 ++
>>   gcc/doc/invoke.texi                      | 19 +++++-
>>   gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 +++++
>>   gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 +++++
>>   gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 ++++++
>>   gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++
>>   gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++
>>   12 files changed, 231 insertions(+), 13 deletions(-)
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c
>>

Re: [PATCH 4/4] rs6000: Add ROP tests

[Bill Schmidt]

On 4/26/21 11:04 AM, will schmidt wrote:
> On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
>> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
>>
>> gcc/testsuite/
>> 	* gcc.target/powerpc/rop-1.c: New.
>> 	* gcc.target/powerpc/rop-2.c: New.
>> 	* gcc.target/powerpc/rop-3.c: New.
>> 	* gcc.target/powerpc/rop-4.c: New.
>> 	* gcc.target/powerpc/rop-5.c: New.
> ok
>
>> ---
>>   gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 ++++++++++++++++
>>   gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 ++++++++++++++++
>>   gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 +++++++++++++++++++
>>   gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++++++++++++
>>   gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++++++++++++++
>>   5 files changed, 82 insertions(+)
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
>>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c
>>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-1.c b/gcc/testsuite/gcc.target/powerpc/rop-1.c
>> new file mode 100644
>> index 00000000000..cf8e2b01dda
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/rop-1.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
>> +
>> +/* Verify that ROP-protect instructions are inserted when a
>> +   call is present.  */
>> +
>> +extern void foo (void);
>> +
>> +int bar ()
>> +{
>> +  foo ();
>> +  return 5;
>> +}
>> +
>> +/* { dg-final { scan-assembler {\mhashst\M} } } */
>> +/* { dg-final { scan-assembler {\mhashchk\M} } } */
> ok
>
>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-2.c b/gcc/testsuite/gcc.target/powerpc/rop-2.c
>> new file mode 100644
>> index 00000000000..dde403b0ef5
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/rop-2.c
>> @@ -0,0 +1,16 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect -mprivileged" } */
>> +
>> +/* Verify that privileged ROP-protect instructions are inserted when a
>> +   call is present.  */
>> +
>> +extern void foo (void);
>> +
>> +int bar ()
>> +{
>> +  foo ();
>> +  return 5;
>> +}
>> +
>> +/* { dg-final { scan-assembler {\mhashstp\M} } } */
>> +/* { dg-final { scan-assembler {\mhashchkp\M} } } */
> ok
>
>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-3.c b/gcc/testsuite/gcc.target/powerpc/rop-3.c
>> new file mode 100644
>> index 00000000000..054f94fda99
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/rop-3.c
>> @@ -0,0 +1,19 @@
>> +/* { dg-do run { target { power10_hw } } } */
>> +/* { dg-require-effective-target power10_ok } */
>> +/* { dg-require-effective-target powerpc_elfv2 } */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
>> +
>> +/* Verify that ROP-protect instructions execute correctly when a
>> +   call is present.  */
>> +
>> +void __attribute__((noinline)) foo ()
>> +{
>> +  asm ("");
>> +}
>> +
>> +int main ()
>> +{
>> +  foo ();
>> +  return 0;
>> +}
>> +
> ok
>
>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-4.c b/gcc/testsuite/gcc.target/powerpc/rop-4.c
>> new file mode 100644
>> index 00000000000..e2be8b2c035
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/rop-4.c
>> @@ -0,0 +1,14 @@
>> +/* { dg-do compile } */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
>> +
>> +/* Verify that no ROP-protect instructions are inserted when no
>> +   call is present.  */
>> +
>> +
>> +int bar ()
>> +{
>> +  return 5;
>> +}
>> +
>> +/* { dg-final { scan-assembler-not {\mhashst\M} } } */
>> +/* { dg-final { scan-assembler-not {\mhashchk\M} } } */
>
> ok
>
>> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-5.c b/gcc/testsuite/gcc.target/powerpc/rop-5.c
>> new file mode 100644
>> index 00000000000..b759fa59979
>> --- /dev/null
>> +++ b/gcc/testsuite/gcc.target/powerpc/rop-5.c
>> @@ -0,0 +1,17 @@
>> +/* { dg-do run { target { power10_hw } } } */
>> +/* { dg-require-effective-target power10_ok } */
>> +/* { dg-require-effective-target powerpc_elfv2 } */
>> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
>> +
>> +/* Verify that __ROP_PROTECT__ is predefined for -mrop-protect.  */
>> +
>> +extern void abort (void);
>> +
>> +int main ()
>> +{
>> +#ifndef __ROP_PROTECT__
>> +  abort ();
>> +#endif
>> +  return 0;
>> +}
>> +
> ok.
>
>
> Does there need to be another test to verify if -mrop-protect is on by
> default without specifying -mrop-protect?   (or is it?)  Question on
> 0/4.

It's off by default (see the Init(0) in patch 1/4).

Bill

> with that noted,
> lgtm,
> thanks,
> -will
>
>
>

Re: [PATCH 0/4] [rs6000] ROP support

[Bill Schmidt]

Hi!  I'd like to ping this series.  It has slightly higher priority from 
my perspective, since I'd like this to be backported in time for GCC 11.2.

Thanks!
Bill

On 4/25/21 8:50 PM, Bill Schmidt via Gcc-patches wrote:
> Add POWER10 support for hashst[p] and hashchk[p] operations.  When
> the -mrop-protect option is selected, any function that loads the link
> register from memory before returning must have protection in the
> prologue and epilogue to ensure the link register save location has
> not been compromised.  If -mprivileged is also specified, the
> protection instructions generated require supervisor privilege.
>
> The patches are broken up into logical chunks:
>   - Option handling
>   - Instruction generation
>   - Predefined macro handling
>   - Test cases
>
> Bootstrapped and tested on a POWER10 system with no regressions.
> Tests on a kernel that enables user-space ROP mitigation were
> successful.  Is this series ok for trunk?  I would also like to later
> backport these patches to GCC for the 11.2 release.
>
> Thanks!
> Bill
>
> Bill Schmidt (4):
>    rs6000: Add -mrop-protect and -mprivileged flags
>    rs6000: Emit ROP-protect instructions in prologue and epilogue
>    rs6000: Conditionally define __ROP_PROTECT__
>    rs6000: Add ROP tests
>
>   gcc/config/rs6000/rs6000-c.c             |  3 +
>   gcc/config/rs6000/rs6000-internal.h      |  2 +
>   gcc/config/rs6000/rs6000-logue.c         | 86 +++++++++++++++++++++---
>   gcc/config/rs6000/rs6000.c               |  7 ++
>   gcc/config/rs6000/rs6000.md              | 39 +++++++++++
>   gcc/config/rs6000/rs6000.opt             |  6 ++
>   gcc/doc/invoke.texi                      | 19 +++++-
>   gcc/testsuite/gcc.target/powerpc/rop-1.c | 16 +++++
>   gcc/testsuite/gcc.target/powerpc/rop-2.c | 16 +++++
>   gcc/testsuite/gcc.target/powerpc/rop-3.c | 19 ++++++
>   gcc/testsuite/gcc.target/powerpc/rop-4.c | 14 ++++
>   gcc/testsuite/gcc.target/powerpc/rop-5.c | 17 +++++
>   12 files changed, 231 insertions(+), 13 deletions(-)
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-1.c
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-2.c
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-3.c
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-4.c
>   create mode 100644 gcc/testsuite/gcc.target/powerpc/rop-5.c
>

Re: [PATCH 1/4] rs6000: Add -mrop-protect and -mprivileged flags

[Segher Boessenkool]

On Sun, Apr 25, 2021 at 08:50:15PM -0500, Bill Schmidt wrote:
> +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> +  if (rs6000_rop_protect)
> +    {
> +      flag_shrink_wrap = 0;
> +      flag_shrink_wrap_separate = 0;
> +    }

Separate shrink-wrapping requires flag_shrink_wrap, so remove that
second assignment please?

===
void
try_shrink_wrapping (edge *entry_edge, rtx_insn *prologue_seq)
{
  /* If we cannot shrink-wrap, are told not to shrink-wrap, or it makes
     no sense to shrink-wrap: then do not shrink-wrap!  */

  if (!SHRINK_WRAPPING_ENABLED)
    return;
===

===
void
try_shrink_wrapping_separate (basic_block first_bb)
{
  if (!(SHRINK_WRAPPING_ENABLED
        && flag_shrink_wrap_separate
        && optimize_function_for_speed_p (cfun)
        && targetm.shrink_wrap.get_separate_components))
    return;
===

and for completeness

===
#define SHRINK_WRAPPING_ENABLED \
  (flag_shrink_wrap && targetm.have_simple_return ())
===

> +-mrop-protect -mprivileged}

-mprivileged should not be CPU-specific, the concept applies on all CPUs
(but of course is only used on p10 so far).  So please document it in
the right location :-)

Okay for trunk and 11 with those changes.  Thanks!


Segher

Re: [PATCH 1/4] rs6000: Add -mrop-protect and -mprivileged flags

[Segher Boessenkool]

On Mon, Apr 26, 2021 at 11:02:53AM -0500, will schmidt wrote:
> On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> > +  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
> > +  if (rs6000_rop_protect)
> > +    {
> > +      flag_shrink_wrap = 0;
> > +      flag_shrink_wrap_separate = 0;
> > +    }
> 
> Does this (shrink-wrap is disabled if/when ROP-protect is enabled) need
> additional commentary somewhere?  

This should be documented in the manual somewhere, probably where
-mrop-protect is described.

It would be nice if we could do both ROP protection and shrink-wrapping
at once, but we aren't there yet (if we ever can do that).

> > +mrop-protect
> > +Target Var(rs6000_rop_protect) Init(0)
> > +
> > +mprivileged
> > +Target Var(rs6000_privileged) Init(0)
> 
> Most but not all of the entries in rs6000.opt have an additional
> description line.  I'd wonder about updating this to be stl

Yeah, that is used for the help text etc.

> > +mrop-protect
> > +Target Var(rs6000_rop_protect) Init(0)
> 
> Enable ROP protection 

This is a bit brief.  Also, these lines are sentences, need to end in a
full stop.

> > +mprivileged
> > +Target Var(rs6000_privileged) Init(0)
> 
> Enable privileged instructions for ROP protection.

-mprivileged is not only for ROP protection: it simply indicates we are
compiling code that will run in a privileged mode (i.e., not in problem
state).

> > +@item -mrop-protect
> > +@itemx -mno-rop-protect
> > +@opindex mrop-protect
> > +@opindex mno-rop-protect
> > +Generate (do not generate) ROP protection instructions when the option
> > +@option{-mcpu=power10} is used.
> 
> Is the option on by default?

Nope.

> if so, may want another testcase to
> verify ROP instructions are generated with just -mcpu=power10.
> if not,
> perhaps the "-mcpu=power10" reference here instead be "-mrop-protect".

This is the help text for -mrop-protect :-)

Probably the help text could be more future-proof though?


Segher

Re: [PATCH 2/4] rs6000: Emit ROP-protect instructions in prologue and epilogue

[Segher Boessenkool]

Hi!

On Sun, Apr 25, 2021 at 08:50:16PM -0500, Bill Schmidt wrote:
> Insert the hashst and hashchk instructions when -mrop-protect has been
> selected.  The encrypted save slot for ROP mitigation is placed
> between the parameter save area and the alloca space (if any;
> otherwise the local variable space).
> 
> Note that ROP-mitigation instructions are currently only provided for
> the ELFv2 ABI.

The good thing about that is we will gain some experience with it before
we have to implement this for other ABIs, heh.

> 	(rs6000_emit_prologue): Assert if WORLD_SAVE used with
> 	-mrop-protect;

WORLD_SAVE is only used for Darwin, so this is pretty useless.  Use full
stops in changelogs please, not semicolons?

> 	* config/rs6000/rs6000.md (unspec): Add UNSPEC_HASHST[P] and
> 	UNSPEC_HASHCHK[P].
> 	(hashst): New define_insn.
> 	(hashstp): Likewise.
> 	(hashchk): Likewise.
> 	(hashchkp): Likewise.

> +  int rop_check_save_offset;	/* offset to save ROP check from initial SP */

Offset to store the hash, right?  It isn't a "Rop check" that is stored
there :-)

> +  if (TARGET_POWER10 && info->calls_p
> +      && DEFAULT_ABI == ABI_ELFv2 && rs6000_rop_protect)
> +    info->rop_check_size = 8;

One && per line please (if it doesn't fit on one line).  It of course
depends what is most readable, but in this case the four clauses are
all completely different, so each && should start a new line.

> +  else if (rs6000_rop_protect && DEFAULT_ABI != ABI_ELFv2)
> +    {
> +      /* We can't check this in rs6000_option_override_internal since
> +	 DEFAULT_ABI isn't established yet.  */
> +      error ("%qs requires the ELFv2 ABI", "-mrop-protect");
> +      info->rop_check_size = 0;
> +    }
> +  else
> +    info->rop_check_size = 0;

Hrm.  It would be nicer if you set rop_check_size to 0 before everything
else, and then overwrite it in the one case you need something else.

> @@ -806,8 +821,14 @@ rs6000_stack_info (void)
>  	  gcc_assert (info->altivec_size == 0
>  		      || info->altivec_save_offset % 16 == 0);
>  
> -	  /* Adjust for AltiVec case.  */
> -	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;
> +	  if (info->rop_check_size != 0)
> +	    {
> +	      info->rop_check_save_offset
> +		= info->altivec_save_offset - info->rop_check_size;
> +	      info->ehrd_offset = info->rop_check_save_offset - ehrd_size;
> +	    }
> +	  else
> +	    info->ehrd_offset = info->altivec_save_offset - ehrd_size;
>  	}

Put the comment back here?  Also maybe easier if you do

	  /* Adjust for AltiVec case.  */
	  info->ehrd_offset = info->altivec_save_offset - ehrd_size;

	  /* Adjust for ROP protection.  */
	  if (info->rop_check_size != 0)
	    {
	      info->rop_check_save_offset
		= info->altivec_save_offset - info->rop_check_size;
	      info->ehrd_offset -= info->rop_check_size;
	    }

(or even make that unconditional, the "if" isn't needed anymore then).

> @@ -3044,7 +3073,6 @@ rs6000_emit_prologue (void)
>  	cfun->machine->r2_setup_needed = true;
>      }
>  
> -
>    if (flag_stack_usage_info)
>      current_function_static_stack_size = info->total_size;
>  

I don' think this improves the code, and it is an unrelated change, but
heh, *shrug*.

> +  /* The ROP hash store must occur before a stack frame is created.  */

Why?  (Explain in the code, not to me :-) )

> +  /* The ROP hash check must occur after the stack pointer is restored,
> +     and is not performed for a sibcall.  */

Same here.

> +   UNSPEC_HASHST
> +   UNSPEC_HASHSTP
> +   UNSPEC_HASHCHK
> +   UNSPEC_HASHCHKP

You don't need a separate unspec (or insns patterns) for privileged
mode: you can just sprintf the template, or maybe you can use
print_operand_punct_valid if you think this 'p' will happen more often
(there exist more insns like this already, we just never generate them).

> +;; ROP mitigation instructions.
> +
> +(define_insn "hashst"
> +  [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
> +        (unspec:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> +	           UNSPEC_HASHST))]

This needs unspec_volatile in principle.  There will be only one per
function, so it is kind of moot, but still.

Thanks,


Segher

Re: [PATCH 3/4] rs6000: Conditionally define __ROP_PROTECT__

[Segher Boessenkool]

On Mon, Apr 26, 2021 at 11:03:22AM -0500, will schmidt wrote:
> On Sun, 2021-04-25 at 20:50 -0500, Bill Schmidt via Gcc-patches wrote:
> > @@ -602,6 +602,9 @@ rs6000_target_modify_macros (bool define_p, HOST_WIDE_INT flags,
> >    /* Whether pc-relative code is being generated.  */
> >    if ((flags & OPTION_MASK_PCREL) != 0)
> >      rs6000_define_or_undefine_macro (define_p, "__PCREL__");
> > +  /* Tell the user -mrop-protect is in play.  */
> > +  if (rs6000_rop_protect)
> > +    rs6000_define_or_undefine_macro (define_p, "__ROP_PROTECT__");
> > 
> 
> I notice that almost all of the other defines are controled by an (if
> (flags & OPTION) logic block.. but this seems OK.

That is rs6000_isa_flags, which is only 64 bits.  Not every option can
be put in there, only the ones used all over the place in the compiler
should be.  Currently there are 53 of 64 bits used, there is some room,
and that is good :-)


Segher

Re: [PATCH 3/4] rs6000: Conditionally define __ROP_PROTECT__

[Segher Boessenkool]

On Sun, Apr 25, 2021 at 08:50:17PM -0500, Bill Schmidt wrote:
> 2021-03-25  Bill Schmidt  <wschmidt@linux.ibm.com>
> 
> gcc/
> 	* config/rs6000/rs6000-c.c (rs6000_target_modify_macros): Define
> 	__ROP_PROTECT__ if -mrop-protect is selected.

Okay for trunk and 11.  Thanks!


Segher

Re: [PATCH 4/4] rs6000: Add ROP tests

[Segher Boessenkool]

On Mon, Apr 26, 2021 at 02:27:45PM -0500, Bill Schmidt wrote:
> On 4/26/21 11:04 AM, will schmidt wrote:
> >Does there need to be another test to verify if -mrop-protect is on by
> >default without specifying -mrop-protect?   (or is it?)  Question on
> >0/4.
> 
> It's off by default (see the Init(0) in patch 1/4).

And in general we only need a test for it if the default isn't fixed.


Segher

Re: [PATCH 4/4] rs6000: Add ROP tests

[Segher Boessenkool]

On Sun, Apr 25, 2021 at 08:50:18PM -0500, Bill Schmidt wrote:
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-1.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */

This should only run on ELFv2 currently, no?

> +/* { dg-final { scan-assembler {\mhashst\M} } } */
> +/* { dg-final { scan-assembler {\mhashchk\M} } } */

Maybe check these are emitted only once each?

> +/* { dg-final { scan-assembler {\mhashchkp\M} } } */
> diff --git a/gcc/testsuite/gcc.target/powerpc/rop-3.c b/gcc/testsuite/gcc.target/powerpc/rop-3.c
> new file mode 100644
> index 00000000000..054f94fda99
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-3.c
> @@ -0,0 +1,19 @@
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */

Why does this need power10_ok?  We always are 64-bit if ELFv2 (and even
if you don't want to depend on that, check for lp64 instead?)

> +/* Verify that ROP-protect instructions execute correctly when a
> +   call is present.  */
> +
> +void __attribute__((noinline)) foo ()
> +{
> +  asm ("");
> +}

Use noipa instead?  noinline should be fine in this super trivial case,
but it is less typing at least ;-)

> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/rop-5.c
> @@ -0,0 +1,17 @@
> +/* { dg-do run { target { power10_hw } } } */
> +/* { dg-require-effective-target power10_ok } */
> +/* { dg-require-effective-target powerpc_elfv2 } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect" } */
> +
> +/* Verify that __ROP_PROTECT__ is predefined for -mrop-protect.  */
> +
> +extern void abort (void);
> +
> +int main ()
> +{
> +#ifndef __ROP_PROTECT__
> +  abort ();
> +#endif
> +  return 0;
> +}

Please do this in a compile test instead?

#ifndef __ROP_PROTECT__
  har har har
#endif


Okay for trunk and 11 with such changes.  Thank you!


Segher