| Message ID | 20190814202219.1870-3-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC: luks/encrypted qcow2 key management | expand |
On 8/14/19 4:22 PM, Maxim Levitsky wrote: > This is also a preparation for key read/write/erase functions > This is a matter of taste and I am not usually reviewing LUKS patches (So don't take me too seriously), but I would prefer not to have "misc" patches and instead split things out by individual changes along with a nice commit message for each change. > * use master key len from the header This touches enough lines that you could make it its own patch, I think. > * prefer to use crypto params in the QCryptoBlockLUKS > over passing them as function arguments I think the same is true here, and highlighting which variables you are sticking into state instead of leaving as functional parameters would be nice to see without all the other changes. > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME This can likely be squashed with whichever patch of yours first needs to use it, because it's so short. > * Add comments to various crypto parameters in the QCryptoBlockLUKS > Can probably be squashed with item #2. > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- > 1 file changed, 105 insertions(+), 108 deletions(-) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index 409ab50f20..48213abde7 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c > @@ -70,6 +70,8 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; > > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL > > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000 > + > static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = { > 'L', 'U', 'K', 'S', 0xBA, 0xBE > }; > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); > struct QCryptoBlockLUKS { > QCryptoBlockLUKSHeader header; > > - /* Cache parsed versions of what's in header fields, > - * as we can't rely on QCryptoBlock.cipher being > - * non-NULL */ > + /* Main encryption algorithm used for encryption*/ > QCryptoCipherAlgorithm cipher_alg; > + > + /* Mode of encryption for the selected encryption algorithm */ > QCryptoCipherMode cipher_mode; > + > + /* Initialization vector generation algorithm */ > QCryptoIVGenAlgorithm ivgen_alg; > + > + /* Hash algorithm used for IV generation*/ > QCryptoHashAlgorithm ivgen_hash_alg; > + > + /* > + * Encryption algorithm used for IV generation. > + * Usually the same as main encryption algorithm > + */ > + QCryptoCipherAlgorithm ivgen_cipher_alg; > + > + /* Hash algorithm used in pbkdf2 function */ > QCryptoHashAlgorithm hash_alg; > }; > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > } > } > > +static int masterkeylen(QCryptoBlockLUKS *luks) > +{ > + return luks->header.key_bytes; > +} > + > + generally QEMU uses snake_case_names; please spell as "master_key_len". > /* > * Given a key slot, and user password, this will attempt to unlock > * the master encryption key from the key slot. > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > */ > static int > qcrypto_block_luks_load_key(QCryptoBlock *block, > - QCryptoBlockLUKSKeySlot *slot, > + uint slot_idx, > const char *password, > - QCryptoCipherAlgorithm cipheralg, > - QCryptoCipherMode ciphermode, > - QCryptoHashAlgorithm hash, > - QCryptoIVGenAlgorithm ivalg, > - QCryptoCipherAlgorithm ivcipheralg, > - QCryptoHashAlgorithm ivhash, > uint8_t *masterkey, > - size_t masterkeylen, > QCryptoBlockReadFunc readfunc, > void *opaque, > Error **errp) > { > QCryptoBlockLUKS *luks = block->opaque; > + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx]; > uint8_t *splitkey; > size_t splitkeylen; > uint8_t *possiblekey; > @@ -439,9 +453,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > return 0; > } > > - splitkeylen = masterkeylen * slot->stripes; > + splitkeylen = masterkeylen(luks) * slot->stripes; > splitkey = g_new0(uint8_t, splitkeylen); > - possiblekey = g_new0(uint8_t, masterkeylen); > + possiblekey = g_new0(uint8_t, masterkeylen(luks)); > > /* > * The user password is used to generate a (possible) > @@ -450,11 +464,11 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > * the key is correct and validate the results of > * decryption later. > */ > - if (qcrypto_pbkdf2(hash, > + if (qcrypto_pbkdf2(luks->hash_alg, > (const uint8_t *)password, strlen(password), > slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN, > slot->iterations, > - possiblekey, masterkeylen, > + possiblekey, masterkeylen(luks), > errp) < 0) { > goto cleanup; > } > @@ -478,19 +492,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > /* Setup the cipher/ivgen that we'll use to try to decrypt > * the split master key material */ > - cipher = qcrypto_cipher_new(cipheralg, ciphermode, > - possiblekey, masterkeylen, > + cipher = qcrypto_cipher_new(luks->cipher_alg, luks->cipher_mode, > + possiblekey, masterkeylen(luks), > errp); > if (!cipher) { > goto cleanup; > } > > - niv = qcrypto_cipher_get_iv_len(cipheralg, > - ciphermode); > - ivgen = qcrypto_ivgen_new(ivalg, > - ivcipheralg, > - ivhash, > - possiblekey, masterkeylen, > + niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, > + luks->cipher_mode); > + ivgen = qcrypto_ivgen_new(luks->ivgen_alg, > + luks->ivgen_cipher_alg, > + luks->ivgen_hash_alg, > + possiblekey, masterkeylen(luks), > errp); > if (!ivgen) { > goto cleanup; > @@ -519,8 +533,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > * Now we've decrypted the split master key, join > * it back together to get the actual master key. > */ > - if (qcrypto_afsplit_decode(hash, > - masterkeylen, > + if (qcrypto_afsplit_decode(luks->hash_alg, > + masterkeylen(luks), > slot->stripes, > splitkey, > masterkey, > @@ -537,8 +551,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > * then comparing that to the hash stored in the key slot > * header > */ > - if (qcrypto_pbkdf2(hash, > - masterkey, masterkeylen, > + if (qcrypto_pbkdf2(luks->hash_alg, > + masterkey, masterkeylen(luks), > luks->header.master_key_salt, > QCRYPTO_BLOCK_LUKS_SALT_LEN, > luks->header.master_key_iterations, > @@ -577,37 +591,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > static int > qcrypto_block_luks_find_key(QCryptoBlock *block, > const char *password, > - QCryptoCipherAlgorithm cipheralg, > - QCryptoCipherMode ciphermode, > - QCryptoHashAlgorithm hash, > - QCryptoIVGenAlgorithm ivalg, > - QCryptoCipherAlgorithm ivcipheralg, > - QCryptoHashAlgorithm ivhash, > - uint8_t **masterkey, > - size_t *masterkeylen, > + uint8_t *masterkey, > QCryptoBlockReadFunc readfunc, > void *opaque, > Error **errp) > { > - QCryptoBlockLUKS *luks = block->opaque; > size_t i; > int rv; > > - *masterkey = g_new0(uint8_t, luks->header.key_bytes); > - *masterkeylen = luks->header.key_bytes; > - > for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > rv = qcrypto_block_luks_load_key(block, > - &luks->header.key_slots[i], > + i, > password, > - cipheralg, > - ciphermode, > - hash, > - ivalg, > - ivcipheralg, > - ivhash, > - *masterkey, > - *masterkeylen, > + masterkey, > readfunc, > opaque, > errp); > @@ -620,11 +616,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, > } > > error_setg(errp, "Invalid password, cannot unlock any keyslot"); > - > error: > - g_free(*masterkey); > - *masterkey = NULL; > - *masterkeylen = 0; > return -1; > } > > @@ -639,21 +631,15 @@ qcrypto_block_luks_open(QCryptoBlock *block, > size_t n_threads, > Error **errp) > { > - QCryptoBlockLUKS *luks; > + QCryptoBlockLUKS *luks = NULL; > Error *local_err = NULL; > int ret = 0; > size_t i; > ssize_t rv; > uint8_t *masterkey = NULL; > - size_t masterkeylen; > char *ivgen_name, *ivhash_name; > - QCryptoCipherMode ciphermode; > - QCryptoCipherAlgorithm cipheralg; > - QCryptoIVGenAlgorithm ivalg; > - QCryptoCipherAlgorithm ivcipheralg; > - QCryptoHashAlgorithm hash; > - QCryptoHashAlgorithm ivhash; > char *password = NULL; > + char *cipher_mode = NULL; > > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > if (!options->u.luks.key_secret) { > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block, > goto fail; > } > > + cipher_mode = g_strdup(luks->header.cipher_mode); > + > /* > * The cipher_mode header contains a string that we have > * to further parse, of the format > @@ -718,7 +706,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > * > * eg cbc-essiv:sha256, cbc-plain64 > */ > - ivgen_name = strchr(luks->header.cipher_mode, '-'); > + ivgen_name = strchr(cipher_mode, '-'); > if (!ivgen_name) { > ret = -EINVAL; > error_setg(errp, "Unexpected cipher mode string format %s", > @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > ivhash_name = strchr(ivgen_name, ':'); > if (!ivhash_name) { > - ivhash = 0; > + luks->ivgen_hash_alg = 0; > } else { > *ivhash_name = '\0'; > ivhash_name++; > > - ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name, > - &local_err); > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block, > } > } > > - ciphermode = qcrypto_block_luks_cipher_mode_lookup(luks->header.cipher_mode, > - &local_err); > + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > goto fail; > } > > - cipheralg = qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > - ciphermode, > - luks->header.key_bytes, > - &local_err); > + luks->cipher_alg = > + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > + luks->cipher_mode, > + luks->header.key_bytes, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > goto fail; > } > > - hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > + luks->hash_alg = > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > &local_err); > if (local_err) { > ret = -ENOTSUP; > @@ -770,23 +760,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, > goto fail; > } > > - ivalg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > - &local_err); > + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > goto fail; > } > > - if (ivalg == QCRYPTO_IVGEN_ALG_ESSIV) { > + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { > if (!ivhash_name) { > ret = -EINVAL; > error_setg(errp, "Missing IV generator hash specification"); > goto fail; > } > - ivcipheralg = qcrypto_block_luks_essiv_cipher(cipheralg, > - ivhash, > - &local_err); > + luks->ivgen_cipher_alg = > + qcrypto_block_luks_essiv_cipher(luks->cipher_alg, > + luks->ivgen_hash_alg, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > @@ -800,21 +791,25 @@ qcrypto_block_luks_open(QCryptoBlock *block, > * ignore hash names with these ivgens rather than report > * an error about the invalid usage > */ > - ivcipheralg = cipheralg; > + luks->ivgen_cipher_alg = luks->cipher_alg; > } > > + > + g_free(cipher_mode); > + cipher_mode = NULL; > + ivgen_name = NULL; > + ivhash_name = NULL; > + > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > /* Try to find which key slot our password is valid for > * and unlock the master key from that slot. > */ > + > + masterkey = g_new0(uint8_t, masterkeylen(luks)); > + > if (qcrypto_block_luks_find_key(block, > password, > - cipheralg, ciphermode, > - hash, > - ivalg, > - ivcipheralg, > - ivhash, > - &masterkey, &masterkeylen, > + masterkey, > readfunc, opaque, > errp) < 0) { > ret = -EACCES; > @@ -824,21 +819,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, > /* We have a valid master key now, so can setup the > * block device payload decryption objects > */ > - block->kdfhash = hash; > - block->niv = qcrypto_cipher_get_iv_len(cipheralg, > - ciphermode); > - block->ivgen = qcrypto_ivgen_new(ivalg, > - ivcipheralg, > - ivhash, > - masterkey, masterkeylen, > + block->kdfhash = luks->hash_alg; > + block->niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, > + luks->cipher_mode); > + > + block->ivgen = qcrypto_ivgen_new(luks->ivgen_alg, > + luks->ivgen_cipher_alg, > + luks->ivgen_hash_alg, > + masterkey, masterkeylen(luks), > errp); > if (!block->ivgen) { > ret = -ENOTSUP; > goto fail; > } > > - ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode, > - masterkey, masterkeylen, n_threads, > + ret = qcrypto_block_init_cipher(block, luks->cipher_alg, > + luks->cipher_mode, > + masterkey, masterkeylen(luks), > + n_threads, > errp); > if (ret < 0) { > ret = -ENOTSUP; > @@ -850,12 +848,6 @@ qcrypto_block_luks_open(QCryptoBlock *block, > block->payload_offset = luks->header.payload_offset * > block->sector_size; > > - luks->cipher_alg = cipheralg; > - luks->cipher_mode = ciphermode; > - luks->ivgen_alg = ivalg; > - luks->ivgen_hash_alg = ivhash; > - luks->hash_alg = hash; > - > g_free(masterkey); > g_free(password); > > @@ -910,7 +902,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); > if (!luks_opts.has_iter_time) { > - luks_opts.iter_time = 2000; > + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME; > } > if (!luks_opts.has_cipher_alg) { > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > luks_opts.has_ivgen_hash_alg = true; > } > } > + > + luks = g_new0(QCryptoBlockLUKS, 1); > + block->opaque = luks; > + > + luks->cipher_alg = luks_opts.cipher_alg; > + luks->cipher_mode = luks_opts.cipher_mode; > + luks->ivgen_alg = luks_opts.ivgen_alg; > + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > + luks->hash_alg = luks_opts.hash_alg; > + > + > /* Note we're allowing ivgen_hash_alg to be set even for > * non-essiv iv generators that don't need a hash. It will > * be silently ignored, for compatibility with dm-crypt */ > @@ -944,8 +947,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, > return -1; > } > > - luks = g_new0(QCryptoBlockLUKS, 1); > - block->opaque = luks; > > memcpy(luks->header.magic, qcrypto_block_luks_magic, > QCRYPTO_BLOCK_LUKS_MAGIC_LEN); > @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > ivcipheralg = luks_opts.cipher_alg; > } > > + luks->ivgen_cipher_alg = ivcipheralg; > + > strcpy(luks->header.cipher_name, cipher_alg); > strcpy(luks->header.cipher_mode, cipher_mode_spec); > strcpy(luks->header.hash_spec, hash_alg); > @@ -1304,12 +1307,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, > goto error; > } > > - luks->cipher_alg = luks_opts.cipher_alg; > - luks->cipher_mode = luks_opts.cipher_mode; > - luks->ivgen_alg = luks_opts.ivgen_alg; > - luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > - luks->hash_alg = luks_opts.hash_alg; > - > memset(masterkey, 0, luks->header.key_bytes); > g_free(masterkey); > memset(slotkey, 0, luks->header.key_bytes); >
On Thu, 2019-08-15 at 17:40 -0400, John Snow wrote: > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > This is also a preparation for key read/write/erase functions > > > > This is a matter of taste and I am not usually reviewing LUKS patches > (So don't take me too seriously), but I would prefer not to have "misc" > patches and instead split things out by individual changes along with a > nice commit message for each change. > > > * use master key len from the header > > This touches enough lines that you could make it its own patch, I think. > > > * prefer to use crypto params in the QCryptoBlockLUKS > > over passing them as function arguments > > I think the same is true here, and highlighting which variables you are > sticking into state instead of leaving as functional parameters would be > nice to see without all the other changes. > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > This can likely be squashed with whichever patch of yours first needs to > use it, because it's so short. > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > Can probably be squashed with item #2. I mostly agree with you! I usually write everything as one big patch, and then split it. It takes time but this has the benefit of much less overhead during the development, and it forces kind of self review on me while doing the split. This patch I probably didn't split enough, and I'll do it later when I send the next revision. I only had split things to the extent that all the patches are readable and reviewable to avoid wasting time, on stuff that I will have to probably rewrite anyway. Thanks a lot for the feedback, Best regards, Maxim Levitsky > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- > > 1 file changed, 105 insertions(+), 108 deletions(-) > > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > > index 409ab50f20..48213abde7 100644 > > --- a/crypto/block-luks.c > > +++ b/crypto/block-luks.c > > @@ -70,6 +70,8 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; > > > > #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL > > > > +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000 > > + > > static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = { > > 'L', 'U', 'K', 'S', 0xBA, 0xBE > > }; > > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); > > struct QCryptoBlockLUKS { > > QCryptoBlockLUKSHeader header; > > > > - /* Cache parsed versions of what's in header fields, > > - * as we can't rely on QCryptoBlock.cipher being > > - * non-NULL */ > > + /* Main encryption algorithm used for encryption*/ > > QCryptoCipherAlgorithm cipher_alg; > > + > > + /* Mode of encryption for the selected encryption algorithm */ > > QCryptoCipherMode cipher_mode; > > + > > + /* Initialization vector generation algorithm */ > > QCryptoIVGenAlgorithm ivgen_alg; > > + > > + /* Hash algorithm used for IV generation*/ > > QCryptoHashAlgorithm ivgen_hash_alg; > > + > > + /* > > + * Encryption algorithm used for IV generation. > > + * Usually the same as main encryption algorithm > > + */ > > + QCryptoCipherAlgorithm ivgen_cipher_alg; > > + > > + /* Hash algorithm used in pbkdf2 function */ > > QCryptoHashAlgorithm hash_alg; > > }; > > > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > } > > } > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > +{ > > + return luks->header.key_bytes; > > +} > > + > > + > > generally QEMU uses snake_case_names; please spell as "master_key_len". > > > /* > > * Given a key slot, and user password, this will attempt to unlock > > * the master encryption key from the key slot. > > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > */ > > static int > > qcrypto_block_luks_load_key(QCryptoBlock *block, > > - QCryptoBlockLUKSKeySlot *slot, > > + uint slot_idx, > > const char *password, > > - QCryptoCipherAlgorithm cipheralg, > > - QCryptoCipherMode ciphermode, > > - QCryptoHashAlgorithm hash, > > - QCryptoIVGenAlgorithm ivalg, > > - QCryptoCipherAlgorithm ivcipheralg, > > - QCryptoHashAlgorithm ivhash, > > uint8_t *masterkey, > > - size_t masterkeylen, > > QCryptoBlockReadFunc readfunc, > > void *opaque, > > Error **errp) > > { > > QCryptoBlockLUKS *luks = block->opaque; > > + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx]; > > uint8_t *splitkey; > > size_t splitkeylen; > > uint8_t *possiblekey; > > @@ -439,9 +453,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > return 0; > > } > > > > - splitkeylen = masterkeylen * slot->stripes; > > + splitkeylen = masterkeylen(luks) * slot->stripes; > > splitkey = g_new0(uint8_t, splitkeylen); > > - possiblekey = g_new0(uint8_t, masterkeylen); > > + possiblekey = g_new0(uint8_t, masterkeylen(luks)); > > > > /* > > * The user password is used to generate a (possible) > > @@ -450,11 +464,11 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > * the key is correct and validate the results of > > * decryption later. > > */ > > - if (qcrypto_pbkdf2(hash, > > + if (qcrypto_pbkdf2(luks->hash_alg, > > (const uint8_t *)password, strlen(password), > > slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN, > > slot->iterations, > > - possiblekey, masterkeylen, > > + possiblekey, masterkeylen(luks), > > errp) < 0) { > > goto cleanup; > > } > > @@ -478,19 +492,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > > > /* Setup the cipher/ivgen that we'll use to try to decrypt > > * the split master key material */ > > - cipher = qcrypto_cipher_new(cipheralg, ciphermode, > > - possiblekey, masterkeylen, > > + cipher = qcrypto_cipher_new(luks->cipher_alg, luks->cipher_mode, > > + possiblekey, masterkeylen(luks), > > errp); > > if (!cipher) { > > goto cleanup; > > } > > > > - niv = qcrypto_cipher_get_iv_len(cipheralg, > > - ciphermode); > > - ivgen = qcrypto_ivgen_new(ivalg, > > - ivcipheralg, > > - ivhash, > > - possiblekey, masterkeylen, > > + niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, > > + luks->cipher_mode); > > + ivgen = qcrypto_ivgen_new(luks->ivgen_alg, > > + luks->ivgen_cipher_alg, > > + luks->ivgen_hash_alg, > > + possiblekey, masterkeylen(luks), > > errp); > > if (!ivgen) { > > goto cleanup; > > @@ -519,8 +533,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > * Now we've decrypted the split master key, join > > * it back together to get the actual master key. > > */ > > - if (qcrypto_afsplit_decode(hash, > > - masterkeylen, > > + if (qcrypto_afsplit_decode(luks->hash_alg, > > + masterkeylen(luks), > > slot->stripes, > > splitkey, > > masterkey, > > @@ -537,8 +551,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > * then comparing that to the hash stored in the key slot > > * header > > */ > > - if (qcrypto_pbkdf2(hash, > > - masterkey, masterkeylen, > > + if (qcrypto_pbkdf2(luks->hash_alg, > > + masterkey, masterkeylen(luks), > > luks->header.master_key_salt, > > QCRYPTO_BLOCK_LUKS_SALT_LEN, > > luks->header.master_key_iterations, > > @@ -577,37 +591,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, > > static int > > qcrypto_block_luks_find_key(QCryptoBlock *block, > > const char *password, > > - QCryptoCipherAlgorithm cipheralg, > > - QCryptoCipherMode ciphermode, > > - QCryptoHashAlgorithm hash, > > - QCryptoIVGenAlgorithm ivalg, > > - QCryptoCipherAlgorithm ivcipheralg, > > - QCryptoHashAlgorithm ivhash, > > - uint8_t **masterkey, > > - size_t *masterkeylen, > > + uint8_t *masterkey, > > QCryptoBlockReadFunc readfunc, > > void *opaque, > > Error **errp) > > { > > - QCryptoBlockLUKS *luks = block->opaque; > > size_t i; > > int rv; > > > > - *masterkey = g_new0(uint8_t, luks->header.key_bytes); > > - *masterkeylen = luks->header.key_bytes; > > - > > for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { > > rv = qcrypto_block_luks_load_key(block, > > - &luks->header.key_slots[i], > > + i, > > password, > > - cipheralg, > > - ciphermode, > > - hash, > > - ivalg, > > - ivcipheralg, > > - ivhash, > > - *masterkey, > > - *masterkeylen, > > + masterkey, > > readfunc, > > opaque, > > errp); > > @@ -620,11 +616,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, > > } > > > > error_setg(errp, "Invalid password, cannot unlock any keyslot"); > > - > > error: > > - g_free(*masterkey); > > - *masterkey = NULL; > > - *masterkeylen = 0; > > return -1; > > } > > > > @@ -639,21 +631,15 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > size_t n_threads, > > Error **errp) > > { > > - QCryptoBlockLUKS *luks; > > + QCryptoBlockLUKS *luks = NULL; > > Error *local_err = NULL; > > int ret = 0; > > size_t i; > > ssize_t rv; > > uint8_t *masterkey = NULL; > > - size_t masterkeylen; > > char *ivgen_name, *ivhash_name; > > - QCryptoCipherMode ciphermode; > > - QCryptoCipherAlgorithm cipheralg; > > - QCryptoIVGenAlgorithm ivalg; > > - QCryptoCipherAlgorithm ivcipheralg; > > - QCryptoHashAlgorithm hash; > > - QCryptoHashAlgorithm ivhash; > > char *password = NULL; > > + char *cipher_mode = NULL; > > > > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > > if (!options->u.luks.key_secret) { > > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > goto fail; > > } > > > > + cipher_mode = g_strdup(luks->header.cipher_mode); > > + > > /* > > * The cipher_mode header contains a string that we have > > * to further parse, of the format > > @@ -718,7 +706,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > * > > * eg cbc-essiv:sha256, cbc-plain64 > > */ > > - ivgen_name = strchr(luks->header.cipher_mode, '-'); > > + ivgen_name = strchr(cipher_mode, '-'); > > if (!ivgen_name) { > > ret = -EINVAL; > > error_setg(errp, "Unexpected cipher mode string format %s", > > @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > > > ivhash_name = strchr(ivgen_name, ':'); > > if (!ivhash_name) { > > - ivhash = 0; > > + luks->ivgen_hash_alg = 0; > > } else { > > *ivhash_name = '\0'; > > ivhash_name++; > > > > - ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name, > > - &local_err); > > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > } > > } > > > > - ciphermode = qcrypto_block_luks_cipher_mode_lookup(luks->header.cipher_mode, > > - &local_err); > > + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > goto fail; > > } > > > > - cipheralg = qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > > - ciphermode, > > - luks->header.key_bytes, > > - &local_err); > > + luks->cipher_alg = > > + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > > + luks->cipher_mode, > > + luks->header.key_bytes, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > goto fail; > > } > > > > - hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > > + luks->hash_alg = > > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > > &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > @@ -770,23 +760,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > goto fail; > > } > > > > - ivalg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > > - &local_err); > > + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > goto fail; > > } > > > > - if (ivalg == QCRYPTO_IVGEN_ALG_ESSIV) { > > + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { > > if (!ivhash_name) { > > ret = -EINVAL; > > error_setg(errp, "Missing IV generator hash specification"); > > goto fail; > > } > > - ivcipheralg = qcrypto_block_luks_essiv_cipher(cipheralg, > > - ivhash, > > - &local_err); > > + luks->ivgen_cipher_alg = > > + qcrypto_block_luks_essiv_cipher(luks->cipher_alg, > > + luks->ivgen_hash_alg, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > @@ -800,21 +791,25 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > * ignore hash names with these ivgens rather than report > > * an error about the invalid usage > > */ > > - ivcipheralg = cipheralg; > > + luks->ivgen_cipher_alg = luks->cipher_alg; > > } > > > > + > > + g_free(cipher_mode); > > + cipher_mode = NULL; > > + ivgen_name = NULL; > > + ivhash_name = NULL; > > + > > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > > /* Try to find which key slot our password is valid for > > * and unlock the master key from that slot. > > */ > > + > > + masterkey = g_new0(uint8_t, masterkeylen(luks)); > > + > > if (qcrypto_block_luks_find_key(block, > > password, > > - cipheralg, ciphermode, > > - hash, > > - ivalg, > > - ivcipheralg, > > - ivhash, > > - &masterkey, &masterkeylen, > > + masterkey, > > readfunc, opaque, > > errp) < 0) { > > ret = -EACCES; > > @@ -824,21 +819,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > /* We have a valid master key now, so can setup the > > * block device payload decryption objects > > */ > > - block->kdfhash = hash; > > - block->niv = qcrypto_cipher_get_iv_len(cipheralg, > > - ciphermode); > > - block->ivgen = qcrypto_ivgen_new(ivalg, > > - ivcipheralg, > > - ivhash, > > - masterkey, masterkeylen, > > + block->kdfhash = luks->hash_alg; > > + block->niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, > > + luks->cipher_mode); > > + > > + block->ivgen = qcrypto_ivgen_new(luks->ivgen_alg, > > + luks->ivgen_cipher_alg, > > + luks->ivgen_hash_alg, > > + masterkey, masterkeylen(luks), > > errp); > > if (!block->ivgen) { > > ret = -ENOTSUP; > > goto fail; > > } > > > > - ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode, > > - masterkey, masterkeylen, n_threads, > > + ret = qcrypto_block_init_cipher(block, luks->cipher_alg, > > + luks->cipher_mode, > > + masterkey, masterkeylen(luks), > > + n_threads, > > errp); > > if (ret < 0) { > > ret = -ENOTSUP; > > @@ -850,12 +848,6 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > block->payload_offset = luks->header.payload_offset * > > block->sector_size; > > > > - luks->cipher_alg = cipheralg; > > - luks->cipher_mode = ciphermode; > > - luks->ivgen_alg = ivalg; > > - luks->ivgen_hash_alg = ivhash; > > - luks->hash_alg = hash; > > - > > g_free(masterkey); > > g_free(password); > > > > @@ -910,7 +902,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > > > memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); > > if (!luks_opts.has_iter_time) { > > - luks_opts.iter_time = 2000; > > + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME; > > } > > if (!luks_opts.has_cipher_alg) { > > luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; > > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > luks_opts.has_ivgen_hash_alg = true; > > } > > } > > + > > + luks = g_new0(QCryptoBlockLUKS, 1); > > + block->opaque = luks; > > + > > + luks->cipher_alg = luks_opts.cipher_alg; > > + luks->cipher_mode = luks_opts.cipher_mode; > > + luks->ivgen_alg = luks_opts.ivgen_alg; > > + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > > + luks->hash_alg = luks_opts.hash_alg; > > + > > + > > /* Note we're allowing ivgen_hash_alg to be set even for > > * non-essiv iv generators that don't need a hash. It will > > * be silently ignored, for compatibility with dm-crypt */ > > @@ -944,8 +947,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > return -1; > > } > > > > - luks = g_new0(QCryptoBlockLUKS, 1); > > - block->opaque = luks; > > > > memcpy(luks->header.magic, qcrypto_block_luks_magic, > > QCRYPTO_BLOCK_LUKS_MAGIC_LEN); > > @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > ivcipheralg = luks_opts.cipher_alg; > > } > > > > + luks->ivgen_cipher_alg = ivcipheralg; > > + > > strcpy(luks->header.cipher_name, cipher_alg); > > strcpy(luks->header.cipher_mode, cipher_mode_spec); > > strcpy(luks->header.hash_spec, hash_alg); > > @@ -1304,12 +1307,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > goto error; > > } > > > > - luks->cipher_alg = luks_opts.cipher_alg; > > - luks->cipher_mode = luks_opts.cipher_mode; > > - luks->ivgen_alg = luks_opts.ivgen_alg; > > - luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > > - luks->hash_alg = luks_opts.hash_alg; > > - > > memset(masterkey, 0, luks->header.key_bytes); > > g_free(masterkey); > > memset(slotkey, 0, luks->header.key_bytes); > >
On 14.08.19 22:22, Maxim Levitsky wrote: > This is also a preparation for key read/write/erase functions > > * use master key len from the header > * prefer to use crypto params in the QCryptoBlockLUKS > over passing them as function arguments > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- > 1 file changed, 105 insertions(+), 108 deletions(-) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index 409ab50f20..48213abde7 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c [...] > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); > struct QCryptoBlockLUKS { > QCryptoBlockLUKSHeader header; > > - /* Cache parsed versions of what's in header fields, > - * as we can't rely on QCryptoBlock.cipher being > - * non-NULL */ Hm, why remove this comment? > + /* Main encryption algorithm used for encryption*/ > QCryptoCipherAlgorithm cipher_alg; > + > + /* Mode of encryption for the selected encryption algorithm */ > QCryptoCipherMode cipher_mode; > + > + /* Initialization vector generation algorithm */ > QCryptoIVGenAlgorithm ivgen_alg; > + > + /* Hash algorithm used for IV generation*/ > QCryptoHashAlgorithm ivgen_hash_alg; > + > + /* > + * Encryption algorithm used for IV generation. > + * Usually the same as main encryption algorithm > + */ > + QCryptoCipherAlgorithm ivgen_cipher_alg; > + > + /* Hash algorithm used in pbkdf2 function */ > QCryptoHashAlgorithm hash_alg; > }; > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > } > } > > +static int masterkeylen(QCryptoBlockLUKS *luks) This should be a const pointer. > +{ > + return luks->header.key_bytes; > +} > + > + > /* > * Given a key slot, and user password, this will attempt to unlock > * the master encryption key from the key slot. > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > */ > static int > qcrypto_block_luks_load_key(QCryptoBlock *block, > - QCryptoBlockLUKSKeySlot *slot, > + uint slot_idx, Did you use uint on purpose or do you mean a plain “unsigned”? > const char *password, > - QCryptoCipherAlgorithm cipheralg, > - QCryptoCipherMode ciphermode, > - QCryptoHashAlgorithm hash, > - QCryptoIVGenAlgorithm ivalg, > - QCryptoCipherAlgorithm ivcipheralg, > - QCryptoHashAlgorithm ivhash, > uint8_t *masterkey, > - size_t masterkeylen, > QCryptoBlockReadFunc readfunc, > void *opaque, > Error **errp) > { > QCryptoBlockLUKS *luks = block->opaque; > + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx]; I think this is a great opportunity to make this a const pointer. > uint8_t *splitkey; > size_t splitkeylen; > uint8_t *possiblekey; [...] > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block, > goto fail; > } > > + cipher_mode = g_strdup(luks->header.cipher_mode); > + This should be freed under the fail label. (And maybe the fact that this no longer modifies luks->header.cipher_mode should be mentioned in the commit message, I don’t know.) > /* > * The cipher_mode header contains a string that we have > * to further parse, of the format [...] > @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > ivhash_name = strchr(ivgen_name, ':'); > if (!ivhash_name) { > - ivhash = 0; > + luks->ivgen_hash_alg = 0; *luks is initialized to 0 anyway, but it doesn’t hurt, of course. > } else { > *ivhash_name = '\0'; > ivhash_name++; > > - ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name, > - &local_err); > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > + &local_err); > if (local_err) { > ret = -ENOTSUP; > error_propagate(errp, local_err); > @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block, [...] > > - hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > + luks->hash_alg = > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > &local_err); Indentation is off now. > if (local_err) { > ret = -ENOTSUP; [...] > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > luks_opts.has_ivgen_hash_alg = true; > } > } > + > + luks = g_new0(QCryptoBlockLUKS, 1); > + block->opaque = luks; > + > + luks->cipher_alg = luks_opts.cipher_alg; > + luks->cipher_mode = luks_opts.cipher_mode; > + luks->ivgen_alg = luks_opts.ivgen_alg; > + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > + luks->hash_alg = luks_opts.hash_alg; > + > + Why did you pull this up? Now @luks is leaked in both of the next error paths. > /* Note we're allowing ivgen_hash_alg to be set even for > * non-essiv iv generators that don't need a hash. It will > * be silently ignored, for compatibility with dm-crypt */ [...] > @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > ivcipheralg = luks_opts.cipher_alg; > } > > + luks->ivgen_cipher_alg = ivcipheralg; > + What’s the point in having a dedicated ivcipheralg variable then? Max > strcpy(luks->header.cipher_name, cipher_alg); > strcpy(luks->header.cipher_mode, cipher_mode_spec); > strcpy(luks->header.hash_spec, hash_alg);
On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote: > On 14.08.19 22:22, Maxim Levitsky wrote: > > This is also a preparation for key read/write/erase functions > > > > * use master key len from the header > > * prefer to use crypto params in the QCryptoBlockLUKS > > over passing them as function arguments > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- > > 1 file changed, 105 insertions(+), 108 deletions(-) > > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > > index 409ab50f20..48213abde7 100644 > > --- a/crypto/block-luks.c > > +++ b/crypto/block-luks.c > > [...] > > > @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); > > struct QCryptoBlockLUKS { > > QCryptoBlockLUKSHeader header; > > > > - /* Cache parsed versions of what's in header fields, > > - * as we can't rely on QCryptoBlock.cipher being > > - * non-NULL */ > > Hm, why remove this comment? Because the intended uses of these fields changed. As Daniel explained to me initially none of the crypto parameters were stored in the QCryptoBlockLUKS, and thus there were all passed as function arguments. Later when qemu-img info was added/implemented, there was need to 'cache' them in the header so that info command could use them after image was opened. Now after my changes this is no longer true. now these crypto parameters are set early on create/load and used everywhere to avoid passing them over and over to each function. > > > + /* Main encryption algorithm used for encryption*/ > > QCryptoCipherAlgorithm cipher_alg; > > + > > + /* Mode of encryption for the selected encryption algorithm */ > > QCryptoCipherMode cipher_mode; > > + > > + /* Initialization vector generation algorithm */ > > QCryptoIVGenAlgorithm ivgen_alg; > > + > > + /* Hash algorithm used for IV generation*/ > > QCryptoHashAlgorithm ivgen_hash_alg; > > + > > + /* > > + * Encryption algorithm used for IV generation. > > + * Usually the same as main encryption algorithm > > + */ > > + QCryptoCipherAlgorithm ivgen_cipher_alg; > > + > > + /* Hash algorithm used in pbkdf2 function */ > > QCryptoHashAlgorithm hash_alg; > > }; > > > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > } > > } > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > This should be a const pointer. I haven't had const in mind while writing this but you are right. Fixed. > > > +{ > > + return luks->header.key_bytes; > > +} > > + > > + > > /* > > * Given a key slot, and user password, this will attempt to unlock > > * the master encryption key from the key slot. > > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > */ > > static int > > qcrypto_block_luks_load_key(QCryptoBlock *block, > > - QCryptoBlockLUKSKeySlot *slot, > > + uint slot_idx, > > Did you use uint on purpose or do you mean a plain “unsigned”? Well there are just 8 slots, but yea I don't mind to make this an unsigned int. > > > const char *password, > > - QCryptoCipherAlgorithm cipheralg, > > - QCryptoCipherMode ciphermode, > > - QCryptoHashAlgorithm hash, > > - QCryptoIVGenAlgorithm ivalg, > > - QCryptoCipherAlgorithm ivcipheralg, > > - QCryptoHashAlgorithm ivhash, > > uint8_t *masterkey, > > - size_t masterkeylen, > > QCryptoBlockReadFunc readfunc, > > void *opaque, > > Error **errp) > > { > > QCryptoBlockLUKS *luks = block->opaque; > > + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx]; > > I think this is a great opportunity to make this a const pointer. Agree. Done. > > > uint8_t *splitkey; > > size_t splitkeylen; > > uint8_t *possiblekey; > > [...] > > > @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > goto fail; > > } > > > > + cipher_mode = g_strdup(luks->header.cipher_mode); > > + > > This should be freed under the fail label. > > (And maybe the fact that this no longer modifies > luks->header.cipher_mode should be mentioned in the commit message, I > don’t know.) I swear I documented that in the commit message... yea in the next commit (:-() Fixed that now. > > > /* > > * The cipher_mode header contains a string that we have > > * to further parse, of the format > > [...] > > > @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > > > ivhash_name = strchr(ivgen_name, ':'); > > if (!ivhash_name) { > > - ivhash = 0; > > + luks->ivgen_hash_alg = 0; > > *luks is initialized to 0 anyway, but it doesn’t hurt, of course. > > > } else { > > *ivhash_name = '\0'; > > ivhash_name++; > > > > - ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name, > > - &local_err); > > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > > + &local_err); > > if (local_err) { > > ret = -ENOTSUP; > > error_propagate(errp, local_err); > > @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > [...] > > > > > - hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > > + luks->hash_alg = > > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > > &local_err); > > Indentation is off now. True! But at least as you probably noticed I now am aware of that rule, and so I have way less such errors now :-) > > > if (local_err) { > > ret = -ENOTSUP; > > [...] > > > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > luks_opts.has_ivgen_hash_alg = true; > > } > > } > > + > > + luks = g_new0(QCryptoBlockLUKS, 1); > > + block->opaque = luks; > > + > > + luks->cipher_alg = luks_opts.cipher_alg; > > + luks->cipher_mode = luks_opts.cipher_mode; > > + luks->ivgen_alg = luks_opts.ivgen_alg; > > + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > > + luks->hash_alg = luks_opts.hash_alg; > > + > > + > > Why did you pull this up? Now @luks is leaked in both of the next error > paths. This is because the purpose of these fields changed. As Daniel explained to me they were initially added after the fact to serve as a cache of into to present in qemu-img info callback. But now I use these everywhere in the code so I won't them to be correct as soon as possible to minimize the risk of calling some function that uses these and would read garbage. Leak is fixed now, thanks! > > > /* Note we're allowing ivgen_hash_alg to be set even for > > * non-essiv iv generators that don't need a hash. It will > > * be silently ignored, for compatibility with dm-crypt */ > > [...] > > > @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > ivcipheralg = luks_opts.cipher_alg; > > } > > > > + luks->ivgen_cipher_alg = ivcipheralg; > > + > > What’s the point in having a dedicated ivcipheralg variable then? Leftover, since I added the luks->ivgen_cipher_alg. The luks->ivgen_cipher_alg is special in the sense that it is not part of the parsed on-disk header, but rather calculated, as a workaround for the dm-crypto 'bug' The long explanation of this is in comment of qcrypto_block_luks_essiv_cipher I'll remove that local variable, because why not. > > Max > > > strcpy(luks->header.cipher_name, cipher_alg); > > strcpy(luks->header.cipher_mode, cipher_mode_spec); > > strcpy(luks->header.hash_spec, hash_alg); > > Best regards, Thanks for the review, Maxim Levitsky
On Thu, Aug 15, 2019 at 05:40:11PM -0400, John Snow wrote: > > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > This is also a preparation for key read/write/erase functions > > > > This is a matter of taste and I am not usually reviewing LUKS patches > (So don't take me too seriously), but I would prefer not to have "misc" > patches and instead split things out by individual changes along with a > nice commit message for each change. > > > * use master key len from the header > > This touches enough lines that you could make it its own patch, I think. > > > * prefer to use crypto params in the QCryptoBlockLUKS > > over passing them as function arguments > > I think the same is true here, and highlighting which variables you are > sticking into state instead of leaving as functional parameters would be > nice to see without all the other changes. > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > This can likely be squashed with whichever patch of yours first needs to > use it, because it's so short. > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > Can probably be squashed with item #2. Agreed, with all these points - it is too hard to review this for correctness with everything merged in one commit, so I'll wait for v2 before reviewing much more. > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > } > > } > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > +{ > > + return luks->header.key_bytes; > > +} > > + > > + > > generally QEMU uses snake_case_names; please spell as "master_key_len". Also naming convention in this file expects "qcrypto_block_luks_" prefix for all methods Regards, Daniel
On Thu, 2019-08-22 at 11:29 +0100, Daniel P. Berrangé wrote: > On Thu, Aug 15, 2019 at 05:40:11PM -0400, John Snow wrote: > > > > > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > > This is also a preparation for key read/write/erase functions > > > > > > > This is a matter of taste and I am not usually reviewing LUKS patches > > (So don't take me too seriously), but I would prefer not to have "misc" > > patches and instead split things out by individual changes along with a > > nice commit message for each change. > > > > > * use master key len from the header > > > > This touches enough lines that you could make it its own patch, I think. > > > > > * prefer to use crypto params in the QCryptoBlockLUKS > > > over passing them as function arguments > > > > I think the same is true here, and highlighting which variables you are > > sticking into state instead of leaving as functional parameters would be > > nice to see without all the other changes. > > > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > > > This can likely be squashed with whichever patch of yours first needs to > > use it, because it's so short. > > > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > > > > Can probably be squashed with item #2. > > Agreed, with all these points - it is too hard to review this > for correctness with everything merged in one commit, so I'll > wait for v2 before reviewing much more. > > > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > > } > > > } > > > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > > +{ > > > + return luks->header.key_bytes; > > > +} > > > + > > > + > > > > generally QEMU uses snake_case_names; please spell as "master_key_len". > > Also naming convention in this file expects "qcrypto_block_luks_" prefix > for all methods > The point of adding the masterkeylen was that IMHO it wasn't clear that luks->header.key_bytes is the master key length. However defining something like qcrypto_block_luks_master_key_len(QCryptoBlockLUKS *luks) is way longer, and will force me to wrap just too many places in the code to keep 80 character limit. Now I am thinking of other ways to make this thing better: 1. How about adding luks->masterkeylen and using it. luks state already has several parsed values from the header, so using another one wouldn't hurt? 2. how about renaming the luks->header.key_bytes to luks->header->master_key_len? Best regards, Maxim Levitsky
On Thu, Aug 22, 2019 at 02:04:28PM +0300, Maxim Levitsky wrote: > On Thu, 2019-08-22 at 11:29 +0100, Daniel P. Berrangé wrote: > > On Thu, Aug 15, 2019 at 05:40:11PM -0400, John Snow wrote: > > > > > > > > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > > > This is also a preparation for key read/write/erase functions > > > > > > > > > > This is a matter of taste and I am not usually reviewing LUKS patches > > > (So don't take me too seriously), but I would prefer not to have "misc" > > > patches and instead split things out by individual changes along with a > > > nice commit message for each change. > > > > > > > * use master key len from the header > > > > > > This touches enough lines that you could make it its own patch, I think. > > > > > > > * prefer to use crypto params in the QCryptoBlockLUKS > > > > over passing them as function arguments > > > > > > I think the same is true here, and highlighting which variables you are > > > sticking into state instead of leaving as functional parameters would be > > > nice to see without all the other changes. > > > > > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > > > > > This can likely be squashed with whichever patch of yours first needs to > > > use it, because it's so short. > > > > > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > > > > > > > Can probably be squashed with item #2. > > > > Agreed, with all these points - it is too hard to review this > > for correctness with everything merged in one commit, so I'll > > wait for v2 before reviewing much more. > > > > > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > > > } > > > > } > > > > > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > > > +{ > > > > + return luks->header.key_bytes; > > > > +} > > > > + > > > > + > > > > > > generally QEMU uses snake_case_names; please spell as "master_key_len". > > > > Also naming convention in this file expects "qcrypto_block_luks_" prefix > > for all methods > > > The point of adding the masterkeylen was that IMHO it wasn't clear that > luks->header.key_bytes is the master key length. > > However defining something like qcrypto_block_luks_master_key_len(QCryptoBlockLUKS *luks) > is way longer, and will force me to wrap just too many places in the code to keep 80 > character limit. > > Now I am thinking of other ways to make this thing better: > > 1. How about adding luks->masterkeylen and using it. luks state already has > several parsed values from the header, so using another one wouldn't hurt? With those the parsed values are actually a different format from the header values, so it makes sense to have duplication. Duplication just for sake of having a different name will just be confusing with some code using one field & some code using the other field when they are identical. > 2. how about renaming the luks->header.key_bytes to luks->header->master_key_len? This is fine. Regards, Daniel
On Thu, 2019-08-22 at 12:10 +0100, Daniel P. Berrangé wrote: > On Thu, Aug 22, 2019 at 02:04:28PM +0300, Maxim Levitsky wrote: > > On Thu, 2019-08-22 at 11:29 +0100, Daniel P. Berrangé wrote: > > > On Thu, Aug 15, 2019 at 05:40:11PM -0400, John Snow wrote: > > > > > > > > > > > > On 8/14/19 4:22 PM, Maxim Levitsky wrote: > > > > > This is also a preparation for key read/write/erase functions > > > > > > > > > > > > > This is a matter of taste and I am not usually reviewing LUKS patches > > > > (So don't take me too seriously), but I would prefer not to have "misc" > > > > patches and instead split things out by individual changes along with a > > > > nice commit message for each change. > > > > > > > > > * use master key len from the header > > > > > > > > This touches enough lines that you could make it its own patch, I think. > > > > > > > > > * prefer to use crypto params in the QCryptoBlockLUKS > > > > > over passing them as function arguments > > > > > > > > I think the same is true here, and highlighting which variables you are > > > > sticking into state instead of leaving as functional parameters would be > > > > nice to see without all the other changes. > > > > > > > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > > > > > > > This can likely be squashed with whichever patch of yours first needs to > > > > use it, because it's so short. > > > > > > > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > > > > > > > > > > Can probably be squashed with item #2. > > > > > > Agreed, with all these points - it is too hard to review this > > > for correctness with everything merged in one commit, so I'll > > > wait for v2 before reviewing much more. > > > > > > > > @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > > > > } > > > > > } > > > > > > > > > > +static int masterkeylen(QCryptoBlockLUKS *luks) > > > > > +{ > > > > > + return luks->header.key_bytes; > > > > > +} > > > > > + > > > > > + > > > > > > > > generally QEMU uses snake_case_names; please spell as "master_key_len". > > > > > > Also naming convention in this file expects "qcrypto_block_luks_" prefix > > > for all methods > > > > > > > The point of adding the masterkeylen was that IMHO it wasn't clear that > > luks->header.key_bytes is the master key length. > > > > However defining something like qcrypto_block_luks_master_key_len(QCryptoBlockLUKS *luks) > > is way longer, and will force me to wrap just too many places in the code to keep 80 > > character limit. > > > > Now I am thinking of other ways to make this thing better: > > > > 1. How about adding luks->masterkeylen and using it. luks state already has > > several parsed values from the header, so using another one wouldn't hurt? > > With those the parsed values are actually a different format from the > header values, so it makes sense to have duplication. Duplication > just for sake of having a different name will just be confusing > with some code using one field & some code using the other field > when they are identical. > > > 2. how about renaming the luks->header.key_bytes to luks->header->master_key_len? > > This is fine. Roger that! Best regards, Maxim Levitsky > > Regards, > Daniel
On 22.08.19 01:59, Maxim Levitsky wrote: > On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote: >> On 14.08.19 22:22, Maxim Levitsky wrote: >>> This is also a preparation for key read/write/erase functions >>> >>> * use master key len from the header >>> * prefer to use crypto params in the QCryptoBlockLUKS >>> over passing them as function arguments >>> * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME >>> * Add comments to various crypto parameters in the QCryptoBlockLUKS >>> >>> Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> >>> --- >>> crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- >>> 1 file changed, 105 insertions(+), 108 deletions(-) [...] >>> @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, >>> */ >>> static int >>> qcrypto_block_luks_load_key(QCryptoBlock *block, >>> - QCryptoBlockLUKSKeySlot *slot, >>> + uint slot_idx, >> >> Did you use uint on purpose or do you mean a plain “unsigned”? > Well there are just 8 slots, but yea I don't mind to make this an unsigned int. My point was that “uint” is not a standard C type. [...] >>> @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, >>> luks_opts.has_ivgen_hash_alg = true; >>> } >>> } >>> + >>> + luks = g_new0(QCryptoBlockLUKS, 1); >>> + block->opaque = luks; >>> + >>> + luks->cipher_alg = luks_opts.cipher_alg; >>> + luks->cipher_mode = luks_opts.cipher_mode; >>> + luks->ivgen_alg = luks_opts.ivgen_alg; >>> + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; >>> + luks->hash_alg = luks_opts.hash_alg; >>> + >>> + >> >> Why did you pull this up? Now @luks is leaked in both of the next error >> paths. > > This is because the purpose of these fields changed. As Daniel explained to me > they were initially added after the fact to serve as a cache of into to present in qemu-img info callback. > But now I use these everywhere in the code so I won't them to be correct as soon as possible to minimize > the risk of calling some function that uses these and would read garbage. I get that, but I was wondering why you pulled the allocation of @luks up above the next two conditional blocks. Allocating and initializing there should have worked just fine. Max
On Thu, 2019-08-22 at 16:32 +0200, Max Reitz wrote: > On 22.08.19 01:59, Maxim Levitsky wrote: > > On Tue, 2019-08-20 at 19:36 +0200, Max Reitz wrote: > > > On 14.08.19 22:22, Maxim Levitsky wrote: > > > > This is also a preparation for key read/write/erase functions > > > > > > > > * use master key len from the header > > > > * prefer to use crypto params in the QCryptoBlockLUKS > > > > over passing them as function arguments > > > > * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME > > > > * Add comments to various crypto parameters in the QCryptoBlockLUKS > > > > > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > > > --- > > > > crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- > > > > 1 file changed, 105 insertions(+), 108 deletions(-) > > > [...] > > > > > @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, > > > > */ > > > > static int > > > > qcrypto_block_luks_load_key(QCryptoBlock *block, > > > > - QCryptoBlockLUKSKeySlot *slot, > > > > + uint slot_idx, > > > > > > Did you use uint on purpose or do you mean a plain “unsigned”? > > > > Well there are just 8 slots, but yea I don't mind to make this an unsigned int. > > My point was that “uint” is not a standard C type. There are lot of non standard types, like the u8/u8/u32/u64 I used to use in the kernel, so I kind of missed that. Won't use that type anymore :-) > > [...] > > > > > @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, > > > > luks_opts.has_ivgen_hash_alg = true; > > > > } > > > > } > > > > + > > > > + luks = g_new0(QCryptoBlockLUKS, 1); > > > > + block->opaque = luks; > > > > + > > > > + luks->cipher_alg = luks_opts.cipher_alg; > > > > + luks->cipher_mode = luks_opts.cipher_mode; > > > > + luks->ivgen_alg = luks_opts.ivgen_alg; > > > > + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; > > > > + luks->hash_alg = luks_opts.hash_alg; > > > > + > > > > + > > > > > > Why did you pull this up? Now @luks is leaked in both of the next error > > > paths. > > > > This is because the purpose of these fields changed. As Daniel explained to me > > they were initially added after the fact to serve as a cache of into to present in qemu-img info callback. > > But now I use these everywhere in the code so I won't them to be correct as soon as possible to minimize > > the risk of calling some function that uses these and would read garbage. > > I get that, but I was wondering why you pulled the allocation of @luks > up above the next two conditional blocks. Allocating and initializing > there should have worked just fine. Yea, I didn't have to, just thought that putting the initialization as above as possible is a good thing for future. > > Max > Best regards, Maxim Levitsky
diff --git a/crypto/block-luks.c b/crypto/block-luks.c index 409ab50f20..48213abde7 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -70,6 +70,8 @@ typedef struct QCryptoBlockLUKSKeySlot QCryptoBlockLUKSKeySlot; #define QCRYPTO_BLOCK_LUKS_SECTOR_SIZE 512LL +#define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME 2000 + static const char qcrypto_block_luks_magic[QCRYPTO_BLOCK_LUKS_MAGIC_LEN] = { 'L', 'U', 'K', 'S', 0xBA, 0xBE }; @@ -199,13 +201,25 @@ QEMU_BUILD_BUG_ON(sizeof(struct QCryptoBlockLUKSHeader) != 592); struct QCryptoBlockLUKS { QCryptoBlockLUKSHeader header; - /* Cache parsed versions of what's in header fields, - * as we can't rely on QCryptoBlock.cipher being - * non-NULL */ + /* Main encryption algorithm used for encryption*/ QCryptoCipherAlgorithm cipher_alg; + + /* Mode of encryption for the selected encryption algorithm */ QCryptoCipherMode cipher_mode; + + /* Initialization vector generation algorithm */ QCryptoIVGenAlgorithm ivgen_alg; + + /* Hash algorithm used for IV generation*/ QCryptoHashAlgorithm ivgen_hash_alg; + + /* + * Encryption algorithm used for IV generation. + * Usually the same as main encryption algorithm + */ + QCryptoCipherAlgorithm ivgen_cipher_alg; + + /* Hash algorithm used in pbkdf2 function */ QCryptoHashAlgorithm hash_alg; }; @@ -397,6 +411,12 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, } } +static int masterkeylen(QCryptoBlockLUKS *luks) +{ + return luks->header.key_bytes; +} + + /* * Given a key slot, and user password, this will attempt to unlock * the master encryption key from the key slot. @@ -410,21 +430,15 @@ qcrypto_block_luks_essiv_cipher(QCryptoCipherAlgorithm cipher, */ static int qcrypto_block_luks_load_key(QCryptoBlock *block, - QCryptoBlockLUKSKeySlot *slot, + uint slot_idx, const char *password, - QCryptoCipherAlgorithm cipheralg, - QCryptoCipherMode ciphermode, - QCryptoHashAlgorithm hash, - QCryptoIVGenAlgorithm ivalg, - QCryptoCipherAlgorithm ivcipheralg, - QCryptoHashAlgorithm ivhash, uint8_t *masterkey, - size_t masterkeylen, QCryptoBlockReadFunc readfunc, void *opaque, Error **errp) { QCryptoBlockLUKS *luks = block->opaque; + QCryptoBlockLUKSKeySlot *slot = &luks->header.key_slots[slot_idx]; uint8_t *splitkey; size_t splitkeylen; uint8_t *possiblekey; @@ -439,9 +453,9 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, return 0; } - splitkeylen = masterkeylen * slot->stripes; + splitkeylen = masterkeylen(luks) * slot->stripes; splitkey = g_new0(uint8_t, splitkeylen); - possiblekey = g_new0(uint8_t, masterkeylen); + possiblekey = g_new0(uint8_t, masterkeylen(luks)); /* * The user password is used to generate a (possible) @@ -450,11 +464,11 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, * the key is correct and validate the results of * decryption later. */ - if (qcrypto_pbkdf2(hash, + if (qcrypto_pbkdf2(luks->hash_alg, (const uint8_t *)password, strlen(password), slot->salt, QCRYPTO_BLOCK_LUKS_SALT_LEN, slot->iterations, - possiblekey, masterkeylen, + possiblekey, masterkeylen(luks), errp) < 0) { goto cleanup; } @@ -478,19 +492,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, /* Setup the cipher/ivgen that we'll use to try to decrypt * the split master key material */ - cipher = qcrypto_cipher_new(cipheralg, ciphermode, - possiblekey, masterkeylen, + cipher = qcrypto_cipher_new(luks->cipher_alg, luks->cipher_mode, + possiblekey, masterkeylen(luks), errp); if (!cipher) { goto cleanup; } - niv = qcrypto_cipher_get_iv_len(cipheralg, - ciphermode); - ivgen = qcrypto_ivgen_new(ivalg, - ivcipheralg, - ivhash, - possiblekey, masterkeylen, + niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, + luks->cipher_mode); + ivgen = qcrypto_ivgen_new(luks->ivgen_alg, + luks->ivgen_cipher_alg, + luks->ivgen_hash_alg, + possiblekey, masterkeylen(luks), errp); if (!ivgen) { goto cleanup; @@ -519,8 +533,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, * Now we've decrypted the split master key, join * it back together to get the actual master key. */ - if (qcrypto_afsplit_decode(hash, - masterkeylen, + if (qcrypto_afsplit_decode(luks->hash_alg, + masterkeylen(luks), slot->stripes, splitkey, masterkey, @@ -537,8 +551,8 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, * then comparing that to the hash stored in the key slot * header */ - if (qcrypto_pbkdf2(hash, - masterkey, masterkeylen, + if (qcrypto_pbkdf2(luks->hash_alg, + masterkey, masterkeylen(luks), luks->header.master_key_salt, QCRYPTO_BLOCK_LUKS_SALT_LEN, luks->header.master_key_iterations, @@ -577,37 +591,19 @@ qcrypto_block_luks_load_key(QCryptoBlock *block, static int qcrypto_block_luks_find_key(QCryptoBlock *block, const char *password, - QCryptoCipherAlgorithm cipheralg, - QCryptoCipherMode ciphermode, - QCryptoHashAlgorithm hash, - QCryptoIVGenAlgorithm ivalg, - QCryptoCipherAlgorithm ivcipheralg, - QCryptoHashAlgorithm ivhash, - uint8_t **masterkey, - size_t *masterkeylen, + uint8_t *masterkey, QCryptoBlockReadFunc readfunc, void *opaque, Error **errp) { - QCryptoBlockLUKS *luks = block->opaque; size_t i; int rv; - *masterkey = g_new0(uint8_t, luks->header.key_bytes); - *masterkeylen = luks->header.key_bytes; - for (i = 0; i < QCRYPTO_BLOCK_LUKS_NUM_KEY_SLOTS; i++) { rv = qcrypto_block_luks_load_key(block, - &luks->header.key_slots[i], + i, password, - cipheralg, - ciphermode, - hash, - ivalg, - ivcipheralg, - ivhash, - *masterkey, - *masterkeylen, + masterkey, readfunc, opaque, errp); @@ -620,11 +616,7 @@ qcrypto_block_luks_find_key(QCryptoBlock *block, } error_setg(errp, "Invalid password, cannot unlock any keyslot"); - error: - g_free(*masterkey); - *masterkey = NULL; - *masterkeylen = 0; return -1; } @@ -639,21 +631,15 @@ qcrypto_block_luks_open(QCryptoBlock *block, size_t n_threads, Error **errp) { - QCryptoBlockLUKS *luks; + QCryptoBlockLUKS *luks = NULL; Error *local_err = NULL; int ret = 0; size_t i; ssize_t rv; uint8_t *masterkey = NULL; - size_t masterkeylen; char *ivgen_name, *ivhash_name; - QCryptoCipherMode ciphermode; - QCryptoCipherAlgorithm cipheralg; - QCryptoIVGenAlgorithm ivalg; - QCryptoCipherAlgorithm ivcipheralg; - QCryptoHashAlgorithm hash; - QCryptoHashAlgorithm ivhash; char *password = NULL; + char *cipher_mode = NULL; if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { if (!options->u.luks.key_secret) { @@ -710,6 +696,8 @@ qcrypto_block_luks_open(QCryptoBlock *block, goto fail; } + cipher_mode = g_strdup(luks->header.cipher_mode); + /* * The cipher_mode header contains a string that we have * to further parse, of the format @@ -718,7 +706,7 @@ qcrypto_block_luks_open(QCryptoBlock *block, * * eg cbc-essiv:sha256, cbc-plain64 */ - ivgen_name = strchr(luks->header.cipher_mode, '-'); + ivgen_name = strchr(cipher_mode, '-'); if (!ivgen_name) { ret = -EINVAL; error_setg(errp, "Unexpected cipher mode string format %s", @@ -730,13 +718,13 @@ qcrypto_block_luks_open(QCryptoBlock *block, ivhash_name = strchr(ivgen_name, ':'); if (!ivhash_name) { - ivhash = 0; + luks->ivgen_hash_alg = 0; } else { *ivhash_name = '\0'; ivhash_name++; - ivhash = qcrypto_block_luks_hash_name_lookup(ivhash_name, - &local_err); + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, + &local_err); if (local_err) { ret = -ENOTSUP; error_propagate(errp, local_err); @@ -744,25 +732,27 @@ qcrypto_block_luks_open(QCryptoBlock *block, } } - ciphermode = qcrypto_block_luks_cipher_mode_lookup(luks->header.cipher_mode, - &local_err); + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, + &local_err); if (local_err) { ret = -ENOTSUP; error_propagate(errp, local_err); goto fail; } - cipheralg = qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, - ciphermode, - luks->header.key_bytes, - &local_err); + luks->cipher_alg = + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, + luks->cipher_mode, + luks->header.key_bytes, + &local_err); if (local_err) { ret = -ENOTSUP; error_propagate(errp, local_err); goto fail; } - hash = qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, + luks->hash_alg = + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, &local_err); if (local_err) { ret = -ENOTSUP; @@ -770,23 +760,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, goto fail; } - ivalg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, - &local_err); + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, + &local_err); if (local_err) { ret = -ENOTSUP; error_propagate(errp, local_err); goto fail; } - if (ivalg == QCRYPTO_IVGEN_ALG_ESSIV) { + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { if (!ivhash_name) { ret = -EINVAL; error_setg(errp, "Missing IV generator hash specification"); goto fail; } - ivcipheralg = qcrypto_block_luks_essiv_cipher(cipheralg, - ivhash, - &local_err); + luks->ivgen_cipher_alg = + qcrypto_block_luks_essiv_cipher(luks->cipher_alg, + luks->ivgen_hash_alg, + &local_err); if (local_err) { ret = -ENOTSUP; error_propagate(errp, local_err); @@ -800,21 +791,25 @@ qcrypto_block_luks_open(QCryptoBlock *block, * ignore hash names with these ivgens rather than report * an error about the invalid usage */ - ivcipheralg = cipheralg; + luks->ivgen_cipher_alg = luks->cipher_alg; } + + g_free(cipher_mode); + cipher_mode = NULL; + ivgen_name = NULL; + ivhash_name = NULL; + if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { /* Try to find which key slot our password is valid for * and unlock the master key from that slot. */ + + masterkey = g_new0(uint8_t, masterkeylen(luks)); + if (qcrypto_block_luks_find_key(block, password, - cipheralg, ciphermode, - hash, - ivalg, - ivcipheralg, - ivhash, - &masterkey, &masterkeylen, + masterkey, readfunc, opaque, errp) < 0) { ret = -EACCES; @@ -824,21 +819,24 @@ qcrypto_block_luks_open(QCryptoBlock *block, /* We have a valid master key now, so can setup the * block device payload decryption objects */ - block->kdfhash = hash; - block->niv = qcrypto_cipher_get_iv_len(cipheralg, - ciphermode); - block->ivgen = qcrypto_ivgen_new(ivalg, - ivcipheralg, - ivhash, - masterkey, masterkeylen, + block->kdfhash = luks->hash_alg; + block->niv = qcrypto_cipher_get_iv_len(luks->cipher_alg, + luks->cipher_mode); + + block->ivgen = qcrypto_ivgen_new(luks->ivgen_alg, + luks->ivgen_cipher_alg, + luks->ivgen_hash_alg, + masterkey, masterkeylen(luks), errp); if (!block->ivgen) { ret = -ENOTSUP; goto fail; } - ret = qcrypto_block_init_cipher(block, cipheralg, ciphermode, - masterkey, masterkeylen, n_threads, + ret = qcrypto_block_init_cipher(block, luks->cipher_alg, + luks->cipher_mode, + masterkey, masterkeylen(luks), + n_threads, errp); if (ret < 0) { ret = -ENOTSUP; @@ -850,12 +848,6 @@ qcrypto_block_luks_open(QCryptoBlock *block, block->payload_offset = luks->header.payload_offset * block->sector_size; - luks->cipher_alg = cipheralg; - luks->cipher_mode = ciphermode; - luks->ivgen_alg = ivalg; - luks->ivgen_hash_alg = ivhash; - luks->hash_alg = hash; - g_free(masterkey); g_free(password); @@ -910,7 +902,7 @@ qcrypto_block_luks_create(QCryptoBlock *block, memcpy(&luks_opts, &options->u.luks, sizeof(luks_opts)); if (!luks_opts.has_iter_time) { - luks_opts.iter_time = 2000; + luks_opts.iter_time = QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME; } if (!luks_opts.has_cipher_alg) { luks_opts.cipher_alg = QCRYPTO_CIPHER_ALG_AES_256; @@ -930,6 +922,17 @@ qcrypto_block_luks_create(QCryptoBlock *block, luks_opts.has_ivgen_hash_alg = true; } } + + luks = g_new0(QCryptoBlockLUKS, 1); + block->opaque = luks; + + luks->cipher_alg = luks_opts.cipher_alg; + luks->cipher_mode = luks_opts.cipher_mode; + luks->ivgen_alg = luks_opts.ivgen_alg; + luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; + luks->hash_alg = luks_opts.hash_alg; + + /* Note we're allowing ivgen_hash_alg to be set even for * non-essiv iv generators that don't need a hash. It will * be silently ignored, for compatibility with dm-crypt */ @@ -944,8 +947,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, return -1; } - luks = g_new0(QCryptoBlockLUKS, 1); - block->opaque = luks; memcpy(luks->header.magic, qcrypto_block_luks_magic, QCRYPTO_BLOCK_LUKS_MAGIC_LEN); @@ -1003,6 +1004,8 @@ qcrypto_block_luks_create(QCryptoBlock *block, ivcipheralg = luks_opts.cipher_alg; } + luks->ivgen_cipher_alg = ivcipheralg; + strcpy(luks->header.cipher_name, cipher_alg); strcpy(luks->header.cipher_mode, cipher_mode_spec); strcpy(luks->header.hash_spec, hash_alg); @@ -1304,12 +1307,6 @@ qcrypto_block_luks_create(QCryptoBlock *block, goto error; } - luks->cipher_alg = luks_opts.cipher_alg; - luks->cipher_mode = luks_opts.cipher_mode; - luks->ivgen_alg = luks_opts.ivgen_alg; - luks->ivgen_hash_alg = luks_opts.ivgen_hash_alg; - luks->hash_alg = luks_opts.hash_alg; - memset(masterkey, 0, luks->header.key_bytes); g_free(masterkey); memset(slotkey, 0, luks->header.key_bytes);
This is also a preparation for key read/write/erase functions * use master key len from the header * prefer to use crypto params in the QCryptoBlockLUKS over passing them as function arguments * define QCRYPTO_BLOCK_LUKS_DEFAULT_ITER_TIME * Add comments to various crypto parameters in the QCryptoBlockLUKS Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- crypto/block-luks.c | 213 ++++++++++++++++++++++---------------------- 1 file changed, 105 insertions(+), 108 deletions(-)