Re: [PATCH] chardev/char-socket: Fix TLS io channels sending too much data to the backend - Marc-André Lureau

From: "Marc-André Lureau" <marcandre.lureau@redhat.com>
To: Thomas Huth <thuth@redhat.com>
Cc: qemu-devel@nongnu.org,
	"Daniel P . Berrangé" <berrange@redhat.com>,
	"Paolo Bonzini" <pbonzini@redhat.com>,
	"Antoine Damhet" <antoine.damhet@shadow.tech>,
	"Charles Frey" <charles.frey@shadow.tech>
Subject: Re: [PATCH] chardev/char-socket: Fix TLS io channels sending too much data to the backend
Date: Thu, 29 Feb 2024 14:51:49 +0400	[thread overview]
Message-ID: <CAMxuvaz97MFUOPVo_8YdT9RSgM94E7xWzVAT0sxwx09o4QGwTQ@mail.gmail.com> (raw)
In-Reply-To: <20240229104339.42574-1-thuth@redhat.com>

On Thu, Feb 29, 2024 at 2:43 PM Thomas Huth <thuth@redhat.com> wrote:
>
> Commit ffda5db65a ("io/channel-tls: fix handling of bigger read buffers")
> changed the behavior of the TLS io channels to schedule a second reading
> attempt if there is still incoming data pending. This caused a regression
> with backends like the sclpconsole that check in their read function that
> the sender does not try to write more bytes to it than the device can
> currently handle.
>
> 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: ../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 second read, which should normally always be done
> before sending bytes to a character device to see how much it can handle,
> so the s->max_size in tcp_chr_read() still contains the old value from the
> previous read. Let's make sure that we use the up-to-date value by calling
> tcp_chr_read_poll() again here.
>
> Fixes: ffda5db65a ("io/channel-tls: fix handling of bigger read buffers")
> Buglink: https://issues.redhat.com/browse/RHEL-24614
> Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>


> Signed-off-by: Thomas Huth <thuth@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
>  Sorry if you've got this mail twice - I forgot to CC: qemu-devel when
>  I sent it out the first time ... *facepalm*
>
>  chardev/char-socket.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 67e3334423..8a0406cc1e 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -496,9 +496,9 @@ static gboolean tcp_chr_read(QIOChannel *chan, GIOCondition cond, void *opaque)
>          s->max_size <= 0) {
>          return TRUE;
>      }
> -    len = sizeof(buf);
> -    if (len > s->max_size) {
> -        len = s->max_size;
> +    len = tcp_chr_read_poll(opaque);
> +    if (len > sizeof(buf)) {
> +        len = sizeof(buf);
>      }
>      size = tcp_chr_recv(chr, (void *)buf, len);
>      if (size == 0 || (size == -1 && errno != EAGAIN)) {
> --
> 2.44.0
>