[PATCH xserver] xwayland: Fix use after free of cursors

[Olivier Fourdan]

Sometimes, Xwayland will try to use a cursor that has just been freed,
leading to a crash when trying to access that cursor data either in
miPointerUpdateSprite() or AnimCurTimerNotify().

CheckMotion() updates the pointer's cursor based on which xwindow
XYToWindow() returns, and Xwayland implements its own xwl_xy_to_window()
to fake a crossing to the root window when the pointer has left the
Wayland surface but is still within the xwindow.

But after an xwindow is unrealized, the last xwindow used to match the
xwindows is cleared so two consecutive calls to xwl_xy_to_window() may
not return the same xwindow.

To avoid this issue, update the last_xwindow based on enter and leave
notifications instead of xwl_xy_to_window(), and check if the xwindow
found by the regular miXYToWindow() is a child of the known last
xwindow, so that multiple consecutive calls to xwl_xy_to_window()
return the same xwindow, being either the one found by miXYToWindow()
or the root window.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
Signed-off-by: Olivier Fourdan 
Tested-by: Vít Ondruch 
Tested-by: Satish Balay 
Reviewed-by: Jonas Ådahl 
---
 hw/xwayland/xwayland-input.c | 17 +
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 3bc7110..580df09 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -312,6 +312,9 @@ pointer_handle_enter(void *data, struct wl_pointer *pointer,
 dx = xwl_seat->focus_window->window->drawable.x;
 dy = xwl_seat->focus_window->window->drawable.y;
 
+/* We just entered a new xwindow, forget about the old last xwindow */
+xwl_seat->last_xwindow = NullWindow;
+
 master = GetMaster(dev, POINTER_OR_FLOAT);
 (*pScreen->SetCursorPosition) (dev, pScreen, dx + sx, dy + sy, TRUE);
 
@@ -360,8 +363,14 @@ pointer_handle_leave(void *data, struct wl_pointer 
*pointer,
 
 xwl_seat->xwl_screen->serial = serial;
 
-xwl_seat->focus_window = NULL;
-CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+/* The pointer has left a known xwindow, save it for a possible match
+ * in sprite_check_lost_focus()
+ */
+if (xwl_seat->focus_window) {
+xwl_seat->last_xwindow = xwl_seat->focus_window->window;
+xwl_seat->focus_window = NULL;
+CheckMotion(NULL, GetMaster(dev, POINTER_OR_FLOAT));
+}
 }
 
 static void
@@ -1252,10 +1261,10 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr 
window)
  */
 if (master->lastSlave == xwl_seat->pointer &&
 xwl_seat->focus_window == NULL &&
-xwl_seat->last_xwindow == window)
+xwl_seat->last_xwindow != NullWindow &&
+IsParent(xwl_seat->last_xwindow, window))
 return TRUE;
 
-xwl_seat->last_xwindow = window;
 return FALSE;
 }
 
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xwayland: Fix use after free of cursors

[Olivier Fourdan]

> Sometimes, Xwayland will try to use a cursor that has just been freed,
> leading to a crash when trying to access that cursor data either in
> miPointerUpdateSprite() or elsewhere.
> [...]

Right, I think I have to withdraw this patch, because it probably doesn't fix 
the issue...

I still believe the problem finds its root in xwl_xy_to_window() but I still 
haven't found an appropriate fix.

Sorry about that,
Olivier

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH xserver] xwayland: Fix use after free of cursors

[Olivier Fourdan]

Sometimes, Xwayland will try to use a cursor that has just been freed,
leading to a crash when trying to access that cursor data either in
miPointerUpdateSprite() or elsewhere.

This issue is very random and hard to reproduce.

Typical backtraces include:

  miPointerUpdateSprite () at mipointer.c
  mieqProcessInputEvents () at mieq.c
  ProcessInputEvents () at xwayland-input.c
  Dispatch () at dispatch.c
  dix_main () at main.c

or

  miPointerUpdateSprite () at mipointer.c
  mieqProcessInputEvents () at mieq.c
  keyboard_handle_modifiers () at xwayland-input.c
  wl_closure_invoke () at src/connection.c
  dispatch_event () at src/wayland-client.c
  dispatch_queue () at src/wayland-client.c
  wl_display_dispatch_queue_pending () at src/wayland-client.c
  wl_display_dispatch_pending () at src/wayland-client.c
  xwl_read_events () at xwayland.c
  WaitForSomething () at WaitFor.c
  Dispatch () at dispatch.c
  dix_main () at main.c

or

  AnimCurTimerNotify () at animcur.c
  DoTimer () at WaitFor.c
  DoTimers () at WaitFor.c
  check_timers () at WaitFor.c
  WaitForSomething () at WaitFor.c
  Dispatch () at dispatch.c
  dix_main () at main.c

CheckMotion() would update the pointer's cursor only when the sprite
windows differ before and after calling XYToWindow(), but Xwayland
implements its own xwl_xy_to_window() which would fake a crossing to
the root window if the pointer has left the Wayland surface but is still
within the Xwindow, which confuses CheckMotion().

Typically, after the cursors have been freed from CloseDownClient(), if
the pointer's sprite window is already the root window, and Xwayland's
xwl_xy_to_window() fakes a transition to the root window as well, the
previous and new sprite windows are already identical and CheckMotion()
will not call PostNewCursor() and thus not invoke
miPointerDisplayCursor() that would have updated the pointer's cursor.

Any further attempt to update the pointer using that cursor will lead to
a crash.

To avoid this issue, modify Xwayland's own xwl_xy_to_window() to avoid
returning the root window if the sprite window is already the root
window, so that the logic in CheckMotion() is preserved.

Bugzilla: https://bugzilla.redhat.com/show_bug.cgi?id=1385258
Signed-off-by: Olivier Fourdan 
---
 hw/xwayland/xwayland-input.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 0526122..88c12f5 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -1256,7 +1256,8 @@ sprite_check_lost_focus(SpritePtr sprite, WindowPtr 
window)
  */
 if (master->lastSlave == xwl_seat->pointer &&
 xwl_seat->focus_window == NULL &&
-xwl_seat->last_xwindow == window)
+xwl_seat->last_xwindow == window &&
+sprite->win != sprite->spriteTrace[0])
 return TRUE;
 
 xwl_seat->last_xwindow = window;
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel