Skip to content

Commit c9dfe46

Browse files
metuxmetux
committed
miext: damage: use drawable pointer instead of list head pointer to avoid realloc() UAF
The original fix (*1) introduced a use-after-free vulnerability by caching a pointer into a realloc()able privates array (pListHead). This fix replaces the pListHead pointer with pListDrawable, storing the drawable pointer at insertion time and re-deriving the list head at removal time via getDrawableDamageRef(), avoiding the realloc() UAF. The fix: 1. Changes pListHead (DamagePtr *) to pListDrawable (DrawablePtr) in damagestr.h 2. Updates damageInsertDamage to store pDamage->pDrawable instead of the list head pointer 3. Updates damageRemoveDamage to take a DrawablePtr and re-derive the list head via getDrawableDamageRef() 4. Updates all call sites to pass the drawable pointer instead of the list head pointer 5. Fixes damagePixmapDestroy to pass the pixmap as the drawable *1) da72833 Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net> (cherry picked from commit ccfc479)
1 parent 550d302 commit c9dfe46

2 files changed

Lines changed: 47 additions & 31 deletions

File tree

‎miext/damage/damage.c‎

Lines changed: 42 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,14 @@
2222

2323
#include <dix-config.h>
2424

25+
#include <stdbool.h>
2526
#include <stdlib.h>
2627

2728
#include "dix/screen_hooks_priv.h"
29+
#include "include/mipict.h"
30+
#include "os/mathx_priv.h"
2831
#include "os/osdep.h"
32+
#include "Xext/render/glyphstr_priv.h"
2933

3034
#include <X11/X.h>
3135
#include "scrnintstr.h"
@@ -35,13 +39,11 @@
3539
#include <X11/fonts/fontstruct.h>
3640
#include <X11/fonts/libxfont2.h>
3741
#include "mi.h"
38-
#include "mipict.h"
3942
#include "regionstr.h"
4043
#include "globals.h"
4144
#include "gcstruct.h"
4245
#include "damage.h"
4346
#include "damagestr.h"
44-
#include "glyphstr_priv.h"
4547

4648
#define wrap(priv, real, mem, func) {\
4749
priv->mem = real->mem; \
@@ -127,7 +129,7 @@ getDrawableDamageRef(DrawablePtr pDrawable)
127129
static void
128130
_damageRegionAppend(DrawablePtr pDrawable, RegionPtr pRegion, Bool clip,
129131
int subWindowMode, const char *where)
130-
#define damageRegionAppend(d,r,c,m) _damageRegionAppend(d,r,c,m,__FUNCTION__)
132+
#define damageRegionAppend(d,r,c,m) _damageRegionAppend(d,r,c,m,__func__)
131133
#else
132134
static void
133135
damageRegionAppend(DrawablePtr pDrawable, RegionPtr pRegion, Bool clip,
@@ -298,7 +300,7 @@ damageRegionProcessPending(DrawablePtr pDrawable)
298300
}
299301

300302
#if DAMAGE_DEBUG_ENABLE
301-
#define damageDamageBox(d,b,m) _damageDamageBox(d,b,m,__FUNCTION__)
303+
#define damageDamageBox(d,b,m) _damageDamageBox(d,b,m,__func__)
302304
static void
303305
_damageDamageBox(DrawablePtr pDrawable, BoxPtr pBox, int subWindowMode,
304306
const char *where)
@@ -340,7 +342,7 @@ damageCreateGC(GCPtr pGC)
340342

341343
damageScrPriv(pScreen);
342344
damageGCPriv(pGC);
343-
Bool ret;
345+
bool ret;
344346

345347
unwrap(pScrPriv, pScreen, CreateGC);
346348
if ((ret = (*pScreen->CreateGC) (pGC))) {
@@ -597,8 +599,8 @@ damageAddTraps(PicturePtr pPicture,
597599
x = pPicture->pDrawable->x + x_off;
598600
y = pPicture->pDrawable->y + y_off;
599601
for (i = 0; i < ntrap; i++) {
600-
pixman_fixed_t l = min(t->top.l, t->bot.l);
601-
pixman_fixed_t r = max(t->top.r, t->bot.r);
602+
pixman_fixed_t l = MIN(t->top.l, t->bot.l);
603+
pixman_fixed_t r = MAX(t->top.r, t->bot.r);
602604
int x1 = x + pixman_fixed_to_int(l);
603605
int x2 = x + pixman_fixed_to_int(pixman_fixed_ceil(r));
604606
int y1 = y + pixman_fixed_to_int(t->top.y);
@@ -1311,7 +1313,7 @@ damageText(DrawablePtr pDrawable,
13111313
CharInfoPtr *charinfo;
13121314
unsigned long i;
13131315
unsigned int n;
1314-
Bool imageblt;
1316+
bool imageblt;
13151317

13161318
imageblt = (textType == TT_IMAGE8) || (textType == TT_IMAGE16);
13171319

@@ -1446,8 +1448,10 @@ damagePushPixels(GCPtr pGC,
14461448
}
14471449

14481450
static void
1449-
damageRemoveDamage(DamagePtr * pPrev, DamagePtr pDamage)
1451+
damageRemoveDamage(DrawablePtr pListDrawable, DamagePtr pDamage)
14501452
{
1453+
DamagePtr *pPrev = getDrawableDamageRef(pListDrawable);
1454+
14511455
while (*pPrev) {
14521456
if (*pPrev == pDamage) {
14531457
*pPrev = pDamage->pNext;
@@ -1461,6 +1465,16 @@ damageRemoveDamage(DamagePtr * pPrev, DamagePtr pDamage)
14611465
#endif
14621466
}
14631467

1468+
/*
1469+
* Link pDamage onto the list headed by pPrev, remembering that list so that damageRemoveDamage() can later unlink it
1470+
* from the same place.
1471+
*
1472+
* The list a window's damage belongs on is chosen by getDrawableDamageRef() and depends on the window pixmap, which can
1473+
* change while the damage is registered (Composite redirection, rootless drawing and Present flips all swap it).
1474+
* Re-deriving the list at removal time can therefore consult a different list than the damage was inserted onto, in
1475+
* which case the removal silently does nothing and leaves an unregistered -- possibly freed -- damage linked where
1476+
* damageRegionAppend() will dereference its NULL pDrawable.
1477+
*/
14641478
static void
14651479
damageInsertDamage(DamagePtr * pPrev, DamagePtr pDamage)
14661480
{
@@ -1475,22 +1489,23 @@ damageInsertDamage(DamagePtr * pPrev, DamagePtr pDamage)
14751489
#endif
14761490
pDamage->pNext = *pPrev;
14771491
*pPrev = pDamage;
1492+
pDamage->pListDrawable = pDamage->pDrawable;
14781493
}
1479-
1480-
static void damagePixmapDestroy(CallbackListPtr *pcbl, ScreenPtr pScreen, PixmapPtr pPixmap)
1494+
static void
1495+
damagePixmapDestroy(CallbackListPtr *pcbl, ScreenPtr pScreen, PixmapPtr pPixmap)
14811496
{
14821497
DamagePtr *pPrev = getPixmapDamageRef(pPixmap);
14831498
DamagePtr pDamage;
14841499

14851500
while ((pDamage = *pPrev)) {
1486-
damageRemoveDamage(pPrev, pDamage);
1501+
damageRemoveDamage((DrawablePtr)pPixmap, pDamage);
14871502
if (!pDamage->isWindow)
14881503
DamageDestroy(pDamage);
14891504
}
14901505
}
14911506

14921507
static void
1493-
damageCopyWindow(WindowPtr pWindow, DDXPointRec ptOldOrg, RegionPtr prgnSrc)
1508+
damageCopyWindow(WindowPtr pWindow, xPoint ptOldOrg, RegionPtr prgnSrc)
14941509
{
14951510
ScreenPtr pScreen = pWindow->drawable.pScreen;
14961511

@@ -1536,11 +1551,9 @@ damageSetWindowPixmap(WindowPtr pWindow, PixmapPtr pPixmap)
15361551
damageScrPriv(pScreen);
15371552

15381553
if ((pDamage = damageGetWinPriv(pWindow))) {
1539-
PixmapPtr pOldPixmap = (*pScreen->GetWindowPixmap) (pWindow);
1540-
DamagePtr *pPrev = getPixmapDamageRef(pOldPixmap);
1541-
15421554
while (pDamage) {
1543-
damageRemoveDamage(pPrev, pDamage);
1555+
if (pDamage->pListDrawable)
1556+
damageRemoveDamage(pDamage->pListDrawable, pDamage);
15441557
pDamage = pDamage->pNextWin;
15451558
}
15461559
}
@@ -1700,6 +1713,7 @@ DamageCreate(DamageReportFunc damageReport,
17001713
return 0;
17011714
pDamage->pNext = 0;
17021715
pDamage->pNextWin = 0;
1716+
pDamage->pListDrawable = NULL;
17031717
RegionNull(&pDamage->damage);
17041718
RegionNull(&pDamage->pendingDamage);
17051719

@@ -1714,9 +1728,8 @@ DamageCreate(DamageReportFunc damageReport,
17141728
pDamage->damageDestroy = damageDestroy;
17151729
pDamage->pScreen = pScreen;
17161730

1717-
if (pScrPriv && pScrPriv->funcs.Create) {
1731+
if (pScrPriv && pScrPriv->funcs.Create)
17181732
pScrPriv->funcs.Create (pDamage);
1719-
}
17201733

17211734
return pDamage;
17221735
}
@@ -1757,10 +1770,8 @@ DamageRegister(DrawablePtr pDrawable, DamagePtr pDamage)
17571770
pDamage->isWindow = FALSE;
17581771
pDamage->pDrawable = pDrawable;
17591772
damageInsertDamage(getDrawableDamageRef(pDrawable), pDamage);
1760-
1761-
if (pScrPriv && pScrPriv->funcs.Register) {
1773+
if (pScrPriv && pScrPriv->funcs.Register)
17621774
pScrPriv->funcs.Register (pDrawable, pDamage);
1763-
}
17641775
}
17651776

17661777
void
@@ -1779,9 +1790,8 @@ DamageUnregister(DamagePtr pDamage)
17791790

17801791
damageScrPriv(pScreen);
17811792

1782-
if (pScrPriv && pScrPriv->funcs.Unregister) {
1793+
if (pScrPriv && pScrPriv->funcs.Unregister)
17831794
pScrPriv->funcs.Unregister (pDrawable, pDamage);
1784-
}
17851795

17861796
if (pDrawable->type == DRAWABLE_WINDOW) {
17871797
WindowPtr pWindow = (WindowPtr) pDrawable;
@@ -1808,8 +1818,9 @@ DamageUnregister(DamagePtr pDamage)
18081818
}
18091819
#endif
18101820
}
1821+
if (pDamage->pListDrawable)
1822+
damageRemoveDamage(pDamage->pListDrawable, pDamage);
18111823
pDamage->pDrawable = 0;
1812-
damageRemoveDamage(getDrawableDamageRef(pDrawable), pDamage);
18131824
}
18141825

18151826
void
@@ -1825,9 +1836,8 @@ DamageDestroy(DamagePtr pDamage)
18251836
if (pDamage->damageDestroy)
18261837
(*pDamage->damageDestroy) (pDamage, pDamage->closure);
18271838

1828-
if (pScrPriv && pScrPriv->funcs.Destroy) {
1839+
if (pScrPriv && pScrPriv->funcs.Destroy)
18291840
pScrPriv->funcs.Destroy (pDamage);
1830-
}
18311841

18321842
RegionUninit(&pDamage->damage);
18331843
RegionUninit(&pDamage->pendingDamage);
@@ -1867,19 +1877,20 @@ DamageSubtract(DamagePtr pDamage, const RegionPtr pRegion)
18671877
void
18681878
DamageEmpty(DamagePtr pDamage)
18691879
{
1870-
RegionEmpty(&pDamage->damage);
1880+
if (pDamage)
1881+
RegionEmpty(&pDamage->damage);
18711882
}
18721883

18731884
RegionPtr
18741885
DamageRegion(DamagePtr pDamage)
18751886
{
1876-
return &pDamage->damage;
1887+
return pDamage ? &pDamage->damage : NULL;
18771888
}
18781889

18791890
RegionPtr
18801891
DamagePendingRegion(DamagePtr pDamage)
18811892
{
1882-
return &pDamage->pendingDamage;
1893+
return pDamage ? &pDamage->pendingDamage : NULL;
18831894
}
18841895

18851896
void
@@ -1925,7 +1936,7 @@ DamageReportDamage(DamagePtr pDamage, RegionPtr pDamageRegion)
19251936
{
19261937
BoxRec tmpBox;
19271938
RegionRec tmpRegion;
1928-
Bool was_empty;
1939+
bool was_empty;
19291940

19301941
switch (pDamage->damageLevel) {
19311942
case DamageReportRawRegion:

‎miext/damage/damagestr.h‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,6 +35,11 @@
3535
typedef struct _damage {
3636
DamagePtr pNext;
3737
DamagePtr pNextWin;
38+
/*
39+
* Drawable on which this damage was inserted. Used to re-derive the list head at removal time
40+
* to avoid caching a pointer into a realloc()able privates array.
41+
*/
42+
DrawablePtr pListDrawable;
3843
RegionRec damage;
3944

4045
DamageReportLevel damageLevel;

0 commit comments

Comments
 (0)