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

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

[Gerd Hoffmann]

Spotted by Coverity:

876     static int vnc_update_client_sync(VncState *vs, int has_dirty)
877     {

(1) Event freed_arg:    "vnc_update_client(VncState *, int)" frees "vs".  [details]
Also see events:        [deref_arg]

878         int ret = vnc_update_client(vs, has_dirty);

(2) Event deref_arg:    Calling "vnc_jobs_join(VncState *)" dereferences freed pointer "vs". [details]
Also see events:        [freed_arg]

879         vnc_jobs_join(vs);
880         return ret;
881     }

Remove vnc_update_client_sync wrapper, replace it with an additional
argument to vnc_update_client, so we can so the sync properly in
vnc_update_client (i.e. skip it in case of a client disconnect).

Cc: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
---
 ui/vnc.c | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/ui/vnc.c b/ui/vnc.c
index 5601cc3..b0efb1f 100644
--- a/ui/vnc.c
+++ b/ui/vnc.c
@@ -416,8 +416,7 @@ out_error:
    3) resolutions > 1024
 */
 
-static int vnc_update_client(VncState *vs, int has_dirty);
-static int vnc_update_client_sync(VncState *vs, int has_dirty);
+static int vnc_update_client(VncState *vs, int has_dirty, bool sync);
 static void vnc_disconnect_start(VncState *vs);
 
 static void vnc_colordepth(VncState *vs);
@@ -750,7 +749,7 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
         if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
             vs->force_update = 1;
-            vnc_update_client_sync(vs, 1);
+            vnc_update_client(vs, 1, true);
             /* vs might be free()ed here */
         }
     }
@@ -873,14 +872,7 @@ static int find_and_clear_dirty_height(struct VncState *vs,
     return h;
 }
 
-static int vnc_update_client_sync(VncState *vs, int has_dirty)
-{
-    int ret = vnc_update_client(vs, has_dirty);
-    vnc_jobs_join(vs);
-    return ret;
-}
-
-static int vnc_update_client(VncState *vs, int has_dirty)
+static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
 {
     if (vs->need_update && vs->csock != -1) {
         VncDisplay *vd = vs->vd;
@@ -939,8 +931,11 @@ static int vnc_update_client(VncState *vs, int has_dirty)
         return n;
     }
 
-    if (vs->csock == -1)
+    if (vs->csock == -1) {
         vnc_disconnect_finish(vs);
+    } else if (sync) {
+        vnc_jobs_join(vs);
+    }
 
     return 0;
 }
@@ -2749,7 +2744,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
     vnc_unlock_display(vd);
 
     QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
-        rects += vnc_update_client(vs, has_dirty);
+        rects += vnc_update_client(vs, has_dirty, false);
         /* vs might be free()ed here */
     }
 
-- 
1.8.3.1

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

[Markus Armbruster]

Gerd Hoffmann <kraxel@redhat.com> writes:

> Spotted by Coverity:
>
> 876     static int vnc_update_client_sync(VncState *vs, int has_dirty)
> 877     {
>
> (1) Event freed_arg:    "vnc_update_client(VncState *, int)" frees "vs".  [details]
> Also see events:        [deref_arg]
>
> 878         int ret = vnc_update_client(vs, has_dirty);
>
> (2) Event deref_arg:    Calling "vnc_jobs_join(VncState *)" dereferences freed pointer "vs". [details]
> Also see events:        [freed_arg]
>
> 879         vnc_jobs_join(vs);
> 880         return ret;
> 881     }
>
> Remove vnc_update_client_sync wrapper, replace it with an additional
> argument to vnc_update_client, so we can so the sync properly in
> vnc_update_client (i.e. skip it in case of a client disconnect).
>
> Cc: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Gerd Hoffmann <kraxel@redhat.com>
> ---
>  ui/vnc.c | 21 ++++++++-------------
>  1 file changed, 8 insertions(+), 13 deletions(-)
>
> diff --git a/ui/vnc.c b/ui/vnc.c
> index 5601cc3..b0efb1f 100644
> --- a/ui/vnc.c
> +++ b/ui/vnc.c
> @@ -416,8 +416,7 @@ out_error:
>     3) resolutions > 1024
>  */
>  
> -static int vnc_update_client(VncState *vs, int has_dirty);
> -static int vnc_update_client_sync(VncState *vs, int has_dirty);
> +static int vnc_update_client(VncState *vs, int has_dirty, bool sync);
>  static void vnc_disconnect_start(VncState *vs);
>  
>  static void vnc_colordepth(VncState *vs);
> @@ -750,7 +749,7 @@ static void vnc_dpy_copy(DisplayChangeListener *dcl,
>      QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
>          if (vnc_has_feature(vs, VNC_FEATURE_COPYRECT)) {
>              vs->force_update = 1;
> -            vnc_update_client_sync(vs, 1);
> +            vnc_update_client(vs, 1, true);
>              /* vs might be free()ed here */
>          }
>      }
> @@ -873,14 +872,7 @@ static int find_and_clear_dirty_height(struct VncState *vs,
>      return h;
>  }
>  
> -static int vnc_update_client_sync(VncState *vs, int has_dirty)
> -{
> -    int ret = vnc_update_client(vs, has_dirty);
> -    vnc_jobs_join(vs);

This is the problematic use, and you move it...

> -    return ret;
> -}
> -
> -static int vnc_update_client(VncState *vs, int has_dirty)
> +static int vnc_update_client(VncState *vs, int has_dirty, bool sync)
>  {
>      if (vs->need_update && vs->csock != -1) {
>          VncDisplay *vd = vs->vd;
> @@ -939,8 +931,11 @@ static int vnc_update_client(VncState *vs, int has_dirty)
>          return n;
>      }
>  
> -    if (vs->csock == -1)
> +    if (vs->csock == -1) {
>          vnc_disconnect_finish(vs);
> +    } else if (sync) {
> +        vnc_jobs_join(vs);
> +    }

... here, under a "not freed" guard.

>  
>      return 0;
>  }
> @@ -2749,7 +2744,7 @@ static void vnc_refresh(DisplayChangeListener *dcl)
>      vnc_unlock_display(vd);
>  
>      QTAILQ_FOREACH_SAFE(vs, &vd->clients, next, vn) {
> -        rects += vnc_update_client(vs, has_dirty);
> +        rects += vnc_update_client(vs, has_dirty, false);
>          /* vs might be free()ed here */
>      }

Reviewed-by: Markus Armbruster <armbru@redhat.com>