| Message ID | 20241007145314.3938424-1-smonga@linux.ibm.com |
|---|---|
| State | New |
| Headers | show |
| Series | powerpc64le: ROP changes for _init/_fini | expand |
On Okt 07 2024, Sachin Monga wrote: > diff --git a/sysdeps/powerpc/powerpc64/multiarch/Makefile b/sysdeps/powerpc/powerpc64/multiarch/Makefile > index b847c19049..769625b70d 100644 > --- a/sysdeps/powerpc/powerpc64/multiarch/Makefile > +++ b/sysdeps/powerpc/powerpc64/multiarch/Makefile > @@ -38,7 +38,9 @@ sysdep_routines += memchr-power10 memcmp-power10 memcpy-power10 \ > strlen-power9 strncpy-power9 stpncpy-power9 strlen-power10 > endif > CFLAGS-strncase-power7.c += -mcpu=power7 -funroll-loops > +CFLAGS-strncase-power7.c := $(filter-out -mrop-protect, $(strncase-power7-CFLAGS)) Where is strncase-power7-CFLAGS coming from?
-mrop-protect is gcc flag for ROP enablement from "configure" command. However, power7 files must not use this flag. On 07/10/24 9:25 pm, Andreas Schwab wrote: > On Okt 07 2024, Sachin Monga wrote: > >> diff --git a/sysdeps/powerpc/powerpc64/multiarch/Makefile b/sysdeps/powerpc/powerpc64/multiarch/Makefile >> index b847c19049..769625b70d 100644 >> --- a/sysdeps/powerpc/powerpc64/multiarch/Makefile >> +++ b/sysdeps/powerpc/powerpc64/multiarch/Makefile >> @@ -38,7 +38,9 @@ sysdep_routines += memchr-power10 memcmp-power10 memcpy-power10 \ >> strlen-power9 strncpy-power9 stpncpy-power9 strlen-power10 >> endif >> CFLAGS-strncase-power7.c += -mcpu=power7 -funroll-loops >> +CFLAGS-strncase-power7.c := $(filter-out -mrop-protect, $(strncase-power7-CFLAGS)) > Where is strncase-power7-CFLAGS coming from? >
On 08/10/24 04:09, Sachin Monga wrote: > -mrop-protect is gcc flag for ROP enablement from "configure" command. > However, power7 files must not use this flag. > > On 07/10/24 9:25 pm, Andreas Schwab wrote: >> On Okt 07 2024, Sachin Monga wrote: >> >>> diff --git a/sysdeps/powerpc/powerpc64/multiarch/Makefile b/sysdeps/powerpc/powerpc64/multiarch/Makefile >>> index b847c19049..769625b70d 100644 >>> --- a/sysdeps/powerpc/powerpc64/multiarch/Makefile >>> +++ b/sysdeps/powerpc/powerpc64/multiarch/Makefile >>> @@ -38,7 +38,9 @@ sysdep_routines += memchr-power10 memcmp-power10 memcpy-power10 \ >>> strlen-power9 strncpy-power9 stpncpy-power9 strlen-power10 >>> endif >>> CFLAGS-strncase-power7.c += -mcpu=power7 -funroll-loops >>> +CFLAGS-strncase-power7.c := $(filter-out -mrop-protect, $(strncase-power7-CFLAGS)) >> Where is strncase-power7-CFLAGS coming from? >> I would be interesting to explain a bit on commit message what ROP aim to do on, and when it was added on POWER ISA so the reviewed can have some context here.
On 10/7/24 9:53 AM, Sachin Monga wrote: > Disable -mrop-protect for power7 files > Macro FRAME_ROP_SAVE defined for storing ROP pointer > Store/Retrieve ROP pointer next to FRAME_MIN_SIZE_PARM > > > # Called during static initialization > diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h > index c439b06121..52253a6a0f 100644 > --- a/sysdeps/powerpc/powerpc64/sysdep.h > +++ b/sysdeps/powerpc/powerpc64/sysdep.h > @@ -24,6 +24,7 @@ > /* Stack frame offsets. */ > #define FRAME_BACKCHAIN 0 > #define FRAME_CR_SAVE 8 > +#define FRAME_ROP_SAVE 8 /* Generally to be stored in red zone */ If I read the patch correctly, it is stored in the redzone, and then a frame stacked around it. Shouldn't the offset be written -8? > #define FRAME_LR_SAVE 16 > #if _CALL_ELF != 2 > #define FRAME_MIN_SIZE 112 Likewise, the frame size should always be 16B aligned. I think you need to update FRAME_MIN_SIZE when ROP is enabled to account for the additional save slot and alignment requirement. Alternatively, FRAME_ROP_SAVE could be split into FRAME_ROP_SIZE and FRAME_ROP_SAVE to signify the difference.
On Okt 08 2024, Sachin Monga wrote: > -mrop-protect is gcc flag for ROP enablement from "configure" command. > However, power7 files must not use this flag. That doesn't answer my question.
Sorry Andreas Its was a typo from my side . It should be */CFLAGS-strncase-power7/ *actually*. *Will change it. Could'nt catch since it didn't give any error while compilation and filtered my flag also. On 08/10/24 8:51 pm, Andreas Schwab wrote: > On Okt 08 2024, Sachin Monga wrote: > >> -mrop-protect is gcc flag for ROP enablement from "configure" command. >> However, power7 files must not use this flag. > That doesn't answer my question. >
On 10/8/24 10:14 AM, Paul E. Murphy wrote: > > On 10/7/24 9:53 AM, Sachin Monga wrote: > >> #define FRAME_LR_SAVE 16 >> #if _CALL_ELF != 2 >> #define FRAME_MIN_SIZE 112 > > Likewise, the frame size should always be 16B aligned. I think you > need to update FRAME_MIN_SIZE when ROP is enabled to account for the > additional save slot and alignment requirement. Alternatively, > FRAME_ROP_SAVE could be split into FRAME_ROP_SIZE and FRAME_ROP_SAVE > to signify the difference. > Actually, don't change the minimum frame size if the hash must be stored relative to the old stack pointer. Some assembly uses it as an offset to the parameter save space. However, is it critical to store the hash before pushing the new frame? If not, you can store the hash alongside the other slots required for a minimal stack frame.
On 10/8/24 7:07 PM, Paul E. Murphy wrote: > However, is it critical to store the hash before pushing the new frame? > If not, you can store the hash alongside the other slots required for > a minimal stack frame. There is nothing that mandates we store the hash before creating the callee's frame. That said, the hashst and hashchk instructions are defined as: hashst RB,offset(RA) hashchk RB,offset(RA) ...and implemented as X-form instructions, similar to ldx, etc. Those types of instructions have a 6-bit major opcode and a 10-bit secondary opcode, 3 5-bit register operands and a 1-bit RC operand. The bits for the offset operand is the concatenation of one of the 5-bit register fields and the 1-bit RC field, giving us a 6-bit offset field. That field is used to encode 64 8-byte aligned offsets between the range -8 and -512. This limitation restricts us to using the caller's stack pointer value plus the negative offset so we store below the caller's stack frame into was is or will become the callee's stack frame. If we emit the hashst before creating the callee's stack frame, then we can use the caller's r1 as is. If we emit the hashst after creating the callee's stack frame, then we have to create a copy of the caller's r1 value before creating the new stack frame and then use that copy as the base register. It's just easier to use the caller's r1 and store the hash in the red-zone below the caller's frame. I'll note that the 32-bit ppc ABI does not have a red-zone, so if we ever implement ROP protection for it, we'll have to use the method of creating a copy of the caller's r1, create the callee's frame and then store the hash using the copy of the caller's r1. I'm unsure if we'll ever do that though. Peter
On 10/8/24 7:15 AM, Adhemerval Zanella Netto wrote: > I would be interesting to explain a bit on commit message what ROP aim > to do on, and when it was added on POWER ISA so the reviewed can have > some context here. The ROP instructions were added in ISA 3.1 (ie, Power10), however they were defined so that if executed on older cpus, they would behave as nops. This allows us to emit them on older cpus and they'd just be ignored, but if run on a Power10, then the binary would be ROP protected. Currently, the binutils assembler accepts hashst and hashchk back to Power8, so that is what we are targeting for protecting in glibc. Peter
diff --git a/sysdeps/powerpc/powerpc64/crti.S b/sysdeps/powerpc/powerpc64/crti.S index 71bdddfb3b..4f009158c7 100644 --- a/sysdeps/powerpc/powerpc64/crti.S +++ b/sysdeps/powerpc/powerpc64/crti.S @@ -68,7 +68,12 @@ BODY_LABEL (_init): LOCALENTRY(_init) mflr 0 std 0, FRAME_LR_SAVE(r1) +#ifdef __ROP_PROTECT__ + hashst 0, -FRAME_ROP_SAVE(r1) + stdu r1, -FRAME_MIN_SIZE_PARM-FRAME_ROP_SAVE(r1) +#else stdu r1, -FRAME_MIN_SIZE_PARM(r1) +#endif #if PREINIT_FUNCTION_WEAK addis r9, r2, .LC0@toc@ha ld r0, .LC0@toc@l(r9) @@ -87,4 +92,9 @@ BODY_LABEL (_fini): LOCALENTRY(_fini) mflr 0 std 0, FRAME_LR_SAVE(r1) +#ifdef __ROP_PROTECT__ + hashst 0, -FRAME_ROP_SAVE(r1) + stdu r1, -FRAME_MIN_SIZE_PARM-FRAME_ROP_SAVE(r1) +#else stdu r1, -FRAME_MIN_SIZE_PARM(r1) +#endif diff --git a/sysdeps/powerpc/powerpc64/crtn.S b/sysdeps/powerpc/powerpc64/crtn.S index 4e91231f2c..45359dbe32 100644 --- a/sysdeps/powerpc/powerpc64/crtn.S +++ b/sysdeps/powerpc/powerpc64/crtn.S @@ -39,13 +39,27 @@ #include <sysdep.h> .section .init,"ax",@progbits +#ifdef __ROP_PROTECT__ + addi r1, r1, FRAME_MIN_SIZE_PARM+FRAME_ROP_SAVE +#else addi r1, r1, FRAME_MIN_SIZE_PARM +#endif ld r0, FRAME_LR_SAVE(r1) mtlr r0 +#ifdef __ROP_PROTECT__ + hashchk 0, -FRAME_ROP_SAVE(r1) +#endif blr .section .fini,"ax",@progbits +#ifdef __ROP_PROTECT__ + addi r1, r1, FRAME_MIN_SIZE_PARM+FRAME_ROP_SAVE +#else addi r1, r1, FRAME_MIN_SIZE_PARM +#endif ld r0, FRAME_LR_SAVE(r1) mtlr r0 +#ifdef __ROP_PROTECT__ + hashchk 0, -FRAME_ROP_SAVE(r1) +#endif blr diff --git a/sysdeps/powerpc/powerpc64/multiarch/Makefile b/sysdeps/powerpc/powerpc64/multiarch/Makefile index b847c19049..769625b70d 100644 --- a/sysdeps/powerpc/powerpc64/multiarch/Makefile +++ b/sysdeps/powerpc/powerpc64/multiarch/Makefile @@ -38,7 +38,9 @@ sysdep_routines += memchr-power10 memcmp-power10 memcpy-power10 \ strlen-power9 strncpy-power9 stpncpy-power9 strlen-power10 endif CFLAGS-strncase-power7.c += -mcpu=power7 -funroll-loops +CFLAGS-strncase-power7.c := $(filter-out -mrop-protect, $(strncase-power7-CFLAGS)) CFLAGS-strncase_l-power7.c += -mcpu=power7 -funroll-loops +CFLAGS-strncase_l-power7.c := $(filter-out -mrop-protect, $(strncase-power7_l-CFLAGS)) endif # Called during static initialization diff --git a/sysdeps/powerpc/powerpc64/sysdep.h b/sysdeps/powerpc/powerpc64/sysdep.h index c439b06121..52253a6a0f 100644 --- a/sysdeps/powerpc/powerpc64/sysdep.h +++ b/sysdeps/powerpc/powerpc64/sysdep.h @@ -24,6 +24,7 @@ /* Stack frame offsets. */ #define FRAME_BACKCHAIN 0 #define FRAME_CR_SAVE 8 +#define FRAME_ROP_SAVE 8 /* Generally to be stored in red zone */ #define FRAME_LR_SAVE 16 #if _CALL_ELF != 2 #define FRAME_MIN_SIZE 112
Disable -mrop-protect for power7 files Macro FRAME_ROP_SAVE defined for storing ROP pointer Store/Retrieve ROP pointer next to FRAME_MIN_SIZE_PARM Signed-off-by: Sachin Monga <smonga@linux.ibm.com> --- sysdeps/powerpc/powerpc64/crti.S | 10 ++++++++++ sysdeps/powerpc/powerpc64/crtn.S | 14 ++++++++++++++ sysdeps/powerpc/powerpc64/multiarch/Makefile | 2 ++ sysdeps/powerpc/powerpc64/sysdep.h | 1 + 4 files changed, 27 insertions(+)