Hi Peter,
on 2024/6/20 05:14, Peter Bergner wrote:
> We currently only emit the ROP-protect hash* insns for Power10, where the
> insns were added to the architecture. We want to emit them for earlier
> cpus (where they operate as NOPs), so that if those older binaries are
> ever executed on a Power10, then they'll be protected from ROP attacks.
> Binutils accepts hashst and hashchk back to Power8, so change GCC to emit
> them for Power8 and later. This matches clang's behavior.
>
> This patch is independent of the ROP shrink-wrap fix submitted earlier.
> This passed bootstrap and regtesting on powerpc64le-linux with no regressions.
> Ok for trunk?
>
> Peter
>
>
>
> 2024-06-19 Peter Bergner <berg...@linux.ibm.com>
>
> gcc/
> PR target/114759
> * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Use TARGET_POWER8.
> (rs6000_emit_prologue): Likewise.
> * config/rs6000/rs6000.md (hashchk): Likewise.
> (hashst): Likewise.
> Fix whitespace.
>
> gcc/testsuite/
> PR target/114759
> * gcc.target/powerpc/pr114759-2.c: New test.
> * lib/target-supports.exp (rop_ok): Use
> check_effective_target_has_arch_pwr8.
> ---
> gcc/config/rs6000/rs6000-logue.cc | 6 +++---
> gcc/config/rs6000/rs6000.md | 6 +++---
> gcc/testsuite/gcc.target/powerpc/pr114759-2.c | 17 +++++++++++++++++
> gcc/testsuite/lib/target-supports.exp | 2 +-
> 4 files changed, 24 insertions(+), 7 deletions(-)
> create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114759-2.c
>
> diff --git a/gcc/config/rs6000/rs6000-logue.cc
> b/gcc/config/rs6000/rs6000-logue.cc
> index c384e48e378..bd363b625a4 100644
> --- a/gcc/config/rs6000/rs6000-logue.cc
> +++ b/gcc/config/rs6000/rs6000-logue.cc
> @@ -716,7 +716,7 @@ rs6000_stack_info (void)
> info->calls_p = (!crtl->is_leaf || cfun->machine->ra_needs_full_frame);
> info->rop_hash_size = 0;
>
> - if (TARGET_POWER10
> + if (TARGET_POWER8
> && info->calls_p
> && DEFAULT_ABI == ABI_ELFv2
> && rs6000_rop_protect)
Nit: I noticed that this is the only place to change
info->rop_hash_size to non-zero, and ...
> @@ -3277,7 +3277,7 @@ rs6000_emit_prologue (void)
> /* 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_hash_size != 0)
> + if (TARGET_POWER8 && rs6000_rop_protect && info->rop_hash_size != 0)
... this condition and ...
> {
> gcc_assert (DEFAULT_ABI == ABI_ELFv2);
> rtx stack_ptr = gen_rtx_REG (Pmode, STACK_POINTER_REGNUM);
> @@ -5056,7 +5056,7 @@ rs6000_emit_epilogue (enum epilogue_type epilogue_type)
>
> /* The ROP hash check must occur after the stack pointer is restored
> (since the hash involves r1), and is not performed for a sibcall. */
> - if (TARGET_POWER10
> + if (TARGET_POWER8> && rs6000_rop_protect
> && info->rop_hash_size != 0
... here, both check info->rop_hash_size isn't zero, I think we can drop these
two TARGET_POWER10 (TARGET_POWER8) and rs6000_rop_protect checks? Instead just
update the inner gcc_assert (now checking DEFAULT_ABI == ABI_ELFv2) by extra
checkings on TARGET_POWER8 && rs6000_rop_protect?
The other looks good to me, ok for trunk with this nit tweaked (if you agree
with it and re-tested well), thanks!
BR,
Kewen
> && epilogue_type != EPILOGUE_TYPE_SIBCALL)
> diff --git a/gcc/config/rs6000/rs6000.md b/gcc/config/rs6000/rs6000.md
> index a5d20594789..694076e311f 100644
> --- a/gcc/config/rs6000/rs6000.md
> +++ b/gcc/config/rs6000/rs6000.md
> @@ -15808,9 +15808,9 @@ (define_insn "*cmpeqb_internal"
>
> (define_insn "hashst"
> [(set (match_operand:DI 0 "simple_offsettable_mem_operand" "=m")
> - (unspec_volatile:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> + (unspec_volatile:DI [(match_operand:DI 1 "int_reg_operand" "r")]
> UNSPEC_HASHST))]
> - "TARGET_POWER10 && rs6000_rop_protect"
> + "TARGET_POWER8 && rs6000_rop_protect"
> {
> static char templ[32];
> const char *p = rs6000_privileged ? "p" : "";
> @@ -15823,7 +15823,7 @@ (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"
> + "TARGET_POWER8 && rs6000_rop_protect"
> {
> static char templ[32];
> const char *p = rs6000_privileged ? "p" : "";
> diff --git a/gcc/testsuite/gcc.target/powerpc/pr114759-2.c
> b/gcc/testsuite/gcc.target/powerpc/pr114759-2.c
> new file mode 100644
> index 00000000000..3881ebd416e
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr114759-2.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power8 -mrop-protect" } */
> +/* { dg-require-effective-target rop_ok } Only enable on supported ABIs. */
> +
> +/* Verify we generate ROP-protect hash insns when compiling for Power8. */
> +
> +extern void foo (void);
> +
> +int
> +bar (void)
> +{
> + foo ();
> + return 5;
> +}
> +
> +/* { dg-final { scan-assembler-times {\mhashst\M} 1 } } */
> +/* { dg-final { scan-assembler-times {\mhashchk\M} 1 } } */
> diff --git a/gcc/testsuite/lib/target-supports.exp
> b/gcc/testsuite/lib/target-supports.exp
> index e307f4e69ef..b1ef4e8eaef 100644
> --- a/gcc/testsuite/lib/target-supports.exp
> +++ b/gcc/testsuite/lib/target-supports.exp
> @@ -7339,7 +7339,7 @@ proc check_effective_target_powerpc_elfv2 { } {
> # Return 1 if this is a PowerPC target supporting -mrop-protect
>
> proc check_effective_target_rop_ok { } {
> - return [check_effective_target_power10_ok] &&
> [check_effective_target_powerpc_elfv2]
> + return [check_effective_target_has_arch_pwr8] &&
> [check_effective_target_powerpc_elfv2]
> }
>
> # The VxWorks SPARC simulator accepts only EM_SPARC executables and
