rs6000: ROP - Do not disable shrink-wrapping for leaf functions [PR114759]

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.  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.

This patch passed bootstrap and regtesting on powerpc64le-linux with no
regressions, with the unpatched build FAILing the new test case and the
patched build PASSing the new test case.
Ok for trunk?

Peter



rs6000: ROP - Do not disable shrink-wrapping for leaf functions [PR114759]

Only disable shrink-wrapping when using -mrop-protect when we know we
will be emitting the ROP instructions (ie, non-leaf functions).

2024-06-17  Peter Bergner  <bergner@linux.ibm.com>

gcc/
	PR target/114759
	* 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.

gcc/testsuite/
	PR target/114759
	* gcc.target/powerpc/pr114759-1.c: New test.
---
 gcc/config/rs6000/rs6000-logue.cc             |  6 +++++-
 gcc/config/rs6000/rs6000.cc                   |  4 ----
 gcc/testsuite/gcc.target/powerpc/pr114759-1.c | 16 ++++++++++++++++
 3 files changed, 21 insertions(+), 5 deletions(-)
 create mode 100644 gcc/testsuite/gcc.target/powerpc/pr114759-1.c

Hi!

On Mon, Jun 17, 2024 at 06:49:18PM -0500, Peter Bergner wrote:
> On 6/17/24 6:11 PM, Segher Boessenkool wrote:
> >> -  /* 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 :-) )
> 
> Yeah, I didn't write that, I only moved it, but I can try to come up with
> an explanation of why we need to disable it now.  That said, my hope is to
> not have to disable shrink-wrapping even when we emit the ROP protect hash
> insns in the future, but that will take some extra work.  If I can manage
> that, then this should all just go away. :-)  Until then, we can stick
> with this patch's micro-optimization.

If you inline one function into another, there is no ROP protection on
their boundary anymore (since there is no such boundary anymore!)  This
is not necessarily a problem, but you do want some noipa or similar
markup where without ROP protection you have no incentive to do that.

Shrink-wrapping allows more inlining, and more inlining allows more
shrink-wrapping, but there is no direct relation between shrink-wrapping
and our ROP protect stuff?  We just need to make sure the hashst and
hashchk things are done at the very start and the very end of the
functions, but we need to make sure of that anyway!

So yeah, please investigate a bit more :-)

> >> --- /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?
> 
> At the moment, yes, since the rop_ok test not only checks for the -mcpu= level,
> it also verifies that the ABI is ok.

Ah right!  Add a short comment?

> Currently, rop_ok makes sure we have
> Power10 and ELFv2 ABI being used.  So currently, if we were to run this test
> on BE, we'd get an UNSUPPORTED using the rop_ok check, but if we removed it,
> we'd see a FAIL.  

Yup.

> As we discussed offline, the plan is to eventually enable emitting the ROP protect
> hash insns on other ABIs, but until then, I think we want to keep the rop_ok check
> so as to keep Bill's CI builder from flagging it as a FAIL.

:-)


Segher

On 6/17/24 7:57 PM, Segher Boessenkool wrote:
> On Mon, Jun 17, 2024 at 06:49:18PM -0500, Peter Bergner wrote:
>> On 6/17/24 6:11 PM, Segher Boessenkool wrote:
>> Yeah, I didn't write that, I only moved it, but I can try to come up with
>> an explanation of why we need to disable it now.  That said, my hope is to
>> not have to disable shrink-wrapping even when we emit the ROP protect hash
>> insns in the future, but that will take some extra work.  If I can manage
>> that, then this should all just go away. :-)  Until then, we can stick
>> with this patch's micro-optimization.
> 
> If you inline one function into another, there is no ROP protection on
> their boundary anymore (since there is no such boundary anymore!)  This
> is not necessarily a problem, but you do want some noipa or similar
> markup where without ROP protection you have no incentive to do that.
> 
> Shrink-wrapping allows more inlining, and more inlining allows more
> shrink-wrapping, but there is no direct relation between shrink-wrapping
> and our ROP protect stuff?  We just need to make sure the hashst and
> hashchk things are done at the very start and the very end of the
> functions, but we need to make sure of that anyway!
> 
> So yeah, please investigate a bit more :-)

So we should be able to shrink-wrap in the presence of the ROP protection.
The ROP attacks work by buffer overrun type issues, clobbering the return
address that was saved on the stack causing us to return to somewhere else.
If we don't need to save the return address on the stack like for leaf
functions, or shrink-wrapped sections that are call free, those codes
are not really susceptible to ROP attacks.  It's the call paths where we
save the return address on the stack that we have to protect.  If inlining
or shrink wrapping increases the amount of code that is call free (ie, we
don't need to save the return address), then that code is not less safe
than before but as safe or safer than before.  It seems the reason we
disabled shrink-wrapping now, was that we were emitting the hashst in the
wrong location (PR101324) causing us to store a bad hash value.  I think
that was just a "bug" that probably should have been fixed rather than
worked around by disabling shrink-wrapping.  It's on my TODO to take a
look at fixing that correctly.





>> At the moment, yes, since the rop_ok test not only checks for the -mcpu= level,
>> it also verifies that the ABI is ok.
> 
> Ah right!  Add a short comment?

Can do.


>> Currently, rop_ok makes sure we have
>> Power10 and ELFv2 ABI being used.  So currently, if we were to run this test
>> on BE, we'd get an UNSUPPORTED using the rop_ok check, but if we removed it,
>> we'd see a FAIL.  
> 
> Yup.


Peter

On Mon, Jun 17, 2024 at 08:54:46PM -0500, Peter Bergner wrote:
> On 6/17/24 7:57 PM, Segher Boessenkool wrote:
> > On Mon, Jun 17, 2024 at 06:49:18PM -0500, Peter Bergner wrote:
> >> On 6/17/24 6:11 PM, Segher Boessenkool wrote:
> >> Yeah, I didn't write that, I only moved it, but I can try to come up with
> >> an explanation of why we need to disable it now.  That said, my hope is to
> >> not have to disable shrink-wrapping even when we emit the ROP protect hash
> >> insns in the future, but that will take some extra work.  If I can manage
> >> that, then this should all just go away. :-)  Until then, we can stick
> >> with this patch's micro-optimization.
> > 
> > If you inline one function into another, there is no ROP protection on
> > their boundary anymore (since there is no such boundary anymore!)  This
> > is not necessarily a problem, but you do want some noipa or similar
> > markup where without ROP protection you have no incentive to do that.
> > 
> > Shrink-wrapping allows more inlining, and more inlining allows more
> > shrink-wrapping, but there is no direct relation between shrink-wrapping
> > and our ROP protect stuff?  We just need to make sure the hashst and
> > hashchk things are done at the very start and the very end of the
> > functions, but we need to make sure of that anyway!
> > 
> > So yeah, please investigate a bit more :-)
> 
> So we should be able to shrink-wrap in the presence of the ROP protection.

Well of course, if we really *cannot* currently that obviously is
just a bug.

But do we want to?  And, how far, in what cases not?

In extremis everything is inlined into main(), and -mrop-protect doesn't
do any protection.  This can be done for *any* program btw.

In practice this isn't likely to happen so much.  But we need to
monitor -- a lot of (target, and backend) optimisations try to reduce
nesting overhead, usually by nesting less.  And -mrop-protect only does
anything at function boundaries :-)

> The ROP attacks work by buffer overrun type issues, clobbering the return
> address that was saved on the stack causing us to return to somewhere else.

Buffer overflows on the stack are the easiest / most common way to do
this, yes.  But there are other ways to do a return address overwrite.
Not as easy to exploit by far, in most programs, sure.

> If we don't need to save the return address on the stack like for leaf
> functions, or shrink-wrapped sections that are call free, those codes
> are not really susceptible to ROP attacks.

That is true in some way, yes.  Your caller will still check the chain
(albeit later).

> It's the call paths where we
> save the return address on the stack that we have to protect.  If inlining
> or shrink wrapping increases the amount of code that is call free (ie, we
> don't need to save the return address), then that code is not less safe
> than before but as safe or safer than before.  It seems the reason we
> disabled shrink-wrapping now, was that we were emitting the hashst in the
> wrong location (PR101324) causing us to store a bad hash value.  I think
> that was just a "bug" that probably should have been fixed rather than
> worked around by disabling shrink-wrapping.  It's on my TODO to take a
> look at fixing that correctly.

Sounds good!


Segher

On 6/18/24 8:20 AM, Segher Boessenkool wrote:
> On Mon, Jun 17, 2024 at 08:54:46PM -0500, Peter Bergner wrote:
>> So we should be able to shrink-wrap in the presence of the ROP protection.
[snip]
> But do we want to?  And, how far, in what cases not?

My answer to the above would be "yes", "as far as we do today without
-mrop-protect" and "none". :-)  I don't think -mrop-protect should affect
whether we shrink-wrap or not.  I don't think shrink-wrapping call free
paths makes the compiled code less secure by not emitting the hashst/hashchk
insns on those paths, so why would we do anything different wrt shrink-wrapping?


Peter

On Tue, Jun 18, 2024 at 12:53:09PM -0500, Peter Bergner wrote:
> On 6/18/24 8:20 AM, Segher Boessenkool wrote:
> > On Mon, Jun 17, 2024 at 08:54:46PM -0500, Peter Bergner wrote:
> >> So we should be able to shrink-wrap in the presence of the ROP protection.
> [snip]
> > But do we want to?  And, how far, in what cases not?
> 
> My answer to the above would be "yes", "as far as we do today without
> -mrop-protect" and "none". :-)  I don't think -mrop-protect should affect
> whether we shrink-wrap or not.

That is a good answer, and I agree :-)

> I don't think shrink-wrapping call free
> paths makes the compiled code less secure by not emitting the hashst/hashchk
> insns on those paths, so why would we do anything different wrt shrink-wrapping?

From my viewpoint, -mrop-protect should not change code generation at
all, except of course it has to emit some hash* insns :-)

If we want to have some functions noipa, then we should just put that
attribute there in the code!  Maybe some applications / libraries /
kernels / whatever should do some of that, but the compiler cannot
really help with policy questions like that.


Segher

On 6/18/24 3:38 PM, Segher Boessenkool wrote:
> From my viewpoint, -mrop-protect should not change code generation at
> all, except of course it has to emit some hash* insns :-)

Ideally, I agree with that.  That said, the hash* insns only accept negative
offsets and the allowed range is rather limited, so from a practical standpoint,
we might have to modify the prologue code slightly to satisfy the restrictions
those insns have.

Peter