Re: [Qemu-block] [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 <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.

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

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.

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

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

Kevin