[musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Alex Rønne Petersen]

This just mirrors what is done in the start code for the affected ports, as well
as what is already done for the three x86 ports. For consistency, I also changed
the x86 ports and the powerpc port to have the child process portion at the end
of clone().

Clearing the frame pointer helps protect FP-based unwinders which have no way of
knowing that the FP register should be considered undefined in the child process
portion of clone(). In practice, we found this change to be necessary when
running the Zig standard library tests under qemu-aarch64_be with musl linked.
---
 src/thread/aarch64/clone.s     |  3 ++-
 src/thread/arm/clone.s         |  3 ++-
 src/thread/i386/clone.s        | 17 ++++++++---------
 src/thread/loongarch64/clone.s |  1 +
 src/thread/m68k/clone.s        |  3 ++-
 src/thread/microblaze/clone.s  |  3 ++-
 src/thread/mips/clone.s        |  3 ++-
 src/thread/mips64/clone.s      |  3 ++-
 src/thread/mipsn32/clone.s     |  3 ++-
 src/thread/or1k/clone.s        |  3 ++-
 src/thread/powerpc/clone.s     | 21 +++++++++------------
 src/thread/x32/clone.s         |  6 +++---
 src/thread/x86_64/clone.s      |  6 +++---
 13 files changed, 40 insertions(+), 35 deletions(-)

diff --git a/src/thread/aarch64/clone.s b/src/thread/aarch64/clone.s
index e3c83395..9ac272bd 100644
--- a/src/thread/aarch64/clone.s
+++ b/src/thread/aarch64/clone.s
@@ -24,7 +24,8 @@ __clone:
 	// parent
 	ret
 	// child
-1:	ldp x1,x0,[sp],#16
+1:	mov fp, 0
+	ldp x1,x0,[sp],#16
 	blr x1
 	mov x8,#93 // SYS_exit
 	svc #0
diff --git a/src/thread/arm/clone.s b/src/thread/arm/clone.s
index bb0965da..4ff0c0e8 100644
--- a/src/thread/arm/clone.s
+++ b/src/thread/arm/clone.s
@@ -19,7 +19,8 @@ __clone:
 	ldmfd sp!,{r4,r5,r6,r7}
 	bx lr
 
-1:	mov r0,r6
+1:	mov fp,#0
+	mov r0,r6
 	bl 3f
 2:	mov r7,#1
 	svc 0
diff --git a/src/thread/i386/clone.s b/src/thread/i386/clone.s
index e237d3c6..f89ddd9d 100644
--- a/src/thread/i386/clone.s
+++ b/src/thread/i386/clone.s
@@ -30,9 +30,15 @@ __clone:
 	mov 8(%ebp),%ebp
 	int $128
 	test %eax,%eax
-	jnz 1f
+	jz 1f
+	add $16,%esp
+	pop %edi
+	pop %esi
+	pop %ebx
+	pop %ebp
+	ret
 
-	mov %ebp,%eax
+1:	mov %ebp,%eax
 	xor %ebp,%ebp
 	call *%eax
 	mov %eax,%ebx
@@ -40,10 +46,3 @@ __clone:
 	inc %eax
 	int $128
 	hlt
-
-1:	add $16,%esp
-	pop %edi
-	pop %esi
-	pop %ebx
-	pop %ebp
-	ret
diff --git a/src/thread/loongarch64/clone.s b/src/thread/loongarch64/clone.s
index a165b365..cb4aacfc 100644
--- a/src/thread/loongarch64/clone.s
+++ b/src/thread/loongarch64/clone.s
@@ -22,6 +22,7 @@ __clone:
 	beqz    $a0, 1f         # whether child process
 	jirl    $zero, $ra, 0   # parent process return
 1:
+	move    $fp, $zero
 	ld.d    $t8, $sp, 0     # function pointer
 	ld.d    $a0, $sp, 8     # argument pointer
 	jirl    $ra, $t8, 0     # call the user's function
diff --git a/src/thread/m68k/clone.s b/src/thread/m68k/clone.s
index f6dfa06f..42ec19f7 100644
--- a/src/thread/m68k/clone.s
+++ b/src/thread/m68k/clone.s
@@ -18,7 +18,8 @@ __clone:
 	beq 1f
 	movem.l (%sp)+,%d2-%d5
 	rts
-1:	move.l %a1,-(%sp)
+1:	suba.l %%fp,%%fp
+	move.l %a1,-(%sp)
 	jsr (%a0)
 	move.l #1,%d0
 	trap #0
diff --git a/src/thread/microblaze/clone.s b/src/thread/microblaze/clone.s
index b68cc5fc..64e3f074 100644
--- a/src/thread/microblaze/clone.s
+++ b/src/thread/microblaze/clone.s
@@ -22,7 +22,8 @@ __clone:
 	rtsd    r15, 8
 	nop
 
-1:	lwi     r3, r1, 0
+1:	add     r19, r0, r0
+	lwi     r3, r1, 0
 	lwi     r5, r1, 4
 	brald   r15, r3
 	nop
diff --git a/src/thread/mips/clone.s b/src/thread/mips/clone.s
index 04463385..229b987e 100644
--- a/src/thread/mips/clone.s
+++ b/src/thread/mips/clone.s
@@ -27,7 +27,8 @@ __clone:
 	addu $sp, $sp, 16
 	jr $ra
 	nop
-1:	lw $25, 0($sp)
+1:	move $fp, $0
+	lw $25, 0($sp)
 	lw $4, 4($sp)
 	jalr $25
 	nop
diff --git a/src/thread/mips64/clone.s b/src/thread/mips64/clone.s
index 2d86899a..8de3db6c 100644
--- a/src/thread/mips64/clone.s
+++ b/src/thread/mips64/clone.s
@@ -25,7 +25,8 @@ __clone:
 	nop
 	jr	$ra
 	nop
-1:	ld	$25, 0($sp)	# function pointer
+1:	move	$fp, $0
+	ld	$25, 0($sp)	# function pointer
 	ld	$4, 8($sp)	# argument pointer
 	jalr	$25		# call the user's function
 	nop
diff --git a/src/thread/mipsn32/clone.s b/src/thread/mipsn32/clone.s
index 4d3c8c7a..9571231a 100644
--- a/src/thread/mipsn32/clone.s
+++ b/src/thread/mipsn32/clone.s
@@ -25,7 +25,8 @@ __clone:
 	nop
 	jr	$ra
 	nop
-1:	lw	$25, 0($sp)	# function pointer
+1:	move	$fp, $0
+	lw	$25, 0($sp)	# function pointer
 	lw	$4, 4($sp)	# argument pointer
 	jalr	$25		# call the user's function
 	nop
diff --git a/src/thread/or1k/clone.s b/src/thread/or1k/clone.s
index 2473ac20..05c55c69 100644
--- a/src/thread/or1k/clone.s
+++ b/src/thread/or1k/clone.s
@@ -23,7 +23,8 @@ __clone:
 	l.jr	r9
 	 l.nop
 
-1:	l.lwz	r11, 0(r1)
+1:	l.ori	r2, r0, 0
+	l.lwz	r11, 0(r1)
 	l.jalr	r11
 	 l.lwz	r3, 4(r1)
 
diff --git a/src/thread/powerpc/clone.s b/src/thread/powerpc/clone.s
index da13f446..019fd62a 100644
--- a/src/thread/powerpc/clone.s
+++ b/src/thread/powerpc/clone.s
@@ -48,9 +48,16 @@ neg 3, 3 #negate the result (errno)
 # compare sc result with 0
 cmpwi cr7, 3, 0
 
-# if not 0, jump to end
-bne cr7, 2f
+# if not 0, restore stack and return
+beq cr7, 2f
 
+lwz 30, 0(1)
+lwz 31, 4(1)
+addi 1, 1, 16
+
+blr
+
+2:
 #else: we're the child
 #call funcptr: move arg (d) into r3
 mr 3, 31
@@ -61,13 +68,3 @@ bctrl
 # mov SYS_exit into r0 (the exit param is already in r3)
 li 0, 1
 sc
-
-2:
-
-# restore stack
-lwz 30, 0(1)
-lwz 31, 4(1)
-addi 1, 1, 16
-
-blr
-
diff --git a/src/thread/x32/clone.s b/src/thread/x32/clone.s
index b870880f..d7134526 100644
--- a/src/thread/x32/clone.s
+++ b/src/thread/x32/clone.s
@@ -15,12 +15,12 @@ __clone:
 	mov %rcx,(%rsi)
 	syscall
 	test %eax,%eax
-	jnz 1f
-	xor %ebp,%ebp
+	jz 1f
+	ret
+1:	xor %ebp,%ebp
 	pop %rdi
 	call *%r9
 	mov %eax,%edi
 	movl $0x4000003c,%eax /* SYS_exit */
 	syscall
 	hlt
-1:	ret
diff --git a/src/thread/x86_64/clone.s b/src/thread/x86_64/clone.s
index 6e47bc0a..3c30220e 100644
--- a/src/thread/x86_64/clone.s
+++ b/src/thread/x86_64/clone.s
@@ -16,8 +16,9 @@ __clone:
 	mov %rcx,(%rsi)
 	syscall
 	test %eax,%eax
-	jnz 1f
-	xor %ebp,%ebp
+	jz 1f
+	ret
+1:	xor %ebp,%ebp
 	pop %rdi
 	call *%r9
 	mov %eax,%edi
@@ -25,4 +26,3 @@ __clone:
 	mov $60,%al
 	syscall
 	hlt
-1:	ret
-- 
2.40.1

[musl] Re: [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Alex Rønne Petersen]

On Fri, Dec 6, 2024 at 7:49 PM Alex Rønne Petersen <alex@alexrp.com> wrote:
>
> This just mirrors what is done in the start code for the affected ports, as well
> as what is already done for the three x86 ports. For consistency, I also changed
> the x86 ports and the powerpc port to have the child process portion at the end
> of clone().
>
> Clearing the frame pointer helps protect FP-based unwinders which have no way of
> knowing that the FP register should be considered undefined in the child process
> portion of clone(). In practice, we found this change to be necessary when
> running the Zig standard library tests under qemu-aarch64_be with musl linked.

I made some observations in musl's startup code while putting this
patch together. As far as I can tell, musl doesn't clear...

* r7 (non-ABI FP) on arm;
* $ra on loongarch64;
* fp and ra on riscv32/riscv64;
* r9 (LR) on or1k;
* r31 (non-ABI FP) on powerpc/powerpc64;
* r11 (non-ABI FP) and r14 (LR) on s390x;
* r14 (FP) and pr (LR) on sh.

Accordingly, I didn't make such changes to clone().

The question is, *should* musl clear these? For what it's worth, in
Zig's non-libc startup code, we're clearing all link and frame pointer
registers, both ABI and non-ABI, because it doesn't really cost us
much and we'd rather be safe than sorry. I think it's also notable
that musl *does* clear the non-ABI FP register (r14) on microblaze, so
there's some precedent there.

Here's what glibc does, for comparison:

* arm: Same as musl.
* loongarch64: Sets $ra to a sensible return address so it can abort().
* riscv32/riscv64: Same as musl.
* or1k: Clears r2 (same as musl), clears r9.
* powerpc/powerpc64: Same as musl.
* s390x: Sets r14 to a sensible return address so it can trigger a
crash right after the brasl.
* sh: Clears r14, sets pr to a sensible return address to it can abort().

If changes are desired for these ports, I can do a separate patch to
change the startup code and clone() code for them.

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Thorsten Glaser]

On Fri, 6 Dec 2024, Alex Rønne Petersen wrote:

>Clearing the frame pointer helps protect FP-based unwinders which have no way of
>knowing that the FP register should be considered undefined in the child process
>portion of clone().

Huh, why?

I thought they would both still be useful and needed (so one can trace
the new process / thread back to from where it was cloned and beyond).

bye,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Markus Wichmann]

Am Fri, Dec 06, 2024 at 09:42:00PM +0100 schrieb Thorsten Glaser:
> On Fri, 6 Dec 2024, Alex Rønne Petersen wrote:
>
> >Clearing the frame pointer helps protect FP-based unwinders which have no way of
> >knowing that the FP register should be considered undefined in the child process
> >portion of clone().
>
> Huh, why?
>
> I thought they would both still be useful and needed (so one can trace
> the new process / thread back to from where it was cloned and beyond).
>
> bye,
> //mirabilos

The "parent" thread can return from the method that called clone(). Then
the illusory back link in the child points to dead memory that may be
overwritten concurrently. It also points to a different stack that may
be freed concurrently. In either case, a back tracer must be stopped
from continuing to trace at the clone() level.

Ciao,
Markus

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Thorsten Glaser]

On Fri, 6 Dec 2024, Markus Wichmann wrote:

>The "parent" thread can return from the method that called clone(). Then

Aaaah, right, for threads indeed.

So there’s really no way to trace threads to even the function
that creates them, let alone further? Ouch!

Thanks,
//mirabilos
-- 
15:41⎜<Lo-lan-do:#fusionforge> Somebody write a testsuite for helloworld :-)

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Rich Felker]

On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> This just mirrors what is done in the start code for the affected ports, as well
> as what is already done for the three x86 ports. For consistency, I also changed
> the x86 ports and the powerpc port to have the child process portion at the end
> of clone().

Can you submit without this independent change? Readers of history
should be able to confirm that the patch does not make any other
functional change, and it's hard to do that when reorganizing code is
mixed with the change.

Rich

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Alex Rønne Petersen]

On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@libc.org> wrote:
>
> On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > This just mirrors what is done in the start code for the affected ports, as well
> > as what is already done for the three x86 ports. For consistency, I also changed
> > the x86 ports and the powerpc port to have the child process portion at the end
> > of clone().
>
> Can you submit without this independent change? Readers of history
> should be able to confirm that the patch does not make any other
> functional change, and it's hard to do that when reorganizing code is
> mixed with the change.

Can do. Should I send it as a separate patch or would you rather I
just omit that particular change?

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Rich Felker]

On Sat, Dec 07, 2024 at 07:47:38PM +0100, Alex Rønne Petersen wrote:
> On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@libc.org> wrote:
> >
> > On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > > This just mirrors what is done in the start code for the affected ports, as well
> > > as what is already done for the three x86 ports. For consistency, I also changed
> > > the x86 ports and the powerpc port to have the child process portion at the end
> > > of clone().
> >
> > Can you submit without this independent change? Readers of history
> > should be able to confirm that the patch does not make any other
> > functional change, and it's hard to do that when reorganizing code is
> > mixed with the change.
> 
> Can do. Should I send it as a separate patch or would you rather I
> just omit that particular change?

My leaning would be omit. For changes like this, my heuristic is to
ask (1) whether, if I were a user, I would be angry if a regression
somehow got introduced through the change (which offered nothing of
value to me as a user in return for the risk), and (2) whether, as a
reviewer reading git log looking for accidentally or intentionally
introduced bugs, I'd appreciate spending my time reading the change.

Rich

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Alex Rønne Petersen]

On Mon, Dec 9, 2024 at 1:30 PM Rich Felker <dalias@libc.org> wrote:
>
> On Sat, Dec 07, 2024 at 07:47:38PM +0100, Alex Rønne Petersen wrote:
> > On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@libc.org> wrote:
> > >
> > > On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > > > This just mirrors what is done in the start code for the affected ports, as well
> > > > as what is already done for the three x86 ports. For consistency, I also changed
> > > > the x86 ports and the powerpc port to have the child process portion at the end
> > > > of clone().
> > >
> > > Can you submit without this independent change? Readers of history
> > > should be able to confirm that the patch does not make any other
> > > functional change, and it's hard to do that when reorganizing code is
> > > mixed with the change.
> >
> > Can do. Should I send it as a separate patch or would you rather I
> > just omit that particular change?
>
> My leaning would be omit. For changes like this, my heuristic is to
> ask (1) whether, if I were a user, I would be angry if a regression
> somehow got introduced through the change (which offered nothing of
> value to me as a user in return for the risk), and (2) whether, as a
> reviewer reading git log looking for accidentally or intentionally
> introduced bugs, I'd appreciate spending my time reading the change.

Just for some background: The reason I included the change was that in
the equivalent Zig standard library code, we did exactly this because
we needed to add DWARF CFI annotations (.cfi_undefined) to the child
process portion of clone(). If the child process portion isn't moved
last, then the CFI annotations end up covering parent process code
that they shouldn't. My thinking was that it might make sense to
similarly rearrange the code in musl just in case CFI annotations ever
need to be added there.

That said, I take your point re: risk, so omitting makes sense to me.

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Rich Felker]

On Mon, Dec 09, 2024 at 02:04:11PM +0100, Alex Rønne Petersen wrote:
> On Mon, Dec 9, 2024 at 1:30 PM Rich Felker <dalias@libc.org> wrote:
> >
> > On Sat, Dec 07, 2024 at 07:47:38PM +0100, Alex Rønne Petersen wrote:
> > > On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@libc.org> wrote:
> > > >
> > > > On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > > > > This just mirrors what is done in the start code for the affected ports, as well
> > > > > as what is already done for the three x86 ports. For consistency, I also changed
> > > > > the x86 ports and the powerpc port to have the child process portion at the end
> > > > > of clone().
> > > >
> > > > Can you submit without this independent change? Readers of history
> > > > should be able to confirm that the patch does not make any other
> > > > functional change, and it's hard to do that when reorganizing code is
> > > > mixed with the change.
> > >
> > > Can do. Should I send it as a separate patch or would you rather I
> > > just omit that particular change?
> >
> > My leaning would be omit. For changes like this, my heuristic is to
> > ask (1) whether, if I were a user, I would be angry if a regression
> > somehow got introduced through the change (which offered nothing of
> > value to me as a user in return for the risk), and (2) whether, as a
> > reviewer reading git log looking for accidentally or intentionally
> > introduced bugs, I'd appreciate spending my time reading the change.
> 
> Just for some background: The reason I included the change was that in
> the equivalent Zig standard library code, we did exactly this because
> we needed to add DWARF CFI annotations (.cfi_undefined) to the child
> process portion of clone(). If the child process portion isn't moved
> last, then the CFI annotations end up covering parent process code
> that they shouldn't. My thinking was that it might make sense to
> similarly rearrange the code in musl just in case CFI annotations ever
> need to be added there.

Thanks for explaning the motivation. I think it would make sense to do
that change with adding CFI if the latter becomes something we want to
do at some point (it might be wanted just as debug info if nothing
else?) but not unless/until that.

Rich

Re: [musl] [PATCH] clone: clear the frame pointer in the child process on relevant ports

[Alex Rønne Petersen]

On Mon, Dec 9, 2024 at 2:07 PM Rich Felker <dalias@libc.org> wrote:
>
> On Mon, Dec 09, 2024 at 02:04:11PM +0100, Alex Rønne Petersen wrote:
> > On Mon, Dec 9, 2024 at 1:30 PM Rich Felker <dalias@libc.org> wrote:
> > >
> > > On Sat, Dec 07, 2024 at 07:47:38PM +0100, Alex Rønne Petersen wrote:
> > > > On Sat, Dec 7, 2024 at 6:04 AM Rich Felker <dalias@libc.org> wrote:
> > > > >
> > > > > On Fri, Dec 06, 2024 at 07:48:53PM +0100, Alex Rønne Petersen wrote:
> > > > > > This just mirrors what is done in the start code for the affected ports, as well
> > > > > > as what is already done for the three x86 ports. For consistency, I also changed
> > > > > > the x86 ports and the powerpc port to have the child process portion at the end
> > > > > > of clone().
> > > > >
> > > > > Can you submit without this independent change? Readers of history
> > > > > should be able to confirm that the patch does not make any other
> > > > > functional change, and it's hard to do that when reorganizing code is
> > > > > mixed with the change.
> > > >
> > > > Can do. Should I send it as a separate patch or would you rather I
> > > > just omit that particular change?
> > >
> > > My leaning would be omit. For changes like this, my heuristic is to
> > > ask (1) whether, if I were a user, I would be angry if a regression
> > > somehow got introduced through the change (which offered nothing of
> > > value to me as a user in return for the risk), and (2) whether, as a
> > > reviewer reading git log looking for accidentally or intentionally
> > > introduced bugs, I'd appreciate spending my time reading the change.
> >
> > Just for some background: The reason I included the change was that in
> > the equivalent Zig standard library code, we did exactly this because
> > we needed to add DWARF CFI annotations (.cfi_undefined) to the child
> > process portion of clone(). If the child process portion isn't moved
> > last, then the CFI annotations end up covering parent process code
> > that they shouldn't. My thinking was that it might make sense to
> > similarly rearrange the code in musl just in case CFI annotations ever
> > need to be added there.
>
> Thanks for explaning the motivation. I think it would make sense to do
> that change with adding CFI if the latter becomes something we want to
> do at some point (it might be wanted just as debug info if nothing
> else?) but not unless/until that.

It would also be for unwind protection; it just protects DWARF-based
unwinders, whereas physically clearing the frame pointer protects
FP-based unwinders. For example, on x86 it'd be .cfi undefined eip, on
aarch64 it'd be .cfi_undefined x30, etc. If there's appetite for this
in musl, I'd be happy to send a separate patch. (FWIW, glibc does this
as well.)