[v7,3/4] crypto.c: cleanup created file when block_crypto_co_create_opts_luks fails

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 | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

Am 03.09.2019 um 15:57 hat Daniel Henrique Barboza geschrieben:
> 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 | 22 ++++++++++++++++++++++
>  1 file changed, 22 insertions(+)
> 
> diff --git a/block/crypto.c b/block/crypto.c
> index 7eb698774e..29496d247e 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,9 +597,30 @@ 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) {

if (ret && bs)

There are error paths before creating and opening the image, and trying
to delete bs can't succeed then.

> +        Error *local_err;

This shadows the function-scope local_err. Worse, it isn't even
initialised to NULL here.

> +        int r_del = bdrv_delete_file(bs, &local_err);
> +        /*
> +         * ENOTSUP will happen if the block driver doesn't support
> +         * 'bdrv_co_delete_file'. ENOENT will be fired by
> +         * 'raw_co_delete_file' if the file doesn't exist. Both are
> +         * predictable (we're not verifying if the driver supports
> +         * file deletion or if the file was created), thus we
> +         * shouldn't report this back to the user.
> +         */

When you check bs above, you don't have to worry about -ENOENT any more
here.

> +        if ((r_del < 0) && (r_del != -ENOTSUP) && (r_del != -ENOENT)) {
> +            error_reportf_err(local_err, "%s: ", bs->filename);

Hm... This will end up on stderr instead of being reported as an error.
Okay because we already have an error about creating the image, which is
more important to report.

However, don't prepend bs->filename here. You already included the
filename in the file-posix driver, so you'd print the filename twice in
the same message.

> +        }
> +    }
> +
>      bdrv_unref(bs);
>      qapi_free_QCryptoBlockCreateOptions(create_opts);
>      qobject_unref(cryptoopts);
> +
>      return ret;
>  }

This additional empty line looks unrelated.

Kevin