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

[Eric Blake]

On 2/7/20 7:55 AM, 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.

> 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).

What's more, we avoid a possible bug where creating a LUKS image backed 
by a block device protocol where the block device happens to already 
contain stale data from an earlier use of that block device in a 
different LUKS image, which could make it appear as though we have 
populated key slots.  It's unlikely that those other slots would decode 
the current image correctly (as the stale keyslot would decode to a 
different master key), but being able to supply the passphrase to that 
stale keyslot to decode garbage out of the new image does not seem 
desirable.

> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---

> +++ b/block/qcow2.c
> @@ -135,13 +135,16 @@ static ssize_t qcow2_crypto_hdr_init_func(QCryptoBlock 
> *block, size_t headerlen,
>       s->crypto_header.length = headerlen;
>       s->crypto_header.offset = ret;
>   
> -    /* Zero fill remaining space in cluster so it has predictable
> -     * content in case of future spec changes */
> +    /*
> +     * Zero fill all space in cluster so it has predictable
> +     * content, as we may not initialize some regions of the
> +     * header (eg only 1 out of 8 key slots will be initialized)
> +     */
>       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;
Reviewed-by: Eric Blake <address@hidden>

--
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org