[PATCH] powerpc64le: ROP changes for clone/clone3

[Adhemerval Zanella Netto]

On 10/10/24 18:01, Peter Bergner wrote:
> On 10/9/24 2:37 PM, Adhemerval Zanella Netto wrote:
>> Do we really need ROP-mitigation on clone/clone3? The initial stack
>> frame is only created so the child can save its TOC (so the initial
>> thread function seems as a normal execution flow), and the LR won't
>> be used to return after the thread function returns (pthread_create
>> will issue exit).
> 
> I think you mean so the child can save its LR, since the LR is saved
> in the caller's stack frame.  If the child needs to save its TOC,
> then that is stored in the child's own frame.  But understood about
> why there is a parent frame.

Not really, I think my comment was confusing because I reference the
initial stack and code changes the svc issuing.  

What I really meant that the only reason we save/restore LR for svc is 
because it is volatile by the kernel ABI, so the ROP change is to
protect an eventual corruption by the *kernel*.  IMHO, if you think
kernel will possible corrupt the userland stack you have a bigger
problem, so I am not sure if this hardening is really helpful here.

> 
> Looking at the disassembly of __clone, I do see some bltlr+ and
> bnslr+ instructions following the restore the the LR, so it does
> seem to be used.  Maybe an error path?  So I don't think we can
> just remove the save/store of the LR altogether.  But you know this
> code better than I do though.
> 
> That said, in your commit c579f48edb "Remove cached PID/TID in clone",
> you removed the only use of r29, but you failed to remove the saving
> and restoring of r29 or the copy of r5 into r29.  That is serendipitous,
> as that leaves the non-volatile r29 free to use to save the LR across
> the scv instruction as Florian suggested, so ROP wouldn't be required
> at all.
> 
> clone3.S isn't as easy, since there is no saved/restored non-volatile
> reg already available, but we could add saving/restoring of r31 similar
> to clone.S and then use that to save/restore the LR.
> 
> Does the following untested patch to clone.S look correct to you?
> 
> I assume we don't care whether the kernel needs to use the same
> non-volatile register so it may save our LR value via the non-volatile
> reg on the kernel's stack frame, since that frame isn't accessible
> to user code to corrupt?
> 
> Peter
> 
> 
> 
> diff --git a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S
> index 164311d2bd..e57cb6e82e 100644
> --- a/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S
> +++ b/sysdeps/unix/sysv/linux/powerpc/powerpc64/clone.S
> @@ -56,7 +56,6 @@ ENTRY (__clone)
>  
>         /* Save fn, args, stack across syscall.  */
>         mr      r30,r3                  /* Function in r30.  */
> -       mr      r29,r5                  /* Flags in r29.  */
>         mr      r31,r6                  /* Argument in r31.  */
>  
>         /* 'flags' argument is first parameter to clone syscall.
> @@ -77,14 +76,12 @@ ENTRY (__clone)
>         CHECK_SCV_SUPPORT r28 0f
>         /* This is equivalent to DO_CALL_SCV, but we cannot use the macro here
>         because it uses CFI directives and we just called cfi_endproc.  */
> -       mflr    r9
> -       std     r9,FRAME_LR_SAVE(r1)
> +       mflr    r29
>         .machine "push"
>         .machine "power9"
>         scv     0
>         .machine "pop"
> -       ld      r9,FRAME_LR_SAVE(r1)
> -       mtlr    r9
> +       mtlr    r29
>  
>         /* Check for child process.  */
>         /* When using scv, error is indicated by negative r3.  */
> 
> 
> 
> 
>