[for-2.10?] block: Do not unref bs->file on error in BD's open
diff mbox

Message ID 20170413154334.23708-1-mreitz@redhat.com
State New
Headers show

Commit Message

Max Reitz April 13, 2017, 3:43 p.m. UTC
The block layer takes care of removing the bs->file child if the block
driver's bdrv_open()/bdrv_file_open() implementation fails. The block
driver therefore does not need to do so, and indeed should not unless it
sets bs->file to NULL afterwards -- because if this is not done, the
bdrv_unref_child() in bdrv_open_inherit() will dereference the freed
memory block at bs->file afterwards, which is not good.

We can now decide whether to add a "bs->file = NULL;" after each of the
offending bdrv_unref_child() invocations, or just drop them altogether.
The latter is simpler, so let's do that.

Cc: qemu-stable <qemu-stable@nongnu.org>
Signed-off-by: Max Reitz <mreitz@redhat.com>
---
It's an issue only in blkdebug, blkreplace and blkverify, and only when
an error occurs in their open functions; therefore I think this is fine
to delay until 2.10.

However, it *is* a use-after-free newly introduced in 2.9, so that's
where the question mark comes from...
---
 block/blkdebug.c  | 4 +---
 block/blkreplay.c | 3 ---
 block/blkverify.c | 3 ---
 3 files changed, 1 insertion(+), 9 deletions(-)

Comments

Eric Blake April 13, 2017, 4:21 p.m. UTC | #1
On 04/13/2017 10:43 AM, Max Reitz wrote:
> The block layer takes care of removing the bs->file child if the block
> driver's bdrv_open()/bdrv_file_open() implementation fails. The block
> driver therefore does not need to do so, and indeed should not unless it
> sets bs->file to NULL afterwards -- because if this is not done, the
> bdrv_unref_child() in bdrv_open_inherit() will dereference the freed
> memory block at bs->file afterwards, which is not good.
> 
> We can now decide whether to add a "bs->file = NULL;" after each of the
> offending bdrv_unref_child() invocations, or just drop them altogether.
> The latter is simpler, so let's do that.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> It's an issue only in blkdebug, blkreplace and blkverify, and only when
> an error occurs in their open functions; therefore I think this is fine
> to delay until 2.10.
> 
> However, it *is* a use-after-free newly introduced in 2.9, so that's
> where the question mark comes from...
> ---
>  block/blkdebug.c  | 4 +---
>  block/blkreplay.c | 3 ---
>  block/blkverify.c | 3 ---
>  3 files changed, 1 insertion(+), 9 deletions(-)

On its own, this patch is not worthy of an -rc5 (it only affects failure
paths, and 2 of the 3 affected drivers are not for production use). But
use-after-free is annoying, and the fact that blkreplay is affected
could trip up a user that mistypes arguments causing a BD open to fail.
I'd argue that since we're looking at -rc5 anyways, this is fair game
for inclusion in that build, since it is a small enough patch and easy
to review for correctness.  Hence I changed the subject in my reply to
'2.9?'

But I won't lose any sleep if we just wait for 2.9.1 and 2.10.

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

> 
> diff --git a/block/blkdebug.c b/block/blkdebug.c
> index 67e8024e36..cc4a146e84 100644
> --- a/block/blkdebug.c
> +++ b/block/blkdebug.c
> @@ -389,14 +389,12 @@ static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
>      } else if (align) {
>          error_setg(errp, "Invalid alignment");
>          ret = -EINVAL;
> -        goto fail_unref;
> +        goto out;
>      }
>  

And this means I get to rebase my blkdebug patches on top of yours, if
yours goes in first.
Stefan Hajnoczi April 18, 2017, 5:17 p.m. UTC | #2
On Thu, Apr 13, 2017 at 05:43:34PM +0200, Max Reitz wrote:
> The block layer takes care of removing the bs->file child if the block
> driver's bdrv_open()/bdrv_file_open() implementation fails. The block
> driver therefore does not need to do so, and indeed should not unless it
> sets bs->file to NULL afterwards -- because if this is not done, the
> bdrv_unref_child() in bdrv_open_inherit() will dereference the freed
> memory block at bs->file afterwards, which is not good.
> 
> We can now decide whether to add a "bs->file = NULL;" after each of the
> offending bdrv_unref_child() invocations, or just drop them altogether.
> The latter is simpler, so let's do that.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>
> ---
> It's an issue only in blkdebug, blkreplace and blkverify, and only when
> an error occurs in their open functions; therefore I think this is fine
> to delay until 2.10.
> 
> However, it *is* a use-after-free newly introduced in 2.9, so that's
> where the question mark comes from...
> ---
>  block/blkdebug.c  | 4 +---
>  block/blkreplay.c | 3 ---
>  block/blkverify.c | 3 ---
>  3 files changed, 1 insertion(+), 9 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
Kevin Wolf April 27, 2017, 2:13 p.m. UTC | #3
Am 13.04.2017 um 17:43 hat Max Reitz geschrieben:
> The block layer takes care of removing the bs->file child if the block
> driver's bdrv_open()/bdrv_file_open() implementation fails. The block
> driver therefore does not need to do so, and indeed should not unless it
> sets bs->file to NULL afterwards -- because if this is not done, the
> bdrv_unref_child() in bdrv_open_inherit() will dereference the freed
> memory block at bs->file afterwards, which is not good.
> 
> We can now decide whether to add a "bs->file = NULL;" after each of the
> offending bdrv_unref_child() invocations, or just drop them altogether.
> The latter is simpler, so let's do that.
> 
> Cc: qemu-stable <qemu-stable@nongnu.org>
> Signed-off-by: Max Reitz <mreitz@redhat.com>

Thanks, applied to block-next.

Kevin

Patch
diff mbox

diff --git a/block/blkdebug.c b/block/blkdebug.c
index 67e8024e36..cc4a146e84 100644
--- a/block/blkdebug.c
+++ b/block/blkdebug.c
@@ -389,14 +389,12 @@  static int blkdebug_open(BlockDriverState *bs, QDict *options, int flags,
     } else if (align) {
         error_setg(errp, "Invalid alignment");
         ret = -EINVAL;
-        goto fail_unref;
+        goto out;
     }
 
     ret = 0;
     goto out;
 
-fail_unref:
-    bdrv_unref_child(bs, bs->file);
 out:
     if (ret < 0) {
         g_free(s->config_file);
diff --git a/block/blkreplay.c b/block/blkreplay.c
index e1102119fb..6aa5fd4156 100755
--- a/block/blkreplay.c
+++ b/block/blkreplay.c
@@ -37,9 +37,6 @@  static int blkreplay_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = 0;
 fail:
-    if (ret < 0) {
-        bdrv_unref_child(bs, bs->file);
-    }
     return ret;
 }
 
diff --git a/block/blkverify.c b/block/blkverify.c
index 9a1e21c6ad..af23281669 100644
--- a/block/blkverify.c
+++ b/block/blkverify.c
@@ -142,9 +142,6 @@  static int blkverify_open(BlockDriverState *bs, QDict *options, int flags,
 
     ret = 0;
 fail:
-    if (ret < 0) {
-        bdrv_unref_child(bs, bs->file);
-    }
     qemu_opts_del(opts);
     return ret;
 }