Re: [Qemu-devel] [Bug] [Patch] MIPS code fails at branch instruction

[Stefan Weil <weil@mail.berlios.de>]

[-- Attachment #1: Type: text/plain, Size: 2635 bytes --]

Thank you, Paul, for your explanation which clarified Thiemo's statement.

I now checked how my published test code could contribute to a DoS attack.

Current QEMU HEAD:

* The code "hangs" as I wrote before. This is from a user's point of view.
  "Hanging" means, that the test process runs in an infinite loop using any
  CPU time it can get in the virtual machine. QEMU uses all available
  CPU time from the host CPU.
  With single stepping enabled or in the debugger, the test code won't
  hang but give a random result.

Patched QEMU HEAD (see appended patch file):

* The code works in a well defined way. An optional message in the log file
  will show the faulty statement. It won't amount to a DoS because it
  is disabled by default.
  Using single stepping, the test code's result remains the same.

So the patch improves the situation. Although it does not model the real
behaviour of an AR7 cpu, it solved my problem with a Zyxel firmware.
Maybe you can apply at least part of it or even improve and extend it to
other branch operations.

Thank you
Stefan


Details of the patch

* show optional message when any branch bits in hflags are already set
  before a branch instruction is generated (so we have a branch in the
  delay slot)

* mask branch bits before setting new ones (implemented only for the jr
  statement because this was the one I needed and examined)
  - this part could be improved

* make gen_intermediate_code_internal static (might improve compiler
  optimizations and is completely unrelated to the other two changes)



Paul Brook wrote:
>>>> So an emulation has several options:
>>>>
>>>> 1. Show undefined behaviour (this is what it does today).
>>>> 2. Emulate the behaviour of existing CPUs as far as possible.
>>>> As different CPUs behave different, this must depend on the
>>>> current CPU.
>>>> 3. Display an error message.
>>> (3) is bad, as it amounts to a DoS.
>> DoS = Denial of Service? Then (1) is some kind of DoS, because QEMU hangs
>> with code which works on real hardware. I don't understand why an
>> error message (something printed to stdout or stderr like other boot
>> messages of QEMU) amounts to a DoS.
>
> It's not the same thing at all. In both cases buggy code crashes. I
> expect
> this could also happen on a fair proportion of real MIPS hardware. It may
> even happen on AR7 hardware is a interrupt or fault happens to trigger
> at the
> wrong time.
>
> With (1) the buggy program crashes, and the rest of the machine keeps
> going.
> With (3) an unprivileged user can effectively bring the whole machine
> down
> just by executing invalid code sequences.
>
> Paul

[-- Attachment #2: translate.patch --]
[-- Type: text/x-diff, Size: 1280 bytes --]

Index: target-mips/translate.c
===================================================================
RCS file: /sources/qemu/qemu/target-mips/translate.c,v
retrieving revision 1.37
diff -u -b -B -u -r1.37 translate.c
--- target-mips/translate.c	18 Mar 2007 00:30:29 -0000	1.37
+++ target-mips/translate.c	19 Mar 2007 20:26:31 -0000
@@ -1371,6 +1371,13 @@
     target_ulong btarget;
     int blink, bcond;
 
+    if (ctx->hflags & MIPS_HFLAG_BMASK) {
+        if (loglevel & CPU_LOG_TB_IN_ASM) {
+            fprintf(logfile,
+                    "undefined branch in delay slot at pc 0x%08x\n", ctx->pc);
+        }
+    }
+
     btarget = -1;
     blink = 0;
     bcond = 0;
@@ -1480,7 +1487,7 @@
             MIPS_DEBUG("jal %08x", btarget);
             break;
         case OPC_JR:
-            ctx->hflags |= MIPS_HFLAG_BR;
+            ctx->hflags = ((ctx->hflags & ~MIPS_HFLAG_BMASK) | MIPS_HFLAG_BR);
             MIPS_DEBUG("jr %s", regnames[rs]);
             break;
         case OPC_JALR:
@@ -4999,7 +5006,7 @@
     }
 }
 
-int gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
+static int gen_intermediate_code_internal (CPUState *env, TranslationBlock *tb,
                                     int search_pc)
 {
     DisasContext ctx, *ctxp = &ctx;