On 01/20/2016 10:38 AM, Daniel P. Berrange wrote: > Add a generic framework for support different block encryption > formats. Upon instantiating a QCryptoBlock object, it will read > the encryption header and extract the encryption keys. It is > then possible to call methods to encrypt/decrypt data buffers. > > There is also a mode whereby it will create/initialize a new > encryption header on a previously unformatted volume. > > The initial framework comes with support for the legacy QCow > AES based encryption. This enables code in the QCow driver to > be consolidated later. > > Signed-off-by: Daniel P. Berrange > --- > crypto/Makefile.objs | 2 + > crypto/block-qcow.c | 167 +++++++++++++++++++++++++++++ > crypto/block-qcow.h | 28 +++++ > crypto/block.c | 263 ++++++++++++++++++++++++++++++++++++++++++++++ > crypto/blockpriv.h | 90 ++++++++++++++++ > include/crypto/block.h | 233 ++++++++++++++++++++++++++++++++++++++++ > qapi/crypto.json | 67 ++++++++++++ > tests/.gitignore | 1 + > tests/Makefile | 2 + > tests/test-crypto-block.c | 239 +++++++++++++++++++++++++++++++++++++++++ > 10 files changed, 1092 insertions(+) > create mode 100644 crypto/block-qcow.c > create mode 100644 crypto/block-qcow.h > create mode 100644 crypto/block.c > create mode 100644 crypto/blockpriv.h > create mode 100644 include/crypto/block.h > create mode 100644 tests/test-crypto-block.c > > +++ b/crypto/block-qcow.c > @@ -0,0 +1,167 @@ > +/* > + * QEMU Crypto block device encryption QCow/QCow2 AES-CBC format > + * > + * 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 . > + * > + */ Maybe worth a big comment stating that this file exists for backwards compatibility, and no one in their right mind should copy the code and/or encrypt new files with it. > > +static gboolean > +qcrypto_block_qcow_has_format(const uint8_t *buf G_GNUC_UNUSED, > + size_t buf_size G_GNUC_UNUSED) > +{ > + return false; > +} When I see gboolean, I think TRUE/FALSE. Yes, C99 'false' happens to promote to the correct value for whatever integer type gboolean is, but this would read nicer if it returned 'bool'. > + > +static int > +qcrypto_block_qcow_init(QCryptoBlock *block, > + const char *keysecret, > + Error **errp) > +{ > + char *password; > + int ret; > + uint8_t keybuf[16]; > + int len, i; > + > + memset(keybuf, 0, 16); > + > + password = qcrypto_secret_lookup_as_utf8(keysecret, errp); > + if (!password) { > + return -1; > + } > + > + len = strlen(password); > + if (len > 16) { > + len = 16; > + } > + for (i = 0; i < len; i++) { > + keybuf[i] = password[i]; > + } What - we really throw away anything longer than 16 bytes? Would a memcpy() be any nicer than an open-coded loop? > + > +static int > +qcrypto_block_qcow_create(QCryptoBlock *block, > + QCryptoBlockCreateOptions *options, > + QCryptoBlockInitFunc initfunc G_GNUC_UNUSED, > + QCryptoBlockWriteFunc writefunc G_GNUC_UNUSED, > + void *opaque G_GNUC_UNUSED, > + Error **errp) > +{ > + if (!options->u.qcow->key_secret) { > + error_setg(errp, "Parameter 'key-secret' is required for cipher"); > + return -1; > + } > + /* QCow2 has no special header, since everything is hardwired */ > + return qcrypto_block_qcow_init(block, options->u.qcow->key_secret, errp); > +} > + > + > +static void > +qcrypto_block_qcow_cleanup(QCryptoBlock *block) > +{ > +} Nothing to free? qcrypto_block_qcow_init cleaned up block->cipher and block->ivgen on error, so shouldn't this do likewise (and then the init could call this function instead of open-coding the cleanup)?... > diff --git a/crypto/block.c b/crypto/block.c > new file mode 100644 > index 0000000..757e28a > --- /dev/null > +++ b/crypto/block.c > @@ -0,0 +1,263 @@ > +uint64_t qcrypto_block_get_payload_offset(QCryptoBlock *block) > +{ > + return block->payload_offset; > +} > + > + > +void qcrypto_block_free(QCryptoBlock *block) > +{ > + if (!block) { > + return; > + } > + > + block->driver->cleanup(block); > + > + qcrypto_cipher_free(block->cipher); > + qcrypto_ivgen_free(block->ivgen); > + g_free(block); ...oh, you centralized that part of the cleanup. > +++ b/crypto/blockpriv.h > + > +typedef struct QCryptoBlockDriver QCryptoBlockDriver; > + > +struct QCryptoBlock { > + QCryptoBlockFormat format; > + > + const QCryptoBlockDriver *driver; > + void *opaque; > + > + QCryptoCipher *cipher; > + QCryptoIVGen *ivgen; > + QCryptoHashAlgorithm kdfhash; > + size_t niv; > + uint64_t payload_offset; /* In 512 byte sectors */ Someday, we may want to support 4k sectors natively. I don't envy the person making the conversion, but we could at least make their life easier by representing this in bytes instead of units of 512-byte sectors. > +}; > + > +struct QCryptoBlockDriver { > + int (*open)(QCryptoBlock *block, > + QCryptoBlockOpenOptions *options, > + QCryptoBlockReadFunc readfunc, > + void *opaque, > + unsigned int flags, > + Error **errp); No documentation on any of these contracts? > + > + gboolean (*has_format)(const uint8_t *buf, > + size_t buflen); Why gboolean instead of bool? > +++ b/include/crypto/block.h > +/** > + * qcrypto_block_has_format: > + * @format: the encryption format > + * @buf: the data from head of the volume > + * @len: the length of @buf in bytes > + * > + * Given @len bytes of data from the head of a storage volume > + * in @buf, probe to determine if the volume has the encryption > + * format specified in @format. > + * > + * Returns: true if the data in @buf matches @format > + */ > +gboolean qcrypto_block_has_format(QCryptoBlockFormat format, Again, this should probably be 'bool', not gboolean. > + const uint8_t *buf, > + size_t buflen); > + > +typedef enum { > + QCRYPTO_BLOCK_OPEN_NO_IO = (1 << 0), > +} QCryptoBlockOpenFlags; > + > +/** > + * qcrypto_block_open: > + * @options: the encryption options > + * @readfunc: callback for reading data from the volume > + * @opaque: data to pass to @readfunc > + * @flags: bitmask of QCryptoBlockOpenFlags values > + * @errp: pointer to a NULL-initialized error object > + * > + * Create a new block encryption object for an existing > + * storage volume encrypted with format identified by > + * the parameters in @options. > + * > + * This will use @readfunc to initialize the encryption > + * context based on the volume header(s), extracting the > + * master key(s) as required. > + * > + * If @flags contains QCRYPTO_BLOCK_OPEN_NO_IO then > + * the open process will be optimized to skip any parts > + * that are only required to perform I/O. In particular > + * this would usually avoid the need to decrypt any > + * master keys. The only thing that can be done with > + * the resulting QCryptoBlock object would be to query > + * metadata such as the payload offset. There will be > + * no cipher or ivgen objects available. > + * > + * If any part of initializing the encryption context > + * fails an error will be returned. This could be due > + * to the volume being in the wrong format, an cipher > + * or IV generator algorithm that is not supoported, s/supoported/supported/ > + * or incorrect passphrases. > + * > + * Returns: a block encryption format, or NULL on error > + */ > +QCryptoBlock *qcrypto_block_open(QCryptoBlockOpenOptions *options, > + QCryptoBlockReadFunc readfunc, > + void *opaque, > + unsigned int flags, > + Error **errp); > + > +/** > + * qcrypto_block_create: > + * @format: the encryption format > + * @initfunc: callback for initializing volume header > + * @writefunc: callback for writing data to the volume header > + * @opaque: data to pass to @initfunc & @writefunc I'd spell it out s/&/and/ > + * @errp: pointer to a NULL-initialized error object > + * > + * Create a new block encryption object for initializing > + * a storage volume to be encrypted with format identified > + * by the parameters in @options. > + * > + * This method will allocate space for a new volume header > + * using @initfunc and then write header data using @writefunc, > + * generating new master keys, etc as required. Any existing > + * data present on the volume will be irrevokably destroyed. s/irrevokably/irrevocably/ > + * > + * If any part of initializing the encryption context > + * fails an error will be returned. This could be due > + * to the volume being in the wrong format, an cipher > + * or IV generator algorithm that is not supoported, s/supoported/supported/ > + * or incorrect passphrases. > + * > + * Returns: a block encryption format, or NULL on error > + */ > +QCryptoBlock *qcrypto_block_create(QCryptoBlockCreateOptions *options, > + QCryptoBlockInitFunc initfunc, > + QCryptoBlockWriteFunc writefunc, > + void *opaque, > + Error **errp); > + > +/** > + * @qcrypto_block_decrypt: > + * @block: the block encryption object > + * @startsector: the sector from which @buf was read From the guest's point of view, right? > + * @buf: the buffer to decrypt > + * @len: the length of @buf in bytes > + * @errp: pointer to a NULL-initialized error object > + * > + * Decrypt @len bytes of cipher text in @buf, writing > + * plain text back into @buf > + * > + * Returns 0 on success, -1 on failure > + */ > +int qcrypto_block_decrypt(QCryptoBlock *block, > + uint64_t startsector, > + uint8_t *buf, > + size_t len, > + Error **errp); > + > +/** > + * @qcrypto_block_encrypt: > + * @block: the block encryption object > + * @startsector: the sector to which @buf will be written Likewise, from the guest's point of view, right? > + * @buf: the buffer to decrypt > + * @len: the length of @buf in bytes > + * @errp: pointer to a NULL-initialized error object > + * > + * Encrypt @len bytes of plain text in @buf, writing > + * cipher text back into @buf > + * > + * Returns 0 on success, -1 on failure > + */ > +int qcrypto_block_encrypt(QCryptoBlock *block, > + uint64_t startsector, > + uint8_t *buf, > + size_t len, > + Error **errp); > + > +/** > + * qcrypto_block_get_payload_offset: > + * @block: the block encryption object > + * > + * Get the offset to the payload indicated by the > + * encryption header. The offset is measured in > + * 512 byte sectors > + * > + * Returns: the payload offset in sectors. > + */ > +uint64_t qcrypto_block_get_payload_offset(QCryptoBlock *block); Please, let's use bytes here, not sectors. > +++ b/tests/test-crypto-block.c Nice tests. Overall looks like we are fairly close to having this ready to commit. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org