Re: [Qemu-devel] [PATCH] block: drop support for using qcow[2] encryption with system emulators

2016-06-11 Thread Eric Blake 
On 06/10/2016 09:32 AM, Daniel P. Berrange wrote:
> Back in the 2.3.0 release we declared qcow[2] encryption as
> deprecated, warning people that it would be removed in a future
> release.
> 
>   commit a1f688f4152e65260b94f37543521ceff8bfebe4
>   Author: Markus Armbruster 
>   Date:   Fri Mar 13 21:09:40 2015 +0100
> 
> block: Deprecate QCOW/QCOW2 encryption
> 
> The code still exists today, but by a (happy?) accident we entirely
> broke the ability to use qcow[2] encryption in the system emulators
> in the 2.4.0 release due to
> 
>   commit 8336aafae1451d54c81dd2b187b45f7c45d2428e
>   Author: Daniel P. Berrange 
>   Date:   Tue May 12 17:09:18 2015 +0100
> 
> qcow2/qcow: protect against uninitialized encryption key
> 
> This commit was designed to prevent future coding bugs which
> might cause QEMU to read/write data on an encrypted block
> device in plain text mode before a decryption key is set.
> 
> It turns out this preventative measure was a little too good,
> because we already had a long standing bug where QEMU read
> encrypted data in plain text mode during system emulator
> startup, in order to guess disk geometry:

Interesting analysis.


> So rather than fix the crash, and backport it to stable
> releases, just go ahead with what we have warned users about
> and disable any use of qcow2 encryption in the system
> emulators. qemu-img/qemu-io/qemu-nbd are still able to access
> qcow2 encrypted images for the sake of data conversion.
> 
> In the future, qcow2 will gain support for the alternative
> luks format, but when this happens it'll be using the
> '-object secret' infrastructure for gettings keys, which
> avoids this problematic scenario entirely.
> 
> Signed-off-by: Daniel P. Berrange 
> ---
>  block/qcow.c   | 11 +++
>  block/qcow2.c  | 11 +++
>  tests/qemu-iotests/087.out | 12 ++--
>  3 files changed, 16 insertions(+), 18 deletions(-)


> +++ b/block/qcow.c
> @@ -162,10 +162,13 @@ static int qcow_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  if (s->crypt_method_header) {
>  if (bdrv_uses_whitelist() &&
>  s->crypt_method_header == QCOW_CRYPT_AES) {
> -error_report("qcow built-in AES encryption is deprecated");
> -error_printf("Support for it will be removed in a future 
> release.\n"
> - "You can use 'qemu-img convert' to switch to an\n"
> - "unencrypted qcow image, or a LUKS raw image.\n");
> +error_setg(errp,
> +   "Use of AES-CBC encrypted qcow images is no longer "
> +   "supported in system emulators. You can use "
> +   "'qemu-img convert' to convert your image to use "
> +   "the LUKS format instead.");

error_setg() should not end in '.'.  Better would be:

error_setg(errp, "Use of AES-CBC encrypted qcow images is not supported");
error_append_hint(errp, "You can use 'qemu-img convert'... instead.\n");

> +++ b/block/qcow2.c
> @@ -968,10 +968,13 @@ static int qcow2_open(BlockDriverState *bs, QDict 
> *options, int flags,
>  if (s->crypt_method_header) {
>  if (bdrv_uses_whitelist() &&
>  s->crypt_method_header == QCOW_CRYPT_AES) {
> -error_report("qcow2 built-in AES encryption is deprecated");
> -error_printf("Support for it will be removed in a future 
> release.\n"
> - "You can use 'qemu-img convert' to switch to an\n"
> - "unencrypted qcow2 image, or a LUKS raw image.\n");
> +error_setg(errp,
> +   "Use of AES-CBC encrypted qcow2 images is no longer "
> +   "supported in system emulators. You can use "
> +   "'qemu-img convert' to convert your image to use "
> +   "the LUKS format instead.");

and again.

> +ret = -ENOSYS;
> +goto fail;
>  }
>  
>  bs->encrypted = 1;
> diff --git a/tests/qemu-iotests/087.out b/tests/qemu-iotests/087.out
> index 055c553..99853c5 100644
> --- a/tests/qemu-iotests/087.out
> +++ b/tests/qemu-iotests/087.out
> @@ -42,22 +42,14 @@ Formatting 'TEST_DIR/t.IMGFMT', fmt=IMGFMT size=134217728 
> encryption=on
>  Testing: -S
>  QMP_VERSION
>  {"return": {}}
> -IMGFMT built-in AES encryption is deprecated
> -Support for it will be removed in a future release.
> -You can use 'qemu-img convert' to switch to an
> -unencrypted IMGFMT image, or a LUKS raw image.
> -{"error": {"class": "GenericError", "desc": "blockdev-add doesn't support 
> encrypted devices"}}
> +{"error": {"class": "GenericError", "desc": "Use of AES-CBC encrypted qcow2 
> images is no longer supported in system emulators. You can use 'qemu-img 
> convert' to convert your image to use the LUKS format instead."}}

And this will need tweaking to match.

I'm

[Qemu-devel] [PATCH] block: drop support for using qcow[2] encryption with system emulators

2016-06-10 Thread Daniel P. Berrange 
Back in the 2.3.0 release we declared qcow[2] encryption as
deprecated, warning people that it would be removed in a future
release.

  commit a1f688f4152e65260b94f37543521ceff8bfebe4
  Author: Markus Armbruster 
  Date:   Fri Mar 13 21:09:40 2015 +0100

block: Deprecate QCOW/QCOW2 encryption

The code still exists today, but by a (happy?) accident we entirely
broke the ability to use qcow[2] encryption in the system emulators
in the 2.4.0 release due to

  commit 8336aafae1451d54c81dd2b187b45f7c45d2428e
  Author: Daniel P. Berrange 
  Date:   Tue May 12 17:09:18 2015 +0100

qcow2/qcow: protect against uninitialized encryption key

This commit was designed to prevent future coding bugs which
might cause QEMU to read/write data on an encrypted block
device in plain text mode before a decryption key is set.

It turns out this preventative measure was a little too good,
because we already had a long standing bug where QEMU read
encrypted data in plain text mode during system emulator
startup, in order to guess disk geometry:

  Thread 10 (Thread 0x7fffd3fff700 (LWP 30373)):
  #0  0x7fffe90b1a28 in raise () at /lib64/libc.so.6
  #1  0x7fffe90b362a in abort () at /lib64/libc.so.6
  #2  0x7fffe90aa227 in __assert_fail_base () at /lib64/libc.so.6
  #3  0x7fffe90aa2d2 in  () at /lib64/libc.so.6
  #4  0x5587ae19 in qcow2_co_readv (bs=0x562accb0, sector_num=0, 
remaining_sectors=1, qiov=0x7fffd260) at block/qcow2.c:1229
  #5  0x5589b60d in bdrv_aligned_preadv (bs=bs@entry=0x562accb0, 
req=req@entry=0x7fffd3ffea50, offset=offset@entry=0, bytes=bytes@entry=512, 
align=align@entry=512, qiov=qiov@entry=0x7fffd260, flags=0) at 
block/io.c:908
  #6  0x5589b8bc in bdrv_co_do_preadv (bs=0x562accb0, offset=0, 
bytes=512, qiov=0x7fffd260, flags=) at block/io.c:999
  #7  0x5589c375 in bdrv_rw_co_entry (opaque=0x7fffd210) at 
block/io.c:544
  #8  0x5586933b in coroutine_thread (opaque=0x57876310) at 
coroutine-gthread.c:134
  #9  0x764e1835 in g_thread_proxy (data=0x562b5590) at 
gthread.c:778
  #10 0x76bb760a in start_thread () at /lib64/libpthread.so.0
  #11 0x7fffe917f59d in clone () at /lib64/libc.so.6

  Thread 1 (Thread 0x77ecab40 (LWP 30343)):
  #0  0x7fffe91797a9 in syscall () at /lib64/libc.so.6
  #1  0x764ff87f in g_cond_wait (cond=cond@entry=0x55e085f0 
, mutex=mutex@entry=0x55e08600 ) at 
gthread-posix.c:1397
  #2  0x558692c3 in qemu_coroutine_switch (co=) at 
coroutine-gthread.c:117
  #3  0x558692c3 in qemu_coroutine_switch (from_=0x562b5e30, 
to_=to_@entry=0x57876310, action=action@entry=COROUTINE_ENTER) at 
coroutine-gthread.c:175
  #4  0x55868a90 in qemu_coroutine_enter (co=0x57876310, 
opaque=0x0) at qemu-coroutine.c:116
  #5  0x55859b84 in thread_pool_completion_bh (opaque=0x7fffd40010e0) 
at thread-pool.c:187
  #6  0x55859514 in aio_bh_poll (ctx=ctx@entry=0x562953b0) at 
async.c:85
  #7  0x55864d10 in aio_dispatch (ctx=ctx@entry=0x562953b0) at 
aio-posix.c:135
  #8  0x55864f75 in aio_poll (ctx=ctx@entry=0x562953b0, 
blocking=blocking@entry=true) at aio-posix.c:291
  #9  0x5589c40d in bdrv_prwv_co (bs=bs@entry=0x562accb0, 
offset=offset@entry=0, qiov=qiov@entry=0x7fffd260, 
is_write=is_write@entry=false, flags=flags@entry=(unknown: 0)) at block/io.c:591
  #10 0x5589c503 in bdrv_rw_co (bs=bs@entry=0x562accb0, 
sector_num=sector_num@entry=0, buf=buf@entry=0x7fffd2e0 "\321,", 
nb_sectors=nb_sectors@entry=21845, is_write=is_write@entry=false, 
flags=flags@entry=(unknown: 0)) at block/io.c:614
  #11 0x5589c562 in bdrv_read_unthrottled (nb_sectors=21845, 
buf=0x7fffd2e0 "\321,", sector_num=0, bs=0x562accb0) at block/io.c:622
  #12 0x5589c562 in bdrv_read_unthrottled (bs=0x562accb0, 
sector_num=sector_num@entry=0, buf=buf@entry=0x7fffd2e0 "\321,", 
nb_sectors=nb_sectors@entry=21845) at block/io.c:634
nb_sectors@entry=1) at block/block-backend.c:504
  #14 0x55752e9f in guess_disk_lchs (blk=blk@entry=0x562a5290, 
pcylinders=pcylinders@entry=0x7fffd52c, pheads=pheads@entry=0x7fffd530, 
psectors=psectors@entry=0x7fffd534) at hw/block/hd-geometry.c:68
  #15 0x55752ff7 in hd_geometry_guess (blk=0x562a5290, 
pcyls=pcyls@entry=0x57875d1c, pheads=pheads@entry=0x57875d20, 
psecs=psecs@entry=0x57875d24, ptrans=ptrans@entry=0x57875d28) at 
hw/block/hd-geometry.c:133
  #16 0x55752b87 in blkconf_geometry (conf=conf@entry=0x57875d00, 
ptrans=ptrans@entry=0x57875d28, cyls_max=cyls_max@entry=65536, 
heads_max=heads_max@entry=16, secs_max=secs_max@entry=255, 
errp=errp@entry=0x7fffd5e0) at hw/block/block.c:71
  #17 0x55799bc4 in ide_dev_initfn (dev=0x57875c80, kind=IDE_HD) at 
hw/ide/qdev.c:174
  #18 0x55768394 in