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

[Stefano Garzarella]

On Thu, Oct 17, 2019 at 03:56:53PM +0100, 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
> +  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);

What about adding 'return 0' here and avoiding the next ifdef?

>      } 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);

Also here...

>      } 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);

And maybe here...

>      } 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
>  
>      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)

Anyway the patch LGTM, but I don't know this code very well, then:

Acked-by: Stefano Garzarella <address@hidden>