[Bug target/114759] New: Power: multiple issues with -mrop-protect

[Bug target/114759] New: Power: multiple issues with -mrop-protect

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

            Bug ID: 114759
           Summary: Power: multiple issues with -mrop-protect
           Product: gcc
           Version: 14.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: target
          Assignee: unassigned at gcc dot gnu.org
          Reporter: bergner at gcc dot gnu.org
  Target Milestone: ---

There are multiple issues with the -mrop-protect option which are all
inter-related.

1. We always define the __ROP_PROTECT__ predefined macro when using
-mrop-protect, even when we've silently disabled ROP protection because of a
too old -mcpu=CPU value.  We should only emit __ROP_PROTECT__ when it's legal
to emit the ROP insns.

2. We always disable shrink-wrapping when -mrop-protect is used, even when
we've silently disabled ROP protection because of a too old -mcpu=CPU value. 
We should not disable shrink-wrapping if we've disabled ROP protection.

3. We silently disable ROP protection for everything other than -mcpu=power10. 
The binutils assembler accepts the ROP insns back to Power8, so we should emit
them for Power8 and later.

4. We give an error when -mrop-protect is used with any -mabi=ABI value not
equal to ELFv2, whereas a too old -mcpu=CPU value only causes us to silently
disable ROP protection.  I think both scenarios should behave similarly, so
either we silently disable ROP protection for both or we give an error for
both.

This is not a regression.  I consider 1. to be a correctness/wrong code bug.

[Bug target/114759] Power: multiple issues with -mrop-protect

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

Peter Bergner <bergner at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |dje at gcc dot gnu.org,
                   |                            |linkw at gcc dot gnu.org,
                   |                            |segher at gcc dot gnu.org
           Assignee|unassigned at gcc dot gnu.org      |bergner at gcc dot gnu.org
             Target|                            |powerpc64le-linux
   Last reconfirmed|                            |2024-04-17
             Status|UNCONFIRMED                 |ASSIGNED
     Ever confirmed|0                           |1

--- Comment #1 from Peter Bergner <bergner at gcc dot gnu.org> ---
Confirmed.

[Bug target/114759] Power: multiple issues with -mrop-protect

[segher at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

--- Comment #2 from Segher Boessenkool <segher at gcc dot gnu.org> ---
> 1. We always define the __ROP_PROTECT__ predefined macro when using -mrop-protect, even when we've silently disabled ROP protection because of a too old -mcpu=CPU value.  We should only emit __ROP_PROTECT__ when it's legal to emit the ROP insns.

No.  Whenever the -mrop-protect option is in effect, we should do that
predefine.

If you want to refuse the option without a -mcpu= that can generate useful code
for it, that's fine, but that is not what we do.  Instead, we generate code
that
will do the ROP-protection boogaloo on CPUs that implement support for that,
and
does nothing otherwise.

> 2.  We always disable shrink-wrapping when -mrop-protect is used, [...]

Yes, this is problematic, and seems to be completely unnecessary.  When using
SWS
at least -- but then we need to define a component for doing the ROP-protection
thing, of course.  After all, it has to be done before anything else in the
function.
By exactly the same argument we should *also* do ROP-protection in all leaf
functions, btw!

> 3.  We silently disable ROP protection for everything other than -mcpu=power10.  The binutils assembler accepts the ROP insns back to Power8, so we should emit them for Power8 and later.

The ISA claims it will work for anything after ISA 2.04, even.

> 4.  We give an error when -mrop-protect is used with any -mabi=ABI value not equal to ELFv2, [...]

Yes, we should make it work everywhere.  Even on -m32.  But it requires
adjusting
the ABI as well!

2) should be fixed, and 4) should be fixed by actually implementing it
everywhere!

[Bug target/114759] Power: multiple issues with -mrop-protect

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

--- Comment #3 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Segher Boessenkool from comment #2)
>> 1. We always define the __ROP_PROTECT__ predefined macro [snip]
> 
> No.  Whenever the -mrop-protect option is in effect, we should do that
> predefine.
What we do now is:

  gcc -dM -E -mcpu=power10 test.c | grep ROP
  gcc -dM -E -mcpu=power10 -mrop-protect test.c | grep ROP
  #define __ROP_PROTECT__ 1
  gcc -dM -E -mcpu=power4 -mrop-protect test.c | grep ROP
  #define __ROP_PROTECT__ 1

...and that last compile is a wrong code bug.  If we want to continue to
silently disable ROP protection with -mcpu=power4, then we also need to not
define __ROP_PROTECT__.  If we decide we want an error for this, which I
mentioned later as an solution, then this is fixed automatically by doing that.


> If you want to refuse the option without a -mcpu= that can generate useful
> code for it, that's fine, but that is not what we do.  Instead, we generate
> code that will do the ROP-protection boogaloo on CPUs that implement support
> for that, and does nothing otherwise.
We do not currently do "nothing" when we see -mcpu=power4 -mrop-protect.
Yes, we do not emit the hashst and hashchk insns, but we *do* emit the
__ROP_PROTECT__ predefined macro and that is bad.  The most common usage
of that macro is in .S assembler files and if we define __ROP_PROTECT__
in the wrong cases, they can end up with assembler errors.  Again, if
we decide to emit an error for -mcpu=power4 -mrop-protect, then this is
just fixed automatically.



>> 2.  We always disable shrink-wrapping when -mrop-protect is used, [...]
> 
> Yes, this is problematic, and seems to be completely unnecessary.  
For the case where we silently ignore -mcpu=power4 -mrop-protect, it is
completely unnecessary.  If we decide to emit an error for this instead,
then like the above, this is just automatically fixed.



> By exactly the same argument we should *also* do ROP-protection in all
> leaf functions, btw!
I'm not 100% convinced we need to "protect" leaf functions, since the return
address of the leaf function ever makes it onto the stack to be potentially
corrupted.  Can you explain how a leaf-function could be attacked if we
never save its return address to the stack?



>> 3.  We silently disable ROP protection for everything other than
>> -mcpu=power10.  The binutils assembler accepts the ROP insns back
>> to Power8, so we should emit them for Power8 and later.
> 
> The ISA claims it will work for anything after ISA 2.04, even.
True, but given the binutils assembler doesn't accept hashst and hashchk
for anything before Power8, it seemed convenient to match that behavior.
If we enable it for ISA 2.04 and later, then we either have to fix
binutils to do the same (which we can do), but we still run the risk of
some compiles failing because the user is using an older unfixed assembler.


>> 4.  We give an error when -mrop-protect is used with any -mabi=ABI
>> value not equal to ELFv2, [...]
> 
> Yes, we should make it work everywhere.  Even on -m32.  But it requires
> adjusting the ABI as well!
That's a nice goal, but I'd like to fix the present issues before tackling
expanding its use to other ABIs.


So the first question to ask is, do we want to silently disable (maybe with
a warning) emitting ROP instructions if used with -mcpu=CPU or -mabi=ABI that
we don't want or can't emit them for?  ...or do we want to produce an error?
The answer to this question will help guide us on how to fix the other
issues or whether we even have to do anything for them.

[Bug target/114759] Power: multiple issues with -mrop-protect

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

--- Comment #4 from Peter Bergner <bergner at gcc dot gnu.org> ---
Created attachment 57977
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=57977&action=edit
Patch that emits an error for invalid ROP option combinations.

Here's a patch that treats invalid ROP option combinations (currently assuming
P7 and earlier are invalid) as an error.

If instead we want to just silently ignore (or with a warning), we'd just need
to modify the rs6000.cc hunk to disable rs6000_rop_protect instead of calling
error().

[Bug target/114759] Power: multiple issues with -mrop-protect

[bergner at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=114759

--- Comment #5 from Peter Bergner <bergner at gcc dot gnu.org> ---
(In reply to Peter Bergner from comment #4)
> If instead we want to just silently ignore (or with a warning), we'd just
> need to modify the rs6000.cc hunk to disable rs6000_rop_protect instead of
> calling error().

Like so:

-  /* If we are inserting ROP-protect instructions, disable shrink wrap.  */
+
   if (rs6000_rop_protect)
-    flag_shrink_wrap = 0;
+    {
+      if (!TARGET_POWER8 || DEFAULT_ABI != ABI_ELFv2)
+       rs6000_rop_protect = 0;
+      else
+       /* If we are inserting ROP-protect instructions, disable shrink wrap. 
*/
+       flag_shrink_wrap = 0;
+    }