TLS: fix read stall with large buffers

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

Posted by antoine.damhet@shadow.tech 3 years, 2 months ago

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

Posted by Thomas Huth 1 year, 11 months ago

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 *

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

Posted by Daniel P. Berrangé 3 years, 2 months ago

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

Posted by Antoine Damhet 3 years, 1 month ago

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

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

Posted by Daniel P. Berrangé 3 years, 1 month ago

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