Re: [PATCH 3/4] crypto: add support for gcrypt's native XTS impl

[Philippe Mathieu-Daudé]

On 10/17/19 4:56 PM, Daniel P. Berrangé wrote:
> Libgcrypt 1.8.0 added support for the XTS mode. Use this because long
> term we wish to delete QEMU's XTS impl to avoid carrying private crypto
> algorithm impls.
>
> As an added benefit, using this improves performance from 531 MB/sec to
> 670 MB/sec, since we are avoiding several layers of function call
> indirection.
>
> This is even more noticable with the gcrypt builds in Fedora or RHEL-8
> which have a non-upstream patch for FIPS mode which does mutex locking.
> This is catastrophic for encryption performance with small block sizes,
> meaning this patch improves encryption from 240 MB/sec to 670 MB/sec.
>
> Signed-off-by: Daniel P. Berrangé <address@hidden>
> ---
>   configure              | 22 ++++++++++++++++++++++
>   crypto/Makefile.objs   |  2 +-
>   crypto/cipher-gcrypt.c | 36 +++++++++++++++++++++++++++++++++++-
>   tests/Makefile.include |  2 +-
>   4 files changed, 59 insertions(+), 3 deletions(-)
>
> diff --git a/configure b/configure
> index 08ca4bcb46..98edb0ff44 100755
> --- a/configure
> +++ b/configure
> @@ -473,6 +473,8 @@ gnutls=""
>   nettle=""
>   gcrypt=""
>   gcrypt_hmac="no"
> +gcrypt_xts="no"
> +qemu_private_xts="yes"
>   auth_pam=""
>   vte=""
>   virglrenderer=""
> @@ -2902,6 +2904,18 @@ EOF
>           if compile_prog "$gcrypt_cflags" "$gcrypt_libs" ; then
>               gcrypt_hmac=yes
>           fi
> +        cat > $TMPC << EOF
> +#include <gcrypt.h>
> +int main(void) {
> +  gcry_cipher_hd_t handle;
> +  gcry_cipher_open(&handle, GCRY_CIPHER_AES, GCRY_CIPHER_MODE_XTS, 0);
> +  return 0;
> +}
> +EOF
> +        if compile_prog "$gcrypt_cflags" "$gcrypt_libs" ; then
> +            gcrypt_xts=yes
> +            qemu_private_xts=no
> +        fi
>       elif test "$gcrypt" = "yes"; then
>           feature_not_found "gcrypt" "Install gcrypt devel >= 1.5.0"
>       else
> @@ -6317,6 +6331,11 @@ echo "VTE support       $vte $(echo_version $vte 
> $vteversion)"
>   echo "TLS priority      $tls_priority"
>   echo "GNUTLS support    $gnutls"
>   echo "libgcrypt         $gcrypt"
> +if test "$gcrypt" = "yes"
> +then
> +   echo "  hmac            $gcrypt_hmac"
> +   echo "  XTS             $gcrypt_xts"
> +fi
>   echo "nettle            $nettle $(echo_version $nettle $nettle_version)"
>   echo "libtasn1          $tasn1"
>   echo "PAM               $auth_pam"
> @@ -6794,6 +6813,9 @@ if test "$nettle" = "yes" ; then
>     echo "CONFIG_NETTLE=y" >> $config_host_mak
>     echo "CONFIG_NETTLE_VERSION_MAJOR=${nettle_version%%.*}" >> $config_host_mak
>   fi
> +if test "$qemu_private_xts" = "yes" ; then

Why not check $gcrypt_xts = "no", so we can avoid using $qemu_private_xts?

> +  echo "CONFIG_QEMU_PRIVATE_XTS=y" >> $config_host_mak
> +fi
>   if test "$tasn1" = "yes" ; then
>     echo "CONFIG_TASN1=y" >> $config_host_mak
>   fi
> diff --git a/crypto/Makefile.objs b/crypto/Makefile.objs
> index 7fe2fa9da2..cdb01f9de9 100644
> --- a/crypto/Makefile.objs
> +++ b/crypto/Makefile.objs
> @@ -31,7 +31,7 @@ crypto-obj-y += ivgen-essiv.o
>   crypto-obj-y += ivgen-plain.o
>   crypto-obj-y += ivgen-plain64.o
>   crypto-obj-y += afsplit.o
> -crypto-obj-y += xts.o
> +crypto-obj-$(CONFIG_QEMU_PRIVATE_XTS) += xts.o
>   crypto-obj-y += block.o
>   crypto-obj-y += block-qcow.o
>   crypto-obj-y += block-luks.o
> diff --git a/crypto/cipher-gcrypt.c b/crypto/cipher-gcrypt.c
> index 5cece9b244..ace719526a 100644
> --- a/crypto/cipher-gcrypt.c
> +++ b/crypto/cipher-gcrypt.c
> @@ -19,7 +19,9 @@
>    */
>   
>   #include "qemu/osdep.h"
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>   #include "crypto/xts.h"
> +#endif
>   #include "cipherpriv.h"
>   
>   #include <gcrypt.h>
> @@ -59,10 +61,12 @@ bool qcrypto_cipher_supports(QCryptoCipherAlgorithm alg,
>   typedef struct QCryptoCipherGcrypt QCryptoCipherGcrypt;
>   struct QCryptoCipherGcrypt {
>       gcry_cipher_hd_t handle;
> -    gcry_cipher_hd_t tweakhandle;
>       size_t blocksize;
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
> +    gcry_cipher_hd_t tweakhandle;
>       /* Initialization vector or Counter */
>       uint8_t *iv;
> +#endif
>   };
>   
>   static void
> @@ -74,10 +78,12 @@ qcrypto_gcrypt_cipher_free_ctx(QCryptoCipherGcrypt *ctx,
>       }
>   
>       gcry_cipher_close(ctx->handle);
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (mode == QCRYPTO_CIPHER_MODE_XTS) {
>           gcry_cipher_close(ctx->tweakhandle);
>       }
>       g_free(ctx->iv);
> +#endif
>       g_free(ctx);
>   }
>   
> @@ -94,8 +100,14 @@ static QCryptoCipherGcrypt *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>   
>       switch (mode) {
>       case QCRYPTO_CIPHER_MODE_ECB:
> +        gcrymode = GCRY_CIPHER_MODE_ECB;
> +        break;
>       case QCRYPTO_CIPHER_MODE_XTS:
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>           gcrymode = GCRY_CIPHER_MODE_ECB;
> +#else
> +        gcrymode = GCRY_CIPHER_MODE_XTS;
> +#endif
>           break;
>       case QCRYPTO_CIPHER_MODE_CBC:
>           gcrymode = GCRY_CIPHER_MODE_CBC;
> @@ -172,6 +184,7 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>                      gcry_strerror(err));
>           goto error;
>       }
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (mode == QCRYPTO_CIPHER_MODE_XTS) {
>           err = gcry_cipher_open(&ctx->tweakhandle, gcryalg, gcrymode, 0);
>           if (err != 0) {
> @@ -180,6 +193,7 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>               goto error;
>           }
>       }
> +#endif
>   
>       if (alg == QCRYPTO_CIPHER_ALG_DES_RFB) {
>           /* We're using standard DES cipher from gcrypt, so we need
> @@ -191,6 +205,7 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>           g_free(rfbkey);
>           ctx->blocksize = 8;
>       } else {
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>           if (mode == QCRYPTO_CIPHER_MODE_XTS) {
>               nkey /= 2;
>               err = gcry_cipher_setkey(ctx->handle, key, nkey);
> @@ -201,8 +216,11 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>               }
>               err = gcry_cipher_setkey(ctx->tweakhandle, key + nkey, nkey);
>           } else {
> +#endif
>               err = gcry_cipher_setkey(ctx->handle, key, nkey);
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>           }
> +#endif
>           if (err != 0) {
>               error_setg(errp, "Cannot set key: %s",
>                          gcry_strerror(err));
> @@ -228,6 +246,7 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>           }
>       }
>   
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (mode == QCRYPTO_CIPHER_MODE_XTS) {
>           if (ctx->blocksize != XTS_BLOCK_SIZE) {
>               error_setg(errp,
> @@ -237,6 +256,7 @@ static QCryptoCipherGcrypt 
> *qcrypto_cipher_ctx_new(QCryptoCipherAlgorithm alg,
>           }
>           ctx->iv = g_new0(uint8_t, ctx->blocksize);
>       }
> +#endif
>   
>       return ctx;
>   
> @@ -253,6 +273,7 @@ qcrypto_gcrypt_cipher_ctx_free(QCryptoCipher *cipher)
>   }
>   
>   
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>   static void qcrypto_gcrypt_xts_encrypt(const void *ctx,
>                                          size_t length,
>                                          uint8_t *dst,
> @@ -272,6 +293,7 @@ static void qcrypto_gcrypt_xts_decrypt(const void *ctx,
>       err = gcry_cipher_decrypt((gcry_cipher_hd_t)ctx, dst, length, src, 
> length);
>       g_assert(err == 0);
>   }
> +#endif
>   
>   static int
>   qcrypto_gcrypt_cipher_encrypt(QCryptoCipher *cipher,
> @@ -289,12 +311,14 @@ qcrypto_gcrypt_cipher_encrypt(QCryptoCipher *cipher,
>           return -1;
>       }
>   
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (cipher->mode == QCRYPTO_CIPHER_MODE_XTS) {
>           xts_encrypt(ctx->handle, ctx->tweakhandle,
>                       qcrypto_gcrypt_xts_encrypt,
>                       qcrypto_gcrypt_xts_decrypt,
>                       ctx->iv, len, out, in);
>       } else {
> +#endif
>           err = gcry_cipher_encrypt(ctx->handle,
>                                     out, len,
>                                     in, len);
> @@ -303,7 +327,9 @@ qcrypto_gcrypt_cipher_encrypt(QCryptoCipher *cipher,
>                          gcry_strerror(err));
>               return -1;
>           }
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       }
> +#endif
>   
>       return 0;
>   }
> @@ -325,12 +351,14 @@ qcrypto_gcrypt_cipher_decrypt(QCryptoCipher *cipher,
>           return -1;
>       }
>   
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (cipher->mode == QCRYPTO_CIPHER_MODE_XTS) {
>           xts_decrypt(ctx->handle, ctx->tweakhandle,
>                       qcrypto_gcrypt_xts_encrypt,
>                       qcrypto_gcrypt_xts_decrypt,
>                       ctx->iv, len, out, in);
>       } else {
> +#endif
>           err = gcry_cipher_decrypt(ctx->handle,
>                                     out, len,
>                                     in, len);
> @@ -339,7 +367,9 @@ qcrypto_gcrypt_cipher_decrypt(QCryptoCipher *cipher,
>                          gcry_strerror(err));
>               return -1;
>           }
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       }
> +#endif
>   
>       return 0;
>   }
> @@ -358,9 +388,11 @@ qcrypto_gcrypt_cipher_setiv(QCryptoCipher *cipher,
>           return -1;
>       }
>   
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       if (ctx->iv) {
>           memcpy(ctx->iv, iv, niv);
>       } else {
> +#endif
>           if (cipher->mode == QCRYPTO_CIPHER_MODE_CTR) {
>               err = gcry_cipher_setctr(ctx->handle, iv, niv);
>               if (err != 0) {
> @@ -377,7 +409,9 @@ qcrypto_gcrypt_cipher_setiv(QCryptoCipher *cipher,
>                   return -1;
>               }
>           }
> +#ifdef CONFIG_QEMU_PRIVATE_XTS
>       }
> +#endif

This is a lot of ifdef'ry...

>   
>       return 0;
>   }
> diff --git a/tests/Makefile.include b/tests/Makefile.include
> index 3543451ed3..2e5b0d3604 100644
> --- a/tests/Makefile.include
> +++ b/tests/Makefile.include
> @@ -133,7 +133,7 @@ check-unit-y += tests/test-base64$(EXESUF)
>   check-unit-$(call land,$(CONFIG_BLOCK),$(if 
> $(CONFIG_NETTLE),y,$(CONFIG_GCRYPT))) += tests/test-crypto-pbkdf$(EXESUF)
>   check-unit-$(CONFIG_BLOCK) += tests/test-crypto-ivgen$(EXESUF)
>   check-unit-$(CONFIG_BLOCK)  += tests/test-crypto-afsplit$(EXESUF)
> -check-unit-$(CONFIG_BLOCK)  += tests/test-crypto-xts$(EXESUF)
> +check-unit-$(if $(CONFIG_BLOCK),$(CONFIG_QEMU_PRIVATE_XTS)) += 
> tests/test-crypto-xts$(EXESUF)
>   check-unit-$(CONFIG_BLOCK)  += tests/test-crypto-block$(EXESUF)
>   check-unit-y += tests/test-logging$(EXESUF)
>   check-unit-$(call land,$(CONFIG_BLOCK),$(CONFIG_REPLICATION)) += 
> tests/test-replication$(EXESUF)