On Fri, Jul 07, 2023 at 01:30:16PM +0400, Marc-André Lureau wrote: > On Wed, Jul 5, 2023 at 10:20 PM Daniel P. Berrangé <berra...@redhat.com> > wrote: > > > The TLS handshake make take some time to complete, during which time an > > I/O watch might be registered with the main loop. If the owner of the > > I/O channel invokes qio_channel_close() while the handshake is waiting > > to continue the I/O watch must be removed. Failing to remove it will > > later trigger the completion callback which the owner is not expecting > > to receive. In the case of the VNC server, this results in a SEGV as > > vnc_disconnect_start() tries to shutdown a client connection that is > > already gone / NULL. > > > > CVE-2023-3354 > > Reported-by: jiangyegen <jiangye...@huawei.com> > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com> > > > > > > > --- > > include/io/channel-tls.h | 1 + > > io/channel-tls.c | 18 ++++++++++++------ > > 2 files changed, 13 insertions(+), 6 deletions(-) > > > > diff --git a/include/io/channel-tls.h b/include/io/channel-tls.h > > index 5672479e9e..26c67f17e2 100644 > > --- a/include/io/channel-tls.h > > +++ b/include/io/channel-tls.h > > @@ -48,6 +48,7 @@ struct QIOChannelTLS { > > QIOChannel *master; > > QCryptoTLSSession *session; > > QIOChannelShutdown shutdown; > > + guint hs_ioc_tag; > > }; > > > > /** > > diff --git a/io/channel-tls.c b/io/channel-tls.c > > index 9805dd0a3f..e327e6a5c2 100644 > > --- a/io/channel-tls.c > > +++ b/io/channel-tls.c > > @@ -198,12 +198,13 @@ static void > > qio_channel_tls_handshake_task(QIOChannelTLS *ioc, > > } > > > > trace_qio_channel_tls_handshake_pending(ioc, status); > > - qio_channel_add_watch_full(ioc->master, > > - condition, > > - qio_channel_tls_handshake_io, > > - data, > > - NULL, > > - context); > > + ioc->hs_ioc_tag = > > + qio_channel_add_watch_full(ioc->master, > > + condition, > > + qio_channel_tls_handshake_io, > > + data, > > + NULL, > > + context); > > } > > } > > > > @@ -218,6 +219,7 @@ static gboolean > > qio_channel_tls_handshake_io(QIOChannel *ioc, > > QIOChannelTLS *tioc = QIO_CHANNEL_TLS( > > qio_task_get_source(task)); > > > > + tioc->hs_ioc_tag = 0; > > g_free(data); > > qio_channel_tls_handshake_task(tioc, task, context); > > > > @@ -378,6 +380,10 @@ static int qio_channel_tls_close(QIOChannel *ioc, > > { > > QIOChannelTLS *tioc = QIO_CHANNEL_TLS(ioc); > > > > + if (tioc->hs_ioc_tag) { > > + g_source_remove(tioc->hs_ioc_tag); > > > > set it to 0 ? > or > g_clear_handle_id(&tios->hs_ioc_tag, g_source_remove);
Yes, close can be called mutliple times, so we must set to zero. > > + } > > + > > return qio_channel_close(tioc->master, errp); > > } > > > > -- > > 2.41.0 > > > > > > > > -- > Marc-André Lureau With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|