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

[Eric Blake]

On 03/18/2016 09:19 AM, Kevin Wolf wrote:

 +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.
> 
> NULL then? It would be very obvious then if it were used against our
> expectations.

Yes, please.  The QMP input visitor has a bug where passing non-NULL at
the top-level visit can cause weird behavior with parsing an extra {}; I
have patches to fix it, but in the meantime consistently passing NULL
for the top-level visit is safest.

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



signature.asc
Description: OpenPGP digital signature

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

[Daniel P. Berrange]

On Fri, Mar 18, 2016 at 03:37:12PM +, Daniel P. Berrange wrote:
> On Fri, Mar 18, 2016 at 04:19:07PM +0100, Kevin Wolf wrote:
> > Am 18.03.2016 um 15:45 hat Daniel P. Berrange geschrieben:
> > > 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:
> > > > > +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.
> > 
> > qcow2 protects a few fields in BDRVQcow2State and metadata that is used
> > and possibly modified by requests. For example, after reading in some
> > metadata, another request could make changes that invalidate it, and we
> > need to protect against that.
> > 
> > I don't see that the crypto driver relies on any global (i.e. not
> > per-request) state either in memory or on disk, except for things that
> > are never changed after open, so the lock might not be needed.
> 
> Actually it does have global state - the QCryptoCipher object that's
> into the QCryptoBlock object must not be used concurrently by multiple
> threads, as each thread will need to initialize different IV data.
> 
> > > If it not possible for two calls to bdrv_co_readv() to run in
> > > parallel, then I can drop this mutex.
> > 
> > They can. The obvious yield point where a coroutine switch can happen is
> > the bdrv_co_readv() call above (but you already unlock for that one).
> > Unless qcrypto_block_decrypt() does some I/O internally, we can't have
> > any other yield points.
> 
> Ok, so we do need the mutex then to protect the cipher object state
> against concurrent use.

Ignore this comment & the one above. Since we're using coroutines the
qcrypto_block_decrypt() calls can't ever be running truely concurrently.


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

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

[Kevin Wolf]

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 
> Signed-off-by: Daniel P. Berrange 

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.

>  block/Makefile.objs|2 +
>  block/crypto.c |  587 
>  qapi/block-core.json   |   22 +-
>  tests/qemu-iotests/149 |  521 ++
>  tests/qemu-iotests/149.out | 2252 
> 
>  tests/qemu-iotests/common  |7 +
>  tests/qemu-iotests/group   |1 +
>  7 files changed, 3390 insertions(+), 2 deletions(-)
>  create mode 100644 block/crypto.c
>  create mode 100755 tests/qemu-iotests/149
>  create mode 100644 tests/qemu-iotests/149.out
> 
> diff --git a/block/Makefile.objs b/block/Makefile.objs
> index cdd8655..3426a15 100644
> --- a/block/Makefile.objs
> +++ b/block/Makefile.objs
> @@ -23,6 +23,8 @@ block-obj-$(CONFIG_LIBSSH2) += ssh.o
>  block-obj-y += accounting.o dirty-bitmap.o
>  block-obj-y += write-threshold.o
>  
> +block-obj-y += crypto.o
> +
>  common-obj-y += stream.o
>  common-obj-y += commit.o
>  common-obj-y += backup.o
> diff --git a/block/crypto.c b/block/crypto.c
> new file mode 100644
> index 000..5efca6c
> --- /dev/null
> +++ b/block/crypto.c
> @@ -0,0 +1,587 @@
> +/*
> + * QEMU block full disk encryption
> + *
> + * Copyright (c) 2015-2016 Red Hat, Inc.
> + *
> + * This library is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU Lesser General Public
> + * License as published by the Free Software Foundation; either
> + * version 2 of the License, or (at your option) any later version.
> + *
> + * This library is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * Lesser General Public License for more details.
> + *
> + * You should have received a copy of the GNU Lesser General Public
> + * License along with this library; if not, see 
> .
> + *
> + */
> +
> +#include "qemu/osdep.h"
> +
> +#include "block/block_int.h"
> +#include "crypto/block.h"
> +#include "qapi/opts-visitor.h"
> +#include "qapi-visit.h"
> +
> +#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"
> +
> +typedef struct BlockCrypto BlockCrypto;
> +
> +struct BlockCrypto {
> +QCryptoBlock *block;
> +CoMutex lock;
> +};
> +
> +
> +static int bl

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

[Kevin Wolf]

Am 18.03.2016 um 15:45 hat Daniel P. Berrange geschrieben:
> 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 
> > > Signed-off-by: Daniel P. Berrange 
> > 
> > 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.

Yes, I only read the test case after writing this comment.

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

Okay, that makes sense. Then we don't need a new test case.

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

NULL then? It would be very obvious then if it were used against our
expectations.

> > > +#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;
> > > 

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

[Daniel P. Berrange]

On Fri, Mar 18, 2016 at 04:19:07PM +0100, Kevin Wolf wrote:
> Am 18.03.2016 um 15:45 hat Daniel P. Berrange geschrieben:
> > 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:
> > > > +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.
> 
> qcow2 protects a few fields in BDRVQcow2State and metadata that is used
> and possibly modified by requests. For example, after reading in some
> metadata, another request could make changes that invalidate it, and we
> need to protect against that.
> 
> I don't see that the crypto driver relies on any global (i.e. not
> per-request) state either in memory or on disk, except for things that
> are never changed after open, so the lock might not be needed.

Actually it does have global state - the QCryptoCipher object that's
into the QCryptoBlock object must not be used concurrently by multiple
threads, as each thread will need to initialize different IV data.

> > If it not possible for two calls to bdrv_co_readv() to run in
> > parallel, then I can drop this mutex.
> 
> They can. The obvious yield point where a coroutine switch can happen is
> the bdrv_co_readv() call above (but you already unlock for that one).
> Unless qcrypto_block_decrypt() does some I/O internally, we can't have
> any other yield points.

Ok, so we do need the mutex then to protect the cipher object state
against concurrent use.

> > > > +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.
> 
> Hm... I was thinking of has_zero_init/discard/get_block_status, but I'm
> not sure how interesting that really is with encryption.
> 
> In theory, we could discard with undefined contents as the result, if we
> don't mind that we would be exposing that information (on the other
> hand, encrypted qcow2 images will expose it, too). And you have to
> enable unmap manually anyway.
> 
> Efficient zero write is out of question, I'm afraid.
> 
> The other thing that would be nice are several functions that provide
> information about the image, like refresh_limits, bdrv_info, etc.

Ok, I'll have a look at these.

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

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

[Daniel P. Berrange]

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 
> > Signed-off-by: Daniel P. Berrange 
> 
> 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 =