Re: [Qemu-devel] [PATCH for-2.9 v2 4/7] crypto: support HMAC algorithms based on libgcrypt

["Daniel P. Berrange" <berrange@redhat.com>]

On Mon, Dec 12, 2016 at 04:08:09PM +0800, Longpeng(Mike) wrote:
> This patch add HMAC algorithms based on libgcrypt support
> 
> Signed-off-by: Longpeng(Mike) <longpeng2@huawei.com>
> ---
>  crypto/hmac-gcrypt.c | 138 +++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 138 insertions(+)
> 
> diff --git a/crypto/hmac-gcrypt.c b/crypto/hmac-gcrypt.c
> index 26f42bc..6cf3046 100644
> --- a/crypto/hmac-gcrypt.c
> +++ b/crypto/hmac-gcrypt.c
> @@ -15,6 +15,142 @@
>  #include "qemu/osdep.h"
>  #include "qapi/error.h"
>  #include "crypto/hmac.h"
> +#include <gcrypt.h>
> +
> +#ifdef CONFIG_GCRYPT_SUPPORT_HMAC

You shouldn't use this conditional in the code.  Instead, you should
use it in crypto/Makefile.objs, so that this file is only ever
compiled when CONFIG_GCRYPTO_HMAC is set - see the way we deal
with CONFIG_NETTLE_KDF for example.


> +static int qcrypto_hmac_alg_map[QCRYPTO_HMAC_ALG__MAX] = {
> +    [QCRYPTO_HMAC_ALG_MD5] = GCRY_MAC_HMAC_MD5,
> +    [QCRYPTO_HMAC_ALG_SHA1] = GCRY_MAC_HMAC_SHA1,
> +    [QCRYPTO_HMAC_ALG_SHA256] = GCRY_MAC_HMAC_SHA256,
> +    [QCRYPTO_HMAC_ALG_SHA512] = GCRY_MAC_HMAC_SHA512,
> +};

Since I requested use of existing QCRYPTO_HASH_ALG_* constants,
can you expand this to handle all 7 hash algoriths we have defined
for qemu


> +QCryptoHmac *qcrypto_hmac_new(QCryptoHmacAlgorithm alg,
> +                                  const uint8_t *key, size_t nkey,
> +                                  Error **errp)

Nitpick, indentation wrt '('

> +{
> +    QCryptoHmac *hmac;
> +    QCryptoHmacGcrypt *ctx;
> +    gcry_error_t err;
> +
> +    if (!qcrypto_hmac_supports(alg)) {
> +        error_setg(errp, "Unsupported hmac algorithm %s",
> +                    QCryptoHmacAlgorithm_lookup[alg]);

Again, nitpick on indentation wrt '('

> +        return NULL;
> +    }
> +
> +    hmac = g_new0(QCryptoHmac, 1);
> +    hmac->alg = alg;
> +
> +    ctx = g_new0(QCryptoHmacGcrypt, 1);
> +
> +    err = gcry_mac_open(&ctx->handle, qcrypto_hmac_alg_map[alg],
> +                    GCRY_MAC_FLAG_SECURE, NULL);

Again, nitpick on indentation

> +    if (err != 0) {
> +        error_setg(errp, "Cannot initialize hmac: %s",
> +                    gcry_strerror(err));

Again

> +        goto error;
> +    }
> +
> +    err = gcry_mac_setkey(ctx->handle, (const void *)key, nkey);
> +    if (err != 0) {
> +        error_setg(errp, "Cannot set key: %s",
> +                    gcry_strerror(err));

Again. I'll stop repeating this now - just fix up all indentation
through this file and all remaining patches in the series.

> +        goto error;
> +    }
> +
> +    hmac->opaque = ctx;
> +    return hmac;
> +
> +error:
> +    g_free(ctx);
> +    g_free(hmac);
> +    return NULL;
> +}
> +
> +void qcrypto_hmac_free(QCryptoHmac *hmac)
> +{
> +    QCryptoHmacGcrypt *ctx;
> +
> +    if (!hmac) {
> +        return;
> +    }
> +
> +    ctx = hmac->opaque;
> +    gcry_mac_close(ctx->handle);
> +
> +    g_free(ctx);
> +    g_free(hmac);
> +}
> +
> +int qcrypto_hmac_bytesv(QCryptoHmac *hmac,
> +                        const struct iovec *iov,
> +                        size_t niov,
> +                        uint8_t **result,
> +                        size_t *resultlen,
> +                        Error **errp)
> +{
> +    QCryptoHmacGcrypt *ctx;
> +    gcry_error_t err;
> +    uint32_t ret;
> +    int i;
> +
> +    ctx = hmac->opaque;
> +
> +    for (i = 0; i < niov; i++) {
> +        gcry_mac_write(ctx->handle, iov[i].iov_base, iov[i].iov_len);
> +    }
> +
> +    ret = gcry_mac_get_algo_maclen(qcrypto_hmac_alg_map[hmac->alg]);
> +    if (ret <= 0) {
> +        error_setg(errp, "Unable to get hmac length: %s",
> +                    gcry_strerror(ret));
> +        return -1;
> +    }
> +
> +    if (*resultlen == 0) {
> +        *resultlen = ret;
> +        *result = g_new0(uint8_t, *resultlen);
> +    } else if (*resultlen != ret) {
> +        error_setg(errp, "Result buffer size %zu is smaller than hmac %d",
> +                    *resultlen, ret);
> +        return -1;
> +    }
> +
> +    err = gcry_mac_read(ctx->handle, *result, resultlen);
> +    if (err != 0) {
> +        error_setg(errp, "Cannot get result: %s",
> +                    gcry_strerror(err));
> +        return -1;
> +    }
> +
> +    err = gcry_mac_reset(ctx->handle);
> +    if (err != 0) {
> +        error_setg(errp, "Cannot reset hmac context: %s",
> +                    gcry_strerror(err));
> +        return -1;
> +    }
> +
> +    return 0;
> +}
> +
> +#else
>  
>  bool qcrypto_hmac_supports(QCryptoHmacAlgorithm alg)
>  {
> @@ -42,3 +178,5 @@ int qcrypto_hmac_bytesv(QCryptoHmac *hmac,
>  {
>      return -1;
>  }
> +
> +#endif

You shouldn't need this else block either, since we should never
compile any no-op code - we want to compile hmac-glib instead
if gcrypt doesn't do hmac

Regards,
Daniel
-- 
|: http://berrange.com      -o-    http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org              -o-             http://virt-manager.org :|
|: http://entangle-photo.org       -o-    http://search.cpan.org/~danberr/ :|