Re: [Qemu-devel] [PATCH v2 09/13] qcrypto-block: extract check and parse header

[Maxim Levitsky <mlevitsk@redhat.com>]

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