Re: [Qemu-devel] [PATCH v2 07/17] crypto: implement the LUKS block encryption format

[Eric Blake]

On 01/20/2016 10:38 AM, Daniel P. Berrange wrote:
> Provide a block encryption implementation that follows the
> LUKS/dm-crypt specification.
> 
> This supports all combinations of hash, cipher algorithm,
> cipher mode and iv generator that are implemented by the
> current crypto layer.
> 
> The notable missing feature is support for the 'xts'
> cipher mode, which is commonly used for disk encryption
> instead of 'cbc'. This is because it is not provided by
> either nettle or libgcrypt. A suitable implementation
> will be identified & integrated later.
> 
> There is support for opening existing volumes formatted
> by dm-crypt, and for formatting new volumes. In the latter
> case it will only use key slot 0.
> 
> Signed-off-by: Daniel P. Berrange <address@hidden>
> ---

> +++ b/crypto/block-luks.c
> @@ -0,0 +1,1105 @@
> +/*
> + * QEMU Crypto block device encryption LUKS format
> + *

> +
> +/*
> + * Reference for format is
> + *
> + * docs/on-disk-format.pdf
> + *
> + * In 'cryptsetup' package source code
> + *

Worth calling out the document version number? I reviewed based on
version 1.2.1, dated Oct 2011.

> + */
> +
> +typedef struct QCryptoBlockLUKS QCryptoBlockLUKS;
> +typedef struct QCryptoBlockLUKSHeader QCryptoBlockLUKSHeader;
> +typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot;
> +
> +#define QCRYPTO_BLOCK_LUKS_VERSION 1
> +
> +#define QCRYPTO_BLOCK_LUKS_MAGIC_LEN 6
> +#define QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN 32
> +#define QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN 32
> +#define QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN 32
> +#define QCRYPTO_BLOCK_LUKS_DIGEST_LEN 20
> +#define QCRYPTO_BLOCK_LUKS_SALT_LEN 32
> +#define QCRYPTO_BLOCK_LUKS_UUID_LEN 40

Matches the spec, but seems awfully wasteful.  A binary UUID is 16
bytes; the standard ASCII representation is only 36 bytes.  But the spec
calls it out as char[] (requiring a 37th byte for a NUL terminator, then
rounding it up for aligned field access).

> +#define QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS 8
> +#define QCRYPTO_BLOCK_LUKS_STRIPES 4000
> +#define QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS 1000
> +#define QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS 1000
> +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET 4096
> +
> +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED 0x0000DEAD
> +#define QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED 0x00AC71F3

Cute l33t spelling choice by the upstream folks :)


> +
> +static const QCryptoBlockLUKSCipherSizeMap
> +qcrypto_block_luks_cipher_size_map_aes[] = {
> +    { 16, QCRYPTO_CIPHER_ALG_AES_128 },
> +    { 24, QCRYPTO_CIPHER_ALG_AES_192 },
> +    { 32, QCRYPTO_CIPHER_ALG_AES_256 },
> +    { 0, 0 },
> +};
> +
> +static const QCryptoBlockLUKSCipherNameMap
> +qcrypto_block_luks_cipher_name_map[] = {
> +    { "aes", qcrypto_block_luks_cipher_size_map_aes, },

Inconsistent on the use of trailing , within the inner {} between these
two arrays.

> +};
> +
> +
> +struct QCryptoBlockLUKSKeySlot {
> +    /* state of keyslot, enabled/disable */
> +    uint32_t active;

May be worth a comment that this struct is written to disk in
big-endian, but used in native-endian for most operations for
convenience; to make sure future developers remember to put byteswaps in
the correct locations.  But it looks like you handled endianness correctly.

> +    /* iterations for PBKDF2 */
> +    uint32_t iterations;
> +    /* salt for PBKDF2 */
> +    uint8_t salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> +    /* start sector of key material */
> +    uint32_t key_offset;
> +    /* number of anti-forensic stripes */
> +    uint32_t stripes;
> +} QEMU_PACKED;

Packed makes sense since we are writing it straight to disk; although it
looks like natural alignment prevails and this did not need to be
packed.  Do we want some sort of BUG_ON() that we have the right size
struct?

> +
> +struct QCryptoBlockLUKSHeader {
> +    /* 'L', 'U', 'K', 'S', '0xBA', '0xBE' */
> +    char magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN];
> +
> +    /* LUKS version, currently 1 */
> +    uint16_t version;
> +
> +    /* cipher name specification (aes, etc */

Missing )

> +    char cipher_name[QCRYPTO_BLOCK_LUKS_CIPHER_NAME_LEN];
> +
> +    /* cipher mode specification () */

text in the ()?

> +    char cipher_mode[QCRYPTO_BLOCK_LUKS_CIPHER_MODE_LEN];
> +
> +    /* hash specication () */

s/specication/specification/, text in the ()?

> +    char hash_spec[QCRYPTO_BLOCK_LUKS_HASH_SPEC_LEN];
> +
> +    /* start offset of the volume data (in sectors) */
> +    uint32_t payload_offset;

Should we say "in 512-byte sectors"?  I could not see anything in the
LUKS specification that said whether SECTOR_SIZE can be larger than 512
- if you have a bare metal 4k sector disk, does a payload_offset of 2
mean 1024 or 8192 bytes into the disk?  Eww. Sounds like we should
submit a bug against the LUKS spec to have it clarified.

> +
> +    /* Number of key bytes */
> +    uint32_t key_bytes;
> +
> +    /* master key checksum after PBKDF2 */
> +    uint8_t master_key_digest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> +
> +    /* salt for master key PBKDF2 */
> +    uint8_t master_key_salt[QCRYPTO_BLOCK_LUKS_SALT_LEN];
> +
> +    /* iterations for master key PBKDF2 */
> +    uint32_t master_key_iterations;
> +
> +    /* UUID of the partition */

Should we say "in standard ASCII representation"?

> +    uint8_t uuid[QCRYPTO_BLOCK_LUKS_UUID_LEN];
> +
> +    /* key slots */
> +    QCryptoBlockLUKSKeySlot key_slots[QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS];
> +} QEMU_PACKED;

Again, alignment should mean that we could get away without packing; but
the packing doesn't hurt; and a BUG_ON may be nice to ensure size.


> +}
> +
> +/* XXX doesn't QEMU already have a string -> enum val convertor ? */
> +static int qcrypto_block_luks_name_lookup(const char *name,

Yes, spelled qapi_enum_parse().  Works if the enum was defined in a
.json file.


> +
> +    error_setg(errp, "%s %s not supported", type, name);
> +    return 0;
> +}
> +
> +#define qcrypto_block_luks_cipher_mode_lookup(name, errp)               \
> +    qcrypto_block_luks_name_lookup(name,                                \
> +                                   QCryptoCipherMode_lookup,            \
> +                                   QCRYPTO_CIPHER_MODE__MAX,            \
> +                                   "Cipher mode",                       \
> +                                   errp)
> +
> +#define qcrypto_block_luks_hash_name_lookup(name, errp)                 \
> +    qcrypto_block_luks_name_lookup(name,                                \
> +                                   QCryptoHashAlgorithm_lookup,         \
> +                                   QCRYPTO_HASH_ALG__MAX,               \
> +                                   "Hash algorithm",                    \
> +                                   errp)

That said, your macro wrappers tweak the error message, so that it is
perhaps more legible than the standard failure mode of
qapi_enum_parse().  So up to you if you want to keep the wrappers.

> +/*
> + * Given a key slot, and user password, this will attempt to unlock
> + * the master encryption key from the key slot
> + */
> +static int
> +qcrypto_block_luks_load_key(QCryptoBlock *block,
> +                            QCryptoBlockLUKSKeySlot *slot,
> +                            const char *password,
> +                            QCryptoCipherAlgorithm cipheralg,
> +                            QCryptoCipherMode ciphermode,
> +                            QCryptoHashAlgorithm hash,
> +                            QCryptoIVGenAlgorithm ivalg,
> +                            QCryptoHashAlgorithm ivhash,
> +                            uint8_t *masterkey,
> +                            size_t masterkeylen,
> +                            QCryptoBlockReadFunc readfunc,
> +                            void *opaque,
> +                            Error **errp)
> +{
> +    QCryptoBlockLUKS *luks = block->opaque;
> +    uint8_t *splitkey;
> +    size_t splitkeylen;
> +    uint8_t *possiblekey;
> +    int ret = -1;
> +    ssize_t rv;
> +    QCryptoCipher *cipher = NULL;
> +    uint8_t keydigest[QCRYPTO_BLOCK_LUKS_DIGEST_LEN];
> +    QCryptoIVGen *ivgen = NULL;
> +    size_t niv;
> +
> +    if (slot->active != QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED) {
> +        return 0;
> +    }

Would be worth documenting the convention of '0' (no key found, but no
errp set), '1' (successful key), and negative (errp set).

> +
> +    splitkeylen = masterkeylen * slot->stripes;

Should we do any validation that masterkeylen was in bounds and this
doesn't overflow?  I don't want a malicious byte stream masquerading as
a LUKS header to cause us to misbehave.

For that matter, the spec states that luks->payload_offset is a
write-once calculation at creation time, and that it could be recomputed
from key layout.  Should we validate that the offset listed makes
algorithmic sense with what we know about header and key data sizes for
the parameters listed in the header?

Side note: I'm a bit surprised the upstream LUKS spec does not include
any checksum over the header contents - it does a great job at ensuring
the master key is only going to be found by someone knowing a correct
password and that a single byte error in a key material section
invalidates that entire slot's usefulness, but it is not as careful on
what happens with certain single-byte errors in the header.

> +    splitkey = g_new0(uint8_t, splitkeylen);
> +    possiblekey = g_new0(uint8_t, masterkeylen);
> +
> +    /*
> +     * The user password is used to generate a (possible)
> +     * decryption key. This may or may not successfully
> +     * decrypt the master key - we just blindly assume
> +     * the key is correct and validate the results of
> +     * decryption later.
> +     */
> +    if (qcrypto_pbkdf2(hash,
> +                       (const uint8_t *)password, strlen(password),
> +                       slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN,
> +                       slot->iterations,
> +                       possiblekey, masterkeylen,
> +                       errp) < 0) {
> +        goto cleanup;
> +    }
> +
> +    /*
> +     * We need to read the master key material from the
> +     * LUKS key material header. What we're reading is
> +     * not the raw master key, but rather the data after
> +     * it has been passed through AFSplit and the result
> +     * then encrypted.
> +     */
> +    rv = readfunc(block,
> +                  slot->key_offset * 512ll,

I tend to write LL rather than ll, to make it obvious I didn't typo '1'.
 Looks like we're hard-coding our sector size (not the first time); but
maybe a constant instead of a magic number will make it easier for the
poor guy down the road that tries to switch to 4k sectors.


...
> +
> +    /*
> +     * Now we're decrypted the split master key, join

s/we're/that we've/

> +     * it back together to get the actual master key.
> +     */
> +    if (qcrypto_afsplit_decode(hash,

...
> +
> +    if (memcmp(keydigest, luks->header.master_key_digest,
> +               QCRYPTO_BLOCK_LUKS_DIGEST_LEN) == 0) {
> +        /* Success, we got the right master key */
> +        ret = 1;
> +        goto cleanup;
> +    }
> +
> +    /* Fail, user's password was not valid for this key slot,
> +     * tell caller to try another slot */
> +    ret = 0;
> +

Matches the spec.  Yay!

Side observation: The LUKS spec doesn't say anything about parallelizing
the lookup loop across all 8 key slots, nor any randomization on which
slot should be attempted first.  The whole point of PBKDF2 iteration
count is to burn CPU time so that an attacker can't brute force things
easily.  That means it is VERY easy to tell by timing how many active
slots were attempted before a key was found, if slots are tried serially
and we immediately break the loop on the first successful decryption.
Is there any information leak caused by the timing observations when
serially searching among 8 active slots, when the master key is only
found in slot 7 vs. slot 0?  But I don't see it as your problem to
solve, so much as something to ask the upstream LUKS design team.

> +
> +
> +static int
> +qcrypto_block_luks_open(QCryptoBlock *block,
> +                        QCryptoBlockOpenOptions *options,
> +                        QCryptoBlockReadFunc readfunc,
> +                        void *opaque,
> +                        unsigned int flags,
> +                        Error **errp)
> +{

> +    /*
> +     * The cipher_mode header contains a string that we have
> +     * to further parse of the format

s/parse/parse,/

> +     *
> +     *    <cipher-mode>-<iv-generator>[:<iv-hash>]
> +     *
> +     * eg  cbc-essiv:sha256, cbc-plain64
> +     */
> +    ivgen_name = strchr(luks->header.cipher_mode, '-');
> +    if (!ivgen_name) {
> +        ret = -EINVAL;
> +        error_setg(errp, "Unexpected cipher mode string format %s",
> +                   luks->header.cipher_mode);
> +        goto fail;
> +    }
> +    *ivgen_name = '\0';
> +    ivgen_name++;

We're modifying luks->header in place; we'd better not write it back out
to disk in the modified form.  I guess you are okay for now - your code
only reads existing LUKS disks, and can only create a single key in slot
0 on conversion (that is, I don't see code for key management of an
existing image).  Probably things we should add later, though, at which
point we'll need to be careful that we don't overwrite too much in the
header.

> +
> +    block->payload_offset = luks->header.payload_offset;

Earlier, I argued that block->payload_offset should be in bytes.  You
are constrained that luks->header.payload_offset is in sectors, but we
need not propagate that craziness any higher.

> +
> +static int
> +qcrypto_block_luks_create(QCryptoBlock *block,
> +                          QCryptoBlockCreateOptions *options,
> +                          QCryptoBlockInitFunc initfunc,
> +                          QCryptoBlockWriteFunc writefunc,
> +                          void *opaque,
> +                          Error **errp)
> +{

> +
> +    memcpy(&luks_opts, options->u.luks, sizeof(luks_opts));
> +    if (!luks_opts.has_cipher_alg) {
> +        luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256;
> +    }
> +    if (!luks_opts.has_cipher_mode) {
> +        /* XXX switch to XTS */
> +        luks_opts.cipher_mode = QCRYPTO_CIPHER_MODE_CBC;
> +    }

Yeah, my (quick) reading made it seem like XTS is a slightly more secure
default than CBC.  But as you said, XTS implementation will come later.


> +    luks = g_new0(QCryptoBlockLUKS, 1);
> +    block->opaque = luks;
> +
> +    memcpy(luks->header.magic, qcrypto_block_luks_magic,
> +           QCRYPTO_BLOCK_LUKS_MAGIC_LEN);
> +
> +    luks->header.version = QCRYPTO_BLOCK_LUKS_VERSION;

Maybe a comment here that endianness will be addressed later, as a lot
of code appears between here and the actual write-to-disk.

> +    uuid_generate(uuid);
> +    uuid_unparse(uuid, (char *)luks->header.uuid);
> +
> +    switch (luks_opts.cipher_alg) {
> +    case QCRYPTO_CIPHER_ALG_AES_128:
> +    case QCRYPTO_CIPHER_ALG_AES_192:
> +    case QCRYPTO_CIPHER_ALG_AES_256:
> +        cipher_alg = "aes";
> +        break;
> +    default:
> +        error_setg(errp, "Cipher algorithm is not supported");
> +        goto error;

That is, we aren't supporting "twofish", "serpent", "cast5", or "cast6".
 Fine for now.

> +    }
> +
> +    cipher_mode = QCryptoCipherMode_lookup[luks_opts.cipher_mode];
> +    ivgen_alg = QCryptoIVGenAlgorithm_lookup[luks_opts.ivgen_alg];
> +    if (luks_opts.has_ivgen_hash_alg) {
> +        ivgen_hash_alg = 
> QCryptoHashAlgorithm_lookup[luks_opts.ivgen_hash_alg];
> +        cipher_mode_spec = g_strdup_printf("%s-%s:%s", cipher_mode, 
> ivgen_alg,
> +                                           ivgen_hash_alg);
> +    } else {
> +        cipher_mode_spec = g_strdup_printf("%s-%s", cipher_mode, ivgen_alg);
> +    }

Should we validate that the mode spec is among the set documented by the
LUKS spec ("ecb", "cbc-plain", "cbc-essiv:hash", "xts-plain64") (well,
we don't support xts yet), and reject combinations that aren't supported
("cbc-plain64" comes to mind as something the LUKS spec doesn't allow,
even though we have the pieces to logically make it happen)?

> +    hash_alg = QCryptoHashAlgorithm_lookup[luks_opts.hash_alg];

Likewise, should we validate that the hash is one of the specified names
("sha1", "sha256", "sha512", "ripemd160")?


> +
> +    memcpy(luks->header.cipher_name, cipher_alg,
> +           strlen(cipher_alg) + 1);
> +    memcpy(luks->header.cipher_mode, cipher_mode_spec,
> +           strlen(cipher_mode_spec) + 1);
> +    memcpy(luks->header.hash_spec, hash_alg,
> +           strlen(hash_alg) + 1);

Couldn't these just be strcpy()?

> +
> +    /* Determine how many iterations we need to hash the master
> +     * key with in order to have 1 second of compute time used

s/with //

> +     */
> +    luks->header.master_key_iterations =
> +        qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
> +                                   masterkey, luks->header.key_bytes,
> +                                   luks->header.master_key_salt,
> +                                   QCRYPTO_BLOCK_LUKS_SALT_LEN,
> +                                   &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto error;
> +    }
> +
> +    /* Why /= 8 ?  That matches cryptsetup, but there's no
> +     * explanation why they chose /= 8... */

My guess?  There's 8 key slots, and if all 8 are active, you'll spend 1
second serially decrypting each of them.  Users got ticked waiting that
long, so the /=8 reduces it to 125ms per slot, and for the typical use
case of slot0 being the user's password, much faster response at opening
the device.  But that's a guess - you're right that it could be
documented better.

> +    luks->header.master_key_iterations /= 8;
> +    luks->header.master_key_iterations = MAX(
> +        luks->header.master_key_iterations,
> +        QCRYPTO_BLOCK_LUKS_MIN_MASTER_KEY_ITERS);
> +
> +
> +    /* Hash the master key, saving the result in the LUKS
> +     * header. This hash is used when opening the encrypted
> +     * device to verify that the user password unlocked a
> +     * valid master key
> +     */
> +    if (qcrypto_pbkdf2(luks_opts.hash_alg,
> +                       masterkey, luks->header.key_bytes,
> +                       luks->header.master_key_salt,
> +                       QCRYPTO_BLOCK_LUKS_SALT_LEN,
> +                       luks->header.master_key_iterations,
> +                       luks->header.master_key_digest,
> +                       QCRYPTO_BLOCK_LUKS_DIGEST_LEN,
> +                       errp) < 0) {
> +        goto error;
> +    }
> +
> +
> +    /* Although LUKS has multiple key slots, we're just going
> +     * to use the first key slot */
> +
> +    splitkeylen = luks->header.key_bytes * QCRYPTO_BLOCK_LUKS_STRIPES;

The LUKS spec says that it is okay to allow the user to specify stripes.
 Should that be one of our options, with a sane default?  But it can be
added later as a followup, this patch is already quite big and I'm fine
with hardcoding stripes for now.

> +    for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) {
> +        luks->header.key_slots[i].active = i == 0 ?
> +            QCRYPTO_BLOCK_LUKS_KEY_SLOT_ENABLED :
> +            QCRYPTO_BLOCK_LUKS_KEY_SLOT_DISABLED;
> +        luks->header.key_slots[i].stripes = QCRYPTO_BLOCK_LUKS_STRIPES;
> +        luks->header.key_slots[i].key_offset =
> +            (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> +            (ROUND_UP(((splitkeylen + 511) / 512),
> +                      (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) * i);

Eww. The spec seems buggy, saying:

> // integer divisions, result rounded down:
> baseOffset = (size of phdr)/SECTOR SIZE + 1
> keymaterialSectors = (stripes*masterKeyLength)/SECTOR SIZE + 1

First, look at the calculation of baseOffset.  Since size of phdr is 592
bytes, and that is NOT a SECTOR SIZE in any disk I know of, that makes
sense that baseOffset will be at least 2 (512-byte) sectors into the
disk, or, if SECTOR SIZE is 4, that will result in an offset of 1
(4096-byte) sector.  However, you've defined
QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET as 4096, which means for our 512-byte
sector usage, you are setting the first key material at sector 4 no
matter what.  I guess that matches what cryptsetup does?  Maybe worth a
comment?

Now, look at the calculation of keymaterial Sectors.  The algorithm in
the spec mishandles the case where stripes happens to be an exact
multiple of sector size (that is, for a keysize of 20 bytes coupled with
512 stripes, it would reserve 21 sectors rather than the 20 actually
required).  I think your use of ROUND_UP() makes more sense, but it
would be nice to file a bug against the LUKS spec to have them fix their
math, and/or document that it is okay to use values larger than the
absolute minimum.

> +    }
> +
> +    if (qcrypto_random_bytes(luks->header.key_slots[0].salt,
> +                             QCRYPTO_BLOCK_LUKS_SALT_LEN,
> +                             errp) < 0) {
> +        goto error;
> +    }
> +
> +    /* Again we determine how many iterations are required to
> +     * hash the user password while consuming 1 second of compute
> +     * time */
> +    luks->header.key_slots[0].iterations =
> +        qcrypto_pbkdf2_count_iters(luks_opts.hash_alg,
> +                                   (uint8_t *)password, strlen(password),
> +                                   luks->header.key_slots[0].salt,
> +                                   QCRYPTO_BLOCK_LUKS_SALT_LEN,
> +                                   &local_err);
> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto error;
> +    }
> +    /* Why /= 2 ?  That matches cryptsetup, but there's no
> +     * explanation why they chose /= 2... */

Again, guessing that half a second feels nicer than 1 second for
responsiveness.

> +    luks->header.key_slots[0].iterations /= 2;
> +    luks->header.key_slots[0].iterations = MAX(
> +        luks->header.key_slots[0].iterations,
> +        QCRYPTO_BLOCK_LUKS_MIN_SLOT_KEY_ITERS);
> +

> +
> +    /* Before storing the master key, we need to vastly
> +     * increase its size, as protection against forensic
> +     * disk data recovery */
> +    splitkey = g_new0(uint8_t, splitkeylen);

If we later add the ability to do key management on an active disk, this
allocation may be large enough that it would warrant the _try variant
and reporting ENOMEM, rather than failing.


> +    }
> +
> +
> +
> +    /* The total size of the LUKS headers is the partition header + key

Three blank lines?

> +     * slot headers, rounded up to the nearest sector, combined with
> +     * the size of each master key material region, also rounded up
> +     * to the nearest sector */
> +    luks->header.payload_offset =
> +        (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512) +
> +        (ROUND_UP(((splitkeylen + 511) / 512),
> +                  (QCRYPTO_BLOCK_LUKS_KEY_SLOT_OFFSET / 512)) *
> +         QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS);
> +
> +    block->payload_offset = luks->header.payload_offset;

Again, I argue that block->payload_offset would be saner in bytes.

> +
> +    /* Reserve header space to match payload offset */
> +    initfunc(block, block->payload_offset * 512, &local_err, opaque);

Especially since initfunc() takes bytes.

> +    if (local_err) {
> +        error_propagate(errp, local_err);
> +        goto error;
> +    }
> +
> +    /* Everything on disk uses Big Endian, so flip header fields
> +     * before writing them */

> +
> +    /* Write out the master key material, starting at the
> +     * sector immediately following the partition header. */
> +    if (writefunc(block,
> +                  luks->header.key_slots[0].key_offset * 512,
> +                  splitkey, splitkeylen,
> +                  errp,
> +                  opaque) != splitkeylen) {
> +        goto error;
> +    }
> +
> +    memset(masterkey, 0, luks->header.key_bytes);
> +    g_free(masterkey);

Nice, trying to avoid leaking the key from a heap attack.

> +
> +static int
> +qcrypto_block_luks_decrypt(QCryptoBlock *block,
> +                           uint64_t startsector,
> +                           uint8_t *buf,
> +                           size_t len,
> +                           Error **errp)
> +{
> +    return qcrypto_block_decrypt_helper(block->cipher,
> +                                        block->niv, block->ivgen,
> +                                        startsector, buf, len, errp);
> +}

What happens when we try to read a LUKS device with 4k sectors?  Worth
worrying about, maybe just as a comment that we are hard-coded to 512
bytes at the moment?

> +++ b/qapi/crypto.json
> @@ -102,12 +102,13 @@
>  #
>  # @qcow: QCow/QCow2 built-in AES-CBC encryption. Use only
>  #        for liberating data from old images.
> +# @luks: LUKS encryption format. Recommended for new images
>  #
>  # Since: 2.6
>  ##
>  { 'enum': 'QCryptoBlockFormat',
>  #  'prefix': 'QCRYPTO_BLOCK_FORMAT',
> -  'data': ['qcow']}
> +  'data': ['qcow', 'luks']}
>  
>  ##
>  # QCryptoBlockOptionsBase:
> @@ -136,6 +137,40 @@
>    'data': { '*key-secret': 'str' }}
>  
>  ##
> +# QCryptoBlockOptionsLUKS:
> +#
> +# The options that apply to LUKS encryption format
> +#
> +# @key-secret: #optional the ID of a QCryptoSecret object providing the
> +#              decryption key

Maybe add "Mandatory except when probing the image for metadata only"

> +# Since: 2.6
> +##
> +{ 'struct': 'QCryptoBlockOptionsLUKS',
> +  'data': { '*key-secret': 'str' }}
> +
> +
> +##
> +# QCryptoBlockCreateOptionsLUKS:
> +#
> +# The options that apply to LUKS encryption format initialization
> +#
> +# @cipher-alg: #optional the cipher algorithm for data encryption
> +# @cipher-mode: #optional the cipher mode for data encryption
> +# @ivgen-alg: #optional the initialization vector generator
> +# @ivgen-hash-alg: #optional the initialization vector generator hash
> +# @hash-alg: #optional the master key hash algorithm

Would be nice to mention the defaults, particularly since we may later
change the default from cbc to xts.

> +# Since: 2.6
> +##
> +{ 'struct': 'QCryptoBlockCreateOptionsLUKS',
> +  'base': 'QCryptoBlockOptionsLUKS',
> +  'data': { '*cipher-alg': 'QCryptoCipherAlgorithm',
> +            '*cipher-mode': 'QCryptoCipherMode',
> +            '*ivgen-alg': 'QCryptoIVGenAlgorithm',
> +            '*ivgen-hash-alg': 'QCryptoHashAlgorithm',
> +            '*hash-alg': 'QCryptoHashAlgorithm'}}
> +

As mentioned earlier, we might also want to make stripes be customizable.

> +++ b/tests/test-crypto-block.c
> @@ -40,6 +40,69 @@ static QCryptoBlockOpenOptions qcow_open_opts = {
>      .u.qcow = &qcow_opts,
>  };
>  

> +    {
> +        .path = "/crypto/block/luks/aes-256-cbc-eesiv",
> +        .create_opts = &luks_create_opts_aes256_cbc_essiv,

s/eesiv/essiv/

Wow. A lot to read through, but looks mostly sane. v3 will probably be
ready to go; meanwhile, I spotted enough typos in the LUKS spec that I'm
half tempted to check out their source code and submit a patch.

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

signature.asc
Description: OpenPGP digital signature