qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[Qemu-devel] [PATCH v2] Do not delete BlockDriverState when deleting the


From: Ryan Harper
Subject: [Qemu-devel] [PATCH v2] Do not delete BlockDriverState when deleting the drive
Date: Mon, 7 Mar 2011 09:59:18 -0600
User-agent: Mutt/1.5.6+20040907i

When removing a drive from the host-side via drive_del we currently have the
following path:

drive_del
qemu_aio_flush()
bdrv_close()
drive_uninit()
bdrv_delete()

When we bdrv_delete() we end up qemu_free() the BlockDriverState pointer
however, the block devices retain a copy of this pointer, see
hw/virtio-blk.c:virtio_blk_init() where we s->bs = conf->bs.

We now have a use-after-free situation.  If the guest continues to issue IO
against the device, and we've reallocated the memory that the BlockDriverState
pointed at, then we will fail the bs->drv checks in the various bdrv_ methods.

To resolve this issue as simply as possible, we can chose to not actually
delete the BlockDriverState pointer.  Since bdrv_close() handles setting the drv
pointer to NULL, we just need to remove the BlockDriverState from the QLIST
that is used to enumerate the block devices.  This is currently handled within
bdrv_delete, so move this into it's own function, bdrv_remove().

The result is that we can now invoke drive_del, this closes the file descriptors
and sets BlockDriverState->drv to NULL which prevents futher IO to the device,
and since we do not free BlockDriverState, we don't have to worry about the copy
retained in the block devices.

Reported-by: Marcus Armbruster <address@hidden>
Signed-off-by: Ryan Harper <address@hidden>
---
v1->v2
 - NULL bs->device_name after removing from list to prevent
   second removal.

 block.c    |   12 +++++++++---
 block.h    |    1 +
 blockdev.c |    2 +-
 3 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/block.c b/block.c
index 1544d81..0df9942 100644
--- a/block.c
+++ b/block.c
@@ -697,14 +697,20 @@ void bdrv_close_all(void)
     }
 }
 
+void bdrv_remove(BlockDriverState *bs)
+{
+    if (bs->device_name[0] != '\0') {
+        QTAILQ_REMOVE(&bdrv_states, bs, list);
+    }
+    bs->device_name[0] = '\0';
+}
+
 void bdrv_delete(BlockDriverState *bs)
 {
     assert(!bs->peer);
 
     /* remove from list, if necessary */
-    if (bs->device_name[0] != '\0') {
-        QTAILQ_REMOVE(&bdrv_states, bs, list);
-    }
+    bdrv_remove(bs);
 
     bdrv_close(bs);
     if (bs->file != NULL) {
diff --git a/block.h b/block.h
index 5d78fc0..8447397 100644
--- a/block.h
+++ b/block.h
@@ -66,6 +66,7 @@ int bdrv_create(BlockDriver *drv, const char* filename,
     QEMUOptionParameter *options);
 int bdrv_create_file(const char* filename, QEMUOptionParameter *options);
 BlockDriverState *bdrv_new(const char *device_name);
+void bdrv_remove(BlockDriverState *bs);
 void bdrv_delete(BlockDriverState *bs);
 int bdrv_file_open(BlockDriverState **pbs, const char *filename, int flags);
 int bdrv_open(BlockDriverState *bs, const char *filename, int flags,
diff --git a/blockdev.c b/blockdev.c
index 0690cc8..1f57b50 100644
--- a/blockdev.c
+++ b/blockdev.c
@@ -760,7 +760,7 @@ int do_drive_del(Monitor *mon, const QDict *qdict, QObject 
**ret_data)
     }
 
     /* clean up host side */
-    drive_uninit(drive_get_by_blockdev(bs));
+    bdrv_remove(bs);
 
     return 0;
 }
-- 
1.7.1


-- 
Ryan Harper
Software Engineer; Linux Technology Center
IBM Corp., Austin, Tx
address@hidden



reply via email to

[Prev in Thread] Current Thread [Next in Thread]