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

[Thomas Huth <thuth@redhat.com>]

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 *