Re: [Qemu-devel] [Qemu-block] [PATCH v1 10/15] qcow2: convert QCow2 to use QCryptoBlock for encryption

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

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