From 1953f460b9ad1a9cdf0fcce70f6ad3310b713d5f Mon Sep 17 00:00:00 2001
From: Peter Hutterer <peter.hutterer@who-t.net>
Date: Thu, 12 Oct 2023 12:44:13 +1000
Subject: [PATCH] fb: properly wrap/unwrap CloseScreen

fbCloseScreen assumes that it overrides miCloseScreen (which just
calls FreePixmap(screen->devPrivates)) and emulates that instead of
wrapping it.

This is a wrong assumption, we may have ShmCloseScreen in the mix too,
resulting in leaks (see below). Fix this by properly setting up the
CloseScreen wrapper.

This means we no longer need the manual DestroyPixmap call in
vfbCloseScreen, reverting d348ab06aae21c153ecbc3511aeafc8ab66d8303

CVE-2023-5574, ZDI-CAN-21213

This vulnerability was discovered by:
Sri working with Trend Micro Zero Day Initiative

Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Adam Jackson <ajax@redhat.com>
---
 fb/fb.h             |  1 +
 fb/fbscreen.c       | 14 ++++++++++----
 hw/vfb/InitOutput.c |  7 -------
 3 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/fb/fb.h b/fb/fb.h
index d157b6956d..cd7bd05d21 100644
--- a/fb/fb.h
+++ b/fb/fb.h
@@ -410,6 +410,7 @@ typedef struct {
 #endif
     DevPrivateKeyRec    gcPrivateKeyRec;
     DevPrivateKeyRec    winPrivateKeyRec;
+    CloseScreenProcPtr  CloseScreen;
 } FbScreenPrivRec, *FbScreenPrivPtr;
 
 #define fbGetScreenPrivate(pScreen) ((FbScreenPrivPtr) \
diff --git a/fb/fbscreen.c b/fb/fbscreen.c
index 4ab807ab50..c481033f98 100644
--- a/fb/fbscreen.c
+++ b/fb/fbscreen.c
@@ -29,6 +29,7 @@
 Bool
 fbCloseScreen(ScreenPtr pScreen)
 {
+    FbScreenPrivPtr screen_priv = fbGetScreenPrivate(pScreen);
     int d;
     DepthPtr depths = pScreen->allowedDepths;
 
@@ -37,9 +38,10 @@ fbCloseScreen(ScreenPtr pScreen)
         free(depths[d].vids);
     free(depths);
     free(pScreen->visuals);
-    if (pScreen->devPrivate)
-        FreePixmap((PixmapPtr)pScreen->devPrivate);
-    return TRUE;
+
+    pScreen->CloseScreen = screen_priv->CloseScreen;
+
+    return pScreen->CloseScreen(pScreen);
 }
 
 Bool
@@ -144,6 +146,7 @@ fbFinishScreenInit(ScreenPtr pScreen, void *pbits, int xsize, int ysize,
                    int dpix, int dpiy, int width, int bpp)
 #endif
 {
+    FbScreenPrivPtr screen_priv;
     VisualPtr visuals;
     DepthPtr depths;
     int nvisuals;
@@ -177,8 +180,11 @@ fbFinishScreenInit(ScreenPtr pScreen, void *pbits, int xsize, int ysize,
                       rootdepth, ndepths, depths,
                       defaultVisual, nvisuals, visuals))
         return FALSE;
-    /* overwrite miCloseScreen with our own */
+
+    screen_priv = fbGetScreenPrivate(pScreen);
+    screen_priv->CloseScreen = pScreen->CloseScreen;
     pScreen->CloseScreen = fbCloseScreen;
+
     return TRUE;
 }
 
diff --git a/hw/vfb/InitOutput.c b/hw/vfb/InitOutput.c
index 48efb61b2f..076fb7defa 100644
--- a/hw/vfb/InitOutput.c
+++ b/hw/vfb/InitOutput.c
@@ -720,13 +720,6 @@ vfbCloseScreen(ScreenPtr pScreen)
 
     pScreen->CloseScreen = pvfb->closeScreen;
 
-    /*
-     * fb overwrites miCloseScreen, so do this here
-     */
-    if (pScreen->devPrivate)
-        (*pScreen->DestroyPixmap) (pScreen->devPrivate);
-    pScreen->devPrivate = NULL;
-
     return pScreen->CloseScreen(pScreen);
 }
 
-- 
GitLab