Re: [PATCH] block: always fill entire LUKS header space with zeros

[Max Reitz]

On 25.11.19 18:43, Daniel P. Berrangé wrote:
> When initializing the LUKS header the size with default encryption
> parameters will currently be 2068480 bytes. This is rounded up to
> a multiple of the cluster size, 2081792, with 64k sectors. If the
> end of the header is not the same as the end of the cluster we fill
> the extra space with zeros. This was forgetting that not even the
> space allocated for the header will be fully initialized, as we
> only write key material for the first key slot. The space left
> for the other 7 slots is never written to.
> 
> An optimization to the ref count checking code:
> 
>   commit a5fff8d4b4d928311a5005efa12d0991fe3b66f9 (refs/bisect/bad)
>   Author: Vladimir Sementsov-Ogievskiy <address@hidden>
>   Date:   Wed Feb 27 16:14:30 2019 +0300
> 
>     qcow2-refcount: avoid eating RAM
> 
> made the assumption that every cluster which was allocated would
> have at least some data written to it. This was violated by way
> the LUKS header is only partially written, with much space simply
> reserved for future use.
> 
> Depending on the cluster size this problem was masked by the
> logic which wrote zeros between the end of the LUKS header and
> the end of the cluster.
> 
> $ qemu-img create --object secret,id=cluster_encrypt0,data=123456 \
>    -f qcow2 -o cluster_size=2k,encrypt.iter-time=1,\
>                encrypt.format=luks,encrypt.key-secret=cluster_encrypt0 \
>              cluster_size_check.qcow2 100M
>   Formatting 'cluster_size_check.qcow2', fmt=qcow2 size=104857600
>     encrypt.format=luks encrypt.key-secret=cluster_encrypt0
>     encrypt.iter-time=1 cluster_size=2048 lazy_refcounts=off refcount_bits=16
> 
> $ qemu-img check --object secret,id=cluster_encrypt0,data=redhat \
>     'json:{"driver": "qcow2", "encrypt.format": "luks", \
>            "encrypt.key-secret": "cluster_encrypt0", \
>          "file.driver": "file", "file.filename": "cluster_size_check.qcow2"}'
> ERROR: counting reference for region exceeding the end of the file by one 
> cluster or more: offset 0x2000 size 0x1f9000
> Leaked cluster 4 refcount=1 reference=0
> ...snip...
> Leaked cluster 130 refcount=1 reference=0
> 
> 1 errors were found on the image.
> Data may be corrupted, or further writes to the image may corrupt it.
> 
> 127 leaked clusters were found on the image.
> This means waste of disk space, but no harm to data.
> Image end offset: 268288
> 
> The problem only exists when the disk image is entirely empty. Writing
> data to the disk image payload will solve the problem by causing the
> end of the file to be extended further.
> 
> The change fixes it by ensuring that the entire allocated LUKS header
> region is fully initialized with zeros. The qemu-img check will still
> fail for any pre-existing disk images created prior to this change,
> unless at least 1 byte of the payload is written to.
> 
> Fully writing zeros to the entire LUKS header is a good idea regardless
> as it ensures that space has been allocated on the host filesystem (or
> whatever block storage backend is used).
> 
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>  block/qcow2.c              |   4 +-
>  tests/qemu-iotests/278     |  88 +++++++++++++++++
>  tests/qemu-iotests/278.out | 197 +++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/group   |   1 +
>  4 files changed, 288 insertions(+), 2 deletions(-)
>  create mode 100755 tests/qemu-iotests/278
>  create mode 100644 tests/qemu-iotests/278.out
> 
> diff --git a/block/qcow2.c b/block/qcow2.c
> index 7c18721741..dcfdd200fc 100644
> --- a/block/qcow2.c
> +++ b/block/qcow2.c
> @@ -140,8 +140,8 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock 
> *block, size_t headerlen,

There’s a comment right here that reads:

>     /* Zero fill remaining space in cluster so it has predictable
>      * content in case of future spec changes */

I think that should be adjusted to reflect the change.

>      clusterlen = size_to_clusters(s, headerlen) * s->cluster_size;
>      assert(qcow2_pre_write_overlap_check(bs, 0, ret, clusterlen, false) == 
> 0);
>      ret = bdrv_pwrite_zeroes(bs->file,
> -                             ret + headerlen,
> -                             clusterlen - headerlen, 0);
> +                             ret,
> +                             clusterlen, 0);
>      if (ret < 0) {
>          error_setg_errno(errp, -ret, "Could not zero fill encryption 
> header");
>          return -1;
> diff --git a/tests/qemu-iotests/278 b/tests/qemu-iotests/278
> new file mode 100755
> index 0000000000..c52f03dd52
> --- /dev/null
> +++ b/tests/qemu-iotests/278
> @@ -0,0 +1,88 @@

[...]

> +IMGSPEC="driver=$IMGFMT,file.filename=$TEST_IMG,encrypt.key-secret=sec0"
> +QEMU_IO_OPTIONS=$QEMU_IO_OPTIONS_NO_FMT
> +
> +_run_test()
> +{
> +        echo

The indentation is off here.

> +     echo "== cluster size $csize"

Or maybe it really is off here, because starting from here everything is
indented with tabs.

> +     echo "== checking image refcounts =="
> +     $QEMU_IMG check --object $SECRET --image-opts $IMGSPEC | _filter_testdir

I think we should use _check_test_img instead in order to filter out the
allocation information.

> +
> +     echo
> +     echo "== writing some data =="
> +     $QEMU_IO --object $SECRET -c "write -P 0x9 0 1" --image-opts $IMGSPEC | 
> _filter_qemu_io | _filter_testdir
> +     echo
> +     echo "== rechecking image refcounts =="
> +     $QEMU_IMG check --object $SECRET --image-opts $IMGSPEC | _filter_testdir
> +
> +     echo
> +     echo "== writing some more data =="
> +     $QEMU_IO --object $SECRET -c "write -P 0x9 $csize 1" --image-opts 
> $IMGSPEC | _filter_qemu_io | _filter_testdir
> +     echo
> +     echo "== rechecking image refcounts =="
> +     $QEMU_IMG check --object $SECRET --image-opts $IMGSPEC | _filter_testdir
> +}
> +
> +
> +echo
> +echo "testing LUKS qcow2 encryption"
> +echo
> +
> +for csize in 512 1024 2048 4096 8192 16384 32768 65536

Is it necessary to actually test all of these cluster sizes?  Wouldn’t
it make more sense to test e.g. just 512, 4096, 64k, 2M?

I’m asking because as-is this test takes more than 20 s for me.

> +do
> +  _make_test_img --object $SECRET -o 
> "encrypt.format=luks,encrypt.key-secret=sec0,encrypt.iter-time=10,cluster_size=$csize"
>  $size
> +  _run_test "driver=$IMGFMT,encrypt.key-secret=sec0,file.filename=$TEST_IMG"

I don’t think this parameter is used (because $IMGSPEC exists).

Max

signature.asc
Description: OpenPGP digital signature