[Qemu-devel] [security bug]code_gen_buffer can be overflowed

[Qemu-devel] [security bug]code_gen_buffer can be overflowed

[TeLeMan]

dyngen_code() can generate more than CODE_GEN_MAX_SIZE bytes, code_gen_buffer
can be overflowed. I hope this security bug will be fixed soon.
-- 
View this message in context: http://www.nabble.com/-security-bug-code_gen_buffer-can-be-overflowed-tf4886083.html#a13985284
Sent from the QEMU - Dev mailing list archive at Nabble.com.

Re: [Qemu-devel] [security bug]code_gen_buffer can be overflowed

[Blue Swirl]

On 11/28/07, TeLeMan <geleman@gmail.com> wrote:
>
> dyngen_code() can generate more than CODE_GEN_MAX_SIZE bytes, code_gen_buffer
> can be overflowed. I hope this security bug will be fixed soon.

Thank you for the analysis. It's true that cpu_gen_code does not pass
CODE_GEN_MAX_SIZE (65536) on to gen_intermediate_code and that should
be fixed. But gen_intermediate_code can only add OPC_MAX_SIZE (512 -
32) instructions more, so there is no security bug.

Re: [Qemu-devel] [security bug]code_gen_buffer can be overflowed

[TeLeMan]

Blue Swirl-2 wrote:
> 
> On 11/28/07, TeLeMan <geleman@gmail.com> wrote:
>>
>> dyngen_code() can generate more than CODE_GEN_MAX_SIZE bytes,
>> code_gen_buffer
>> can be overflowed. I hope this security bug will be fixed soon.
> 
> Thank you for the analysis. It's true that cpu_gen_code does not pass
> CODE_GEN_MAX_SIZE (65536) on to gen_intermediate_code and that should
> be fixed. But gen_intermediate_code can only add OPC_MAX_SIZE (512 -
> 32) instructions more, so there is no security bug.
> 
> 

This POC is a windows exe and was tested on QEMU v0.9.0 (Guest OS is Windows
XP SP2).
This overflow will overwrite the TranslationBlock buffer.

http://www.nabble.com/file/p14101223/qemu-dos.rar qemu-dos.rar 
-- 
View this message in context: http://www.nabble.com/-security-bug-code_gen_buffer-can-be-overflowed-tf4886083.html#a14101223
Sent from the QEMU - Dev mailing list archive at Nabble.com.

Re: [Qemu-devel] [security bug]code_gen_buffer can be overflowed

[Blue Swirl]

On 12/1/07, TeLeMan <geleman@gmail.com> wrote:
>
>
> Blue Swirl-2 wrote:
> >
> > On 11/28/07, TeLeMan <geleman@gmail.com> wrote:
> >>
> >> dyngen_code() can generate more than CODE_GEN_MAX_SIZE bytes,
> >> code_gen_buffer
> >> can be overflowed. I hope this security bug will be fixed soon.
> >
> > Thank you for the analysis. It's true that cpu_gen_code does not pass
> > CODE_GEN_MAX_SIZE (65536) on to gen_intermediate_code and that should
> > be fixed. But gen_intermediate_code can only add OPC_MAX_SIZE (512 -
> > 32) instructions more, so there is no security bug.
> >
> >
>
> This POC is a windows exe and was tested on QEMU v0.9.0 (Guest OS is Windows
> XP SP2).
> This overflow will overwrite the TranslationBlock buffer.
>
> http://www.nabble.com/file/p14101223/qemu-dos.rar qemu-dos.rar

I see my error, gen_intermediate_code produces ops, not host
instructions. For each op several host instructions are generated, for
Sparc32 maximum on my machine is 170 but for ARM this can be 840. In
the worst case, (512 - 32) * 840 = 403200 bytes are generated, thus a
buffer overflow is indeed possible.

I can see a few possible fixes for this.

The buffer size can be increased from 64k to 512k or the buffer can be
allocated dynamically after calculating the maximum instruction size.

OPC_BUF_SIZE can be decreased from 512 to 50.

All ops can be made smaller by introducing more helpers.

dyngen_code loop could check for buffer size.

Re: [Qemu-devel] [security bug]code_gen_buffer can be overflowed

[Blue Swirl]

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

On 12/1/07, Blue Swirl <blauwirbel@gmail.com> wrote:
> On 12/1/07, TeLeMan <geleman@gmail.com> wrote:
> >
> >
> > Blue Swirl-2 wrote:
> > >
> > > On 11/28/07, TeLeMan <geleman@gmail.com> wrote:
> > >>
> > >> dyngen_code() can generate more than CODE_GEN_MAX_SIZE bytes,
> > >> code_gen_buffer
> > >> can be overflowed. I hope this security bug will be fixed soon.
> > >
> > > Thank you for the analysis. It's true that cpu_gen_code does not pass
> > > CODE_GEN_MAX_SIZE (65536) on to gen_intermediate_code and that should
> > > be fixed. But gen_intermediate_code can only add OPC_MAX_SIZE (512 -
> > > 32) instructions more, so there is no security bug.
> > >
> > >
> >
> > This POC is a windows exe and was tested on QEMU v0.9.0 (Guest OS is Windows
> > XP SP2).
> > This overflow will overwrite the TranslationBlock buffer.
> >
> > http://www.nabble.com/file/p14101223/qemu-dos.rar qemu-dos.rar
>
> I see my error, gen_intermediate_code produces ops, not host
> instructions. For each op several host instructions are generated, for
> Sparc32 maximum on my machine is 170 but for ARM this can be 840. In
> the worst case, (512 - 32) * 840 = 403200 bytes are generated, thus a
> buffer overflow is indeed possible.
>
> I can see a few possible fixes for this.
>
> The buffer size can be increased from 64k to 512k or the buffer can be
> allocated dynamically after calculating the maximum instruction size.
>
> OPC_BUF_SIZE can be decreased from 512 to 50.
>
> All ops can be made smaller by introducing more helpers.
>
> dyngen_code loop could check for buffer size.

Actually the buffer size is OK, but the safety margin was not large
enough. In this patch the margin is calculated from maximum block
size.

GCC could calculate the maximum on compile time, but it doesn't, so
the code is not optimal. Any suggestions for more advanced CPP magic
to calculate the maximum of a list of constants?

The patch works for Sparc target on x86_64 host. I didn't test other
combinations, so especially ARM target on RISC hosts with larger
generated code (ia64?) and/or smaller CODE_GEN_BUFFER_SIZE (alpha)
should be checked. The maximum should not exceed the buffer size or no
code can be generated. In that case, also OPC_BUF_SIZE should be
decreased.

Because of the security aspects, I think it's better to commit this
pretty soon and not wait for the confirmation for all host/target
combinations. If some combination happens to break, it can be fixed
quickly.

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: fix_code_gen_of.diff --]
[-- Type: text/x-diff; name=fix_code_gen_of.diff, Size: 3466 bytes --]

Index: qemu/cpu-exec.c
===================================================================
--- qemu.orig/cpu-exec.c	2007-12-09 07:30:36.000000000 +0000
+++ qemu/cpu-exec.c	2007-12-09 07:32:56.000000000 +0000
@@ -133,7 +133,7 @@
     tb->tc_ptr = tc_ptr;
     tb->cs_base = cs_base;
     tb->flags = flags;
-    cpu_gen_code(env, tb, CODE_GEN_MAX_SIZE, &code_gen_size);
+    cpu_gen_code(env, tb, &code_gen_size);
     code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
 
     /* check next page if needed */
Index: qemu/exec-all.h
===================================================================
--- qemu.orig/exec-all.h	2007-12-09 07:14:24.000000000 +0000
+++ qemu/exec-all.h	2007-12-09 08:03:57.000000000 +0000
@@ -64,8 +64,9 @@
 int gen_intermediate_code(CPUState *env, struct TranslationBlock *tb);
 int gen_intermediate_code_pc(CPUState *env, struct TranslationBlock *tb);
 void dump_ops(const uint16_t *opc_buf, const uint32_t *opparam_buf);
+unsigned long code_gen_max_block_size(void);
 int cpu_gen_code(CPUState *env, struct TranslationBlock *tb,
-                 int max_code_size, int *gen_code_size_ptr);
+                 int *gen_code_size_ptr);
 int cpu_restore_state(struct TranslationBlock *tb,
                       CPUState *env, unsigned long searched_pc,
                       void *puc);
@@ -94,7 +95,6 @@
     return tlb_set_page_exec(env, vaddr, paddr, prot, mmu_idx, is_softmmu);
 }
 
-#define CODE_GEN_MAX_SIZE        65536
 #define CODE_GEN_ALIGN           16 /* must be >= of the size of a icache line */
 
 #define CODE_GEN_PHYS_HASH_BITS     15
Index: qemu/exec.c
===================================================================
--- qemu.orig/exec.c	2007-12-09 07:13:43.000000000 +0000
+++ qemu/exec.c	2007-12-09 07:58:53.000000000 +0000
@@ -56,7 +56,7 @@
 #endif
 
 /* threshold to flush the translated code buffer */
-#define CODE_GEN_BUFFER_MAX_SIZE (CODE_GEN_BUFFER_SIZE - CODE_GEN_MAX_SIZE)
+#define CODE_GEN_BUFFER_MAX_SIZE (CODE_GEN_BUFFER_SIZE - code_gen_max_block_size())
 
 #define SMC_BITMAP_USE_THRESHOLD 10
 
@@ -622,7 +622,7 @@
     tb->cs_base = cs_base;
     tb->flags = flags;
     tb->cflags = cflags;
-    cpu_gen_code(env, tb, CODE_GEN_MAX_SIZE, &code_gen_size);
+    cpu_gen_code(env, tb, &code_gen_size);
     code_gen_ptr = (void *)(((unsigned long)code_gen_ptr + code_gen_size + CODE_GEN_ALIGN - 1) & ~(CODE_GEN_ALIGN - 1));
 
     /* check next page if needed */
Index: qemu/translate-all.c
===================================================================
--- qemu.orig/translate-all.c	2007-12-09 07:13:49.000000000 +0000
+++ qemu/translate-all.c	2007-12-09 08:25:07.000000000 +0000
@@ -132,14 +132,27 @@
     }
 }
 
+unsigned long code_gen_max_block_size(void)
+{
+    static unsigned long max;
+
+    if (max == 0) {
+#define DEF(s, n, copy_size) max = copy_size > max? copy_size : max;
+#include "opc.h"
+#undef DEF
+        max *= OPC_MAX_SIZE;
+    }
+
+    return max;
+}
+
 /* return non zero if the very first instruction is invalid so that
    the virtual CPU can trigger an exception.
 
    '*gen_code_size_ptr' contains the size of the generated code (host
    code).
 */
-int cpu_gen_code(CPUState *env, TranslationBlock *tb,
-                 int max_code_size, int *gen_code_size_ptr)
+int cpu_gen_code(CPUState *env, TranslationBlock *tb, int *gen_code_size_ptr)
 {
     uint8_t *gen_code_buf;
     int gen_code_size;