Re: [PATCH 1/4] crypto: implement workaround for GNUTLS thread safety problems

[Fabiano Rosas <farosas@suse.de>]

Daniel P. Berrangé <berrange@redhat.com> writes:

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

Hi, the CI caught a couple of issues. I'll fixup on the branch, no need
to resend.

...

> +void qcrypto_tls_session_require_thread_safety(QCryptoTLSSession *sess)
> +{
> +    sess->requireThreadSafety = true;
> +}

Needs a stub.

...

> @@ -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) {
> +        gnutls_cipher_algorithm_t cipher =
> +            gnutls_cipher_get(session->handle);
> +

Unused var when the config is not enabled.

> +#ifdef CONFIG_GNUTLS_BUG1717_WORKAROUND
> +        /*
> +         * 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;
>      }