[Qemu-devel] [PATCH] rbd: Fix use after free in rbd_open()

[Qemu-devel] [PATCH] rbd: Fix use after free in rbd_open()

[Kevin Wolf]

Commit 56d1b4d2 frees the QemuOpts for the driver-specific options
immediately, even though it still needs the filename string that is
contained there. This doesn't work. Move the deletion of the QemuOpts to
the end of the function where its content isn't needed any more.

Signed-off-by: Kevin Wolf <kwolf@redhat.com>
---
 block/rbd.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/block/rbd.c b/block/rbd.c
index 1826411..0f2608b 100644
--- a/block/rbd.c
+++ b/block/rbd.c
@@ -478,20 +478,20 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags)
     }
 
     filename = qemu_opt_get(opts, "filename");
-    qemu_opts_del(opts);
 
     if (qemu_rbd_parsename(filename, pool, sizeof(pool),
                            snap_buf, sizeof(snap_buf),
                            s->name, sizeof(s->name),
                            conf, sizeof(conf)) < 0) {
-        return -EINVAL;
+        r = -EINVAL;
+        goto failed_opts;
     }
 
     clientname = qemu_rbd_parse_clientname(conf, clientname_buf);
     r = rados_create(&s->cluster, clientname);
     if (r < 0) {
         error_report("error initializing");
-        return r;
+        goto failed_opts;
     }
 
     s->snap = NULL;
@@ -557,6 +557,7 @@ static int qemu_rbd_open(BlockDriverState *bs, QDict *options, int flags)
                             NULL, qemu_rbd_aio_flush_cb, s);
 
 
+    qemu_opts_del(opts);
     return 0;
 
 failed:
@@ -566,6 +567,8 @@ failed_open:
 failed_shutdown:
     rados_shutdown(s->cluster);
     g_free(s->snap);
+failed_opts:
+    qemu_opts_del(opts);
     return r;
 }
 
-- 
1.8.1.4

Re: [Qemu-devel] [PATCH] rbd: Fix use after free in rbd_open()

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 942 bytes --]

On 04/25/2013 07:59 AM, Kevin Wolf wrote:
> Commit 56d1b4d2 frees the QemuOpts for the driver-specific options

It looks like commit a9ccedc3d was the real culprit that introduced the
problem into rbd.c, but that the problem was latent (thanks to reference
counting) until 56d1b4d2 later moved its qdict_del earlier in block.c.

Oops, both commits bear my review tag, which means I missed this the
first time around.

> immediately, even though it still needs the filename string that is
> contained there. This doesn't work. Move the deletion of the QemuOpts to
> the end of the function where its content isn't needed any more.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/rbd.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 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: 621 bytes --]

Re: [Qemu-devel] [PATCH] rbd: Fix use after free in rbd_open()

[Kevin Wolf]

Am 25.04.2013 um 17:03 hat Eric Blake geschrieben:
> On 04/25/2013 07:59 AM, Kevin Wolf wrote:
> > Commit 56d1b4d2 frees the QemuOpts for the driver-specific options
> 
> It looks like commit a9ccedc3d was the real culprit that introduced the
> problem into rbd.c, but that the problem was latent (thanks to reference
> counting) until 56d1b4d2 later moved its qdict_del earlier in block.c.

Er, yes. Don't know where that commit ID came from, I actually meant
a9ccedc3. Stefan, please fix the commit message before you apply it.

Kevin

Re: [Qemu-devel] [PATCH] rbd: Fix use after free in rbd_open()

[Stefan Hajnoczi]

On Thu, Apr 25, 2013 at 03:59:27PM +0200, Kevin Wolf wrote:
> Commit 56d1b4d2 frees the QemuOpts for the driver-specific options
> immediately, even though it still needs the filename string that is
> contained there. This doesn't work. Move the deletion of the QemuOpts to
> the end of the function where its content isn't needed any more.
> 
> Signed-off-by: Kevin Wolf <kwolf@redhat.com>
> ---
>  block/rbd.c | 9 ++++++---
>  1 file changed, 6 insertions(+), 3 deletions(-)

Thanks, applied to my block tree:
https://github.com/stefanha/qemu/commits/block

Stefan