crypto: implement workaround for GNUTLS thread safety problems

When TLS 1.3 is negotiated on a TLS session, GNUTLS will perform
automatic rekeying of the session after 16 million records. This
is done for all algorithms except CHACHA20_POLY1305 which does
not require rekeying.

Unfortunately the rekeying breaks GNUTLS' promise that it is safe
to use a gnutls_session_t object concurrently from multiple threads
if they are exclusively calling gnutls_record_send/recv.

This patch implements a workaround for QEMU that adds a mutex lock
around any gnutls_record_send/recv call to serialize execution
within GNUTLS code. When GNUTLS calls into the push/pull functions
we can release the lock so the OS level I/O calls can at least
have some parallelism.

The big downside of this is that the actual encryption/decryption
code is fully serialized, which will halve performance of that
cipher operations if two threads are contending.

The workaround is not enabled by default, since most use of GNUTLS
in QEMU does not tickle the problem, only non-multifd migration
with a return path open is affected. Fortunately the migration
code also won't trigger the halving of performance, since only
the outbound channel diretion needs to sustain high data rates,
the inbound direction is low volume.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Fabiano Rosas <farosas@suse.de>
Link: https://lore.kernel.org/qemu-devel/20250718150514.2635338-2-berrange@redhat.com
[add stub for qcrypto_tls_session_require_thread_safety; fix unused var]
Signed-off-by: Fabiano Rosas <farosas@suse.de>

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index 6d8f8df62323bbf71fb37089bd7b69304842a9fd..baef878fa0cc011cbb6514a71c2a5668955f6fbc 100644 (file)
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -19,6 +19,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/thread.h"
 #include "crypto/tlssession.h"
 #include "crypto/tlscredsanon.h"
 #include "crypto/tlscredspsk.h"
@@ -51,6 +52,14 @@ struct QCryptoTLSSession {
      */
     Error *rerr;
     Error *werr;
+
+    /*
+     * Used to protect against broken GNUTLS thread safety
+     * https://gitlab.com/gnutls/gnutls/-/issues/1717
+     */
+    bool requireThreadSafety;
+    bool lockEnabled;
+    QemuMutex lock;
 };
 
 
@@ -69,6 +78,7 @@ qcrypto_tls_session_free(QCryptoTLSSession *session)
     g_free(session->peername);
     g_free(session->authzid);
     object_unref(OBJECT(session->creds));
+    qemu_mutex_destroy(&session->lock);
     g_free(session);
 }
 
@@ -84,10 +94,19 @@ qcrypto_tls_session_push(void *opaque, const void *buf, size_t len)
         return -1;
     };
 
+    if (session->lockEnabled) {
+        qemu_mutex_unlock(&session->lock);
+    }
+
     error_free(session->werr);
     session->werr = NULL;
 
     ret = session->writeFunc(buf, len, session->opaque, &session->werr);
+
+    if (session->lockEnabled) {
+        qemu_mutex_lock(&session->lock);
+    }
+
     if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
         errno = EAGAIN;
         return -1;
@@ -114,7 +133,16 @@ qcrypto_tls_session_pull(void *opaque, void *buf, size_t len)
     error_free(session->rerr);
     session->rerr = NULL;
 
+    if (session->lockEnabled) {
+        qemu_mutex_unlock(&session->lock);
+    }
+
     ret = session->readFunc(buf, len, session->opaque, &session->rerr);
+
+    if (session->lockEnabled) {
+        qemu_mutex_lock(&session->lock);
+    }
+
     if (ret == QCRYPTO_TLS_SESSION_ERR_BLOCK) {
         errno = EAGAIN;
         return -1;
@@ -153,6 +181,8 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     session->creds = creds;
     object_ref(OBJECT(creds));
 
+    qemu_mutex_init(&session->lock);
+
     if (creds->endpoint != endpoint) {
         error_setg(errp, "Credentials endpoint doesn't match session");
         goto error;
@@ -289,6 +319,11 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds,
     return NULL;
 }
 
+void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess)
+{
+    sess->requireThreadSafety = true;
+}
+
 static int
 qcrypto_tls_session_check_certificate(QCryptoTLSSession *session,
                                       Error **errp)
@@ -480,7 +515,17 @@ qcrypto_tls_session_write(QCryptoTLSSession *session,
                           size_t len,
                           Error **errp)
 {
-    ssize_t ret = gnutls_record_send(session->handle, buf, len);
+    ssize_t ret;
+
+    if (session->lockEnabled) {
+        qemu_mutex_lock(&session->lock);
+    }
+
+    ret = gnutls_record_send(session->handle, buf, len);
+
+    if (session->lockEnabled) {
+        qemu_mutex_unlock(&session->lock);
+    }
 
     if (ret < 0) {
         if (ret == GNUTLS_E_AGAIN) {
@@ -509,7 +554,17 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
                          bool gracefulTermination,
                          Error **errp)
 {
-    ssize_t ret = gnutls_record_recv(session->handle, buf, len);
+    ssize_t ret;
+
+    if (session->lockEnabled) {
+        qemu_mutex_lock(&session->lock);
+    }
+
+    ret = gnutls_record_recv(session->handle, buf, len);
+
+    if (session->lockEnabled) {
+        qemu_mutex_unlock(&session->lock);
+    }
 
     if (ret < 0) {
         if (ret == GNUTLS_E_AGAIN) {
@@ -545,8 +600,29 @@ int
 qcrypto_tls_session_handshake(QCryptoTLSSession *session,
                               Error **errp)
 {
-    int ret = gnutls_handshake(session->handle);
+    int ret;
+    ret = gnutls_handshake(session->handle);
+
     if (!ret) {
+#ifdef CONFIG_GNUTLS_BUG1717_WORKAROUND
+        gnutls_cipher_algorithm_t cipher =
+            gnutls_cipher_get(session->handle);
+
+        /*
+         * Any use of rekeying in TLS 1.3 is unsafe for
+         * a gnutls with bug 1717, however, we know that
+         * QEMU won't initiate manual rekeying. Thus we
+         * only have to protect against automatic rekeying
+         * which doesn't trigger with CHACHA20
+         */
+        if (session->requireThreadSafety &&
+            gnutls_protocol_get_version(session->handle) ==
+            GNUTLS_TLS1_3 &&
+            cipher != GNUTLS_CIPHER_CHACHA20_POLY1305) {
+            session->lockEnabled = true;
+        }
+#endif
+
         session->handshakeComplete = true;
         return QCRYPTO_TLS_HANDSHAKE_COMPLETE;
     }
@@ -584,8 +660,15 @@ qcrypto_tls_session_bye(QCryptoTLSSession *session, Error **errp)
         return 0;
     }
 
+    if (session->lockEnabled) {
+        qemu_mutex_lock(&session->lock);
+    }
     ret = gnutls_bye(session->handle, GNUTLS_SHUT_WR);
 
+    if (session->lockEnabled) {
+        qemu_mutex_unlock(&session->lock);
+    }
+
     if (!ret) {
         return QCRYPTO_TLS_BYE_COMPLETE;
     }
@@ -651,6 +734,9 @@ qcrypto_tls_session_new(QCryptoTLSCreds *creds G_GNUC_UNUSED,
     return NULL;
 }
 
+void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess)
+{
+}
 
 void
 qcrypto_tls_session_free(QCryptoTLSSession *sess G_GNUC_UNUSED)

diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index d77ae0d4237090b2923bd3729e5522016f64b6d7..2f62ce2d67a3fdc5f7ab56b9a147703c38e834ec 100644 (file)
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -165,6 +165,20 @@ void qcrypto_tls_session_free(QCryptoTLSSession *sess);
 
 G_DEFINE_AUTOPTR_CLEANUP_FUNC(QCryptoTLSSession, qcrypto_tls_session_free)
 
+/**
+ * qcrypto_tls_session_require_thread_safety:
+ * @sess: the TLS session object
+ *
+ * Mark that this TLS session will require thread safety
+ * for concurrent I/O in both directions. This must be
+ * called before the handshake is performed.
+ *
+ * This will activate a workaround for GNUTLS thread
+ * safety issues, where appropriate for the negotiated
+ * TLS session parameters.
+ */
+void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess);
+
 /**
  * qcrypto_tls_session_check_credentials:
  * @sess: the TLS session object

diff --git a/meson.build b/meson.build
index c2bc3eeedce9133b8d42e875030aa4d59eeb501e..e53cd5b413847f33c972540e1f67a092164a200d 100644 (file)
--- a/meson.build
+++ b/meson.build
@@ -1809,6 +1809,7 @@ endif
 
 gnutls = not_found
 gnutls_crypto = not_found
+gnutls_bug1717_workaround = false
 if get_option('gnutls').enabled() or (get_option('gnutls').auto() and have_system)
   # For general TLS support our min gnutls matches
   # that implied by our platform support matrix
@@ -1834,6 +1835,12 @@ if get_option('gnutls').enabled() or (get_option('gnutls').auto() and have_syste
                         method: 'pkg-config',
                         required: get_option('gnutls'))
   endif
+
+  if gnutls.found() and not get_option('gnutls-bug1717-workaround').disabled()
+    # XXX: when bug 1717 is resolved, add logic to probe for
+    # the GNUTLS fixed version number to handle the 'auto' case
+    gnutls_bug1717_workaround = true
+  endif
 endif
 
 # We prefer use of gnutls for crypto, unless the options
@@ -2585,6 +2592,7 @@ config_host_data.set('CONFIG_KEYUTILS', keyutils.found())
 config_host_data.set('CONFIG_GETTID', has_gettid)
 config_host_data.set('CONFIG_GNUTLS', gnutls.found())
 config_host_data.set('CONFIG_GNUTLS_CRYPTO', gnutls_crypto.found())
+config_host_data.set('CONFIG_GNUTLS_BUG1717_WORKAROUND', gnutls_bug1717_workaround)
 config_host_data.set('CONFIG_TASN1', tasn1.found())
 config_host_data.set('CONFIG_GCRYPT', gcrypt.found())
 config_host_data.set('CONFIG_NETTLE', nettle.found())
@@ -4869,6 +4877,7 @@ summary_info += {'TLS priority':      get_option('tls_priority')}
 summary_info += {'GNUTLS support':    gnutls}
 if gnutls.found()
   summary_info += {'  GNUTLS crypto':   gnutls_crypto.found()}
+  summary_info += {'  GNUTLS bug 1717 workaround': gnutls_bug1717_workaround }
 endif
 summary_info += {'libgcrypt':         gcrypt}
 summary_info += {'nettle':            nettle}

diff --git a/meson_options.txt b/meson_options.txt
index fff1521e580de20ee277aec2a09f206267c13da9..dd335307505fb52c5be469aa4fab8883c93ec5c9 100644 (file)
--- a/meson_options.txt
+++ b/meson_options.txt
@@ -174,6 +174,8 @@ option('libcbor', type : 'feature', value : 'auto',
        description: 'libcbor support')
 option('gnutls', type : 'feature', value : 'auto',
        description: 'GNUTLS cryptography support')
+option('gnutls-bug1717-workaround', type: 'feature', value : 'auto',
+       description: 'GNUTLS workaround for https://gitlab.com/gnutls/gnutls/-/issues/1717')
 option('nettle', type : 'feature', value : 'auto',
        description: 'nettle cryptography support')
 option('gcrypt', type : 'feature', value : 'auto',

diff --git a/scripts/meson-buildoptions.sh b/scripts/meson-buildoptions.sh
index 0ebe6bc52a6b9f7c605af8e71828cfe65b7265e9..d559e260ed17e50a423aa25b27a86777489565d7 100644 (file)
--- a/scripts/meson-buildoptions.sh
+++ b/scripts/meson-buildoptions.sh
@@ -123,6 +123,9 @@ meson_options_help() {
   printf "%s\n" '  gio             use libgio for D-Bus support'
   printf "%s\n" '  glusterfs       Glusterfs block device driver'
   printf "%s\n" '  gnutls          GNUTLS cryptography support'
+  printf "%s\n" '  gnutls-bug1717-workaround'
+  printf "%s\n" '                  GNUTLS workaround for'
+  printf "%s\n" '                  https://gitlab.com/gnutls/gnutls/-/issues/1717'
   printf "%s\n" '  gtk             GTK+ user interface'
   printf "%s\n" '  gtk-clipboard   clipboard support for the gtk UI (EXPERIMENTAL, MAY HANG)'
   printf "%s\n" '  guest-agent     Build QEMU Guest Agent'
@@ -331,6 +334,8 @@ _meson_option_parse() {
     --disable-glusterfs) printf "%s" -Dglusterfs=disabled ;;
     --enable-gnutls) printf "%s" -Dgnutls=enabled ;;
     --disable-gnutls) printf "%s" -Dgnutls=disabled ;;
+    --enable-gnutls-bug1717-workaround) printf "%s" -Dgnutls-bug1717-workaround=enabled ;;
+    --disable-gnutls-bug1717-workaround) printf "%s" -Dgnutls-bug1717-workaround=disabled ;;
     --enable-gtk) printf "%s" -Dgtk=enabled ;;
     --disable-gtk) printf "%s" -Dgtk=disabled ;;
     --enable-gtk-clipboard) printf "%s" -Dgtk_clipboard=enabled ;;