| Message ID | 20240229104339.42574-1-thuth@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | chardev/char-socket: Fix TLS io channels sending too much data to the backend | expand |
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 >
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 >
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)) {
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> --- 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(-)