[PATCH 2/4] [v2] kvm: qemu: Fix leak of ioperm data

[PATCH 2/4] [v2] kvm: qemu: Fix leak of ioperm data

[Han, Weidong]

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

Free ioperm data in free_assigned_device.
And also, define ioperm_data structure and declare related functions when USE_KVM_DEVICE_ASSIGNMENT is defined, because ioperm data is only used by device assignment.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 qemu/hw/device-assignment.c |    2 ++
 qemu/qemu-kvm.c             |   15 +++++++++++++++
 qemu/qemu-kvm.h             |   12 +++++++-----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 74f183a..04b6450 100644
--- a/qemu/hw/device-assignment.c
+++ b/qemu/hw/device-assignment.c
@@ -460,6 +460,8 @@ void free_assigned_device(AssignedDevInfo *adev)
             if (!pci_region->valid || !(pci_region->type & IORESOURCE_MEM))
                 continue;
 
+            kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
+
             if (region->u.r_virtbase) {
                 int ret = munmap(region->u.r_virtbase,
                                  (pci_region->size + 0xFFF) & 0xFFFFF000);
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 067cf03..604f84e 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -1145,6 +1145,21 @@ void kvm_add_ioperm_data(struct ioperm_data *data)
     LIST_INSERT_HEAD(&ioperm_head, data, entries);
 }
 
+void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num)
+{
+    struct ioperm_data *data;
+
+    data = LIST_FIRST(&ioperm_head);
+    while (data) {
+        if (data->start_port == start_port && data->num == num) {
+            LIST_REMOVE(data, entries);
+            qemu_free(data);
+        }
+
+        data = LIST_NEXT(data, entries);
+    }
+}
+
 void kvm_ioperm(CPUState *env, void *data)
 {
     if (kvm_enabled() && qemu_system_ready)
diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
index 90fadcd..3e86586 100644
--- a/qemu/qemu-kvm.h
+++ b/qemu/qemu-kvm.h
@@ -90,11 +90,6 @@ int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
 
 void qemu_kvm_system_reset_request(void);
 
-#ifdef USE_KVM_DEVICE_ASSIGNMENT
-void kvm_ioperm(CPUState *env, void *data);
-void kvm_arch_do_ioperm(void *_data);
-#endif
-
 #ifdef TARGET_PPC
 int handle_powerpc_dcr_read(int vcpu, uint32_t dcrn, uint32_t *data);
 int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
@@ -110,6 +105,7 @@ int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
 extern int kvm_allowed;
 extern kvm_context_t kvm_context;
 
+#ifdef USE_KVM_DEVICE_ASSIGNMENT
 struct ioperm_data {
     unsigned long start_port;
     unsigned long num;
@@ -117,6 +113,12 @@ struct ioperm_data {
     LIST_ENTRY(ioperm_data) entries;
 };
 
+void kvm_ioperm(CPUState *env, void *data);
+void kvm_add_ioperm_data(struct ioperm_data *data);
+void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num);
+void kvm_arch_do_ioperm(void *_data);
+#endif
+
 int qemu_kvm_has_sync_mmu(void);
 
 #define kvm_enabled() (kvm_allowed)
-- 
1.6.0.4

[-- Attachment #2: 0002-Fix-leak-of-ioperm-data.patch --]
[-- Type: application/octet-stream, Size: 3156 bytes --]

From 1c12982cd96d9b7f5bd06051c6fdbb1946f9ed0c Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Wed, 10 Dec 2008 21:16:56 +0800
Subject: [PATCH] Fix leak of ioperm data

Free ioperm data in free_assigned_device.
And also, define ioperm_data structure and declare related functions when USE_KVM_DEVICE_ASSIGNMENT is defined, because ioperm data is only used by device assignment.

Signed-off-by: Weidong Han <weidong.han@intel.com>
---
 qemu/hw/device-assignment.c |    2 ++
 qemu/qemu-kvm.c             |   15 +++++++++++++++
 qemu/qemu-kvm.h             |   12 +++++++-----
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 74f183a..04b6450 100644
--- a/qemu/hw/device-assignment.c
+++ b/qemu/hw/device-assignment.c
@@ -460,6 +460,8 @@ void free_assigned_device(AssignedDevInfo *adev)
             if (!pci_region->valid || !(pci_region->type & IORESOURCE_MEM))
                 continue;
 
+            kvm_remove_ioperm_data(region->u.r_baseport, region->r_size);
+
             if (region->u.r_virtbase) {
                 int ret = munmap(region->u.r_virtbase,
                                  (pci_region->size + 0xFFF) & 0xFFFFF000);
diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
index 067cf03..604f84e 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -1145,6 +1145,21 @@ void kvm_add_ioperm_data(struct ioperm_data *data)
     LIST_INSERT_HEAD(&ioperm_head, data, entries);
 }
 
+void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num)
+{
+    struct ioperm_data *data;
+
+    data = LIST_FIRST(&ioperm_head);
+    while (data) {
+        if (data->start_port == start_port && data->num == num) {
+            LIST_REMOVE(data, entries);
+            qemu_free(data);
+        }
+
+        data = LIST_NEXT(data, entries);
+    }
+}
+
 void kvm_ioperm(CPUState *env, void *data)
 {
     if (kvm_enabled() && qemu_system_ready)
diff --git a/qemu/qemu-kvm.h b/qemu/qemu-kvm.h
index 90fadcd..3e86586 100644
--- a/qemu/qemu-kvm.h
+++ b/qemu/qemu-kvm.h
@@ -90,11 +90,6 @@ int qemu_kvm_unregister_coalesced_mmio(target_phys_addr_t addr,
 
 void qemu_kvm_system_reset_request(void);
 
-#ifdef USE_KVM_DEVICE_ASSIGNMENT
-void kvm_ioperm(CPUState *env, void *data);
-void kvm_arch_do_ioperm(void *_data);
-#endif
-
 #ifdef TARGET_PPC
 int handle_powerpc_dcr_read(int vcpu, uint32_t dcrn, uint32_t *data);
 int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
@@ -110,6 +105,7 @@ int handle_powerpc_dcr_write(int vcpu,uint32_t dcrn, uint32_t data);
 extern int kvm_allowed;
 extern kvm_context_t kvm_context;
 
+#ifdef USE_KVM_DEVICE_ASSIGNMENT
 struct ioperm_data {
     unsigned long start_port;
     unsigned long num;
@@ -117,6 +113,12 @@ struct ioperm_data {
     LIST_ENTRY(ioperm_data) entries;
 };
 
+void kvm_ioperm(CPUState *env, void *data);
+void kvm_add_ioperm_data(struct ioperm_data *data);
+void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num);
+void kvm_arch_do_ioperm(void *_data);
+#endif
+
 int qemu_kvm_has_sync_mmu(void);
 
 #define kvm_enabled() (kvm_allowed)
-- 
1.6.0.4

Re: [PATCH 2/4] [v2] kvm: qemu: Fix leak of ioperm data

[Mark McLoughlin]

On Wed, 2008-12-10 at 21:22 +0800, Han, Weidong wrote:
> 
> +void kvm_remove_ioperm_data(unsigned long start_port, unsigned long num)
> +{
> +    struct ioperm_data *data;
> +
> +    data = LIST_FIRST(&ioperm_head);
> +    while (data) {
> +        if (data->start_port == start_port && data->num == num) {
> +            LIST_REMOVE(data, entries);
> +            qemu_free(data);
> +        }
> +
> +        data = LIST_NEXT(data, entries);
> +    }
> +}

Repeating what I said last time:

     You've a "use after free bug" here; you free the structure and
     LIST_NEXT de-references the pointer to it in order to obtain the
     pointer to the next structure.

What you need is:

{
    struct ioperm_data *data;

    data = LIST_FIRST(&ioperm_head);
    while (data) {
        struct ioperm_data *next = LIST_NEXT(data, entries);

        if (data->start_port == start_port && data->num == num) {
            LIST_REMOVE(data, entries);
            qemu_free(data);
        }

        data = next;
    }
}

Cheers,
Mark.

RE: [PATCH 2/4] [v2] kvm: qemu: Fix leak of ioperm data

[Han, Weidong]

Mark McLoughlin wrote:
> On Wed, 2008-12-10 at 21:22 +0800, Han, Weidong wrote:
>> 
>> +void kvm_remove_ioperm_data(unsigned long start_port, unsigned long
>> num) +{ +    struct ioperm_data *data;
>> +
>> +    data = LIST_FIRST(&ioperm_head);
>> +    while (data) {
>> +        if (data->start_port == start_port && data->num == num) {
>> +            LIST_REMOVE(data, entries);
>> +            qemu_free(data);
>> +        }
>> +
>> +        data = LIST_NEXT(data, entries);
>> +    }
>> +}
> 
> Repeating what I said last time:
> 
>      You've a "use after free bug" here; you free the structure and
>      LIST_NEXT de-references the pointer to it in order to obtain the
>      pointer to the next structure.
> 

my God, will fix it soon. Thanks.

Regards,
Weidong

> What you need is:
> 
> {
>     struct ioperm_data *data;
> 
>     data = LIST_FIRST(&ioperm_head);
>     while (data) {
>         struct ioperm_data *next = LIST_NEXT(data, entries);
> 
>         if (data->start_port == start_port && data->num == num) {
>             LIST_REMOVE(data, entries);
>             qemu_free(data);
>         }
> 
>         data = next;
>     }
> }
> 
> Cheers,
> Mark.

RE: [PATCH 2/4] [v2] kvm: qemu: Fix leak of ioperm data

[Han, Weidong]

Mark McLoughlin wrote:
> On Wed, 2008-12-10 at 21:22 +0800, Han, Weidong wrote:
>> 
>> +void kvm_remove_ioperm_data(unsigned long start_port, unsigned long
>> num) +{ +    struct ioperm_data *data;
>> +
>> +    data = LIST_FIRST(&ioperm_head);
>> +    while (data) {
>> +        if (data->start_port == start_port && data->num == num) {
>> +            LIST_REMOVE(data, entries);
>> +            qemu_free(data);
>> +        }
>> +
>> +        data = LIST_NEXT(data, entries);
>> +    }
>> +}
> 
> Repeating what I said last time:
> 
>      You've a "use after free bug" here; you free the structure and
>      LIST_NEXT de-references the pointer to it in order to obtain the
>      pointer to the next structure.
> 
> What you need is:
> 
> {
>     struct ioperm_data *data;
> 
>     data = LIST_FIRST(&ioperm_head);
>     while (data) {
>         struct ioperm_data *next = LIST_NEXT(data, entries);
> 
>         if (data->start_port == start_port && data->num == num) {
>             LIST_REMOVE(data, entries);
>             qemu_free(data);
>         }
> 
>         data = next;
>     }
> }
> 
> Cheers,
> Mark.