Re: [PATCH] rs6000: ROP - Emit hashst and hashchk insns on Power8 and later [PR114759]
on 2024/7/3 23:05, Peter Bergner wrote:
> On 7/3/24 4:01 AM, Kewen.Lin wrote:
>>> - 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!
>
> I agree with you, because the next patch I haven't submitted yet (waiting
> on this to get in), makes that simplification as part of the adding earlier
> checking of invalid options. :-) The follow-on patch will not only remove
> the TARGET_* and the 2nd/3rd rs6000_rop_protect usage, but will also remove
> the test and asserts of ELFv2...because we've already verified valid option
> usage earlier in the normal options handling code.
>
> Therefore, I'd like to keep this patch as simple as possible and limited to
> the TARGET_POWER10 -> TARGET_POWER8 change and the cleanup of those tests is
> coming in the next patch...which has already been tested.
Looking forward to the upcoming patch, then this patch is ok for trunk, thanks!
BR,
Kewen
Re: [PATCH] rs6000: ROP - Emit hashst and hashchk insns on Power8 and later [PR114759]
On 7/3/24 4:01 AM, Kewen.Lin wrote:
>> - 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!
I agree with you, because the next patch I haven't submitted yet (waiting
on this to get in), makes that simplification as part of the adding earlier
checking of invalid options. :-) The follow-on patch will not only remove
the TARGET_* and the 2nd/3rd rs6000_rop_protect usage, but will also remove
the test and asserts of ELFv2...because we've already verified valid option
usage earlier in the normal options handling code.
Therefore, I'd like to keep this patch as simple as possible and limited to
the TARGET_POWER10 -> TARGET_POWER8 change and the cleanup of those tests is
coming in the next patch...which has already been tested.
Peter
Re: [PATCH] rs6000: ROP - Emit hashst and hashchk insns on Power8 and later [PR114759]
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
>
> 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 000..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
[PING][PATCH] rs6000: ROP - Emit hashst and hashchk insns on Power8 and later [PR114759]
Ping.[Message-ID: <1a420e3e-3285-4e0b-87bd-6714fedc0...@linux.ibm.com>]
Peter
On 6/19/24 4:14 PM, 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
>
> 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)
> @@ -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)
> {
>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
>&& 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 000..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
[PATCH] rs6000: ROP - Emit hashst and hashchk insns on Power8 and later [PR114759]
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
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)
@@ -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)
{
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
&& 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 000..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] &&
