| Message ID | 20190826135103.22410-10-mlevitsk@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | RFC crypto/luks: preparation for encryption key managment | expand |
On Mon, Aug 26, 2019 at 04:50:59PM +0300, Maxim Levitsky wrote: > This is just to make qcrypto_block_luks_open more > reasonable in size. > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > --- > crypto/block-luks.c | 254 +++++++++++++++++++++++++------------------- > 1 file changed, 146 insertions(+), 108 deletions(-) > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > index b4dc6fc899..cc9a52c9af 100644 > --- a/crypto/block-luks.c > +++ b/crypto/block-luks.c > @@ -508,6 +508,148 @@ fail: > return ret; > } > > +/* > + * Does basic sanity checks on the LUKS header > + */ > +static int > +qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) > +{ > + int ret; > + > + if (memcmp(luks->header.magic, qcrypto_block_luks_magic, > + QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { > + error_setg(errp, "Volume is not in LUKS format"); > + ret = -EINVAL; > + goto fail; > + } Just 'return -1' here immediately - don't return an errno, as we're using Error objects for reporting. > + > + if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) { > + error_setg(errp, "LUKS version %" PRIu32 " is not supported", > + luks->header.version); > + ret = -ENOTSUP; > + goto fail; > + } > + > + return 0; > +fail: > + return ret; > +} > + > +/* > + * Parses the crypto parameters that are stored in the LUKS header > + */ > + > +static int > +qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp) > +{ > + g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode); > + char *ivgen_name, *ivhash_name; > + int ret = -1; > + Error *local_err = NULL; > + > + /* > + * The cipher_mode header contains a string that we have > + * to further parse, of the format > + * > + * <cipher-mode>-<iv-generator>[:<iv-hash>] > + * > + * eg cbc-essiv:sha256, cbc-plain64 > + */ > + ivgen_name = strchr(cipher_mode, '-'); > + if (!ivgen_name) { > + ret = -EINVAL; Again, don't use errnos - just return -1 in this method. > + error_setg(errp, "Unexpected cipher mode string format %s", > + luks->header.cipher_mode); > + goto out; > + } > + *ivgen_name = '\0'; > + ivgen_name++; > + > + ivhash_name = strchr(ivgen_name, ':'); > + if (!ivhash_name) { > + luks->ivgen_hash_alg = 0; > + } else { > + *ivhash_name = '\0'; > + ivhash_name++; > + > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > + &local_err); > + if (local_err) { > + ret = -ENOTSUP; > + error_propagate(errp, local_err); > + goto out; > + } > + } > + > + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, > + &local_err); > + if (local_err) { > + ret = -ENOTSUP; > + error_propagate(errp, local_err); > + goto out; > + } > + > + luks->cipher_alg = > + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > + luks->cipher_mode, > + luks->header.master_key_len, > + &local_err); > + if (local_err) { > + ret = -ENOTSUP; > + error_propagate(errp, local_err); > + goto out; > + } > + > + luks->hash_alg = > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > + &local_err); > + if (local_err) { > + ret = -ENOTSUP; > + error_propagate(errp, local_err); > + goto out; > + } > + > + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > + &local_err); > + if (local_err) { > + ret = -ENOTSUP; > + error_propagate(errp, local_err); > + goto out; > + } > + > + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { > + if (!ivhash_name) { > + ret = -EINVAL; > + error_setg(errp, "Missing IV generator hash specification"); > + goto out; > + } > + 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); > + goto out; > + } > + } else { > + > + /* > + * Note we parsed the ivhash_name earlier in the cipher_mode > + * spec string even with plain/plain64 ivgens, but we > + * will ignore it, since it is irrelevant for these ivgens. > + * This is for compat with dm-crypt which will silently > + * ignore hash names with these ivgens rather than report > + * an error about the invalid usage > + */ > + luks->ivgen_cipher_alg = luks->cipher_alg; > + } > + ret = 0; > +out: > + return ret; > + > +} > + > /* > * Given a key slot, and user password, this will attempt to unlock > * the master encryption key from the key slot. > @@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block, > Error **errp) > { > QCryptoBlockLUKS *luks = NULL; > - Error *local_err = NULL; > int ret = 0; > g_autofree uint8_t *masterkey = NULL; > - char *ivgen_name, *ivhash_name; > g_autofree char *password = NULL; > - g_autofree char *cipher_mode = NULL; > > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > if (!options->u.luks.key_secret) { > @@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block, > goto fail; > } > > - > - if (memcmp(luks->header.magic, qcrypto_block_luks_magic, > - QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { > - error_setg(errp, "Volume is not in LUKS format"); > - ret = -EINVAL; I can't remember why I set ret to an errno in my original code. it is entirely pointless, as the caller of this method merely checks "ret < 0" and doesn't do anything else with the return value. IOW, we should just purge the errnors from this existing code entirely. Regards, Daniel
On Fri, 2019-09-06 at 14:11 +0100, Daniel P. Berrangé wrote: > On Mon, Aug 26, 2019 at 04:50:59PM +0300, Maxim Levitsky wrote: > > This is just to make qcrypto_block_luks_open more > > reasonable in size. > > > > Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> > > --- > > crypto/block-luks.c | 254 +++++++++++++++++++++++++------------------- > > 1 file changed, 146 insertions(+), 108 deletions(-) > > > > diff --git a/crypto/block-luks.c b/crypto/block-luks.c > > index b4dc6fc899..cc9a52c9af 100644 > > --- a/crypto/block-luks.c > > +++ b/crypto/block-luks.c > > @@ -508,6 +508,148 @@ fail: > > return ret; > > } > > > > +/* > > + * Does basic sanity checks on the LUKS header > > + */ > > +static int > > +qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) > > +{ > > + int ret; > > + > > + if (memcmp(luks->header.magic, qcrypto_block_luks_magic, > > + QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { > > + error_setg(errp, "Volume is not in LUKS format"); > > + ret = -EINVAL; > > + goto fail; > > + } > > Just 'return -1' here immediately - don't return an errno, as we're > using Error objects for reporting. > > > + > > + if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) { > > + error_setg(errp, "LUKS version %" PRIu32 " is not supported", > > + luks->header.version); > > + ret = -ENOTSUP; > > + goto fail; > > + } > > + > > + return 0; > > +fail: > > + return ret; > > +} > > + > > +/* > > + * Parses the crypto parameters that are stored in the LUKS header > > + */ > > + > > +static int > > +qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp) > > +{ > > + g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode); > > + char *ivgen_name, *ivhash_name; > > + int ret = -1; > > + Error *local_err = NULL; > > + > > + /* > > + * The cipher_mode header contains a string that we have > > + * to further parse, of the format > > + * > > + * <cipher-mode>-<iv-generator>[:<iv-hash>] > > + * > > + * eg cbc-essiv:sha256, cbc-plain64 > > + */ > > + ivgen_name = strchr(cipher_mode, '-'); > > + if (!ivgen_name) { > > + ret = -EINVAL; > > Again, don't use errnos - just return -1 in this method. > > > + error_setg(errp, "Unexpected cipher mode string format %s", > > + luks->header.cipher_mode); > > + goto out; > > + } > > + *ivgen_name = '\0'; > > + ivgen_name++; > > + > > + ivhash_name = strchr(ivgen_name, ':'); > > + if (!ivhash_name) { > > + luks->ivgen_hash_alg = 0; > > + } else { > > + *ivhash_name = '\0'; > > + ivhash_name++; > > + > > + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, > > + &local_err); > > + if (local_err) { > > + ret = -ENOTSUP; > > + error_propagate(errp, local_err); > > + goto out; > > + } > > + } > > + > > + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, > > + &local_err); > > + if (local_err) { > > + ret = -ENOTSUP; > > + error_propagate(errp, local_err); > > + goto out; > > + } > > + > > + luks->cipher_alg = > > + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, > > + luks->cipher_mode, > > + luks->header.master_key_len, > > + &local_err); > > + if (local_err) { > > + ret = -ENOTSUP; > > + error_propagate(errp, local_err); > > + goto out; > > + } > > + > > + luks->hash_alg = > > + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, > > + &local_err); > > + if (local_err) { > > + ret = -ENOTSUP; > > + error_propagate(errp, local_err); > > + goto out; > > + } > > + > > + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, > > + &local_err); > > + if (local_err) { > > + ret = -ENOTSUP; > > + error_propagate(errp, local_err); > > + goto out; > > + } > > + > > + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { > > + if (!ivhash_name) { > > + ret = -EINVAL; > > + error_setg(errp, "Missing IV generator hash specification"); > > + goto out; > > + } > > + 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); > > + goto out; > > + } > > + } else { > > + > > + /* > > + * Note we parsed the ivhash_name earlier in the cipher_mode > > + * spec string even with plain/plain64 ivgens, but we > > + * will ignore it, since it is irrelevant for these ivgens. > > + * This is for compat with dm-crypt which will silently > > + * ignore hash names with these ivgens rather than report > > + * an error about the invalid usage > > + */ > > + luks->ivgen_cipher_alg = luks->cipher_alg; > > + } > > + ret = 0; > > +out: > > + return ret; > > + > > +} > > + > > /* > > * Given a key slot, and user password, this will attempt to unlock > > * the master encryption key from the key slot. > > @@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > Error **errp) > > { > > QCryptoBlockLUKS *luks = NULL; > > - Error *local_err = NULL; > > int ret = 0; > > g_autofree uint8_t *masterkey = NULL; > > - char *ivgen_name, *ivhash_name; > > g_autofree char *password = NULL; > > - g_autofree char *cipher_mode = NULL; > > > > if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { > > if (!options->u.luks.key_secret) { > > @@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block, > > goto fail; > > } > > > > - > > - if (memcmp(luks->header.magic, qcrypto_block_luks_magic, > > - QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { > > - error_setg(errp, "Volume is not in LUKS format"); > > - ret = -EINVAL; > > I can't remember why I set ret to an errno in my original code. > it is entirely pointless, as the caller of this method merely > checks "ret < 0" and doesn't do anything else with the return > value. IOW, we should just purge the errnors from this existing > code entirely. Nothing against that, I'll do that in a separate patch now. Best regards, Maxim Levitsky
diff --git a/crypto/block-luks.c b/crypto/block-luks.c index b4dc6fc899..cc9a52c9af 100644 --- a/crypto/block-luks.c +++ b/crypto/block-luks.c @@ -508,6 +508,148 @@ fail: return ret; } +/* + * Does basic sanity checks on the LUKS header + */ +static int +qcrypto_block_luks_check_header(const QCryptoBlockLUKS *luks, Error **errp) +{ + int ret; + + if (memcmp(luks->header.magic, qcrypto_block_luks_magic, + QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { + error_setg(errp, "Volume is not in LUKS format"); + ret = -EINVAL; + goto fail; + } + + if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) { + error_setg(errp, "LUKS version %" PRIu32 " is not supported", + luks->header.version); + ret = -ENOTSUP; + goto fail; + } + + return 0; +fail: + return ret; +} + +/* + * Parses the crypto parameters that are stored in the LUKS header + */ + +static int +qcrypto_block_luks_parse_header(QCryptoBlockLUKS *luks, Error **errp) +{ + g_autofree char *cipher_mode = g_strdup(luks->header.cipher_mode); + char *ivgen_name, *ivhash_name; + int ret = -1; + Error *local_err = NULL; + + /* + * The cipher_mode header contains a string that we have + * to further parse, of the format + * + * <cipher-mode>-<iv-generator>[:<iv-hash>] + * + * eg cbc-essiv:sha256, cbc-plain64 + */ + ivgen_name = strchr(cipher_mode, '-'); + if (!ivgen_name) { + ret = -EINVAL; + error_setg(errp, "Unexpected cipher mode string format %s", + luks->header.cipher_mode); + goto out; + } + *ivgen_name = '\0'; + ivgen_name++; + + ivhash_name = strchr(ivgen_name, ':'); + if (!ivhash_name) { + luks->ivgen_hash_alg = 0; + } else { + *ivhash_name = '\0'; + ivhash_name++; + + luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, + &local_err); + if (local_err) { + ret = -ENOTSUP; + error_propagate(errp, local_err); + goto out; + } + } + + luks->cipher_mode = qcrypto_block_luks_cipher_mode_lookup(cipher_mode, + &local_err); + if (local_err) { + ret = -ENOTSUP; + error_propagate(errp, local_err); + goto out; + } + + luks->cipher_alg = + qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, + luks->cipher_mode, + luks->header.master_key_len, + &local_err); + if (local_err) { + ret = -ENOTSUP; + error_propagate(errp, local_err); + goto out; + } + + luks->hash_alg = + qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, + &local_err); + if (local_err) { + ret = -ENOTSUP; + error_propagate(errp, local_err); + goto out; + } + + luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, + &local_err); + if (local_err) { + ret = -ENOTSUP; + error_propagate(errp, local_err); + goto out; + } + + if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { + if (!ivhash_name) { + ret = -EINVAL; + error_setg(errp, "Missing IV generator hash specification"); + goto out; + } + 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); + goto out; + } + } else { + + /* + * Note we parsed the ivhash_name earlier in the cipher_mode + * spec string even with plain/plain64 ivgens, but we + * will ignore it, since it is irrelevant for these ivgens. + * This is for compat with dm-crypt which will silently + * ignore hash names with these ivgens rather than report + * an error about the invalid usage + */ + luks->ivgen_cipher_alg = luks->cipher_alg; + } + ret = 0; +out: + return ret; + +} + /* * Given a key slot, and user password, this will attempt to unlock * the master encryption key from the key slot. @@ -720,12 +862,9 @@ qcrypto_block_luks_open(QCryptoBlock *block, Error **errp) { QCryptoBlockLUKS *luks = NULL; - Error *local_err = NULL; int ret = 0; g_autofree uint8_t *masterkey = NULL; - char *ivgen_name, *ivhash_name; g_autofree char *password = NULL; - g_autofree char *cipher_mode = NULL; if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { if (!options->u.luks.key_secret) { @@ -748,117 +887,16 @@ qcrypto_block_luks_open(QCryptoBlock *block, goto fail; } - - if (memcmp(luks->header.magic, qcrypto_block_luks_magic, - QCRYPTO_BLOCK_LUKS_MAGIC_LEN) != 0) { - error_setg(errp, "Volume is not in LUKS format"); - ret = -EINVAL; - goto fail; - } - if (luks->header.version != QCRYPTO_BLOCK_LUKS_VERSION) { - error_setg(errp, "LUKS version %" PRIu32 " is not supported", - luks->header.version); - ret = -ENOTSUP; - 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 - * - * <cipher-mode>-<iv-generator>[:<iv-hash>] - * - * eg cbc-essiv:sha256, cbc-plain64 - */ - ivgen_name = strchr(cipher_mode, '-'); - if (!ivgen_name) { - ret = -EINVAL; - error_setg(errp, "Unexpected cipher mode string format %s", - cipher_mode); - goto fail; - } - *ivgen_name = '\0'; - ivgen_name++; - - ivhash_name = strchr(ivgen_name, ':'); - if (!ivhash_name) { - luks->ivgen_hash_alg = 0; - } else { - *ivhash_name = '\0'; - ivhash_name++; - - luks->ivgen_hash_alg = qcrypto_block_luks_hash_name_lookup(ivhash_name, - &local_err); - if (local_err) { - ret = -ENOTSUP; - error_propagate(errp, local_err); - goto fail; - } - } - - 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; - } - - luks->cipher_alg = - qcrypto_block_luks_cipher_name_lookup(luks->header.cipher_name, - luks->cipher_mode, - luks->header.master_key_len, - &local_err); - if (local_err) { - ret = -ENOTSUP; - error_propagate(errp, local_err); - goto fail; - } - - luks->hash_alg = - qcrypto_block_luks_hash_name_lookup(luks->header.hash_spec, - &local_err); - if (local_err) { - ret = -ENOTSUP; - error_propagate(errp, local_err); + ret = qcrypto_block_luks_check_header(luks, errp); + if (ret) { goto fail; } - luks->ivgen_alg = qcrypto_block_luks_ivgen_name_lookup(ivgen_name, - &local_err); - if (local_err) { - ret = -ENOTSUP; - error_propagate(errp, local_err); + ret = qcrypto_block_luks_parse_header(luks, errp); + if (ret) { goto fail; } - if (luks->ivgen_alg == QCRYPTO_IVGEN_ALG_ESSIV) { - if (!ivhash_name) { - ret = -EINVAL; - error_setg(errp, "Missing IV generator hash specification"); - goto fail; - } - 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); - goto fail; - } - } else { - /* Note we parsed the ivhash_name earlier in the cipher_mode - * spec string even with plain/plain64 ivgens, but we - * will ignore it, since it is irrelevant for these ivgens. - * This is for compat with dm-crypt which will silently - * ignore hash names with these ivgens rather than report - * an error about the invalid usage - */ - luks->ivgen_cipher_alg = luks->cipher_alg; - } if (!(flags & QCRYPTO_BLOCK_OPEN_NO_IO)) { /* Try to find which key slot our password is valid for
This is just to make qcrypto_block_luks_open more reasonable in size. Signed-off-by: Maxim Levitsky <mlevitsk@redhat.com> --- crypto/block-luks.c | 254 +++++++++++++++++++++++++------------------- 1 file changed, 146 insertions(+), 108 deletions(-)