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

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

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
> 

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