On Wed, Jan 13, 2016 at 07:42:20PM +0100, Kevin Wolf wrote:
> Am 12.01.2016 um 19:56 hat Daniel P. Berrange geschrieben:
> > This converts the qcow2 driver to make use of the QCryptoBlock
> > APIs for encrypting image content. As well as continued support
> > for the legacy QCow2 encryption format, the appealing benefit
> > is that it enables support for the LUKS format inside qcow2.
> >
> > With the LUKS format it is neccessary to store the LUKS
> > partition header and key material in the QCow2 file. This
> > data can be many MB in size, so cannot go into the QCow2
> > header region directly. Thus the spec is defines a FDE
> > (Full Disk Encryption) header extension that specifies
> > the offset of a set of clusters to hold the FDE headers,
> > as well as the length of that region. The LUKS header is
> > thus stored in these extra allocated clusters before the
> > main image payload.
> >
> > With this change it is now required to use the QCryptoSecret
> > object for providing passwords, instead of the current block
> > password APIs / interactive prompting.
> >
> > $QEMU \
> > -object secret,id=sec0,filename=/home/berrange/encrypted.pw \
> > -drive file=/home/berrange/encrypted.qcow2,key-id=sec0
> >
> > The new LUKS format is set as the new default format when
> > creating encrypted images. ie
> >
> > # qemu-img create --object secret,data=123456,id=sec0 \
> > -f qcow2 -o encryption,key-id=sec0 \
> > test.qcow2 10G
> >
> > Results in creation of an image using the LUKS format.
> >
> > For compatibility the old qcow2 AES format can still be used
> > via the 'encryption-format' parameter which accepts the
> > values 'luks' or 'qcowaes'.
> >
> > # qemu-img create --object secret,data=123456,id=sec0 \
> > -f qcow2 -o encryption,key-id=sec0,encryption-format=qcowaes \
> > test.qcow2 10G
> >
> > Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
>
> I think for your additional pointer to some clusters you need to change
> some function(s) called by qcow2_check_refcounts(). Otherwise 'qemu-img
> check' won't count the reference and helpfully free the "leaked"
> cluster.
Opps, yes, having those garbage collected would be a very bad
thing for your ability to decrypt your data :-)
> > @@ -60,6 +62,7 @@ typedef struct {
> > #define QCOW2_EXT_MAGIC_END 0
> > #define QCOW2_EXT_MAGIC_BACKING_FORMAT 0xE2792ACA
> > #define QCOW2_EXT_MAGIC_FEATURE_TABLE 0x6803f857
> > +#define QCOW2_EXT_MAGIC_FDE_HEADER 0x4c554b53
>
> General naming comment on this series: I would prefer avoiding "FDE" in
> favour of "encryption" or "crypt" in the block layer parts. With all
> image formats having their own terminology, "FDE" could mean anything.
Ok, will rename this - wasn't too happy with FDE myself either.
> > static int qcow2_probe(const uint8_t *buf, int buf_size, const char
> > *filename)
> > {
> > @@ -74,6 +77,83 @@ static int qcow2_probe(const uint8_t *buf, int buf_size,
> > const char *filename)
> > }
> >
> >
> > +static ssize_t qcow2_fde_header_read_func(QCryptoBlock *block,
> > + size_t offset,
> > + uint8_t *buf,
> > + size_t buflen,
> > + Error **errp,
> > + void *opaque)
> > +{
> > + BlockDriverState *bs = opaque;
> > + BDRVQcow2State *s = bs->opaque;
> > + ssize_t ret;
> > +
> > + if ((offset + buflen) > s->fde_header.length) {
> > + error_setg_errno(errp, EINVAL,
> > + "Request for data outside of extension header");
>
> error_setg_errno() with a constant errno doesn't look very useful.
> Better use plain error_setg() in such cases.
I wasn't too sure - I figured since the block layer seems to
propagate errno's around alot, that I ought to report an
errno here, but will happiyl drop it.
> > + return -1;
>
> Here returning -EINVAL could be useful, I'm not sure what your crypto
> API requires. At least you seem to be returning -errno below and mixing
> -1 and -errno is probably a bad idea.
The crypto API doesn't deal with errno's at all - it uses the
Error object exclusively, so yeah, I can drop it from the
place below.
>
> > + }
> > +
> > + ret = bdrv_pread(bs->file->bs,
> > + s->fde_header.offset + offset, buf, buflen);
> > + if (ret < 0) {
> > + error_setg_errno(errp, -ret, "Could not read encryption header");
> > + return ret;
> > + }
> > + return ret;
>
> return 0? You already processed ret in the if block and two 'return ret'
> in a row look odd.
>
> > +}
> > +
> > +
> > +static ssize_t qcow2_fde_header_init_func(QCryptoBlock *block,
> > + size_t headerlen,
> > + Error **errp,
> > + void *opaque)
> > +{
> > + BlockDriverState *bs = opaque;
> > + BDRVQcow2State *s = bs->opaque;
> > + int64_t ret;
> > +
> > + s->fde_header.length = headerlen + (headerlen % s->cluster_size);
> > +
> > + ret = qcow2_alloc_clusters(bs, s->fde_header.length);
> > + if (ret < 0) {
> > + s->fde_header.length = 0;
> > + error_setg(errp, "Cannot allocate cluster for LUKS header size
> > %zu",
> > + headerlen);
>
> I think ret is -errno on failure, so use error_setg_errno()?
Ok.
>
> > + return -1;
> > + }
> > +
> > + s->fde_header.offset = ret;
> > + return 0;
> > +}
> > +
> > +
> > +static ssize_t qcow2_fde_header_write_func(QCryptoBlock *block,
> > + size_t offset,
> > + const uint8_t *buf,
> > + size_t buflen,
> > + Error **errp,
> > + void *opaque)
> > +{
> > + BlockDriverState *bs = opaque;
> > + BDRVQcow2State *s = bs->opaque;
> > + ssize_t ret;
> > +
> > + if ((offset + buflen) > s->fde_header.length) {
> > + error_setg_errno(errp, EINVAL,
> > + "Request for data outside of extension header");
>
> error_setg(). Probably worth checking all error paths whether there is a
> useful errno or not. I won't comment on additional instances.
>
> > + return -1;
> > + }
> > +
> > + ret = bdrv_pwrite(bs->file->bs, s->fde_header.offset + offset, buf,
> > buflen);
> > + if (ret < 0) {
> > + error_setg_errno(errp, -ret, "Could not read encryption header");
> > + return ret;
> > + }
> > + return ret;
> > +}
>
> Mixing -1 and -errno again.
Will fix as per the read_func() above.
> > @@ -83,12 +163,14 @@ static int qcow2_probe(const uint8_t *buf, int
> > buf_size, const char *filename)
> > */
> > static int qcow2_read_extensions(BlockDriverState *bs, uint64_t
> > start_offset,
> > uint64_t end_offset, void
> > **p_feature_table,
> > + int flags,
> > Error **errp)
> > {
> > BDRVQcow2State *s = bs->opaque;
> > QCowExtension ext;
> > uint64_t offset;
> > int ret;
> > + unsigned int cflags = 0;
>
> Should we create a block for QCOW2_EXT_MAGIC_FDE_HEADER and declare it
> there?
Yep, I can do that.
> > #ifdef DEBUG_EXT
> > printf("qcow2_read_extensions: start=%ld end=%ld\n", start_offset,
> > end_offset);
> > @@ -159,6 +241,35 @@ static int qcow2_read_extensions(BlockDriverState *bs,
> > uint64_t start_offset,
> > }
> > break;
> >
> > + case QCOW2_EXT_MAGIC_FDE_HEADER:
> > + if (s->crypt_method_header != QCOW_CRYPT_LUKS) {
> > + error_setg(errp, "FDE header extension only "
> > + "expected with LUKS encryption method");
> > + return -EINVAL;
> > + }
> > + if (ext.len != sizeof(Qcow2FDEHeaderExtension)) {
> > + error_setg(errp, "LUKS header extension size %u, "
> > + "but expected size %zu", ext.len,
> > + sizeof(Qcow2FDEHeaderExtension));
> > + return -EINVAL;
> > + }
> > +
> > + ret = bdrv_pread(bs->file->bs, offset, &s->fde_header,
> > ext.len);
>
> No error check?
>
> > + be64_to_cpu(s->fde_header.offset);
> > + be64_to_cpu(s->fde_header.length);
> > +
> > + if (flags & BDRV_O_NO_IO) {
> > + cflags |= QCRYPTO_BLOCK_OPEN_NO_IO;
> > + }
> > + s->fde = qcrypto_block_open(s->fde_opts,
> > + qcow2_fde_header_read_func,
> > + bs,
> > + cflags,
> > + errp);
>
> The surrounding code doesn't put every parameter on its own line if the
> next parameter can fit on the same line. There are more instances of
> this in the patch, I won't comment on each one.
Ok, that's just my habit from libvirt - where we either put everything
on one line, or everything on a separate line and don't mix.
> > @@ -2213,12 +2517,15 @@ static int qcow2_create2(const char *filename,
> > int64_t total_size,
> > /*
> > * And now open the image and make it consistent first (i.e. increase
> > the
> > * refcount of the cluster that is occupied by the header and the
> > refcount
> > - * table)
> > + * table). Using BDRV_O_NO_IO since we've not written any encryption
> > + * header yet and thus should not read/write payload data or initialize
> > + * the decryption context
> > */
> > options = qdict_new();
> > qdict_put(options, "driver", qstring_from_str("qcow2"));
> > ret = bdrv_open(&bs, filename, NULL, options,
> > - BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH,
> > + BDRV_O_RDWR | BDRV_O_CACHE_WB | BDRV_O_NO_FLUSH |
> > + BDRV_O_NO_IO,
> > &local_err);
>
> Somehow I feel that saying BDRV_O_NO_IO, but doing I/O will bite us
> sooner or later.
Indeed, once I add the assertions you suggested in the previous
patch this will probably break horribly.
> Why not leave header->crypt_method = 0 initially and only set up
> encryption after opening the image with the qcow2 driver?
Oh yes, good idea.
> > diff --git a/block/qcow2.h b/block/qcow2.h
> > index ae04285..d33afb2 100644
> > --- a/block/qcow2.h
> > +++ b/block/qcow2.h
> > @@ -25,7 +25,7 @@
> > #ifndef BLOCK_QCOW2_H
> > #define BLOCK_QCOW2_H
> >
> > -#include "crypto/cipher.h"
> > +#include "crypto/block.h"
> > #include "qemu/coroutine.h"
> >
> > //#define DEBUG_ALLOC
> > @@ -36,6 +36,7 @@
> >
> > #define QCOW_CRYPT_NONE 0
> > #define QCOW_CRYPT_AES 1
> > +#define QCOW_CRYPT_LUKS 2
> >
> > #define QCOW_MAX_CRYPT_CLUSTERS 32
> > #define QCOW_MAX_SNAPSHOTS 65536
> > @@ -98,6 +99,15 @@
> > #define QCOW2_OPT_REFCOUNT_CACHE_SIZE "refcount-cache-size"
> > #define QCOW2_OPT_CACHE_CLEAN_INTERVAL "cache-clean-interval"
> >
> > +#define QCOW2_OPT_ENC_FORMAT "encryption-format"
> > +#define QCOW2_OPT_KEY_ID "key-id"
> > +#define QCOW2_OPT_CIPHER_ALG "cipher-alg"
> > +#define QCOW2_OPT_CIPHER_MODE "cipher-mode"
> > +#define QCOW2_OPT_IVGEN_ALG "ivgen-alg"
> > +#define QCOW2_OPT_IVGEN_HASH_ALG "ivgen-hash-alg"
> > +#define QCOW2_OPT_HASH_ALG "hash-alg"
> > +
> > +
> > typedef struct QCowHeader {
> > uint32_t magic;
> > uint32_t version;
> > @@ -163,6 +173,11 @@ typedef struct QCowSnapshot {
> > struct Qcow2Cache;
> > typedef struct Qcow2Cache Qcow2Cache;
> >
> > +typedef struct Qcow2FDEHeaderExtension {
> > + uint64_t offset;
> > + uint64_t length;
> > +} Qcow2FDEHeaderExtension;
>
> Packed? It seems to be read directly from the file.
Yes
> > @@ -166,6 +168,75 @@ the header extension data. Each entry look like this:
> > terminated if it has full length)
> >
> >
> > +== Full disk encryption (FDE) header pointer ==
> > +
> > +For 'crypt_method' header values which require additional header metadata
> > +to be stored (eg 'LUKS' header), the full disk encryption header pointer
> > +extension is mandatory.
>
> I think you want to make that "is present if, and only if, the
> 'crypt_method' header value requires metadata".
>
> At least, we need to forbid it for the existing AES images, because old
> qemu-img version would stll fix the "leaked clusters", without removing
> the header extension that refers to them.
Yes, we shouldn't be using the header with the AES crypt method,
only the LUKS method for now.
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 :|
