[Qemu-devel] [PATCH 0/2] ARM: fix crash/abort when setting invalid mode

[Qemu-devel] [PATCH 0/2] ARM: fix crash/abort when setting invalid mode

[Peter Maydell]

https://bugs.launchpad.net/qemu/+bug/607794 reports a problem
where qemu will segfault if you try to set CPSR to an invalid
mode via the gdb stub. These patches fix the segfault by not
using cpu_single_env when it might not be valid, and also avoid
the problem by not aborting in this situation. Instead we simply
ignore attempts to set the mode field to an invalid value.
(This case is UNPREDICTABLE so this is a legitimate choice, and
behaviour that can be provoked by guests or confused users at
the debugger shouldn't cause us to abort.)

Peter Maydell (2):
  target-arm: Don't use cpu_single_env in bank_number()
  target-arm: Ignore attempts to set invalid modes in CPSR

 target-arm/helper.c |   42 +++++++++++++++++++++++++++++++++++-------
 1 files changed, 35 insertions(+), 7 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCH 1/2] target-arm: Don't use cpu_single_env in bank_number()

[Peter Maydell]

Avoid using cpu_single_env in bank_number() -- if we were
called via the gdb stub reading or writing the CPSR then
it is NULL and we will segfault if we take the cpu_abort().

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 65f4fbf..5b994d5 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -642,7 +642,7 @@ uint32_t HELPER(get_r13_banked)(CPUState *env, uint32_t mode)
 extern int semihosting_enabled;
 
 /* Map CPU modes onto saved register banks.  */
-static inline int bank_number (int mode)
+static inline int bank_number(CPUState *env, int mode)
 {
     switch (mode) {
     case ARM_CPU_MODE_USR:
@@ -659,7 +659,7 @@ static inline int bank_number (int mode)
     case ARM_CPU_MODE_FIQ:
         return 5;
     }
-    cpu_abort(cpu_single_env, "Bad mode %x\n", mode);
+    cpu_abort(env, "Bad mode %x\n", mode);
     return -1;
 }
 
@@ -680,12 +680,12 @@ void switch_mode(CPUState *env, int mode)
         memcpy (env->regs + 8, env->fiq_regs, 5 * sizeof(uint32_t));
     }
 
-    i = bank_number(old_mode);
+    i = bank_number(env, old_mode);
     env->banked_r13[i] = env->regs[13];
     env->banked_r14[i] = env->regs[14];
     env->banked_spsr[i] = env->spsr;
 
-    i = bank_number(mode);
+    i = bank_number(env, mode);
     env->regs[13] = env->banked_r13[i];
     env->regs[14] = env->banked_r14[i];
     env->spsr = env->banked_spsr[i];
@@ -2125,7 +2125,7 @@ void HELPER(set_r13_banked)(CPUState *env, uint32_t mode, uint32_t val)
     if ((env->uncached_cpsr & CPSR_M) == mode) {
         env->regs[13] = val;
     } else {
-        env->banked_r13[bank_number(mode)] = val;
+        env->banked_r13[bank_number(env, mode)] = val;
     }
 }
 
@@ -2134,7 +2134,7 @@ uint32_t HELPER(get_r13_banked)(CPUState *env, uint32_t mode)
     if ((env->uncached_cpsr & CPSR_M) == mode) {
         return env->regs[13];
     } else {
-        return env->banked_r13[bank_number(mode)];
+        return env->banked_r13[bank_number(env, mode)];
     }
 }
 
-- 
1.7.5.4

[Qemu-devel] [PATCH 2/2] target-arm: Ignore attempts to set invalid modes in CPSR

[Peter Maydell]

Ignore attempts to set the CPSR mode field to an invalid value.
This is UNPREDICTABLE, but we should not cpu_abort() for things
a malicious guest (or a confused user on the gdbstub interface)
can provoke.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/helper.c |   30 +++++++++++++++++++++++++++++-
 1 files changed, 29 insertions(+), 1 deletions(-)

diff --git a/target-arm/helper.c b/target-arm/helper.c
index 5b994d5..261d547 100644
--- a/target-arm/helper.c
+++ b/target-arm/helper.c
@@ -463,6 +463,26 @@ void cpu_arm_close(CPUARMState *env)
     g_free(env);
 }
 
+static int bad_mode_switch(CPUState *env, int mode)
+{
+    /* Return true if it is not valid for us to switch to
+     * this CPU mode (ie all the UNPREDICTABLE cases in
+     * the ARM ARM CPSRWriteByInstr pseudocode).
+     */
+    switch (mode) {
+    case ARM_CPU_MODE_USR:
+    case ARM_CPU_MODE_SYS:
+    case ARM_CPU_MODE_SVC:
+    case ARM_CPU_MODE_ABT:
+    case ARM_CPU_MODE_UND:
+    case ARM_CPU_MODE_IRQ:
+    case ARM_CPU_MODE_FIQ:
+        return 0;
+    default:
+        return 1;
+    }
+}
+
 uint32_t cpsr_read(CPUARMState *env)
 {
     int ZF;
@@ -499,7 +519,15 @@ void cpsr_write(CPUARMState *env, uint32_t val, uint32_t mask)
     }
 
     if ((env->uncached_cpsr ^ val) & mask & CPSR_M) {
-        switch_mode(env, val & CPSR_M);
+        if (bad_mode_switch(env, val & CPSR_M)) {
+            /* Attempt to switch to an invalid mode: this is UNPREDICTABLE.
+             * We choose to ignore the attempt and leave the CPSR M field
+             * untouched.
+             */
+            mask &= ~CPSR_M;
+        } else {
+            switch_mode(env, val & CPSR_M);
+        }
     }
     mask &= ~CACHED_CPSR_BITS;
     env->uncached_cpsr = (env->uncached_cpsr & ~mask) | (val & mask);
-- 
1.7.5.4