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

[Daniel P. Berrange]

On Fri, Feb 05, 2016 at 03:20:43PM -0700, Eric Blake wrote:
> 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?

These are options for image creation, so they must still use -o.
My patches only deal with options for pre-existing images.

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

Yes, its wrong.

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

It is supposed to be reporting  10G - luks header size. Not sure
why this example shown 2k less - probably a bug from a much earlier
version of the patchset.  I think it probably dates from when I had
mistakenly not multiplied payload offset by sector size or some such.

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

Yes, I had a todo item in the code asking which was better

    /* XXX Should we treat size as being total physical size
     * of the image (ie payload + encryption header), or just
     * the logical size of the image (ie payload). If the latter
     * then we need to extend 'size' to include the header
     * size */
    qemu_opt_set_number(opts, BLOCK_OPT_SIZE, size, &error_abort);

And Fam suggested the same as you - show full 10 G to the guest.

The complication is that we need to create the backing file
before we format the luks header, and we don't know the size
of the luks header at that point. So either I could format
the luks header into a temporary in-memory buffer, or I have
to create the file and then resize it afterwards, or I have
to provide some way to calculate the eventual header size
prior to creating it. I didn't much like any of those options :-)

> > 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?

My intent is to add something to tests/qemu-iotests that will check
interoperability between qemu + cryptsetup. Slight complication is
that those io tests all expect to run unprivileged. So it'll need
a manual step run privileged to create the cryptsetup disk images
for testing with.


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

Same note as before - cryptsetup hardcodes is to 4k, so I figure
it is fine todo the same right now.

> > +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?

I don't think we need to care about that here. We're just honouring
reads from whereever the crypto driver wants - we don't want the
block driver here to have specific knowledge of the underlying
crypto format.

> 
> > +        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?

Yeah, this is just used for batching I/O to the underling
block device, so that we don't need to allocate an arbitrarily
large temporary buffer for (en|de)cryption.

> > +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?

No particular reason - this code pattern is just copied from
equivalent methods in qcow2.c

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

Yep

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

Ah ok

> 
> >  ##
> > +# @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.

Yes


Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://autobuild.org       -o-         http://search.cpan.org/~danberr/ :|
|: http://entangle-photo.org       -o-       http://live.gnome.org/gtk-vnc :|