[PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

The version 8 of this patch series got buried and it's now
conflicting with master. Rebase and re-sending it.

Also, I contemplated the idea of moving/copying the password
verification in qcrypto_block_luks_create() all the way back to the
start of block_crypto_co_create_opts_luks(), failing early before the
bdrv_create_file(), avoiding the problem altogether without the
need of a delete_file API I'm trying to push here (see patch 03
commit message for detailed info about the bug).

This idea was dropped after I saw that:

- We would need to store the resulting password, now being retrieved
early in block_crypto_co_create_opts_luks(), in a new
QCryptoBlockCreateOptions string to be used inside
qcrypto_block_luks_create() as intended. An alternative would be to
call qcrypto_secret_lookup_as_utf8() twice, discarding the first
string;

- There are a lot of ways to fail in qcrypto_block_luks_create()
other than a non-UTF8 password that would trigger the same problem.
A more appropiate way of doing what I intended, instead of
copying/hacking code around to fail before bdrv_create(), is some sort
of bdrv_validate() API that would encapsulate everything that is
related to user input validation for the security drivers. This
API could then be called before the file creation (maybe inside
bdrv_create itself) and fail early if needed. This is too overkill
for what I'm trying to fix here, and I'm not sure if it would be
a net gain compared to the delete_file API.


All that said, I believe that this patch series presents a sane
solution with the code we have ATM.


changes in this version:
- rebase with current master at 204aa60b37
- previous version:
https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html


Daniel Henrique Barboza (4):
  block: introducing 'bdrv_co_delete_file' interface
  block.c: adding bdrv_co_delete_file
  crypto.c: cleanup created file when block_crypto_co_create_opts_luks
    fails
  qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

 block.c                    | 26 +++++++++++++++
 block/crypto.c             | 18 ++++++++++
 block/file-posix.c         | 23 +++++++++++++
 include/block/block.h      |  1 +
 include/block/block_int.h  |  4 +++
 tests/qemu-iotests/282     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/282.out | 11 +++++++
 tests/qemu-iotests/group   |  1 +
 8 files changed, 151 insertions(+)
 create mode 100755 tests/qemu-iotests/282
 create mode 100644 tests/qemu-iotests/282.out

-- 
2.24.1

[PATCH v9 1/4] block: introducing 'bdrv_co_delete_file' interface

[Daniel Henrique Barboza]

Adding to Block Drivers the capability of being able to clean up
its created files can be useful in certain situations. For the
LUKS driver, for instance, a failure in one of its authentication
steps can leave files in the host that weren't there before.

This patch adds the 'bdrv_co_delete_file' interface to block
drivers and add it to the 'file' driver in file-posix.c. The
implementation is given by 'raw_co_delete_file'.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/file-posix.c        | 23 +++++++++++++++++++++++
 include/block/block_int.h |  4 ++++
 2 files changed, 27 insertions(+)

diff --git a/block/file-posix.c b/block/file-posix.c
index 1b805bd938..ed28234bb8 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -2386,6 +2386,28 @@ static int coroutine_fn raw_co_create_opts(const char *filename, QemuOpts *opts,
     return raw_co_create(&options, errp);
 }
 
+static int coroutine_fn raw_co_delete_file(BlockDriverState *bs,
+                                           Error **errp)
+{
+    struct stat st;
+    int ret;
+
+    if (!(stat(bs->filename, &st) == 0) || !S_ISREG(st.st_mode)) {
+        error_setg_errno(errp, ENOENT, "%s is not a regular file",
+                         bs->filename);
+        return -ENOENT;
+    }
+
+    ret = unlink(bs->filename);
+    if (ret < 0) {
+        ret = -errno;
+        error_setg_errno(errp, -ret, "Error when deleting file %s",
+                         bs->filename);
+    }
+
+    return ret;
+}
+
 /*
  * Find allocation range in @bs around offset @start.
  * May change underlying file descriptor's file offset.
@@ -3017,6 +3039,7 @@ BlockDriver bdrv_file = {
     .bdrv_co_block_status = raw_co_block_status,
     .bdrv_co_invalidate_cache = raw_co_invalidate_cache,
     .bdrv_co_pwrite_zeroes = raw_co_pwrite_zeroes,
+    .bdrv_co_delete_file = raw_co_delete_file,
 
     .bdrv_co_preadv         = raw_co_preadv,
     .bdrv_co_pwritev        = raw_co_pwritev,
diff --git a/include/block/block_int.h b/include/block/block_int.h
index dd033d0b37..d938d3e8d2 100644
--- a/include/block/block_int.h
+++ b/include/block/block_int.h
@@ -314,6 +314,10 @@ struct BlockDriver {
      */
     int coroutine_fn (*bdrv_co_flush)(BlockDriverState *bs);
 
+    /* Delete a created file. */
+    int coroutine_fn (*bdrv_co_delete_file)(BlockDriverState *bs,
+                                            Error **errp);
+
     /*
      * Flushes all data that was already written to the OS all the way down to
      * the disk (for example file-posix.c calls fsync()).
-- 
2.24.1

[PATCH v9 2/4] block.c: adding bdrv_co_delete_file

[Daniel Henrique Barboza]

Using the new 'bdrv_co_delete_file' interface, a pure co_routine function
'bdrv_co_delete_file' inside block.c can can be used in a way similar of
the existing bdrv_create_file to to clean up a created file.

We're creating a pure co_routine because the only caller of
'bdrv_co_delete_file' will be already in co_routine context, thus there
is no need to add all the machinery to check for qemu_in_coroutine() and
create a separated co_routine to do the job.

Suggested-by: Daniel P. Berrangé <berrange@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block.c               | 26 ++++++++++++++++++++++++++
 include/block/block.h |  1 +
 2 files changed, 27 insertions(+)

diff --git a/block.c b/block.c
index ecd09dbbfd..c26d8271a1 100644
--- a/block.c
+++ b/block.c
@@ -548,6 +548,32 @@ int bdrv_create_file(const char *filename, QemuOpts *opts, Error **errp)
     return ret;
 }
 
+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp)
+{
+    Error *local_err = NULL;
+    int ret;
+
+    assert(bs != NULL);
+
+    if (!bs->drv) {
+        error_setg(errp, "Block node '%s' is not opened", bs->filename);
+        return -ENOMEDIUM;
+    }
+
+    if (!bs->drv->bdrv_co_delete_file) {
+        error_setg(errp, "Driver '%s' does not support image deletion",
+                   bs->drv->format_name);
+        return -ENOTSUP;
+    }
+
+    ret = bs->drv->bdrv_co_delete_file(bs, &local_err);
+    if (ret < 0) {
+        error_propagate(errp, local_err);
+    }
+
+    return ret;
+}
+
 /**
  * Try to get @bs's logical and physical block size.
  * On success, store them in @bsz struct and return 0.
diff --git a/include/block/block.h b/include/block/block.h
index e9dcfef7fa..f7db094859 100644
--- a/include/block/block.h
+++ b/include/block/block.h
@@ -373,6 +373,7 @@ bool bdrv_is_backing_chain_frozen(BlockDriverState *bs, BlockDriverState *base,
 int bdrv_freeze_backing_chain(BlockDriverState *bs, BlockDriverState *base,
                               Error **errp);
 void bdrv_unfreeze_backing_chain(BlockDriverState *bs, BlockDriverState *base);
+int coroutine_fn bdrv_co_delete_file(BlockDriverState *bs, Error **errp);
 
 
 typedef struct BdrvCheckResult {
-- 
2.24.1

[PATCH v9 3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

When using a non-UTF8 secret to create a volume using qemu-img, the
following error happens:

$ qemu-img create -f luks --object secret,id=vol_1_encrypt0,file=vol_resize_pool.vol_1.secret.qzVQrI -o key-secret=vol_1_encrypt0 /var/tmp/pool_target/vol_1 10240K

Formatting '/var/tmp/pool_target/vol_1', fmt=luks size=10485760 key-secret=vol_1_encrypt0
qemu-img: /var/tmp/pool_target/vol_1: Data from secret vol_1_encrypt0 is not valid UTF-8

However, the created file '/var/tmp/pool_target/vol_1' is left behind in the
file system after the failure. This behavior can be observed when creating
the volume using Libvirt, via 'virsh vol-create', and then getting "volume
target path already exist" errors when trying to re-create the volume.

The volume file is created inside block_crypto_co_create_opts_luks(), in
block/crypto.c. If the bdrv_create_file() call is successful but any
succeeding step fails*, the existing 'fail' label does not take into
account the created file, leaving it behind.

This patch changes block_crypto_co_create_opts_luks() to delete
'filename' in case of failure. A failure in this point means that
the volume is now truncated/corrupted, so even if 'filename' was an
existing volume before calling qemu-img, it is now unusable. Deleting
the file it is not much worse than leaving it in the filesystem in
this scenario, and we don't have to deal with checking the file
pre-existence in the code.

* in our case, block_crypto_co_create_generic calls qcrypto_block_create,
which calls qcrypto_block_luks_create, and this function fails when
calling qcrypto_secret_lookup_as_utf8.

Reported-by: Srikanth Aithal <bssrikanth@in.ibm.com>
Suggested-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 block/crypto.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/block/crypto.c b/block/crypto.c
index 24823835c1..00e8ec537d 100644
--- a/block/crypto.c
+++ b/block/crypto.c
@@ -30,6 +30,7 @@
 #include "qapi/error.h"
 #include "qemu/module.h"
 #include "qemu/option.h"
+#include "qemu/cutils.h"
 #include "crypto.h"
 
 typedef struct BlockCrypto BlockCrypto;
@@ -596,6 +597,23 @@ static int coroutine_fn block_crypto_co_create_opts_luks(const char *filename,
 
     ret = 0;
 fail:
+    /*
+     * If an error occurred, delete 'filename'. Even if the file existed
+     * beforehand, it has been truncated and corrupted in the process.
+     */
+    if (ret && bs) {
+        Error *local_delete_err = NULL;
+        int r_del = bdrv_co_delete_file(bs, &local_delete_err);
+        /*
+         * ENOTSUP will happen if the block driver doesn't support
+         * the 'bdrv_co_delete_file' interface. This is a predictable
+         * scenario and shouldn't be reported back to the user.
+         */
+        if ((r_del < 0) && (r_del != -ENOTSUP)) {
+            error_report_err(local_delete_err);
+        }
+    }
+
     bdrv_unref(bs);
     qapi_free_QCryptoBlockCreateOptions(create_opts);
     qobject_unref(cryptoopts);
-- 
2.24.1

[PATCH v9 4/4] qemu-iotests: adding LUKS cleanup for non-UTF8 secret error

[Daniel Henrique Barboza]

This patch adds a new test file to exercise the case where
qemu-img fails to complete for the LUKS format when a non-UTF8
secret is used.

Signed-off-by: Daniel Henrique Barboza <danielhb413@gmail.com>
---
 tests/qemu-iotests/282     | 67 ++++++++++++++++++++++++++++++++++++++
 tests/qemu-iotests/282.out | 11 +++++++
 tests/qemu-iotests/group   |  1 +
 3 files changed, 79 insertions(+)
 create mode 100755 tests/qemu-iotests/282
 create mode 100644 tests/qemu-iotests/282.out

diff --git a/tests/qemu-iotests/282 b/tests/qemu-iotests/282
new file mode 100755
index 0000000000..081eb12080
--- /dev/null
+++ b/tests/qemu-iotests/282
@@ -0,0 +1,67 @@
+#!/usr/bin/env bash
+#
+# Test qemu-img file cleanup for LUKS when using a non-UTF8 secret
+#
+# Copyright (C) 2020, IBM Corporation.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+seq=`basename $0`
+echo "QA output created by $seq"
+
+status=1	# failure is the default!
+TEST_IMAGE_FILE='vol.img'
+
+_cleanup()
+{
+  _cleanup_test_img
+  rm non_utf8_secret
+  rm -f $TEST_IMAGE_FILE
+}
+trap "_cleanup; exit \$status" 0 1 2 3 15
+
+# get standard environment, filters and checks
+. ./common.rc
+. ./common.filter
+
+_supported_fmt luks
+_supported_proto generic
+_unsupported_proto vxhs
+
+echo "== Create non-UTF8 secret =="
+echo -n -e '\x3a\x3c\x3b\xff' > non_utf8_secret
+SECRET="secret,id=sec0,file=non_utf8_secret"
+
+echo "== Throws an error because of invalid UTF-8 secret =="
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Image file should not exist after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+echo "== Create a stub image file and run qemu-img again =="
+touch $TEST_IMAGE_FILE
+$QEMU_IMG create -f $IMGFMT --object $SECRET -o "key-secret=sec0" $TEST_IMAGE_FILE 4M
+
+echo "== Pre-existing image file should also be deleted after the error =="
+if test -f "$TEST_IMAGE_FILE"; then
+    exit 1
+fi
+
+# success, all done
+echo "*** done"
+rm -f $seq.full
+status=0
diff --git a/tests/qemu-iotests/282.out b/tests/qemu-iotests/282.out
new file mode 100644
index 0000000000..5d079dabce
--- /dev/null
+++ b/tests/qemu-iotests/282.out
@@ -0,0 +1,11 @@
+QA output created by 282
+== Create non-UTF8 secret ==
+== Throws an error because of invalid UTF-8 secret ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Image file should not exist after the error ==
+== Create a stub image file and run qemu-img again ==
+qemu-img: vol.img: Data from secret sec0 is not valid UTF-8
+Formatting 'vol.img', fmt=luks size=4194304 key-secret=sec0
+== Pre-existing image file should also be deleted after the error ==
+ *** done
diff --git a/tests/qemu-iotests/group b/tests/qemu-iotests/group
index e041cc1ee3..9d8bf8f783 100644
--- a/tests/qemu-iotests/group
+++ b/tests/qemu-iotests/group
@@ -289,3 +289,4 @@
 279 rw backing quick
 280 rw migration quick
 281 rw quick
+282 rw img quick
-- 
2.24.1

Re: [PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[no-reply]

Patchew URL: https://patchew.org/QEMU/20200130213907.2830642-1-danielhb413@gmail.com/



Hi,

This series failed the docker-quick@centos7 build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.

=== TEST SCRIPT BEGIN ===
#!/bin/bash
make docker-image-centos7 V=1 NETWORK=1
time make docker-test-quick@centos7 SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===

qemu-system-x86_64: -accel kvm: failed to initialize kvm: No such file or directory
qemu-system-x86_64: falling back to tcg
**
ERROR:/tmp/qemu-test/src/tests/qtest/migration-helpers.c:119:check_migration_status: assertion failed (current_status != "completed"): ("completed" != "completed")
ERROR - Bail out! ERROR:/tmp/qemu-test/src/tests/qtest/migration-helpers.c:119:check_migration_status: assertion failed (current_status != "completed"): ("completed" != "completed")
make: *** [check-qtest-x86_64] Error 1
make: *** Waiting for unfinished jobs....
  TEST    iotest-qcow2: 184
  TEST    iotest-qcow2: 186
---
    raise CalledProcessError(retcode, cmd)
subprocess.CalledProcessError: Command '['sudo', '-n', 'docker', 'run', '--label', 'com.qemu.instance.uuid=0caca978bcdd4f61b584e7fe2d95050e', '-u', '1001', '--security-opt', 'seccomp=unconfined', '--rm', '-e', 'TARGET_LIST=', '-e', 'EXTRA_CONFIGURE_OPTS=', '-e', 'V=', '-e', 'J=14', '-e', 'DEBUG=', '-e', 'SHOW_ENV=1', '-e', 'CCACHE_DIR=/var/tmp/ccache', '-v', '/home/patchew/.cache/qemu-docker-ccache:/var/tmp/ccache:z', '-v', '/var/tmp/patchew-tester-tmp-g_8db43e/src/docker-src.2020-01-30-17.50.31.3585:/var/tmp/qemu:z,ro', 'qemu:centos7', '/var/tmp/qemu/run', 'test-quick']' returned non-zero exit status 2.
filter=--filter=label=com.qemu.instance.uuid=0caca978bcdd4f61b584e7fe2d95050e
make[1]: *** [docker-run] Error 1
make[1]: Leaving directory `/var/tmp/patchew-tester-tmp-g_8db43e/src'
make: *** [docker-run-test-quick@centos7] Error 2

real    11m4.604s
user    0m9.507s


The full log is available at
http://patchew.org/logs/20200130213907.2830642-1-danielhb413@gmail.com/testing.docker-quick@centos7/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com

Re: [PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Daniel Henrique Barboza]

Ping

On 1/30/20 6:39 PM, Daniel Henrique Barboza wrote:
> The version 8 of this patch series got buried and it's now
> conflicting with master. Rebase and re-sending it.
> 
> Also, I contemplated the idea of moving/copying the password
> verification in qcrypto_block_luks_create() all the way back to the
> start of block_crypto_co_create_opts_luks(), failing early before the
> bdrv_create_file(), avoiding the problem altogether without the
> need of a delete_file API I'm trying to push here (see patch 03
> commit message for detailed info about the bug).
> 
> This idea was dropped after I saw that:
> 
> - We would need to store the resulting password, now being retrieved
> early in block_crypto_co_create_opts_luks(), in a new
> QCryptoBlockCreateOptions string to be used inside
> qcrypto_block_luks_create() as intended. An alternative would be to
> call qcrypto_secret_lookup_as_utf8() twice, discarding the first
> string;
> 
> - There are a lot of ways to fail in qcrypto_block_luks_create()
> other than a non-UTF8 password that would trigger the same problem.
> A more appropiate way of doing what I intended, instead of
> copying/hacking code around to fail before bdrv_create(), is some sort
> of bdrv_validate() API that would encapsulate everything that is
> related to user input validation for the security drivers. This
> API could then be called before the file creation (maybe inside
> bdrv_create itself) and fail early if needed. This is too overkill
> for what I'm trying to fix here, and I'm not sure if it would be
> a net gain compared to the delete_file API.
> 
> 
> All that said, I believe that this patch series presents a sane
> solution with the code we have ATM.
> 
> 
> changes in this version:
> - rebase with current master at 204aa60b37
> - previous version:
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html
> 
> 
> Daniel Henrique Barboza (4):
>    block: introducing 'bdrv_co_delete_file' interface
>    block.c: adding bdrv_co_delete_file
>    crypto.c: cleanup created file when block_crypto_co_create_opts_luks
>      fails
>    qemu-iotests: adding LUKS cleanup for non-UTF8 secret error
> 
>   block.c                    | 26 +++++++++++++++
>   block/crypto.c             | 18 ++++++++++
>   block/file-posix.c         | 23 +++++++++++++
>   include/block/block.h      |  1 +
>   include/block/block_int.h  |  4 +++
>   tests/qemu-iotests/282     | 67 ++++++++++++++++++++++++++++++++++++++
>   tests/qemu-iotests/282.out | 11 +++++++
>   tests/qemu-iotests/group   |  1 +
>   8 files changed, 151 insertions(+)
>   create mode 100755 tests/qemu-iotests/282
>   create mode 100644 tests/qemu-iotests/282.out
> 

Re: [PATCH v9 0/4] delete created files when block_crypto_co_create_opts_luks fails

[Kevin Wolf]

Am 30.01.2020 um 22:39 hat Daniel Henrique Barboza geschrieben:
> The version 8 of this patch series got buried and it's now
> conflicting with master. Rebase and re-sending it.
> 
> Also, I contemplated the idea of moving/copying the password
> verification in qcrypto_block_luks_create() all the way back to the
> start of block_crypto_co_create_opts_luks(), failing early before the
> bdrv_create_file(), avoiding the problem altogether without the
> need of a delete_file API I'm trying to push here (see patch 03
> commit message for detailed info about the bug).
> 
> This idea was dropped after I saw that:
> 
> - We would need to store the resulting password, now being retrieved
> early in block_crypto_co_create_opts_luks(), in a new
> QCryptoBlockCreateOptions string to be used inside
> qcrypto_block_luks_create() as intended. An alternative would be to
> call qcrypto_secret_lookup_as_utf8() twice, discarding the first
> string;
> 
> - There are a lot of ways to fail in qcrypto_block_luks_create()
> other than a non-UTF8 password that would trigger the same problem.
> A more appropiate way of doing what I intended, instead of
> copying/hacking code around to fail before bdrv_create(), is some sort
> of bdrv_validate() API that would encapsulate everything that is
> related to user input validation for the security drivers. This
> API could then be called before the file creation (maybe inside
> bdrv_create itself) and fail early if needed. This is too overkill
> for what I'm trying to fix here, and I'm not sure if it would be
> a net gain compared to the delete_file API.
> 
> 
> All that said, I believe that this patch series presents a sane
> solution with the code we have ATM.
> 
> 
> changes in this version:
> - rebase with current master at 204aa60b37
> - previous version:
> https://lists.gnu.org/archive/html/qemu-devel/2019-11/msg01551.html

Thanks, applied to the block branch.

Kevin