[PATCH] microblaze: don't clobber r3/r4 restored by rt_sigreturn

[Ramin Moussavi]

ret_from_trap starts by storing the system call return values r3 and r4
back into the saved user pt_regs:

	C_ENTRY(ret_from_trap):
		swi	r3, r1, PT_R3
		swi	r4, r1, PT_R4

That is right for an ordinary system call, where r3 holds the return
value.  sys_rt_sigreturn() also returns through this path, but it has to
be transparent: restore_sigcontext() has just filled the whole pt_regs
from the signal frame, and every register must reach userspace exactly as
saved.  The two stores overwrite the restored r3/r4 with whatever the C
function left in those registers.

The stores exist because RESTORE_REGS reloads r3/r4 from pt_regs -- see
commit 36f6095419b1 ("microblaze: Save and restore r3/r4 in
SAVE/RESTORE_REGS macros"): "In ret_from_trap function we are saving
returning value from syscall to pt_regs on stack that's why we don't need
to save and restore these values before kernel functions".  Clobbering
r3/r4 is fine for a real system call, since they are the volatile ABI
return value registers and userspace cannot expect them to survive one.
rt_sigreturn is not a return from a system call, though: it restores a
state, and must leave every register alone.

Only r4 is actually lost, and that follows from how sigreturn is written
everywhere rather than from chance.  Architectures have
sys_rt_sigreturn() return the restored return value register precisely so
that this writeback stores the restored value -- arm returns
regs->ARM_r0, riscv and csky regs->a0, arc regs->r0.  microblaze does the
same, in restore_sigcontext():

	*rval_p = regs->r3;

returned by sys_rt_sigreturn().  But the macros treat r3 and r4 as a
pair, so ret_from_trap writes back two registers while a C function has
only one return value.  Nothing can carry the restored r4 into the live
r4, and nothing does.

Commit 791d0a169b91 ("microblaze: Fix sys_rt_sigreturn_wrapper")
introduced this in v2.6.37.  Before it, the wrapper compensated:

	C_ENTRY(sys_rt_sigreturn_wrapper):
		swi	r3, r1, PTO+PT_R3;
		swi	r4, r1, PTO+PT_R4;
		brlid	r15, sys_rt_sigreturn
		lwi	r3, r1, PTO+PT_R3;
		lwi	r4, r1, PTO+PT_R4;
		bri	ret_from_trap

The commit removed the save/reload because "_user_exception (syscall
handler) already setup return address".  The return address indeed did
not need setting up -- but switching brlid to brid also removed the only
point where r3/r4 were reloaded from pt_regs after the call, and the
stores in ret_from_trap have been overwriting them ever since.

Fix it the way the sibling paths already behave: ret_from_irq and
ret_from_exc restore the full register set and do not perform these
stores at all, because they are wrong for anything that is not a system
call return.  Point r15 at ret_from_trap so that the rtsd r15, 8 in
sys_rt_sigreturn lands at ret_from_trap + 8, past the two stores.
Restoring the old save/reload would also work, but it costs four memory
accesses and an extra branch to undo damage that is better not done.

Why this went unnoticed for 15 years: r3 and r4 are the ABI return value
registers and therefore volatile, so ordinary code rarely keeps anything
live in them across an arbitrary instruction -- and half of the writeback
is harmless anyway, because the return value idiom covers r3.  What is
left needs a signal to land inside a short window that keeps an address
in r4.  The
lwx/swx compare-and-swap retry loops gcc emits for atomics are such
windows; in the uClibc-ng libc.so alone, 50 of 364 such loops hold the
address in r4 (another 35 use r3 and are therefore harmless).  Hitting
that requires threads plus a signal storm, which is not a typical
microblaze workload.

Nothing about this is specific to one libc, or to atomics: any value the
compiler happens to keep in r4 across a signal is lost.  Atomics are just
where the windows are tightest and the register is chosen by the
compiler -- musl's a_cas passes the address as "r"(p), and glibc has no
microblaze atomic-machine.h, so it uses gcc's __atomic builtins, which
expand to the same lwx/swx retry loops.

Reproduced on qemu-system-microblazeel, machine petalogix-s3adsp1800,
Linux v7.0 built with microblazeel gcc 12.5.0, running the uClibc-ng
NPTL test tst-eintr1 (SIGUSR1 storm across worker threads while
__sync_val_compare_and_swap keeps the counter address in r4) in a loop of
40 iterations:

	unpatched: 6 of 40 iterations killed by SIGSEGV
	patched:   0 of 40

The kernel register dump of a failing iteration shows the signature --
a zero r4 and a fault at address 0, while r3 came back intact:

	tst-eintr1: potentially unexpected fatal signal 11.
	 r1=BFCF6BB4, r2=00000000, r3=480E160C, r4=00000000
	 ...
	 msr=000053A2, ear=00000000, esr=00000412, fsr=00000000

Fixes: 791d0a169b91 ("microblaze: Fix sys_rt_sigreturn_wrapper")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Ramin Moussavi <ramin.moussavi@xxxxxxxxx>
---
 arch/microblaze/kernel/entry.S | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/arch/microblaze/kernel/entry.S b/arch/microblaze/kernel/entry.S
index 582d7256d815..619b02a4806d 100644
--- a/arch/microblaze/kernel/entry.S
+++ b/arch/microblaze/kernel/entry.S
@@ -518,6 +518,13 @@ C_ENTRY(ret_from_kernel_thread):
 
 C_ENTRY(sys_rt_sigreturn_wrapper):
 	addik	r30, r0, 0		/* no restarts */
+	/*
+	 * rt_sigreturn restores the full register set from the signal
+	 * context, so it must skip the r3/r4 syscall-return stores at the
+	 * head of ret_from_trap which would otherwise overwrite the
+	 * just-restored r3/r4.  Return to ret_from_trap + 8 (past them).
+	 */
+	addik	r15, r0, ret_from_trap
 	brid	sys_rt_sigreturn	/* Do real work */
 	addik	r5, r1, 0;		/* add user context as 1st arg */
 
-- 
2.53.0