diff mbox series

[11/12] chardev: disallow TLS/telnet/websocket with tcp_chr_wait_connected

Message ID 20190115145256.9593-12-berrange@redhat.com (mailing list archive)
State New, archived
Headers show
Series chardev: refactoring & many bugfixes related tcp_chr_wait_connected | expand

Commit Message

Daniel P. Berrangé Jan. 15, 2019, 2:52 p.m. UTC
In the previous commit

    commit 1dc8a6695c731abb7461c637b2512c3670d82be4
    Author: Marc-André Lureau <marcandre.lureau@redhat.com>
    Date:   Tue Aug 16 12:33:32 2016 +0400

      char: fix waiting for TLS and telnet connection

the tcp_chr_wait_connected() method was changed to check for a non-NULL
's->ioc' as a sign that there is already a connection present, as
opposed to checking the "connected" flag to supposedly fix handling of
TLS/telnet connections.

The original code would repeatedly call tcp_chr_wait_connected creating
many connections as 'connected' would never become true. The changed
code would still repeatedly call tcp_chr_wait_connected busy waiting
because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
IOW, the code is still broken with TLS/telnet, but in a different way.

Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
will be ready for a TLS/telnet connection. These protocols (and the
websocket protocol) all require the main loop to be running in order
to complete the protocol handshake before emitting CHR_EVENT_OPENED.
The tcp_chr_wait_connected() method is only used during early startup
before a main loop is running, so TLS/telnet/websock connections can
never complete initialization.

Making this work would require changing tcp_chr_wait_connected to run
a main loop. This is quite complex since we must not allow GSource's
that other parts of QEMU have registered to run yet. The current callers
of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
protocols, so the simplest option is to just forbid this combination
completely for now.

Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
 chardev/char-socket.c | 16 ++++++++++++++--
 1 file changed, 14 insertions(+), 2 deletions(-)

Comments

Marc-André Lureau Jan. 15, 2019, 9:54 p.m. UTC | #1
On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
>
> In the previous commit
>
>     commit 1dc8a6695c731abb7461c637b2512c3670d82be4
>     Author: Marc-André Lureau <marcandre.lureau@redhat.com>
>     Date:   Tue Aug 16 12:33:32 2016 +0400
>
>       char: fix waiting for TLS and telnet connection
>
> the tcp_chr_wait_connected() method was changed to check for a non-NULL
> 's->ioc' as a sign that there is already a connection present, as
> opposed to checking the "connected" flag to supposedly fix handling of
> TLS/telnet connections.
>
> The original code would repeatedly call tcp_chr_wait_connected creating
> many connections as 'connected' would never become true. The changed
> code would still repeatedly call tcp_chr_wait_connected busy waiting
> because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
> IOW, the code is still broken with TLS/telnet, but in a different way.
>
> Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
> will be ready for a TLS/telnet connection. These protocols (and the
> websocket protocol) all require the main loop to be running in order
> to complete the protocol handshake before emitting CHR_EVENT_OPENED.
> The tcp_chr_wait_connected() method is only used during early startup
> before a main loop is running, so TLS/telnet/websock connections can
> never complete initialization.
>
> Making this work would require changing tcp_chr_wait_connected to run
> a main loop. This is quite complex since we must not allow GSource's
> that other parts of QEMU have registered to run yet. The current callers
> of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
> protocols, so the simplest option is to just forbid this combination
> completely for now.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>

> ---
>  chardev/char-socket.c | 16 ++++++++++++++--
>  1 file changed, 14 insertions(+), 2 deletions(-)
>
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 91d775e9c5..7e98a95bbd 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
>  static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
>  {
>      SocketChardev *s = SOCKET_CHARDEV(chr);
> -    /* It can't wait on s->connected, since it is set asynchronously
> -     * in TLS and telnet cases, only wait for an accepted socket */
> +    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
> +    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
> +    size_t i;
> +
> +    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
> +    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
> +        if (optset[i]) {
> +            error_setg(errp,
> +                       "'%s' option is incompatible with waiting for "
> +                       "connection during early startup", opts[i]);

"during early startup" ? I think you could also reach this by using
chardev-add & netdev_add.

> +            return -1;
> +        }
> +    }
> +
>      while (!s->ioc) {
>          if (s->is_listen) {
>              tcp_chr_accept_server_sync(chr);
> --
> 2.20.1
>
Daniel P. Berrangé Jan. 16, 2019, 9:37 a.m. UTC | #2
On Wed, Jan 16, 2019 at 01:54:54AM +0400, Marc-André Lureau wrote:
> On Tue, Jan 15, 2019 at 6:54 PM Daniel P. Berrangé <berrange@redhat.com> wrote:
> >
> > In the previous commit
> >
> >     commit 1dc8a6695c731abb7461c637b2512c3670d82be4
> >     Author: Marc-André Lureau <marcandre.lureau@redhat.com>
> >     Date:   Tue Aug 16 12:33:32 2016 +0400
> >
> >       char: fix waiting for TLS and telnet connection
> >
> > the tcp_chr_wait_connected() method was changed to check for a non-NULL
> > 's->ioc' as a sign that there is already a connection present, as
> > opposed to checking the "connected" flag to supposedly fix handling of
> > TLS/telnet connections.
> >
> > The original code would repeatedly call tcp_chr_wait_connected creating
> > many connections as 'connected' would never become true. The changed
> > code would still repeatedly call tcp_chr_wait_connected busy waiting
> > because s->ioc is set but the chardev will never see CHR_EVENT_OPENED.
> > IOW, the code is still broken with TLS/telnet, but in a different way.
> >
> > Checking for a non-NULL 's->ioc' does not mean that a CHR_EVENT_OPENED
> > will be ready for a TLS/telnet connection. These protocols (and the
> > websocket protocol) all require the main loop to be running in order
> > to complete the protocol handshake before emitting CHR_EVENT_OPENED.
> > The tcp_chr_wait_connected() method is only used during early startup
> > before a main loop is running, so TLS/telnet/websock connections can
> > never complete initialization.
> >
> > Making this work would require changing tcp_chr_wait_connected to run
> > a main loop. This is quite complex since we must not allow GSource's
> > that other parts of QEMU have registered to run yet. The current callers
> > of tcp_chr_wait_connected do not require use of the TLS/telnet/websocket
> > protocols, so the simplest option is to just forbid this combination
> > completely for now.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> 
> Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> 
> > ---
> >  chardev/char-socket.c | 16 ++++++++++++++--
> >  1 file changed, 14 insertions(+), 2 deletions(-)
> >
> > diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> > index 91d775e9c5..7e98a95bbd 100644
> > --- a/chardev/char-socket.c
> > +++ b/chardev/char-socket.c
> > @@ -951,8 +951,20 @@ static void tcp_chr_accept_server_sync(Chardev *chr)
> >  static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
> >  {
> >      SocketChardev *s = SOCKET_CHARDEV(chr);
> > -    /* It can't wait on s->connected, since it is set asynchronously
> > -     * in TLS and telnet cases, only wait for an accepted socket */
> > +    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
> > +    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
> > +    size_t i;
> > +
> > +    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
> > +    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
> > +        if (optset[i]) {
> > +            error_setg(errp,
> > +                       "'%s' option is incompatible with waiting for "
> > +                       "connection during early startup", opts[i]);
> 
> "during early startup" ? I think you could also reach this by using
> chardev-add & netdev_add.

Yes, I didn't realize that at first. It also means that patch 12 is in
fact still broken in the hotplug case.


Regards,
Daniel
diff mbox series

Patch

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 91d775e9c5..7e98a95bbd 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -951,8 +951,20 @@  static void tcp_chr_accept_server_sync(Chardev *chr)
 static int tcp_chr_wait_connected(Chardev *chr, Error **errp)
 {
     SocketChardev *s = SOCKET_CHARDEV(chr);
-    /* It can't wait on s->connected, since it is set asynchronously
-     * in TLS and telnet cases, only wait for an accepted socket */
+    const char *opts[] = { "telnet", "tn3270", "websock", "tls-creds" };
+    bool optset[] = { s->is_telnet, s->is_tn3270, s->is_websock, s->tls_creds };
+    size_t i;
+
+    QEMU_BUILD_BUG_ON(G_N_ELEMENTS(opts) != G_N_ELEMENTS(optset));
+    for (i = 0; i < G_N_ELEMENTS(opts); i++) {
+        if (optset[i]) {
+            error_setg(errp,
+                       "'%s' option is incompatible with waiting for "
+                       "connection during early startup", opts[i]);
+            return -1;
+        }
+    }
+
     while (!s->ioc) {
         if (s->is_listen) {
             tcp_chr_accept_server_sync(chr);