[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption
From: |
Daniel P. Berrange |
Subject: |
Re: [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption driver |
Date: |
Fri, 18 Mar 2016 14:45:42 +0000 |
User-agent: |
Mutt/1.5.24 (2015-08-30) |
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 <address@hidden>
> > Signed-off-by: Daniel P. Berrange <address@hidden>
>
> 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 :|
- [Qemu-devel] [PATCH v5 3/7] tests: redirect stderr to stdout for iotests, (continued)
- [Qemu-devel] [PATCH v5 3/7] tests: redirect stderr to stdout for iotests, Daniel P. Berrange, 2016/03/17
- [Qemu-devel] [PATCH v5 4/7] tests: refactor python I/O tests helper main method, Daniel P. Berrange, 2016/03/17
- [Qemu-devel] [PATCH v5 5/7] tests: add output filter to python I/O tests helper, Daniel P. Berrange, 2016/03/17
- [Qemu-devel] [PATCH v5 1/7] block: add flag to indicate that no I/O will be performed, Daniel P. Berrange, 2016/03/17
- [Qemu-devel] [PATCH v5 7/7] block: drop support for using qcow[2] encryption with system emulators, Daniel P. Berrange, 2016/03/17
- [Qemu-devel] [PATCH v5 6/7] block: add generic full disk encryption driver, Daniel P. Berrange, 2016/03/17