* [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free
@ 2017-04-03 17:51 Max Reitz
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 1/2] " Max Reitz
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Max Reitz @ 2017-04-03 17:51 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
And the exciting 2.9 ride continues!
When mirroring from a BDS with no parents at all (such as those added
with -blockdev or blockdev-add), we have a use-after-free in mirror's
error path. The first patch of this series fixes that, the other adds a
patch so we don't regress.
What issue will we find next? Stay tuned!
Max Reitz (2):
block/mirror: Fix use-after-free
iotests: Add mirror tests for orphaned source
block/mirror.c | 12 +++++++++--
tests/qemu-iotests/041 | 46 +++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/041.out | 4 ++--
tests/qemu-iotests/iotests.py | 15 ++++++++++++++
4 files changed, 73 insertions(+), 4 deletions(-)
--
2.12.1
^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH for-2.9 1/2] block/mirror: Fix use-after-free
2017-04-03 17:51 [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free Max Reitz
@ 2017-04-03 17:51 ` Max Reitz
2017-04-03 18:36 ` Philippe Mathieu-Daudé
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source Max Reitz
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2017-04-03 17:51 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
If @bs does not have any parents, the only reference to @mirror_top_bs
will be held by the BlockJob object after the bdrv_unref() following
block_job_create(). However, if block_job_create() fails, this reference
will not exist and @mirror_top_bs will have been deleted when we
goto fail.
The issue comes back at all later entries to the fail label: We delete
the BlockJob object before rolling back our changes to the node graph.
This means that we will delete @mirror_top_bs in the process.
All in all, whenever @bs does not have any parents and we go down the
fail path we will dereference @mirror_top_bs after it has been deleted.
Fix this by invoking bdrv_unref() only when block_job_create() was
successful and by bdrv_ref()'ing @mirror_top_bs in the fail path before
deleting the BlockJob object. Finally, bdrv_unref() it at the end of the
fail path after we actually no longer need it.
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
block/mirror.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/block/mirror.c b/block/mirror.c
index 9e2fecc15e..46ecd38ef0 100644
--- a/block/mirror.c
+++ b/block/mirror.c
@@ -1150,7 +1150,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
mirror_top_bs->total_sectors = bs->total_sectors;
/* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
- * it alive until block_job_create() even if bs has no parent. */
+ * it alive until block_job_create() succeeds even if bs has no parent. */
bdrv_ref(mirror_top_bs);
bdrv_drained_begin(bs);
bdrv_append(mirror_top_bs, bs, &local_err);
@@ -1168,10 +1168,12 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
creation_flags, cb, opaque, errp);
- bdrv_unref(mirror_top_bs);
if (!s) {
goto fail;
}
+ /* The block job now has a reference to this node */
+ bdrv_unref(mirror_top_bs);
+
s->source = bs;
s->mirror_top_bs = mirror_top_bs;
@@ -1242,6 +1244,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
fail:
if (s) {
+ /* Make sure this BDS does not go away until we have completed the graph
+ * changes below */
+ bdrv_ref(mirror_top_bs);
+
g_free(s->replaces);
blk_unref(s->target);
block_job_unref(&s->common);
@@ -1250,6 +1256,8 @@ fail:
bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
&error_abort);
bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
+
+ bdrv_unref(mirror_top_bs);
}
void mirror_start(const char *job_id, BlockDriverState *bs,
--
2.12.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source
2017-04-03 17:51 [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free Max Reitz
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 1/2] " Max Reitz
@ 2017-04-03 17:51 ` Max Reitz
2017-04-03 19:36 ` Eric Blake
2017-04-03 20:23 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free John Snow
2017-04-06 17:53 ` [Qemu-devel] " Kevin Wolf
3 siblings, 1 reply; 7+ messages in thread
From: Max Reitz @ 2017-04-03 17:51 UTC (permalink / raw)
To: qemu-block; +Cc: qemu-devel, Max Reitz, Kevin Wolf
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
tests/qemu-iotests/041 | 46 +++++++++++++++++++++++++++++++++++++++++++
tests/qemu-iotests/041.out | 4 ++--
tests/qemu-iotests/iotests.py | 15 ++++++++++++++
3 files changed, 63 insertions(+), 2 deletions(-)
diff --git a/tests/qemu-iotests/041 b/tests/qemu-iotests/041
index bc6cf782fe..2f54986434 100755
--- a/tests/qemu-iotests/041
+++ b/tests/qemu-iotests/041
@@ -966,5 +966,51 @@ class TestRepairQuorum(iotests.QMPTestCase):
# to check that this file is really driven by quorum
self.vm.shutdown()
+# Test mirroring with a source that does not have any parents (not even a
+# BlockBackend)
+class TestOrphanedSource(iotests.QMPTestCase):
+ def setUp(self):
+ blk0 = { 'node-name': 'src',
+ 'driver': 'null-co' }
+
+ blk1 = { 'node-name': 'dest',
+ 'driver': 'null-co' }
+
+ blk2 = { 'node-name': 'dest-ro',
+ 'driver': 'null-co',
+ 'read-only': 'on' }
+
+ self.vm = iotests.VM()
+ self.vm.add_blockdev(self.qmp_to_opts(blk0))
+ self.vm.add_blockdev(self.qmp_to_opts(blk1))
+ self.vm.add_blockdev(self.qmp_to_opts(blk2))
+ self.vm.launch()
+
+ def tearDown(self):
+ self.vm.shutdown()
+
+ def test_no_job_id(self):
+ self.assert_no_active_block_jobs()
+
+ result = self.vm.qmp('blockdev-mirror', device='src', sync='full',
+ target='dest')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
+ def test_success(self):
+ self.assert_no_active_block_jobs()
+
+ result = self.vm.qmp('blockdev-mirror', job_id='job', device='src',
+ sync='full', target='dest')
+ self.assert_qmp(result, 'return', {})
+
+ self.complete_and_wait('job')
+
+ def test_failing_permissions(self):
+ self.assert_no_active_block_jobs()
+
+ result = self.vm.qmp('blockdev-mirror', device='src', sync='full',
+ target='dest-ro')
+ self.assert_qmp(result, 'error/class', 'GenericError')
+
if __name__ == '__main__':
iotests.main(supported_fmts=['qcow2', 'qed'])
diff --git a/tests/qemu-iotests/041.out b/tests/qemu-iotests/041.out
index b67d0504a6..e30fd3b05b 100644
--- a/tests/qemu-iotests/041.out
+++ b/tests/qemu-iotests/041.out
@@ -1,5 +1,5 @@
-............................................................................
+...............................................................................
----------------------------------------------------------------------
-Ran 76 tests
+Ran 79 tests
OK
diff --git a/tests/qemu-iotests/iotests.py b/tests/qemu-iotests/iotests.py
index bec8eb4b8d..abcf3c10e2 100644
--- a/tests/qemu-iotests/iotests.py
+++ b/tests/qemu-iotests/iotests.py
@@ -177,6 +177,14 @@ class VM(qtest.QEMUQtestMachine):
self._num_drives += 1
return self
+ def add_blockdev(self, opts):
+ self._args.append('-blockdev')
+ if isinstance(opts, str):
+ self._args.append(opts)
+ else:
+ self._args.append(','.join(opts))
+ return self
+
def pause_drive(self, drive, event=None):
'''Pause drive r/w operations'''
if not event:
@@ -235,6 +243,13 @@ class QMPTestCase(unittest.TestCase):
output[basestr[:-1]] = obj # Strip trailing '.'
return output
+ def qmp_to_opts(self, obj):
+ obj = self.flatten_qmp_object(obj)
+ output_list = list()
+ for key in obj:
+ output_list += [key + '=' + obj[key]]
+ return ','.join(output_list)
+
def assert_qmp_absent(self, d, path):
try:
result = self.dictpath(d, path)
--
2.12.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 1/2] block/mirror: Fix use-after-free
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 1/2] " Max Reitz
@ 2017-04-03 18:36 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2017-04-03 18:36 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
nice analysis/catch
On 04/03/2017 02:51 PM, Max Reitz wrote:
> If @bs does not have any parents, the only reference to @mirror_top_bs
> will be held by the BlockJob object after the bdrv_unref() following
> block_job_create(). However, if block_job_create() fails, this reference
> will not exist and @mirror_top_bs will have been deleted when we
> goto fail.
>
> The issue comes back at all later entries to the fail label: We delete
> the BlockJob object before rolling back our changes to the node graph.
> This means that we will delete @mirror_top_bs in the process.
>
> All in all, whenever @bs does not have any parents and we go down the
> fail path we will dereference @mirror_top_bs after it has been deleted.
>
> Fix this by invoking bdrv_unref() only when block_job_create() was
> successful and by bdrv_ref()'ing @mirror_top_bs in the fail path before
> deleting the BlockJob object. Finally, bdrv_unref() it at the end of the
> fail path after we actually no longer need it.
>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
> block/mirror.c | 12 ++++++++++--
> 1 file changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/block/mirror.c b/block/mirror.c
> index 9e2fecc15e..46ecd38ef0 100644
> --- a/block/mirror.c
> +++ b/block/mirror.c
> @@ -1150,7 +1150,7 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> mirror_top_bs->total_sectors = bs->total_sectors;
>
> /* bdrv_append takes ownership of the mirror_top_bs reference, need to keep
> - * it alive until block_job_create() even if bs has no parent. */
> + * it alive until block_job_create() succeeds even if bs has no parent. */
> bdrv_ref(mirror_top_bs);
> bdrv_drained_begin(bs);
> bdrv_append(mirror_top_bs, bs, &local_err);
> @@ -1168,10 +1168,12 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
> BLK_PERM_CONSISTENT_READ | BLK_PERM_WRITE_UNCHANGED |
> BLK_PERM_WRITE | BLK_PERM_GRAPH_MOD, speed,
> creation_flags, cb, opaque, errp);
> - bdrv_unref(mirror_top_bs);
> if (!s) {
> goto fail;
> }
> + /* The block job now has a reference to this node */
> + bdrv_unref(mirror_top_bs);
> +
> s->source = bs;
> s->mirror_top_bs = mirror_top_bs;
>
> @@ -1242,6 +1244,10 @@ static void mirror_start_job(const char *job_id, BlockDriverState *bs,
>
> fail:
> if (s) {
> + /* Make sure this BDS does not go away until we have completed the graph
> + * changes below */
> + bdrv_ref(mirror_top_bs);
> +
> g_free(s->replaces);
> blk_unref(s->target);
> block_job_unref(&s->common);
> @@ -1250,6 +1256,8 @@ fail:
> bdrv_child_try_set_perm(mirror_top_bs->backing, 0, BLK_PERM_ALL,
> &error_abort);
> bdrv_replace_node(mirror_top_bs, backing_bs(mirror_top_bs), &error_abort);
> +
> + bdrv_unref(mirror_top_bs);
> }
>
> void mirror_start(const char *job_id, BlockDriverState *bs,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source Max Reitz
@ 2017-04-03 19:36 ` Eric Blake
0 siblings, 0 replies; 7+ messages in thread
From: Eric Blake @ 2017-04-03 19:36 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
[-- Attachment #1: Type: text/plain, Size: 490 bytes --]
On 04/03/2017 12:51 PM, Max Reitz wrote:
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> tests/qemu-iotests/041 | 46 +++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/041.out | 4 ++--
> tests/qemu-iotests/iotests.py | 15 ++++++++++++++
> 3 files changed, 63 insertions(+), 2 deletions(-)
Reviewed-by: Eric Blake <eblake@redhat.com>
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free
2017-04-03 17:51 [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free Max Reitz
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 1/2] " Max Reitz
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source Max Reitz
@ 2017-04-03 20:23 ` John Snow
2017-04-06 17:53 ` [Qemu-devel] " Kevin Wolf
3 siblings, 0 replies; 7+ messages in thread
From: John Snow @ 2017-04-03 20:23 UTC (permalink / raw)
To: Max Reitz, qemu-block; +Cc: Kevin Wolf, qemu-devel
On 04/03/2017 01:51 PM, Max Reitz wrote:
> And the exciting 2.9 ride continues!
>
> When mirroring from a BDS with no parents at all (such as those added
> with -blockdev or blockdev-add), we have a use-after-free in mirror's
> error path. The first patch of this series fixes that, the other adds a
> patch so we don't regress.
>
> What issue will we find next? Stay tuned!
>
Reviewed-by: John Snow <jsnow@redhat.com>
>
> Max Reitz (2):
> block/mirror: Fix use-after-free
> iotests: Add mirror tests for orphaned source
>
> block/mirror.c | 12 +++++++++--
> tests/qemu-iotests/041 | 46 +++++++++++++++++++++++++++++++++++++++++++
> tests/qemu-iotests/041.out | 4 ++--
> tests/qemu-iotests/iotests.py | 15 ++++++++++++++
> 4 files changed, 73 insertions(+), 4 deletions(-)
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free
2017-04-03 17:51 [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free Max Reitz
` (2 preceding siblings ...)
2017-04-03 20:23 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free John Snow
@ 2017-04-06 17:53 ` Kevin Wolf
3 siblings, 0 replies; 7+ messages in thread
From: Kevin Wolf @ 2017-04-06 17:53 UTC (permalink / raw)
To: Max Reitz; +Cc: qemu-block, qemu-devel
Am 03.04.2017 um 19:51 hat Max Reitz geschrieben:
> And the exciting 2.9 ride continues!
>
> When mirroring from a BDS with no parents at all (such as those added
> with -blockdev or blockdev-add), we have a use-after-free in mirror's
> error path. The first patch of this series fixes that, the other adds a
> patch so we don't regress.
>
> What issue will we find next? Stay tuned!
Thanks, applied to the block branch.
Kevin
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2017-04-06 17:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-03 17:51 [Qemu-devel] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free Max Reitz
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 1/2] " Max Reitz
2017-04-03 18:36 ` Philippe Mathieu-Daudé
2017-04-03 17:51 ` [Qemu-devel] [PATCH for-2.9 2/2] iotests: Add mirror tests for orphaned source Max Reitz
2017-04-03 19:36 ` Eric Blake
2017-04-03 20:23 ` [Qemu-devel] [Qemu-block] [PATCH for-2.9 0/2] block/mirror: Fix use-after-free John Snow
2017-04-06 17:53 ` [Qemu-devel] " Kevin Wolf
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.