vnc: fix use-after-free

[Qemu-devel] [PATCH] vnc: fix use-after-free

Posted by Gerd Hoffmann 6 years ago

When vnc_client_read() return value is -1
vs is not valid any more.

Fixes: d49b87f0d1e0520443a990fc610d0f02bc63c556
Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index e164eb798c..5526e54f48 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -1539,13 +1539,14 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
     VncState *vs = opaque;
     if (condition & G_IO_IN) {
         if (vnc_client_read(vs) < 0) {
-            goto end;
+            /* vs is free()ed here */
+            return TRUE;
         }
     }
     if (condition & G_IO_OUT) {
         vnc_client_write(vs);
     }
-end:
+
     if (vs->disconnecting) {
         if (vs->ioc_tag != 0) {
             g_source_remove(vs->ioc_tag);
-- 
2.9.3

Re: [Qemu-devel] [PATCH] vnc: fix use-after-free

Posted by Marc-André Lureau 6 years ago

Hi

On Fri, Apr 20, 2018 at 10:48 AM, Gerd Hoffmann <kraxel@redhat.com> wrote:
> When vnc_client_read() return value is -1
> vs is not valid any more.
>
> Fixes: d49b87f0d1e0520443a990fc610d0f02bc63c556
> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

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


> ---
>  ui/vnc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e164eb798c..5526e54f48 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1539,13 +1539,14 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
>      VncState *vs = opaque;
>      if (condition & G_IO_IN) {
>          if (vnc_client_read(vs) < 0) {
> -            goto end;
> +            /* vs is free()ed here */
> +            return TRUE;
>          }
>      }
>      if (condition & G_IO_OUT) {
>          vnc_client_write(vs);
>      }
> -end:
> +
>      if (vs->disconnecting) {
>          if (vs->ioc_tag != 0) {
>              g_source_remove(vs->ioc_tag);
> --
> 2.9.3
>
>



-- 
Marc-André Lureau

Re: [Qemu-devel] [PATCH] vnc: fix use-after-free

Posted by Daniel P. Berrangé 6 years ago

On Fri, Apr 20, 2018 at 10:48:19AM +0200, Gerd Hoffmann wrote:
> When vnc_client_read() return value is -1
> vs is not valid any more.
> 
> Fixes: d49b87f0d1e0520443a990fc610d0f02bc63c556
> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)

Reviewed-by: Daniel P. Berrangé <berrange@redhat.com>

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 :|

Re: [Qemu-devel] [PATCH] vnc: fix use-after-free

Posted by Philippe Mathieu-Daudé 6 years ago

Cc'ing qemu-stable

On 04/20/2018 05:48 AM, Gerd Hoffmann wrote:
> When vnc_client_read() return value is -1
> vs is not valid any more.
> 
> Fixes: d49b87f0d1e0520443a990fc610d0f02bc63c556
> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Tested-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

> ---
>  ui/vnc.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/ui/vnc.c b/ui/vnc.c
> index e164eb798c..5526e54f48 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -1539,13 +1539,14 @@ gboolean vnc_client_io(QIOChannel *ioc G_GNUC_UNUSED,
>      VncState *vs = opaque;
>      if (condition & G_IO_IN) {
>          if (vnc_client_read(vs) < 0) {
> -            goto end;
> +            /* vs is free()ed here */
> +            return TRUE;
>          }
>      }
>      if (condition & G_IO_OUT) {
>          vnc_client_write(vs);
>      }
> -end:
> +
>      if (vs->disconnecting) {
>          if (vs->ioc_tag != 0) {
>              g_source_remove(vs->ioc_tag);
>