[Qemu-devel][BUG][PATCH] Fix crash in kvm.c

[Qemu-devel][BUG][PATCH] Fix crash in kvm.c

[Stefan Weil]

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

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


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

Fix crash with kvm enabled.

Signed-off-by: Stefan Weil <weil@mail.berlios.de> 

Index: target-i386/kvm.c
===================================================================
--- target-i386/kvm.c	(Revision 5889)
+++ target-i386/kvm.c	(Arbeitskopie)
@@ -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;

Re: [Qemu-devel][BUG][PATCH] Fix crash in kvm.c

[Anthony Liguori]

Stefan Weil wrote:
> 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.

> 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

> Stefan
>
>   

Re: [Qemu-devel][BUG][PATCH] Fix crash in kvm.c

[Stefan Weil]

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

Anthony Liguori schrieb:
> Stefan Weil wrote:
>> Anthony Liguori schrieb:
>>  
>>> Stefan Weil wrote:
>>>    
>>>> 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.
>>>     
>>
>> 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.
>>   
>
> 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



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

Fix crash with kvm enabled.

Signed-off-by: Stefan Weil <weil@mail.berlios.de> 

Index: target-i386/kvm.c
===================================================================
--- target-i386/kvm.c	(revision 5889)
+++ target-i386/kvm.c	(working copy)
@@ -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;

Re: [Qemu-devel][BUG][PATCH] Fix crash in kvm.c

[Anthony Liguori]

Stefan Weil wrote:
> 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

> Regards
> Stefan
>
>
>