Changeset
block.c | 26 ++++++++++++++------------
1 file changed, 14 insertions(+), 12 deletions(-)
Git apply log
Switched to a new branch '20170714143548.32559-1-el13635@mail.ntua.gr'
Applying: block: fix dangling bs->explicit_options in block.c
Applying: block: fix leaks in bdrv_open_driver()
To https://github.com/patchew-project/qemu
 + e2b99ac...5288955 patchew/20170714143548.32559-1-el13635@mail.ntua.gr -> patchew/20170714143548.32559-1-el13635@mail.ntua.gr (forced update)
Test passed: FreeBSD

loading

Test passed: s390x

loading

Test passed: checkpatch

loading

Test passed: docker

loading

[Qemu-devel] [PATCH v3 0/2] fix leaks in bdrv_open_driver()
Posted by Manos Pitsidianakis, 53 weeks ago
v3:
 new commit: block: fix dangling bs->explicit_options in block.c
 rework error paths in bdrv_open_driver()
v2:
 move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
 do not set bs->drv to NULL if open succeeds

Manos Pitsidianakis (2):
  block: fix dangling bs->explicit_options in block.c
  block: fix leaks in bdrv_open_driver()

 block.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

-- 
2.11.0


Re: [Qemu-devel] [PATCH v3 0/2] fix leaks in bdrv_open_driver()
Posted by Kevin Wolf, 51 weeks ago
Am 14.07.2017 um 16:35 hat Manos Pitsidianakis geschrieben:
> v3:
>  new commit: block: fix dangling bs->explicit_options in block.c
>  rework error paths in bdrv_open_driver()
> v2:
>  move bdrv_unref_child(bs, bs->file) to bdrv_open_driver
>  do not set bs->drv to NULL if open succeeds

Thanks, applied to the block branch.

Kevin

[Qemu-devel] [PATCH v3 1/2] block: fix dangling bs->explicit_options in block.c
Posted by Manos Pitsidianakis, 53 weeks ago
In some error paths it is possible to QDECREF a freed dangling
explicit_options, resulting in a heap overflow crash.  For example
bdrv_open_inherit()'s fail unrefs it, then calls bdrv_unref which calls
bdrv_close which also unrefs it.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/block.c b/block.c
index 98a9209371..96b912d229 100644
--- a/block.c
+++ b/block.c
@@ -2608,6 +2608,7 @@ fail:
     QDECREF(bs->options);
     QDECREF(options);
     bs->options = NULL;
+    bs->explicit_options = NULL;
     bdrv_unref(bs);
     error_propagate(errp, local_err);
     return NULL;
@@ -3087,6 +3088,7 @@ static void bdrv_close(BlockDriverState *bs)
         QDECREF(bs->options);
         QDECREF(bs->explicit_options);
         bs->options = NULL;
+        bs->explicit_options = NULL;
         QDECREF(bs->full_open_options);
         bs->full_open_options = NULL;
     }
-- 
2.11.0


Re: [Qemu-devel] [PATCH v3 1/2] block: fix dangling bs->explicit_options in block.c
Posted by Eric Blake, 53 weeks ago
On 07/14/2017 09:35 AM, Manos Pitsidianakis wrote:
> In some error paths it is possible to QDECREF a freed dangling
> explicit_options, resulting in a heap overflow crash.  For example
> bdrv_open_inherit()'s fail unrefs it, then calls bdrv_unref which calls
> bdrv_close which also unrefs it.
> 
> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
> ---
>  block.c | 2 ++
>  1 file changed, 2 insertions(+)

Reviewed-by: Eric Blake <eblake@redhat.com>

Can you pinpoint which commit introduced the bug, in order to decide if
this affects 2.9 and should therefore be cc'd to qemu-stable?

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [Qemu-devel] [PATCH v3 1/2] block: fix dangling bs->explicit_options in block.c
Posted by Manos Pitsidianakis, 53 weeks ago
On Fri, Jul 14, 2017 at 09:42:22AM -0500, Eric Blake wrote:
>On 07/14/2017 09:35 AM, Manos Pitsidianakis wrote:
>> In some error paths it is possible to QDECREF a freed dangling
>> explicit_options, resulting in a heap overflow crash.  For example
>> bdrv_open_inherit()'s fail unrefs it, then calls bdrv_unref which calls
>> bdrv_close which also unrefs it.
>>
>> Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
>> ---
>>  block.c | 2 ++
>>  1 file changed, 2 insertions(+)
>
>Reviewed-by: Eric Blake <eblake@redhat.com>
>
>Can you pinpoint which commit introduced the bug, in order to decide if
>this affects 2.9 and should therefore be cc'd to qemu-stable?

I think the particular error path was unreachable in every case since 
bdrv_open_driver() erroneously sets bs->drv to NULL, so bdrv_close never 
performs cleanups. I fix this in the next patch. I am not completely 
sure if it's possible to trigger this otherwise.
[Qemu-devel] [PATCH v3 2/2] block: fix leaks in bdrv_open_driver()
Posted by Manos Pitsidianakis, 53 weeks ago
bdrv_open_driver() is called in two places, bdrv_new_open_driver() and
bdrv_open_common(). In the latter, failure cleanup in is in its caller,
bdrv_open_inherit(), which unrefs the bs->file of the failed driver open
if it exists.

Let's move the bs->file cleanup to bdrv_open_driver() to take care of
all callers and do not set bs->drv to NULL unless the driver's open
function failed. When bs is destroyed by removing its last reference, it
calls bdrv_close() which checks bs->drv to perform the needed cleanups
and also call the driver's close function. Since it cleans up options
and opaque we must take care not leave dangling pointers.

The error paths in bdrv_open_driver() are now two:
If open fails, drv->bdrv_close() should not be called. Unref the child
if it exists, free what we allocated and set bs->drv to NULL. Return the
error and let callers free their stuff.

If open succeeds but we fail after, return the error and let callers
unref and delete their bs, while cleaning up their allocations.

Signed-off-by: Manos Pitsidianakis <el13635@mail.ntua.gr>
---
 block.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/block.c b/block.c
index 96b912d229..9ff62439be 100644
--- a/block.c
+++ b/block.c
@@ -1119,20 +1119,19 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
         } else {
             error_setg_errno(errp, -ret, "Could not open image");
         }
-        goto free_and_fail;
+        goto open_failed;
     }
 
     ret = refresh_total_sectors(bs, bs->total_sectors);
     if (ret < 0) {
         error_setg_errno(errp, -ret, "Could not refresh total sector count");
-        goto free_and_fail;
+        return ret;
     }
 
     bdrv_refresh_limits(bs, &local_err);
     if (local_err) {
         error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto free_and_fail;
+        return -EINVAL;
     }
 
     assert(bdrv_opt_mem_align(bs) != 0);
@@ -1140,12 +1139,14 @@ static int bdrv_open_driver(BlockDriverState *bs, BlockDriver *drv,
     assert(is_power_of_2(bs->bl.request_alignment));
 
     return 0;
-
-free_and_fail:
-    /* FIXME Close bs first if already opened*/
-    g_free(bs->opaque);
-    bs->opaque = NULL;
+open_failed:
     bs->drv = NULL;
+    if (bs->file != NULL) {
+        bdrv_unref_child(bs, bs->file);
+        bs->file = NULL;
+    }
+    g_free(bs->opaque);
+    bs->opaque = NULL;
     return ret;
 }
 
@@ -1166,7 +1167,9 @@ BlockDriverState *bdrv_new_open_driver(BlockDriver *drv, const char *node_name,
     ret = bdrv_open_driver(bs, drv, node_name, bs->options, flags, errp);
     if (ret < 0) {
         QDECREF(bs->explicit_options);
+        bs->explicit_options = NULL;
         QDECREF(bs->options);
+        bs->options = NULL;
         bdrv_unref(bs);
         return NULL;
     }
@@ -2600,9 +2603,6 @@ static BlockDriverState *bdrv_open_inherit(const char *filename,
 
 fail:
     blk_unref(file);
-    if (bs->file != NULL) {
-        bdrv_unref_child(bs, bs->file);
-    }
     QDECREF(snapshot_options);
     QDECREF(bs->explicit_options);
     QDECREF(bs->options);
-- 
2.11.0