Skip to contentSkip to conversation

Fix crash in kvm.c

PATCH Applied Stefan Weil 2 participants 4 messages Dec 5, 2008 – Dec 11, 2008 2 messages not in the archive

lore
Share & tools
lore

Stefan Weil

author
#0 lore raw

PATCH 1/1 Fix crash in kvm.c

I got a crash (array access out of bounds results in access fault) with the current Qemu trunk when kvm is enabled:

qemu -fda fd.img -cdrom cdrom.img -hda hda.img -hdb raw.img -m 256 -boot c -enable-kvm

Host is Debian x86_64, the crash occurs before any code is emulated.

With the patch, the emulation (Win 98) boots, but has problems with the display of
icons and the mouse cursor. Qemu displays lots of
"BUG: kvm_physical_sync_dirty_bitmap: invalid parameters" messages.

Stefan

Fix crash with kvm enabled.

Signed-off-by Stefan Weil <[email protected]>
kvm.c +5 -1
Index: target-i386/kvm.c
===================================================================
@@ -12,6 +12,7 @@
  *
  */
 
+#include <assert.h>
 #include <sys/types.h>
 #include <sys/ioctl.h>
 #include <sys/mman.h>

@@ -39,7 +40,8 @@
         struct kvm_cpuid cpuid;
         struct kvm_cpuid_entry entries[100];
     } __attribute__((packed)) cpuid_data;
-    int limit, i, cpuid_i;
+    int limit, cpuid_i;
+    unsigned i;
     uint32_t eax, ebx, ecx, edx;
 
     cpuid_i = 0;

@@ -49,6 +51,7 @@
 
     for (i = 0; i <= limit; i++) {
         struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+        assert(cpuid_i < 100);
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;

@@ -63,6 +66,7 @@
 
     for (i = 0x80000000; i <= limit; i++) {
         struct kvm_cpuid_entry *c = &cpuid_data.entries[cpuid_i++];
+        assert(cpuid_i < 100);
 
         cpu_x86_cpuid(env, i, &eax, &ebx, &ecx, &edx);
         c->function = i;
#1 lore raw
Quoted 8 lines · Stefan Weil

I got a crash (array access out of bounds results in access fault) with the current Qemu trunk when kvm is enabled:

qemu -fda fd.img -cdrom cdrom.img -hda hda.img -hdb raw.img -m 256 -boot c -enable-kvm

Host is Debian x86_64, the crash occurs before any code is emulated.

Is the patch incomplete, perhaps? It seems to just add asserts which shouldn't fix anything.

I don't think any leaf should return a max leaf greater than 100 elements so I'd be pretty surprised to see this happen. I'd really like to see the back trace to see which leaf is the problematic one and what the greatest leaf being reported is.

Quoted 5 lines

With the patch, the emulation (Win 98) boots, but has problems with the display of
icons and the mouse cursor. Qemu displays lots of
"BUG: kvm_physical_sync_dirty_bitmap: invalid parameters" messages.

It looks like Avi and Glauber found some bugs in this code. There are patches for kvm-userspace right now that need porting to QEMU. I'll take a look at that this weekend unless Glauber was already planning too.

Regards,

Anthony Liguori

Quoted 3 lines

Stefan

A message that is not in the archive (sent off-list or not yet received): [email protected]

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#4 lore raw
Quoted 23 lines · Anthony Liguori
Quoted 13 lines · Stefan Weil
Quoted 11 lines · Anthony Liguori
Quoted 9 lines · Stefan Weil

I got a crash (array access out of bounds results in access fault) with the current Qemu trunk when kvm is enabled:

qemu -fda fd.img -cdrom cdrom.img -hda hda.img -hdb raw.img -m 256 -boot
c -enable-kvm

Host is Debian x86_64, the crash occurs before any code is emulated.

Quoted 8 lines
Quoted 3 lines

Is the patch incomplete, perhaps? It seems to just add asserts which shouldn't fix anything.

The essential change was to replace "int i" by "unsigned i". i = 0x80000000 is negative, so the for loop went until there was an access fault.

Quoted 6 lines

We should change limit too, right?

Regards,

Anthony Liguori

Right, and cpuid_i, too. "uint32_t" is even better than "unsigned". Here is a new patch, now without assertions, so it can be applied to Qemu trunk.

Regards
Stefan

Fix crash with kvm enabled.

Signed-off-by Stefan Weil <[email protected]>
kvm.c +1 -1
Index: target-i386/kvm.c
===================================================================
@@ -39,7 +39,7 @@
         struct kvm_cpuid cpuid;
         struct kvm_cpuid_entry entries[100];
     } __attribute__((packed)) cpuid_data;
-    int limit, i, cpuid_i;
+    uint32_t limit, i, cpuid_i;
     uint32_t eax, ebx, ecx, edx;
 
     cpuid_i = 0;
Applied
#5 lore raw
Quoted 4 lines · Stefan Weil

Right, and cpuid_i, too. "uint32_t" is even better than "unsigned". Here is a new patch, now without assertions, so it can be applied to Qemu trunk.

Applied. Thanks.

Regards,

Anthony Liguori

Quoted 5 lines

Regards
Stefan