[PATCH 0/2] TLS: fix read stall with large buffers

[PATCH 0/2] TLS: fix read stall with large buffers

[antoine.damhet]

From: Antoine Damhet <antoine.damhet@shadow.tech>

At least with the TCP backend the tls implementation can stall because
the current notification mechanism is based on the readyness status of
the underlying file descriptor but gnutls can read all the data
available while the consumer has only handled part (eg: the TCP
implementation is capped to 4096 bytes per read).

We encountered the bug on the real world using encrypted
USB-redirection but we could reproduce it with virtio-serial.

On the host side:

```
$ mkdir /tmp/tls-test
$ echo 'test:8e6da54b954357236ec2eed1df0dc3542163dd03bf9c94f2d90781a3a3e443c9' > /tmp/tls-test/keys.psk
$ qemu-system-x86_64 -m 4G -enable-kvm -device virtio-serial \
	-object tls-creds-psk,id=tls0,endpoint=server,dir=/tmp/tls-test \
	-chardev socket,server=on,wait=off,id=serial-sock,port=4242,host=127.0.0.1,tls-creds=tls0 \
	-device virtserialport,chardev=serial-sock,name=test.serial \
	-cdrom archlinux-2022.11.01-x86_64.iso
# The sleep is to keep the socket open during the test
$ python -c 'print(8192 * "a"); import time; time.sleep(600);' | \
	openssl s_client -connect 127.0.0.1:4242 \
	-psk 8e6da54b954357236ec2eed1df0dc3542163dd03bf9c94f2d90781a3a3e443c9 \
	-psk_identity test
```

On the guest side:

```
# The socket is forced open, we stop cat manually
$ cat /dev/virtio-ports/test.serial > test.data
^C
# only part of the data was readable
$ wc -c test.data
4096
```

Antoine Damhet (2):
  crypto: TLS: introduce `check_pending`
  io/channel-tls: fix handling of bigger read buffers

 crypto/tlssession.c         | 14 ++++++++
 include/crypto/tlssession.h | 11 +++++++
 io/channel-tls.c            | 66 ++++++++++++++++++++++++++++++++++++-
 3 files changed, 90 insertions(+), 1 deletion(-)

-- 
2.38.1

[PATCH 1/2] crypto: TLS: introduce `check_pending`

[antoine.damhet]

From: Antoine Damhet <antoine.damhet@shadow.tech>

The new `qcrypto_tls_session_check_pending` function allows the caller
to know if data have already been consumed from the backend and is
already available.

Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
---
 crypto/tlssession.c         | 14 ++++++++++++++
 include/crypto/tlssession.h | 11 +++++++++++
 2 files changed, 25 insertions(+)

diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index b302d835d2..1e98f44e0d 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -493,6 +493,13 @@ qcrypto_tls_session_read(QCryptoTLSSession *session,
 }
 
 
+size_t
+qcrypto_tls_session_check_pending(QCryptoTLSSession *session)
+{
+    return gnutls_record_check_pending(session->handle);
+}
+
+
 int
 qcrypto_tls_session_handshake(QCryptoTLSSession *session,
                               Error **errp)
@@ -615,6 +622,13 @@ qcrypto_tls_session_read(QCryptoTLSSession *sess,
 }
 
 
+size_t
+qcrypto_tls_session_check_pending(QCryptoTLSSession *session)
+{
+    return 0;
+}
+
+
 int
 qcrypto_tls_session_handshake(QCryptoTLSSession *sess,
                               Error **errp)
diff --git a/include/crypto/tlssession.h b/include/crypto/tlssession.h
index 15b9cef086..571049bd0e 100644
--- a/include/crypto/tlssession.h
+++ b/include/crypto/tlssession.h
@@ -248,6 +248,17 @@ ssize_t qcrypto_tls_session_read(QCryptoTLSSession *sess,
                                  char *buf,
                                  size_t len);
 
+/**
+ * qcrypto_tls_session_check_pending:
+ * @sess: the TLS session object
+ *
+ * Check if there are unread data in the TLS buffers that have
+ * already been read from the underlying data source.
+ *
+ * Returns: the number of bytes available or zero
+ */
+size_t qcrypto_tls_session_check_pending(QCryptoTLSSession *sess);
+
 /**
  * qcrypto_tls_session_handshake:
  * @sess: the TLS session object
-- 
2.38.1

Re: [PATCH 1/2] crypto: TLS: introduce `check_pending`

[Daniel P. Berrangé]

On Tue, Nov 15, 2022 at 03:23:28PM +0100, antoine.damhet@shadow.tech wrote:
> From: Antoine Damhet <antoine.damhet@shadow.tech>
> 
> The new `qcrypto_tls_session_check_pending` function allows the caller
> to know if data have already been consumed from the backend and is
> already available.
> 
> Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> ---
>  crypto/tlssession.c         | 14 ++++++++++++++
>  include/crypto/tlssession.h | 11 +++++++++++
>  2 files changed, 25 insertions(+)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH 2/2] io/channel-tls: fix handling of bigger read buffers

[antoine.damhet]

From: Antoine Damhet <antoine.damhet@shadow.tech>

Since the TLS backend can read more data from the underlying QIOChannel
we introduce a minimal child GSource to notify if we still have more
data available to be read.

Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
Signed-off-by: Charles Frey <charles.frey@shadow.tech>
---
 io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 65 insertions(+), 1 deletion(-)

diff --git a/io/channel-tls.c b/io/channel-tls.c
index 4ce890a538..4f2b8828f9 100644
--- a/io/channel-tls.c
+++ b/io/channel-tls.c
@@ -388,12 +388,76 @@ static void qio_channel_tls_set_aio_fd_handler(QIOChannel *ioc,
     qio_channel_set_aio_fd_handler(tioc->master, ctx, io_read, io_write, opaque);
 }
 
+typedef struct QIOChannelTLSSource QIOChannelTLSSource;
+struct QIOChannelTLSSource {
+    GSource parent;
+    QIOChannelTLS *tioc;
+};
+
+static gboolean
+qio_channel_tls_source_check(GSource *source)
+{
+    QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source;
+
+    return qcrypto_tls_session_check_pending(tsource->tioc->session) > 0;
+}
+
+static gboolean
+qio_channel_tls_source_prepare(GSource *source, gint *timeout)
+{
+    *timeout = -1;
+    return qio_channel_tls_source_check(source);
+}
+
+static gboolean
+qio_channel_tls_source_dispatch(GSource *source, GSourceFunc callback,
+                                gpointer user_data)
+{
+    return G_SOURCE_CONTINUE;
+}
+
+static void
+qio_channel_tls_source_finalize(GSource *source)
+{
+    QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source;
+
+    object_unref(OBJECT(tsource->tioc));
+}
+
+static GSourceFuncs qio_channel_tls_source_funcs = {
+    qio_channel_tls_source_prepare,
+    qio_channel_tls_source_check,
+    qio_channel_tls_source_dispatch,
+    qio_channel_tls_source_finalize
+};
+
+static void
+qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
+{
+    GSource *child;
+    QIOChannelTLSSource *tlssource;
+
+    child = g_source_new(&qio_channel_tls_source_funcs,
+                          sizeof(QIOChannelTLSSource));
+    tlssource = (QIOChannelTLSSource *)child;
+
+    tlssource->tioc = tioc;
+    object_ref(OBJECT(tioc));
+
+    g_source_add_child_source(source, child);
+}
+
 static GSource *qio_channel_tls_create_watch(QIOChannel *ioc,
                                              GIOCondition condition)
 {
     QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
+    GSource *source = qio_channel_create_watch(tioc->master, condition);
+
+    if (condition & G_IO_IN) {
+        qio_channel_tls_read_watch(tioc, source);
+    }
 
-    return qio_channel_create_watch(tioc->master, condition);
+    return source;
 }
 
 QCryptoTLSSession *
-- 
2.38.1

Re: [PATCH 2/2] io/channel-tls: fix handling of bigger read buffers

[Daniel P. Berrangé]

On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote:
> From: Antoine Damhet <antoine.damhet@shadow.tech>
> 
> Since the TLS backend can read more data from the underlying QIOChannel
> we introduce a minimal child GSource to notify if we still have more
> data available to be read.
> 
> Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> Signed-off-by: Charles Frey <charles.frey@shadow.tech>
> ---
>  io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 65 insertions(+), 1 deletion(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] io/channel-tls: fix handling of bigger read buffers

[Antoine Damhet]

[-- Attachment #1: Type: text/plain, Size: 1171 bytes --]

On Wed, Nov 16, 2022 at 10:52:20AM +0000, Daniel P. Berrangé wrote:
> On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote:
> > From: Antoine Damhet <antoine.damhet@shadow.tech>
> > 
> > Since the TLS backend can read more data from the underlying QIOChannel
> > we introduce a minimal child GSource to notify if we still have more
> > data available to be read.
> > 
> > Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> > Signed-off-by: Charles Frey <charles.frey@shadow.tech>
> > ---
> >  io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 file changed, 65 insertions(+), 1 deletion(-)
> 
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

Thanks,

Now that the 7.2.0 is released, can we hope to get this queued ? If not
what should I do ?

Best regards,

-- 
Antoine 'xdbob' Damhet

> 
> 
> With regards,
> Daniel
> -- 
> |: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org         -o-            https://fstop138.berrange.com :|
> |: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 2/2] io/channel-tls: fix handling of bigger read buffers

[Daniel P. Berrangé]

On Fri, Dec 16, 2022 at 11:38:03AM +0100, Antoine Damhet wrote:
> On Wed, Nov 16, 2022 at 10:52:20AM +0000, Daniel P. Berrangé wrote:
> > On Tue, Nov 15, 2022 at 03:23:29PM +0100, antoine.damhet@shadow.tech wrote:
> > > From: Antoine Damhet <antoine.damhet@shadow.tech>
> > > 
> > > Since the TLS backend can read more data from the underlying QIOChannel
> > > we introduce a minimal child GSource to notify if we still have more
> > > data available to be read.
> > > 
> > > Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> > > Signed-off-by: Charles Frey <charles.frey@shadow.tech>
> > > ---
> > >  io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 file changed, 65 insertions(+), 1 deletion(-)
> > 
> > Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Thanks,
> 
> Now that the 7.2.0 is released, can we hope to get this queued ? If not
> what should I do ?

Yes, I have this queued already for my next pull req.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [PATCH 2/2] io/channel-tls: fix handling of bigger read buffers

[Thomas Huth]

On 15/11/2022 15.23, antoine.damhet@shadow.tech wrote:
> From: Antoine Damhet <antoine.damhet@shadow.tech>
> 
> Since the TLS backend can read more data from the underlying QIOChannel
> we introduce a minimal child GSource to notify if we still have more
> data available to be read.
> 
> Signed-off-by: Antoine Damhet <antoine.damhet@shadow.tech>
> Signed-off-by: Charles Frey <charles.frey@shadow.tech>
> ---
>   io/channel-tls.c | 66 +++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 65 insertions(+), 1 deletion(-)

  Hi Antoine, Charles and Daniel!

This patch can cause a crash with character devices in QEMU that check for a 
valid amount of data that should be written to it, like e.g. the sclpconsole 
of qemu-system-s390x. And I guess other character devices are affected, too, 
in the sense that they are dropping characters silently.

The problem can be reproduced like this:

1) In one terminal, do this:

mkdir qemu-pki
cd qemu-pki
openssl genrsa 2048 > ca-key.pem
openssl req -new -x509 -nodes -days 365000 -key ca-key.pem -out ca-cert.pem
# enter some dummy value for the cert
openssl genrsa 2048 > server-key.pem
openssl req -new -x509 -nodes -days 365000 -key server-key.pem \
  -out server-cert.pem
# enter some other dummy values for the cert

gnutls-serv --echo --x509cafile ca-cert.pem --x509keyfile server-key.pem \
             --x509certfile server-cert.pem -p 8338

2) In another terminal, do this:

wget 
https://download.fedoraproject.org/pub/fedora-secondary/releases/39/Cloud/s390x/images/Fedora-Cloud-Base-39-1.5.s390x.qcow2

qemu-system-s390x -nographic -nodefaults \
   -hda Fedora-Cloud-Base-39-1.5.s390x.qcow2 \
   -object 
tls-creds-x509,id=tls0,endpoint=client,verify-peer=false,dir=$PWD/qemu-pki \
   -chardev socket,id=tls_chardev,host=localhost,port=8338,tls-creds=tls0 \
   -device sclpconsole,chardev=tls_chardev,id=tls_serial

QEMU then aborts after a second or two with:

qemu-system-s390x: ../../devel/qemu/hw/char/sclpconsole.c:73: chr_read: 
Assertion `size <= SIZE_BUFFER_VT220 - scon->iov_data_len' failed.
Aborted (core dumped)

It looks like the second read does not trigger the chr_can_read() function 
to be called before the read, which should normally always be done before 
sending bytes to a character device to see how much it can handle. ... do 
you maybe have any ideas how to fix this?

  Thanks,
   Thomas


> diff --git a/io/channel-tls.c b/io/channel-tls.c
> index 4ce890a538..4f2b8828f9 100644
> --- a/io/channel-tls.c
> +++ b/io/channel-tls.c
> @@ -388,12 +388,76 @@ static void qio_channel_tls_set_aio_fd_handler(QIOChannel *ioc,
>       qio_channel_set_aio_fd_handler(tioc->master, ctx, io_read, io_write, opaque);
>   }
>   
> +typedef struct QIOChannelTLSSource QIOChannelTLSSource;
> +struct QIOChannelTLSSource {
> +    GSource parent;
> +    QIOChannelTLS *tioc;
> +};
> +
> +static gboolean
> +qio_channel_tls_source_check(GSource *source)
> +{
> +    QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source;
> +
> +    return qcrypto_tls_session_check_pending(tsource->tioc->session) > 0;
> +}
> +
> +static gboolean
> +qio_channel_tls_source_prepare(GSource *source, gint *timeout)
> +{
> +    *timeout = -1;
> +    return qio_channel_tls_source_check(source);
> +}
> +
> +static gboolean
> +qio_channel_tls_source_dispatch(GSource *source, GSourceFunc callback,
> +                                gpointer user_data)
> +{
> +    return G_SOURCE_CONTINUE;
> +}
> +
> +static void
> +qio_channel_tls_source_finalize(GSource *source)
> +{
> +    QIOChannelTLSSource *tsource = (QIOChannelTLSSource *)source;
> +
> +    object_unref(OBJECT(tsource->tioc));
> +}
> +
> +static GSourceFuncs qio_channel_tls_source_funcs = {
> +    qio_channel_tls_source_prepare,
> +    qio_channel_tls_source_check,
> +    qio_channel_tls_source_dispatch,
> +    qio_channel_tls_source_finalize
> +};
> +
> +static void
> +qio_channel_tls_read_watch(QIOChannelTLS *tioc, GSource *source)
> +{
> +    GSource *child;
> +    QIOChannelTLSSource *tlssource;
> +
> +    child = g_source_new(&qio_channel_tls_source_funcs,
> +                          sizeof(QIOChannelTLSSource));
> +    tlssource = (QIOChannelTLSSource *)child;
> +
> +    tlssource->tioc = tioc;
> +    object_ref(OBJECT(tioc));
> +
> +    g_source_add_child_source(source, child);
> +}
> +
>   static GSource *qio_channel_tls_create_watch(QIOChannel *ioc,
>                                                GIOCondition condition)
>   {
>       QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc);
> +    GSource *source = qio_channel_create_watch(tioc->master, condition);
> +
> +    if (condition & G_IO_IN) {
> +        qio_channel_tls_read_watch(tioc, source);
> +    }
>   
> -    return qio_channel_create_watch(tioc->master, condition);
> +    return source;
>   }
>   
>   QCryptoTLSSession *