Skip to contentSkip to conversation

qemu-img: fix regression copying secrets during convert

PATCH Superseded Daniel P. Berrangé 2 participants 3 messages Aug 14, 2018

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Daniel P. Berrangé

author
#0 lore raw

PATCH 1/1 qemu-img: fix regression copying secrets during convert

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 <[email protected]>
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é <[email protected]>
---
 qemu-img.c | 33 +++++++++++++++------------------
 1 file changed, 15 insertions(+), 18 deletions(-)
qemu-img.c +15 -18
index 1acddf693c..66c388986e 100644
@@ -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++) {
Signature · 1 line
2.17.1
#1 lore raw

Am 14.08.2018 um 11:35 hat Daniel P. Berrangé geschrieben:

Quoted 16 lines

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 <[email protected]>
Date: Thu Jan 11 16:18:08 2018 +0100

Quoted 6 lines

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é <[email protected]>
Cc [email protected]

qemu-img.c out: · line 2464

Quoted 5 lines
qemu-img.c +0 -1
@@ -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-img.c out: · line 2466

Quoted 6 lines
qemu-img.c +1 -0
     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

author
#2 lore raw
Quoted 26 lines · Kevin Wolf

Am 14.08.2018 um 11:35 hat Daniel P. Berrangé geschrieben:

Quoted 16 lines

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 <[email protected]>
Date: Thu Jan 11 16:18:08 2018 +0100

Quoted 6 lines

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é <[email protected]>
Cc [email protected]
Quoted 7 lines

qemu-img.c out: · line 2464

Quoted 5 lines
qemu-img.c +0 -1
@@ -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....

Quoted 7 lines

qemu-img.c out: · line 2466

Quoted 7 lines
qemu-img.c +1 -0
     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

Signature · 3 lines
|: 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 :|