| Message ID | ZMQVnqY+F+5sTNFd@p100 |
|---|---|
| State | New |
| Headers | show |
| Series | [v3] linux-user/armeb: Fix __kernel_cmpxchg() for armeb | expand |
On 7/28/23 12:23, Helge Deller wrote: > Commit 7f4f0d9ea870 ("linux-user/arm: Implement __kernel_cmpxchg with host > atomics") switched to use qatomic_cmpxchg() to swap a word with the memory > content, but missed to endianess-swap the oldval and newval values when > emulating an armeb CPU, which expects words to be stored in big endian in > the guest memory. > > The bug can be verified with qemu >= v7.0 on any little-endian host, when > starting the armeb binary of the upx program, which just hangs without > this patch. > > Signed-off-by: Helge Deller <deller@gmx.de> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-stable@nongnu.org > Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com> > Reported-by: John Reiser <jreiser@BitWagon.com> > Closes: https://github.com/upx/upx/issues/687 > > -- > v3: > - use tswap64() in arm_kernel_cmpxchg64_helper() [review from Richard] > - mention that bug exists since qemu v7.0 [info from Markus] > v2: > - add tswap32() in arm_kernel_cmpxchg64_helper() [bisected by John] Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 7/28/23 12:23, Helge Deller wrote: > + /* endianess-swap if emulating armeb */ > + oldval = tswap64(oldval); > + newval = tswap64(newval); Oh btw, it's not about arm vs armeb, but guest vs host. This also fixes armel on big-endian hosts. r~
On 7/28/23 13:51, Richard Henderson wrote: > On 7/28/23 12:23, Helge Deller wrote: >> + /* endianess-swap if emulating armeb */ >> + oldval = tswap64(oldval); >> + newval = tswap64(newval); > > Oh btw, it's not about arm vs armeb, but guest vs host. > This also fixes armel on big-endian hosts. Anyway, I adjusted the comment and queued. r~
On 7/28/23 22:57, Richard Henderson wrote: > On 7/28/23 13:51, Richard Henderson wrote: >> On 7/28/23 12:23, Helge Deller wrote: >>> + /* endianess-swap if emulating armeb */ >>> + oldval = tswap64(oldval); >>> + newval = tswap64(newval); >> >> Oh btw, it's not about arm vs armeb, but guest vs host. >> This also fixes armel on big-endian hosts. Even better: One patch fixes multiple issues :-) > Anyway, I adjusted the comment and queued. Thank you! Helge
On 28/7/23 21:23, Helge Deller wrote: > Commit 7f4f0d9ea870 ("linux-user/arm: Implement __kernel_cmpxchg with host > atomics") switched to use qatomic_cmpxchg() to swap a word with the memory > content, but missed to endianess-swap the oldval and newval values when > emulating an armeb CPU, which expects words to be stored in big endian in > the guest memory. > > The bug can be verified with qemu >= v7.0 on any little-endian host, when > starting the armeb binary of the upx program, which just hangs without > this patch. > > Signed-off-by: Helge Deller <deller@gmx.de> > Cc: Richard Henderson <richard.henderson@linaro.org> > Cc: Peter Maydell <peter.maydell@linaro.org> > Cc: qemu-stable@nongnu.org > Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com> > Reported-by: John Reiser <jreiser@BitWagon.com> > Closes: https://github.com/upx/upx/issues/687 > > -- > v3: > - use tswap64() in arm_kernel_cmpxchg64_helper() [review from Richard] > - mention that bug exists since qemu v7.0 [info from Markus] > v2: > - add tswap32() in arm_kernel_cmpxchg64_helper() [bisected by John] Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c index a992423257..f58154108e 100644 --- a/linux-user/arm/cpu_loop.c +++ b/linux-user/arm/cpu_loop.c @@ -117,8 +117,9 @@ static void arm_kernel_cmpxchg32_helper(CPUARMState *env) { uint32_t oldval, newval, val, addr, cpsr, *host_addr; - oldval = env->regs[0]; - newval = env->regs[1]; + /* endianess-swap if emulating armeb */ + oldval = tswap32(env->regs[0]); + newval = tswap32(env->regs[1]); addr = env->regs[2]; mmap_lock(); @@ -174,6 +175,10 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env) return; } + /* endianess-swap if emulating armeb */ + oldval = tswap64(oldval); + newval = tswap64(newval); + #ifdef CONFIG_ATOMIC64 val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval); cpsr = (val == oldval) * CPSR_C;
Commit 7f4f0d9ea870 ("linux-user/arm: Implement __kernel_cmpxchg with host atomics") switched to use qatomic_cmpxchg() to swap a word with the memory content, but missed to endianess-swap the oldval and newval values when emulating an armeb CPU, which expects words to be stored in big endian in the guest memory. The bug can be verified with qemu >= v7.0 on any little-endian host, when starting the armeb binary of the upx program, which just hangs without this patch. Signed-off-by: Helge Deller <deller@gmx.de> Cc: Richard Henderson <richard.henderson@linaro.org> Cc: Peter Maydell <peter.maydell@linaro.org> Cc: qemu-stable@nongnu.org Reported-by: "Markus F.X.J. Oberhumer" <markus@oberhumer.com> Reported-by: John Reiser <jreiser@BitWagon.com> Closes: https://github.com/upx/upx/issues/687 -- v3: - use tswap64() in arm_kernel_cmpxchg64_helper() [review from Richard] - mention that bug exists since qemu v7.0 [info from Markus] v2: - add tswap32() in arm_kernel_cmpxchg64_helper() [bisected by John]