Hi!
On Mon, Jun 17, 2024 at 05:26:39PM -0500, Peter Bergner wrote:
> While auditing our ROP code generation for some test cases I wrote, I noticed
> a few issues which I'm tracking in PR114759. The first issue I noticed is we
> disable shrink-wrapping when using -mrop-protect, even in the cases where we
> never emit the ROP instructions because they're not needed.
Please don't call this "ROP instructions". -mrop-protect tries to make
it much harder to succesfully do exploits in a style called "return-
oriented programming", starting from a stack overwrite normally. It
does this by hashing the return address together with the stack pointer
value and with the previous hash value (so the whole call stack hashed),
and checking that before returning.
"ROP insns" are the instructions used in such exploits, not what you
mean here :-)
The instructions are called "hash*"C, so maybe call tbem "hash insns"
or "ROP protect hash insns"?.
> The problem is
> we disable shrink-wrapping too early, before we know whether we will need to
> emit the ROP instructions or not. The fix is to delay disabling shrink
> wrapping until we've decided whether we will or won't be emitting the ROP
> instructions.
> * config/rs6000/rs6000.cc (rs6000_override_options_after_change): Move
> the disabling of shrink-wrapping from here....
> * config/rs6000/rs6000-logue.cc (rs6000_stack_info): ...to here.
Hrm. Can you do it in some particular caller of rs6000_stack_info,
instead? The rs6000_stack_info function itself is not suppposed to
change any state whatsoever.
> --- a/gcc/config/rs6000/rs6000-logue.cc
> +++ b/gcc/config/rs6000/rs6000-logue.cc
> @@ -720,7 +720,11 @@ rs6000_stack_info (void)
> && info->calls_p
> && DEFAULT_ABI == ABI_ELFv2
> && rs6000_rop_protect)
> - info->rop_hash_size = 8;
> + {
> + /* If we are inserting ROP-protect instructions, disable shrink wrap.
> */
> + flag_shrink_wrap = 0;
> + info->rop_hash_size = 8;
> + }
The comment should say *why*! The fact that we do is clear from the
code itself already. But why do we want this?
> --- a/gcc/config/rs6000/rs6000.cc
> +++ b/gcc/config/rs6000/rs6000.cc
> @@ -3427,10 +3427,6 @@ rs6000_override_options_after_change (void)
> }
> else if (!OPTION_SET_P (flag_cunroll_grow_size))
> flag_cunroll_grow_size = flag_peel_loops || optimize >= 3;
> -
> - /* If we are inserting ROP-protect instructions, disable shrink wrap. */
> - if (rs6000_rop_protect)
> - flag_shrink_wrap = 0;
> }
(Yes, I know the original code didn't say either, but let's try to make
things better :-) )
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/powerpc/pr114759-1.c
> @@ -0,0 +1,16 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -mdejagnu-cpu=power10 -mrop-protect
> -fdump-rtl-pro_and_epilogue" } */
> +/* { dg-require-effective-target rop_ok } */
Do you want rop_ok while you are *forcing* it to be okay anyway? Why?
Segher
