[Qemu-devel] [PATCH] qemu-img: fix regression copying secrets during convert

[Qemu-devel] [PATCH] qemu-img: fix regression copying secrets during convert

[Daniel P. Berrangé]

When the convert command is creating an output file that needs
secrets, we need to ensure those secrets are passed to both the
blk_new_open and bdrv_create API calls.

This is done by qemu-img extracting all opts matching the name
suffix "key-secret". Unfortunately the code doing this was run after the
call to bdrv_create(), which meant the QemuOpts it was extracting
secrets from was now empty.

Previously this worked by luks as a bug meant the "key-secret"
parameters were not purged from the QemuOpts. This bug was fixed in

  commit b76b4f604521e59f857d6177bc55f6f2e41fd392
  Author: Kevin Wolf <kwolf@redhat.com>
  Date:   Thu Jan 11 16:18:08 2018 +0100

    qcow2: Use visitor for options in qcow2_create()

Exposing the latent bug in qemu-img. This fix simply moves the copying
of secrets to before the bdrv_create() call.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 qemu-img.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/qemu-img.c b/qemu-img.c
index 1acddf693c..66c388986e 100644
--- a/qemu-img.c
+++ b/qemu-img.c
@@ -345,21 +345,6 @@ static int img_add_key_secrets(void *opaque,
     return 0;
 }
 
-static BlockBackend *img_open_new_file(const char *filename,
-                                       QemuOpts *create_opts,
-                                       const char *fmt, int flags,
-                                       bool writethrough, bool quiet,
-                                       bool force_share)
-{
-    QDict *options = NULL;
-
-    options = qdict_new();
-    qemu_opt_foreach(create_opts, img_add_key_secrets, options, &error_abort);
-
-    return img_open_file(filename, options, fmt, flags, writethrough, quiet,
-                         force_share);
-}
-
 
 static BlockBackend *img_open(bool image_opts,
                               const char *filename,
@@ -2018,6 +2003,7 @@ static int img_convert(int argc, char **argv)
     BlockDriverState *out_bs;
     QemuOpts *opts = NULL, *sn_opts = NULL;
     QemuOptsList *create_opts = NULL;
+    QDict *open_opts = NULL;
     char *options = NULL;
     Error *local_err = NULL;
     bool writethrough, src_writethrough, quiet = false, image_opts = false,
@@ -2362,6 +2348,16 @@ static int img_convert(int argc, char **argv)
         }
     }
 
+    /*
+     * The later open call will need any decryption secrets, and
+     * bdrv_create() will purge "opts", so extract them now before
+     * they are lost.
+     */
+    if (!skip_create) {
+        open_opts = qdict_new();
+        qemu_opt_foreach(opts, img_add_key_secrets, open_opts, &error_abort);
+    }
+
     if (!skip_create) {
         /* Create the new image */
         ret = bdrv_create(drv, out_filename, opts, &local_err);
@@ -2388,8 +2384,9 @@ static int img_convert(int argc, char **argv)
          * That has to wait for bdrv_create to be improved
          * to allow filenames in option syntax
          */
-        s.target = img_open_new_file(out_filename, opts, out_fmt,
-                                     flags, writethrough, quiet, false);
+        s.target = img_open_file(out_filename, open_opts, out_fmt,
+                                 flags, writethrough, quiet, false);
+        open_opts = NULL; /* blk_new_open will have freed it */
     }
     if (!s.target) {
         ret = -1;
@@ -2461,9 +2458,9 @@ out:
         qemu_progress_print(100, 0);
     }
     qemu_progress_end();
-    qemu_opts_del(opts);
     qemu_opts_free(create_opts);
     qemu_opts_del(sn_opts);
+    qobject_unref(open_opts);
     blk_unref(s.target);
     if (s.src) {
         for (bs_i = 0; bs_i < s.src_num; bs_i++) {
-- 
2.17.1

Re: [Qemu-devel] [PATCH] qemu-img: fix regression copying secrets during convert

[Kevin Wolf]

Am 14.08.2018 um 11:35 hat Daniel P. Berrangé geschrieben:
> When the convert command is creating an output file that needs
> secrets, we need to ensure those secrets are passed to both the
> blk_new_open and bdrv_create API calls.
> 
> This is done by qemu-img extracting all opts matching the name
> suffix "key-secret". Unfortunately the code doing this was run after the
> call to bdrv_create(), which meant the QemuOpts it was extracting
> secrets from was now empty.
> 
> Previously this worked by luks as a bug meant the "key-secret"
> parameters were not purged from the QemuOpts. This bug was fixed in
> 
>   commit b76b4f604521e59f857d6177bc55f6f2e41fd392
>   Author: Kevin Wolf <kwolf@redhat.com>
>   Date:   Thu Jan 11 16:18:08 2018 +0100
> 
>     qcow2: Use visitor for options in qcow2_create()
> 
> Exposing the latent bug in qemu-img. This fix simply moves the copying
> of secrets to before the bdrv_create() call.
> 
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Cc: qemu-stable@nongnu.org

> @@ -2461,9 +2458,9 @@ out:
>          qemu_progress_print(100, 0);
>      }
>      qemu_progress_end();
> -    qemu_opts_del(opts);

Why don't we need to free opts any more?

>      qemu_opts_free(create_opts);
>      qemu_opts_del(sn_opts);
> +    qobject_unref(open_opts);
>      blk_unref(s.target);
>      if (s.src) {
>          for (bs_i = 0; bs_i < s.src_num; bs_i++) {

Kevin

Re: [Qemu-devel] [PATCH] qemu-img: fix regression copying secrets during convert

[Daniel P. Berrangé]

On Tue, Aug 14, 2018 at 01:38:24PM +0200, Kevin Wolf wrote:
> Am 14.08.2018 um 11:35 hat Daniel P. Berrangé geschrieben:
> > When the convert command is creating an output file that needs
> > secrets, we need to ensure those secrets are passed to both the
> > blk_new_open and bdrv_create API calls.
> > 
> > This is done by qemu-img extracting all opts matching the name
> > suffix "key-secret". Unfortunately the code doing this was run after the
> > call to bdrv_create(), which meant the QemuOpts it was extracting
> > secrets from was now empty.
> > 
> > Previously this worked by luks as a bug meant the "key-secret"
> > parameters were not purged from the QemuOpts. This bug was fixed in
> > 
> >   commit b76b4f604521e59f857d6177bc55f6f2e41fd392
> >   Author: Kevin Wolf <kwolf@redhat.com>
> >   Date:   Thu Jan 11 16:18:08 2018 +0100
> > 
> >     qcow2: Use visitor for options in qcow2_create()
> > 
> > Exposing the latent bug in qemu-img. This fix simply moves the copying
> > of secrets to before the bdrv_create() call.
> > 
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Cc: qemu-stable@nongnu.org
> 
> > @@ -2461,9 +2458,9 @@ out:
> >          qemu_progress_print(100, 0);
> >      }
> >      qemu_progress_end();
> > -    qemu_opts_del(opts);
> 
> Why don't we need to free opts any more?

Urgh, that's a mistake, I deleted the wrong line. v2 arriving soon....

> 
> >      qemu_opts_free(create_opts);
> >      qemu_opts_del(sn_opts);
> > +    qobject_unref(open_opts);
> >      blk_unref(s.target);
> >      if (s.src) {
> >          for (bs_i = 0; bs_i < s.src_num; bs_i++) {

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|