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.
Confirmed.
> 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!
(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.
Created attachment 57977 [details] 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().
(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; + }
Updated patch for item 2. submitted.
Patch for item 3. submitted. https://gcc.gnu.org/pipermail/gcc-patches/2024-June/655164.html
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:a05c3d23d1e1c8d2971b123804fc7a61a3561adb commit r15-2080-ga05c3d23d1e1c8d2971b123804fc7a61a3561adb Author: Peter Bergner <bergner@linux.ibm.com> Date: Wed Jun 19 16:07:29 2024 -0500 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. 2024-06-19 Peter Bergner <bergner@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.
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:6f2bab9b5d1ce1914c748b7dcd8638dafaa98df7 commit r15-2081-g6f2bab9b5d1ce1914c748b7dcd8638dafaa98df7 Author: Peter Bergner <bergner@linux.ibm.com> Date: Mon Jul 15 16:57:32 2024 -0500 rs6000: Error on CPUs and ABIs that don't support the ROP protection insns [PR114759] We currently silently ignore the -mrop-protect option for old CPUs we don't support with the ROP hash insns, but we throw an error for unsupported ABIs. This patch treats unsupported CPUs and ABIs similarly by throwing an error both both. This matches clang behavior and allows us to simplify our tests in the code that generates our prologue and epilogue code. 2024-06-26 Peter Bergner <bergner@linux.ibm.com> gcc/ PR target/114759 * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disallow CPUs and ABIs that do no support the ROP protection insns. * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Remove now unneeded tests. (rs6000_emit_prologue): Likewise. Remove unneeded gcc_assert. (rs6000_emit_epilogue): Likewise. * config/rs6000/rs6000.md: Likewise. gcc/testsuite/ PR target/114759 * gcc.target/powerpc/pr114759-3.c: New test.
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:b2f47a5c1d5204131660ea0372a08e692df8844e commit r15-2155-gb2f47a5c1d5204131660ea0372a08e692df8844e Author: Peter Bergner <bergner@linux.ibm.com> Date: Thu Jul 18 18:01:46 2024 -0500 rs6000: Catch unsupported ABI errors when using -mrop-protect [PR114759,PR115988] 2024-07-18 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/114759 PR target/115988 * gcc.target/powerpc/pr114759-3.c: Catch unsupported ABI errors.
The releases/gcc-14 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:e2d746e5ab73b0b1f1a8104101c09b1f4ab3fa25 commit r14-10492-ge2d746e5ab73b0b1f1a8104101c09b1f4ab3fa25 Author: Peter Bergner <bergner@linux.ibm.com> Date: Wed Jun 19 16:07:29 2024 -0500 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. 2024-06-19 Peter Bergner <bergner@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. (cherry picked from commit a05c3d23d1e1c8d2971b123804fc7a61a3561adb)
The releases/gcc-14 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:35e5c2d2e4af20d143ee1a4f8f4f2bd8b24c4af1 commit r14-10493-g35e5c2d2e4af20d143ee1a4f8f4f2bd8b24c4af1 Author: Peter Bergner <bergner@linux.ibm.com> Date: Mon Jul 15 16:57:32 2024 -0500 rs6000: Error on CPUs and ABIs that don't support the ROP protection insns [PR114759] We currently silently ignore the -mrop-protect option for old CPUs we don't support with the ROP hash insns, but we throw an error for unsupported ABIs. This patch treats unsupported CPUs and ABIs similarly by throwing an error both both. This matches clang behavior and allows us to simplify our tests in the code that generates our prologue and epilogue code. 2024-06-26 Peter Bergner <bergner@linux.ibm.com> gcc/ PR target/114759 * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disallow CPUs and ABIs that do no support the ROP protection insns. * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Remove now unneeded tests. (rs6000_emit_prologue): Likewise. Remove unneeded gcc_assert. (rs6000_emit_epilogue): Likewise. * config/rs6000/rs6000.md: Likewise. gcc/testsuite/ PR target/114759 * gcc.target/powerpc/pr114759-3.c: New test. (cherry picked from commit 6f2bab9b5d1ce1914c748b7dcd8638dafaa98df7)
The releases/gcc-14 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:bd535b46aaffe637b2eeb634e56ee6e9efa511bf commit r14-10494-gbd535b46aaffe637b2eeb634e56ee6e9efa511bf Author: Peter Bergner <bergner@linux.ibm.com> Date: Thu Jul 18 18:01:46 2024 -0500 rs6000: Catch unsupported ABI errors when using -mrop-protect [PR114759,PR115988] 2024-07-18 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/114759 PR target/115988 * gcc.target/powerpc/pr114759-3.c: Catch unsupported ABI errors. (cherry picked from commit b2f47a5c1d5204131660ea0372a08e692df8844e)
The releases/gcc-13 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:77fd352a47137d79e6b7a480503ce4368f13c3e5 commit r13-8940-g77fd352a47137d79e6b7a480503ce4368f13c3e5 Author: Peter Bergner <bergner@linux.ibm.com> Date: Wed Jun 19 16:07:29 2024 -0500 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. 2024-06-19 Peter Bergner <bergner@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. (cherry picked from commit a05c3d23d1e1c8d2971b123804fc7a61a3561adb)
The releases/gcc-13 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:63b1b3e23c3c589c2859d481705dc706cbff35a1 commit r13-8941-g63b1b3e23c3c589c2859d481705dc706cbff35a1 Author: Peter Bergner <bergner@linux.ibm.com> Date: Mon Jul 15 16:57:32 2024 -0500 rs6000: Error on CPUs and ABIs that don't support the ROP protection insns [PR114759] We currently silently ignore the -mrop-protect option for old CPUs we don't support with the ROP hash insns, but we throw an error for unsupported ABIs. This patch treats unsupported CPUs and ABIs similarly by throwing an error both both. This matches clang behavior and allows us to simplify our tests in the code that generates our prologue and epilogue code. 2024-06-26 Peter Bergner <bergner@linux.ibm.com> gcc/ PR target/114759 * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disallow CPUs and ABIs that do no support the ROP protection insns. * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Remove now unneeded tests. (rs6000_emit_prologue): Likewise. Remove unneeded gcc_assert. (rs6000_emit_epilogue): Likewise. * config/rs6000/rs6000.md: Likewise. gcc/testsuite/ PR target/114759 * gcc.target/powerpc/pr114759-3.c: New test. (cherry picked from commit 6f2bab9b5d1ce1914c748b7dcd8638dafaa98df7)
The releases/gcc-13 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:9a4603d323d890dfab6d27ede17dc904abdccd9b commit r13-8942-g9a4603d323d890dfab6d27ede17dc904abdccd9b Author: Peter Bergner <bergner@linux.ibm.com> Date: Thu Jul 18 18:01:46 2024 -0500 rs6000: Catch unsupported ABI errors when using -mrop-protect [PR114759,PR115988] 2024-07-18 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/114759 PR target/115988 * gcc.target/powerpc/pr114759-3.c: Catch unsupported ABI errors. (cherry picked from commit b2f47a5c1d5204131660ea0372a08e692df8844e)
The releases/gcc-12 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:aa293f40770bae5e94f33d4700f2f0ce9eff712b commit r12-10640-gaa293f40770bae5e94f33d4700f2f0ce9eff712b Author: Peter Bergner <bergner@linux.ibm.com> Date: Wed Jun 19 16:07:29 2024 -0500 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. 2024-06-19 Peter Bergner <bergner@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. (cherry picked from commit a05c3d23d1e1c8d2971b123804fc7a61a3561adb)
The releases/gcc-12 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:25cf4d2a2200903fe868f8cbd9d24f35768041c1 commit r12-10641-g25cf4d2a2200903fe868f8cbd9d24f35768041c1 Author: Peter Bergner <bergner@linux.ibm.com> Date: Mon Jul 15 16:57:32 2024 -0500 rs6000: Error on CPUs and ABIs that don't support the ROP protection insns [PR114759] We currently silently ignore the -mrop-protect option for old CPUs we don't support with the ROP hash insns, but we throw an error for unsupported ABIs. This patch treats unsupported CPUs and ABIs similarly by throwing an error both both. This matches clang behavior and allows us to simplify our tests in the code that generates our prologue and epilogue code. 2024-06-26 Peter Bergner <bergner@linux.ibm.com> gcc/ PR target/114759 * config/rs6000/rs6000.cc (rs6000_option_override_internal): Disallow CPUs and ABIs that do no support the ROP protection insns. * config/rs6000/rs6000-logue.cc (rs6000_stack_info): Remove now unneeded tests. (rs6000_emit_prologue): Likewise. Remove unneeded gcc_assert. (rs6000_emit_epilogue): Likewise. * config/rs6000/rs6000.md: Likewise. gcc/testsuite/ PR target/114759 * gcc.target/powerpc/pr114759-3.c: New test. (cherry picked from commit 6f2bab9b5d1ce1914c748b7dcd8638dafaa98df7)
The releases/gcc-12 branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:f7bebf4c07dffaa75c77152e8004aa0ccbf6eeac commit r12-10642-gf7bebf4c07dffaa75c77152e8004aa0ccbf6eeac Author: Peter Bergner <bergner@linux.ibm.com> Date: Thu Jul 18 18:01:46 2024 -0500 rs6000: Catch unsupported ABI errors when using -mrop-protect [PR114759,PR115988] 2024-07-18 Peter Bergner <bergner@linux.ibm.com> gcc/testsuite/ PR target/114759 PR target/115988 * gcc.target/powerpc/pr114759-3.c: Catch unsupported ABI errors. (cherry picked from commit b2f47a5c1d5204131660ea0372a08e692df8844e)
The master branch has been updated by Peter Bergner <bergner@gcc.gnu.org>: https://gcc.gnu.org/g:0451bc503da9c858e9f1ddfb8faec367c2e032c8 commit r15-2896-g0451bc503da9c858e9f1ddfb8faec367c2e032c8 Author: Peter Bergner <bergner@linux.ibm.com> Date: Tue Jun 18 17:42:45 2024 -0500 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-protect hash 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_emit_prologue): ...to here. gcc/testsuite/ PR target/114759 * gcc.target/powerpc/pr114759-1.c: New test.