* [PATCH 2/4] kvm: qemu: Fix leak of ioperm data
@ 2008-12-10 9:44 Han, Weidong
2008-12-10 10:07 ` Mark McLoughlin
0 siblings, 1 reply; 3+ messages in thread
From: Han, Weidong @ 2008-12-10 9:44 UTC (permalink / raw)
To: 'Avi Kivity'; +Cc: kvm@vger.kernel.org
[-- Attachment #1: Type: text/plain, Size: 2991 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 | 12 ++++++++++++
qemu/qemu-kvm.h | 12 +++++++-----
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 7240158..c72c57c 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..283bfa9 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -1145,6 +1145,18 @@ 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;
+
+ LIST_FOREACH(data, &ioperm_head, entries) {
+ if (data->start_port == start_port &&
+ data->num == num)
+ LIST_REMOVE(data, entries);
+ qemu_free(data);
+ }
+}
+
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: 3100 bytes --]
From 5e92167ad95abcade15c268090e111e8d31bf941 Mon Sep 17 00:00:00 2001
From: Weidong Han <weidong.han@intel.com>
Date: Wed, 10 Dec 2008 17:22:11 +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 | 12 ++++++++++++
qemu/qemu-kvm.h | 12 +++++++-----
3 files changed, 21 insertions(+), 5 deletions(-)
diff --git a/qemu/hw/device-assignment.c b/qemu/hw/device-assignment.c
index 7240158..c72c57c 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..283bfa9 100644
--- a/qemu/qemu-kvm.c
+++ b/qemu/qemu-kvm.c
@@ -1145,6 +1145,18 @@ 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;
+
+ LIST_FOREACH(data, &ioperm_head, entries) {
+ if (data->start_port == start_port &&
+ data->num == num)
+ LIST_REMOVE(data, entries);
+ qemu_free(data);
+ }
+}
+
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/4] kvm: qemu: Fix leak of ioperm data
2008-12-10 9:44 [PATCH 2/4] kvm: qemu: Fix leak of ioperm data Han, Weidong
@ 2008-12-10 10:07 ` Mark McLoughlin
2008-12-10 10:20 ` Han, Weidong
0 siblings, 1 reply; 3+ messages in thread
From: Mark McLoughlin @ 2008-12-10 10:07 UTC (permalink / raw)
To: Han, Weidong; +Cc: 'Avi Kivity', kvm@vger.kernel.org
On Wed, 2008-12-10 at 17:44 +0800, Han, Weidong wrote:
> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
> index 067cf03..283bfa9 100644
> --- a/qemu/qemu-kvm.c
> +++ b/qemu/qemu-kvm.c
> @@ -1145,6 +1145,18 @@ 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;
> +
> + LIST_FOREACH(data, &ioperm_head, entries) {
> + if (data->start_port == start_port &&
> + data->num == num)
> + LIST_REMOVE(data, entries);
> + qemu_free(data);
Two issues here:
1) You've a "use after free bug" here; you free the structure and
LIST_FOREACH de-references the pointer to it in order to obtain the
pointer to the next structure.
2) Even more serious, you're missing a set of braces so you're
actually freeing every 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.
^ permalink raw reply [flat|nested] 3+ messages in thread
* RE: [PATCH 2/4] kvm: qemu: Fix leak of ioperm data
2008-12-10 10:07 ` Mark McLoughlin
@ 2008-12-10 10:20 ` Han, Weidong
0 siblings, 0 replies; 3+ messages in thread
From: Han, Weidong @ 2008-12-10 10:20 UTC (permalink / raw)
To: 'Mark McLoughlin'
Cc: 'Avi Kivity', 'kvm@vger.kernel.org'
Mark McLoughlin wrote:
> On Wed, 2008-12-10 at 17:44 +0800, Han, Weidong wrote:
>
>> diff --git a/qemu/qemu-kvm.c b/qemu/qemu-kvm.c
>> index 067cf03..283bfa9 100644
>> --- a/qemu/qemu-kvm.c
>> +++ b/qemu/qemu-kvm.c
>> @@ -1145,6 +1145,18 @@ 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;
>> +
>> + LIST_FOREACH(data, &ioperm_head, entries) {
>> + if (data->start_port == start_port &&
>> + data->num == num)
>> + LIST_REMOVE(data, entries);
>> + qemu_free(data);
>
> Two issues here:
>
> 1) You've a "use after free bug" here; you free the structure and
> LIST_FOREACH de-references the pointer to it in order to obtain
> the pointer to the next structure.
>
> 2) Even more serious, you're missing a set of braces so you're
> actually freeing every 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;
> }
> }
>
will fix them in next version. Thanks.
Regards,
Weidong
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2008-12-10 10:20 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2008-12-10 9:44 [PATCH 2/4] kvm: qemu: Fix leak of ioperm data Han, Weidong
2008-12-10 10:07 ` Mark McLoughlin
2008-12-10 10:20 ` Han, Weidong
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox