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

[Max Reitz]

On 07.02.20 14:55, 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>
> ---
> 
> In v2:
> 
>  - Cut down test scenarios to speed up
>  - Use _check_test_img helper
>  - Fix outdated comments
>  - Use space not tabs

Using eight spaces for indentation is...  Interesting, but at least
consistent. :-)

>  block/qcow2.c              | 11 +++--
>  tests/qemu-iotests/284     | 97 ++++++++++++++++++++++++++++++++++++++
>  tests/qemu-iotests/284.out | 62 ++++++++++++++++++++++++
>  tests/qemu-iotests/group   |  1 +
>  4 files changed, 167 insertions(+), 4 deletions(-)
>  create mode 100755 tests/qemu-iotests/284
>  create mode 100644 tests/qemu-iotests/284.out
Thanks, applied to my block branch:

https://git.xanclic.moe/XanClic/qemu/commits/branch/block

Max

signature.asc
Description: OpenPGP digital signature