Re: [Qemu-block] [PATCH v2 10/17] block: add generic full disk encryption driver

[Eric Blake]

On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> Add a block driver that is capable of supporting any full disk
> encryption format. This utilizes the previously added block
> encryption code, and at this time supports the LUKS format.
> 
> The driver code is capable of supporting any format supported
> by the QCryptoBlock module, so it registers one block driver
> for each format.
> 
> At this time, the "luks" driver is registered. New LUKS
> compatible volume can be formatted using qemu-img
> 
> $ qemu-img create --object secret,data=123456,id=sec0 \
>       -f luks -o key-secret=sec0,cipher-alg=aes-256,\
>                  cipher-mode=cbc,ivgen-alg=plain64,hash-alg=sha256 \
>       demo.luks 10G

Are you still adding -o options like this, or is it better to use the
new --image-opts flag from your other series for this purpose?

> 
> And query its size
> 
> $ qemu-img info --object secret,data=123456,id=sec0  --source 
> demo.luks,driver=luks,key-secret=sec0

And this definitely looks stale.

> image: json:{"key-secret": "sec0", "driver": "luks", "file": {"driver": 
> "file", "filename": "demo.luks"}}
> file format: luks
> virtual size: 10.0G (10737416192 bytes)

That size is 2048 bytes less than 10G; what happened?  With default
striping, you have 4000 stripes * 32 byte aes-256 key * 8 key material +
header (where I pointed out that you are using a default offset of 4096,
even though 1024 would be sufficient on 512-byte sectors), or roughly
1M, occupied by the LUKS header (in fact, having the payload aligned to
1M is probably wise, even if it can pack in tighter, just because a
1M-aligned disk with a 1M header is a GOOD thing).  Therefore, you
should either be counting the _entire_ LUKS header as part of the image
(and report a full 10G), or you should _only_ be counting the payload
size (and report 10G-1M, not 10G-2k).

My vote: do the same as we do for qcow2 or any other format.  Make the
size requested by the user as the size visible to the guest, and a
fully-allocated image will take more space on the host than what the
guest is using (that is, a fully-allocated 10G LUKS disk would present
10G payload to the guest but occupy 10G+1M on the host).

> disk size: 132K

This, however, looks reasonable - with the defaults, key material 0 will
occupy just under 128k, and nothing is written in key material 1-7.

> 
> All volumes created by this new 'luks' driver should be
> capable of being opened by the kernel dm-crypt driver.
> With this initial impl, not all volumes created with
> dm-crypt can be opened by the QEMU 'luks' driver. This
> is due to lack of support for certain algorithms, in
> particular the 'xts' cipher mode. These limitations will
> be addressed in a later series of patches, with the
> intent that QEMU should be able to open anything that
> dm-crypt LUKS supports.

Cool!  Would it be worth augmenting the commit message to show a
sequence of commands where you loopback mount a LUKS image file created
by qemu, then wire that up with cryptsetup, as a proof that the two are
compatible implementations?

> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---
>  block/Makefile.objs  |   2 +
>  block/crypto.c       | 541 
> +++++++++++++++++++++++++++++++++++++++++++++++++++
>  qapi/block-core.json |  18 +-
>  3 files changed, 560 insertions(+), 1 deletion(-)
>  create mode 100644 block/crypto.c
> 

> +++ b/block/crypto.c

> +
> +#define BLOCK_CRYPTO_OPT_LUKS_KEY_SECRET "key-secret"
> +#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_ALG "cipher-alg"
> +#define BLOCK_CRYPTO_OPT_LUKS_CIPHER_MODE "cipher-mode"
> +#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_ALG "ivgen-alg"
> +#define BLOCK_CRYPTO_OPT_LUKS_IVGEN_HASH_ALG "ivgen-hash-alg"
> +#define BLOCK_CRYPTO_OPT_LUKS_HASH_ALG "hash-alg"

I suggested in 7/17 that user-specified striping might be a parameter
worth adding; I guess this would be impacted as well.

> +
> +static int block_crypto_probe_generic(QCryptoBlockFormat format,
> +                                      const uint8_t *buf,
> +                                      int buf_size,
> +                                      const char *filename)
> +{
> +    if (qcrypto_block_has_format(format,
> +                                 buf, buf_size)) {
> +        return 100;

I don't know why we have particular values for any of the probes, but at
least this matches the arbitrary value reported by qcow2.

> +    } else {
> +        return 0;
> +    }
> +}
> +
> +
> +static ssize_t block_crypto_read_func(QCryptoBlock *block,
> +                                      size_t offset,
> +                                      uint8_t *buf,
> +                                      size_t buflen,
> +                                      Error **errp,
> +                                      void *opaque)
> +{
> +    BlockDriverState *bs = opaque;
> +    ssize_t ret;
> +
> +    ret = bdrv_pread(bs->file->bs, offset, buf, buflen);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not read encryption header");

Are we guaranteed that the offset being read here will always lie in the
encryption header portion of the file?

> +        return ret;
> +    }
> +    return ret;
> +}
> +
> +
> +static ssize_t block_crypto_write_func(QCryptoBlock *block,
> +                                       size_t offset,
> +                                       const uint8_t *buf,
> +                                       size_t buflen,
> +                                       Error **errp,
> +                                       void *opaque)
> +{
> +    BlockDriverState *bs = opaque;
> +    ssize_t ret;
> +
> +    ret = bdrv_pwrite(bs, offset, buf, buflen);
> +    if (ret < 0) {
> +        error_setg_errno(errp, -ret, "Could not write encryption header");

Likewise.

> +
> +#define BLOCK_CRYPTO_MAX_SECTORS 32

16k is not enough space to represent the LUKS header and the first key
material.  Does this number need to be larger to allow reading up to 1M
of the image to find all 8 key materials?

Or is this just being used to limit the maximum amount we'll
encrypt/decrypt in one pass of the loop?

> +
> +static coroutine_fn int
> +block_crypto_co_readv(BlockDriverState *bs, int64_t sector_num,
> +                      int remaining_sectors, QEMUIOVector *qiov)
> +{
> +    BlockCrypto *crypto = bs->opaque;
> +    int cur_nr_sectors; /* number of sectors in current iteration */
> +    uint64_t bytes_done = 0;
> +    uint8_t *cipher_data = NULL;
> +    QEMUIOVector hd_qiov;
> +    int ret = 0;
> +    size_t payload_offset = qcrypto_block_get_payload_offset(crypto->block);
> +
> +    qemu_iovec_init(&hd_qiov, qiov->niov);
> +
> +    qemu_co_mutex_lock(&crypto->lock);
> +
> +    while (remaining_sectors) {
> +        cur_nr_sectors = remaining_sectors;
> +
> +        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
> +            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
> +        }
> +        cipher_data =
> +            qemu_try_blockalign(bs->file->bs, cur_nr_sectors * 512);

Why are you allocating the block in the loop?  Can you hoist the
allocation outside, and reuse the memory across iterations?

> +
> +static int64_t block_crypto_getlength(BlockDriverState *bs)
> +{
> +    BlockCrypto *crypto = bs->opaque;
> +    int64_t len = bdrv_getlength(bs->file->bs);
> +
> +    ssize_t offset = qcrypto_block_get_payload_offset(crypto->block);
> +
> +    len -= (offset * 512);

Will need a tweak if you fix qcrypto_block_get_payload_offset to be in
bytes.

> +++ b/qapi/block-core.json
> @@ -1546,7 +1546,7 @@
>  { 'enum': 'BlockdevDriver',
>    'data': [ 'archipelago', 'blkdebug', 'blkverify', 'bochs', 'cloop',
>              'dmg', 'file', 'ftp', 'ftps', 'host_cdrom', 'host_device',
> -            'http', 'https', 'null-aio', 'null-co', 'parallels',
> +            'http', 'https', 'luks', 'null-aio', 'null-co', 'parallels',

Missing documentation; BlockDeviceInfo has been where we've documented
which release introduced which drivers.

>  ##
> +# @BlockdevOptionsLUKS
> +#
> +# Driver specific block device options for LUKS.
> +#
> +# @key-secret: #optional the ID of a QCryptoSecret object providing
> +#              the decryption key (since 2.6)

Worth mentioning that it is mandatory except when doing a metadata-only
probe.

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

signature.asc
Description: OpenPGP digital signature