1. Patchew
  2. QEMU
  3. View series

[PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb

Helge Deller posted 1 patch 9 months, 1 week ago
Failed in applying to current master (apply log)
There is a newer version of this series
[PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb
Posted by Helge Deller 9 months, 1 week ago 
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.2 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

--
v2:
- add tswap32() in arm_kernel_cmpxchg64_helper()


diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
index a992423257..0907cd8c15 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 = tswap32(oldval);
+    newval = tswap32(newval);
+
 #ifdef CONFIG_ATOMIC64
     val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
     cpsr = (val == oldval) * CPSR_C;
Re: [PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb
Posted by Richard Henderson 9 months, 1 week ago 
On 7/27/23 21:54, Helge Deller wrote:
> @@ -174,6 +175,10 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
>           return;
>       }
> 
> +    /* endianess-swap if emulating armeb */
> +    oldval = tswap32(oldval);
> +    newval = tswap32(newval);

Must be tswap64.


r~
Re: [PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb
Posted by Helge Deller 9 months, 1 week ago 
On 7/28/23 17:35, Richard Henderson wrote:
> On 7/27/23 21:54, Helge Deller wrote:
>> @@ -174,6 +175,10 @@ static void arm_kernel_cmpxchg64_helper(CPUARMState *env)
>>           return;
>>       }
>>
>> +    /* endianess-swap if emulating armeb */
>> +    oldval = tswap32(oldval);
>> +    newval = tswap32(newval);
>
> Must be tswap64.

Oh, of course!
I send a v3.

Helge
Re: [PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb
Posted by Markus F.X.J. Oberhumer 9 months, 1 week ago 
The fix in arm_kernel_cmpxchg64_helper probably should use tswap64() instead
of tswap32().

~Markus

On 2023-07-28 06:54, 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.2 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
> 
> --
> v2:
> - add tswap32() in arm_kernel_cmpxchg64_helper()
> 
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index a992423257..0907cd8c15 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 = tswap32(oldval);
> +    newval = tswap32(newval);
> +
>  #ifdef CONFIG_ATOMIC64
>      val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
>      cpsr = (val == oldval) * CPSR_C;
> 

-- 
Markus Oberhumer, <markus@oberhumer.com>, http://www.oberhumer.com/
Re: [PATCH v2] linux-user/armeb: Fix __kernel_cmpxchg() for armeb
Posted by Markus F.X.J. Oberhumer 9 months, 1 week ago 
Nitpick: the bug was introduced between 6.2.0 and 7.0.0, so "qemu >= v7.0.0"

~Markus

On 2023-07-28 06:54, 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.2 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
> 
> --
> v2:
> - add tswap32() in arm_kernel_cmpxchg64_helper()
> 
> 
> diff --git a/linux-user/arm/cpu_loop.c b/linux-user/arm/cpu_loop.c
> index a992423257..0907cd8c15 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 = tswap32(oldval);
> +    newval = tswap32(newval);
> +
>  #ifdef CONFIG_ATOMIC64
>      val = qatomic_cmpxchg__nocheck(host_addr, oldval, newval);
>      cpsr = (val == oldval) * CPSR_C;
> 

-- 
Markus Oberhumer, <markus@oberhumer.com>, http://www.oberhumer.com/

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.