| Message ID | 20170413154334.23708-1-mreitz@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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>
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
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; }
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(-)