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

[Antoine Damhet]

On Thu, Feb 29, 2024 at 11:43:37AM +0100, Thomas Huth 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>

Reviewed-by: Antoine Damhet <antoine.damhet@blade-group.com>
Tested-by: Antoine Damhet <antoine.damhet@blade-group.com>

> Signed-off-by: Thomas Huth <thuth@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
> 

-- 
Antoine 'xdbob' Damhet

signature.asc
Description: PGP signature