Re: [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption driver

["Daniel P. Berrange" <berrange@redhat.com>]

On Fri, Mar 18, 2016 at 01:09:35PM +0100, Kevin Wolf wrote:
> Am 17.03.2016 um 18:51 hat Daniel P. Berrange geschrieben:
> > 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. This patch only registers the "luks" driver
> > since the "qcow" driver is there only for back-compatibility
> > with existing qcow built-in encryption.
> > 
> > New LUKS compatible volumes can be formatted using qemu-img
> > with defaults for all settings.
> > 
> > $ qemu-img create --object secret,data=123456,id=sec0 \
> >       -f luks -o key-secret=sec0 demo.luks 10G
> > 
> > Alternatively the cryptographic settings can be explicitly
> > set
> > 
> > $ 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
> > 
> > And query its size
> > 
> > $ qemu-img info demo.img
> > image: demo.img
> > file format: luks
> > virtual size: 10G (10737418240 bytes)
> > disk size: 132K
> > encrypted: yes
> > 
> > Note that it was not necessary to provide the password
> > when querying info for the volume. The password is only
> > required when performing I/O on the volume
> > 
> > All volumes created by this new 'luks' driver should be
> > capable of being opened by the kernel dm-crypt driver.
> > 
> > The only algorithms listed in the LUKS spec that are
> > not currently supported by this impl are sha512 and
> > ripemd160 hashes and cast6 cipher.
> > 
> > A new I/O test is added which is used to validate the
> > interoperability of the QEMU implementation of LUKS,
> > with the dm-crypt/cryptsetup implementation. Due to
> > the need to run cryptsetup as root, this test requires
> > that the user have password-less sudo configured.
> > 
> > The test is set to only run against the 'luks' format
> > so needs to be manually invoked with
> > 
> >   cd tests/qemu-iotests
> >   ./check -luks 149
> > 
> > Reviewed-by: Eric Blake <eblake@redhat.com>
> > Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
> 
> This is a huge patch. I'm not sure if it wouldn't be better to split off
> the test. I also think that having some test cases that don't require
> root privileges would be helpful, so maybe the test can be split in two
> halves as well, one requiring passwordless sudo and the other without
> special requirements.

I can certainly split off the test, however, I don't think it can be
split into 2 as you describe, as both halves of the two require sudo
privileges. So in one half we're creating images with dm-crypt and
processing them with qemu, and in the other half we're creating images
with qemu-img and processing them with dm-crypt.

This test was specifically designed to validate dm-crypt interoperability,
and my intention was that "pure" QEMU block driver testing of it would be
covered by the various pre-existing I/O tests. The problem is that I need
to update those existing tests to know how to pass the right args to
qemu-img to setup passwords. If I can do that, then we'll have some good
testing cover of the luks driver without needing sudo.

So I'll look at converting a couple of key pre-existing tests to get
such coverage, and put this test in its own patch.


> > +static ssize_t block_crypto_init_func(QCryptoBlock *block,
> > +                                      size_t headerlen,
> > +                                      Error **errp,
> > +                                      void *opaque)
> > +{
> > +    struct BlockCryptoCreateData *data = opaque;
> > +    int ret;
> > +
> > +    /* User provided size should reflect amount of space made
> > +     * available to the guest, so we must take account of that
> > +     * which will be used by the crypto header
> > +     */
> > +    data->size += headerlen;
> > +
> > +    qemu_opt_set_number(data->opts, BLOCK_OPT_SIZE, data->size, &error_abort);
> > +    ret = bdrv_create_file(data->filename, data->opts, errp);
> > +    if (ret < 0) {
> > +        return -1;
> > +    }
> > +
> > +    ret = bdrv_open(&data->bs, data->filename, NULL, NULL,
> > +                    BDRV_O_RDWR | BDRV_O_PROTOCOL, errp);
> 
> We should probably use blk_open() here like the other image format
> drivers do now, and preferably use blk_*() functions to access the
> image.
> 
> You will also want to specify BDRV_O_CACHE_WB (while it exists, I'm
> going to remove it soon).

Ok, will do.


> > +static QCryptoBlockOpenOptions *
> > +block_crypto_open_opts_init(QCryptoBlockFormat format,
> > +                            QemuOpts *opts,
> > +                            Error **errp)
> > +{
> > +    OptsVisitor *ov;
> > +    QCryptoBlockOpenOptions *ret = NULL;
> > +    Error *local_err = NULL;
> > +
> > +    ret = g_new0(QCryptoBlockOpenOptions, 1);
> > +    ret->format = format;
> > +
> > +    ov = opts_visitor_new(opts);
> > +
> > +    visit_start_struct(opts_get_visitor(ov),
> > +                       "luks", NULL, 0, &local_err);
> 
> As this refers to "luks" specifically, shouldn't it be inside the switch
> below?

Or probably better if I just change it to "", since this parameter
is not actually used in this context IIRC.

> > +    if (local_err) {
> > +        goto out;
> > +    }
> > +
> > +    switch (format) {
> > +    case Q_CRYPTO_BLOCK_FORMAT_LUKS:
> > +        visit_type_QCryptoBlockOptionsLUKS_members(
> > +            opts_get_visitor(ov), &ret->u.luks, &local_err);
> > +        break;
> > +
> > +    default:
> > +        error_setg(&local_err, "Unsupported block format %d", format);
> > +        break;
> > +    }
> > +    error_propagate(errp, local_err);
> > +    local_err = NULL;
> > +
> > +    visit_end_struct(opts_get_visitor(ov), &local_err);
> > +
> > + out:
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
> > +        qapi_free_QCryptoBlockOpenOptions(ret);
> > +        ret = NULL;
> > +    }
> > +    opts_visitor_cleanup(ov);
> > +    return ret;
> > +}


> > +#define BLOCK_CRYPTO_MAX_SECTORS 32
> > +
> > +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) / 512;
> > +
> > +    qemu_iovec_init(&hd_qiov, qiov->niov);
> > +
> > +    qemu_co_mutex_lock(&crypto->lock);
> > +
> > +    /* Bounce buffer so we have a linear mem region for
> > +     * entire sector. XXX optimize so we avoid bounce
> > +     * buffer in case that qiov->niov == 1
> > +     */
> > +    cipher_data =
> > +        qemu_try_blockalign(bs->file->bs, BLOCK_CRYPTO_MAX_SECTORS * 512);
> 
> As long as we create an individual buffer for each request, shouldn't
> MIN(BLOCK_CRYPTO_MAX_SECTORS * 512, qiov->size) be enough?

Yes, you're right. I didn't realize there was a pre-calcuated total
size field in QEMUIOVector

> > +    if (cipher_data == NULL) {
> > +        ret = -ENOMEM;
> > +        goto cleanup;
> > +    }
> > +
> > +    while (remaining_sectors) {
> > +        cur_nr_sectors = remaining_sectors;
> > +
> > +        if (cur_nr_sectors > BLOCK_CRYPTO_MAX_SECTORS) {
> > +            cur_nr_sectors = BLOCK_CRYPTO_MAX_SECTORS;
> > +        }
> > +
> > +        qemu_iovec_reset(&hd_qiov);
> > +        qemu_iovec_add(&hd_qiov, cipher_data, cur_nr_sectors * 512);
> > +
> > +        qemu_co_mutex_unlock(&crypto->lock);
> 
> Between qemu_co_mutex_lock() and here, there is no yield point...
> 
> > +        ret = bdrv_co_readv(bs->file->bs,
> > +                            payload_offset + sector_num,
> > +                            cur_nr_sectors, &hd_qiov);
> > +        qemu_co_mutex_lock(&crypto->lock);
> > +        if (ret < 0) {
> > +            goto cleanup;
> > +        }
> > +
> > +        if (qcrypto_block_decrypt(crypto->block,
> > +                                  sector_num,
> > +                                  cipher_data, cur_nr_sectors * 512,
> > +                                  NULL) < 0) {
> > +            ret = -1;
> 
> Need a real -errno code here.
> 
> > +            goto cleanup;
> > +        }
> 
> ...nor is there one between here and the end of the function.
> 
> So what does this CoMutex protect? If qcrypto_block_decrypt() needs this
> for some reason (it doesn't seem to be touching anything that isn't per
> request, but maybe I'm missing something), would it be clearer to put
> the locking only around that call?

This just a result of me blindly copying the locking pattern from
qcow2.c qcow2_co_readv() method without really understanding what
it was protecting.

If it not possible for two calls to bdrv_co_readv() to run in
parallel, then I can drop this mutex.

> 
> > +
> > +        qemu_iovec_from_buf(qiov, bytes_done,
> > +                            cipher_data, cur_nr_sectors * 512);
> > +
> > +        remaining_sectors -= cur_nr_sectors;
> > +        sector_num += cur_nr_sectors;
> > +        bytes_done += cur_nr_sectors * 512;
> > +    }
> > +
> > + cleanup:
> > +    qemu_co_mutex_unlock(&crypto->lock);
> > +
> > +    qemu_iovec_destroy(&hd_qiov);
> > +    qemu_vfree(cipher_data);
> > +
> > +    return ret;
> > +}
> > +

> > +BlockDriver bdrv_crypto_luks = {
> > +    .format_name        = "luks",
> > +    .instance_size      = sizeof(BlockCrypto),
> > +    .bdrv_probe         = block_crypto_probe_luks,
> > +    .bdrv_open          = block_crypto_open_luks,
> > +    .bdrv_close         = block_crypto_close,
> > +    .bdrv_create        = block_crypto_create_luks,
> > +    .create_opts        = &block_crypto_create_opts_luks,
> > +
> > +    .bdrv_co_readv      = block_crypto_co_readv,
> > +    .bdrv_co_writev     = block_crypto_co_writev,
> > +    .bdrv_getlength     = block_crypto_getlength,
> > +};
> 
> Rather minimalistic, but we can always add the missing functions later.

Do you have any recommendations on which are top priority / important
callbacks to add support for so I can prioritize future effort.


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