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

[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 <address@hidden>
>   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 <address@hidden>
>   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 <address@hidden>
> ---
>  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 in favor of the idea behind the patch, but the error_setg() usage
needs to be fixed for v2.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature