From 16a1242d0ffc7f45ed3c595ee7564b5c04287e0b Mon Sep 17 00:00:00 2001 From: Olivier Fourdan Date: Mon, 20 Jan 2025 16:52:01 +0100 Subject: [PATCH] sync: Do not let sync objects uninitialized When changing an alarm, the change mask values are evaluated one after the other, changing the trigger values as requested and eventually, SyncInitTrigger() is called. SyncInitTrigger() will evaluate the XSyncCACounter first and may free the existing sync object. Other changes are then evaluated and may trigger an error and an early return, not adding the new sync object. This can be used to cause a use after free when the alarm eventually triggers. To avoid the issue, delete the existing sync object as late as possible only once we are sure that no further error will cause an early exit. CVE-2025-26601, ZDI-CAN-25870 This vulnerability was discovered by: Jan-Niklas Sohn working with Trend Micro Zero Day Initiative Signed-off-by: Olivier Fourdan Reviewed-by: Peter Hutterer Part-of: --- Xext/sync.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/Xext/sync.c b/Xext/sync.c index ee0010e657..585cfa6f68 100644 --- a/Xext/sync.c +++ b/Xext/sync.c @@ -360,11 +360,6 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger, XID syncObject, client->errorValue = syncObject; return rc; } - if (pSync != pTrigger->pSync) { /* new counter for trigger */ - SyncDeleteTriggerFromSyncObject(pTrigger); - pTrigger->pSync = pSync; - newSyncObject = TRUE; - } } /* if system counter, ask it what the current value is */ @@ -432,6 +427,14 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger, XID syncObject, } } + if (changes & XSyncCACounter) { + if (pSync != pTrigger->pSync) { /* new counter for trigger */ + SyncDeleteTriggerFromSyncObject(pTrigger); + pTrigger->pSync = pSync; + newSyncObject = TRUE; + } + } + /* we wait until we're sure there are no errors before registering * a new counter on a trigger */ -- GitLab From f52cea2f93a0c891494eb3334894442a92368030 Mon Sep 17 00:00:00 2001 From: Olivier Fourdan Date: Mon, 20 Jan 2025 16:54:30 +0100 Subject: [PATCH] sync: Check values before applying changes In SyncInitTrigger(), we would set the CheckTrigger function before validating the counter value. As a result, if the counter value overflowed, we would leave the function SyncInitTrigger() with the CheckTrigger applied but without updating the trigger object. To avoid that issue, move the portion of code checking for the trigger check value before updating the CheckTrigger function. Related to CVE-2025-26601, ZDI-CAN-25870 Signed-off-by: Olivier Fourdan Reviewed-by: Peter Hutterer Part-of: --- Xext/sync.c | 36 ++++++++++++++++++------------------ 1 file changed, 18 insertions(+), 18 deletions(-) diff --git a/Xext/sync.c b/Xext/sync.c index 585cfa6f68..10302160fb 100644 --- a/Xext/sync.c +++ b/Xext/sync.c @@ -381,6 +381,24 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger, XID syncObject, } } + if (changes & (XSyncCAValueType | XSyncCAValue)) { + if (pTrigger->value_type == XSyncAbsolute) + pTrigger->test_value = pTrigger->wait_value; + else { /* relative */ + Bool overflow; + + if (pCounter == NULL) + return BadMatch; + + overflow = checked_int64_add(&pTrigger->test_value, + pCounter->value, pTrigger->wait_value); + if (overflow) { + client->errorValue = pTrigger->wait_value >> 32; + return BadValue; + } + } + } + if (changes & XSyncCATestType) { if (pSync && SYNC_FENCE == pSync->type) { @@ -409,24 +427,6 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger, XID syncObject, } } - if (changes & (XSyncCAValueType | XSyncCAValue)) { - if (pTrigger->value_type == XSyncAbsolute) - pTrigger->test_value = pTrigger->wait_value; - else { /* relative */ - Bool overflow; - - if (pCounter == NULL) - return BadMatch; - - overflow = checked_int64_add(&pTrigger->test_value, - pCounter->value, pTrigger->wait_value); - if (overflow) { - client->errorValue = pTrigger->wait_value >> 32; - return BadValue; - } - } - } - if (changes & XSyncCACounter) { if (pSync != pTrigger->pSync) { /* new counter for trigger */ SyncDeleteTriggerFromSyncObject(pTrigger); -- GitLab From 8cbc90c8817306af75a60f494ec9dbb1061e50db Mon Sep 17 00:00:00 2001 From: Olivier Fourdan Date: Mon, 20 Jan 2025 17:06:07 +0100 Subject: [PATCH] sync: Do not fail SyncAddTriggerToSyncObject() We do not want to return a failure at the very last step in SyncInitTrigger() after having all changes applied. SyncAddTriggerToSyncObject() must not fail on memory allocation, if the allocation of the SyncTriggerList fails, trigger a FatalError() instead. Related to CVE-2025-26601, ZDI-CAN-25870 Signed-off-by: Olivier Fourdan Reviewed-by: Peter Hutterer Part-of: --- Xext/sync.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Xext/sync.c b/Xext/sync.c index 10302160fb..65f2d43780 100644 --- a/Xext/sync.c +++ b/Xext/sync.c @@ -201,8 +201,8 @@ SyncAddTriggerToSyncObject(SyncTrigger * pTrigger) return Success; } - if (!(pCur = malloc(sizeof(SyncTriggerList)))) - return BadAlloc; + /* Failure is not an option, it's succeed or burst! */ + pCur = XNFalloc(sizeof(SyncTriggerList)); pCur->pTrigger = pTrigger; pCur->next = pTrigger->pSync->pTriglist; @@ -439,8 +439,7 @@ SyncInitTrigger(ClientPtr client, SyncTrigger * pTrigger, XID syncObject, * a new counter on a trigger */ if (newSyncObject) { - if ((rc = SyncAddTriggerToSyncObject(pTrigger)) != Success) - return rc; + SyncAddTriggerToSyncObject(pTrigger); } else if (pCounter && IsSystemCounter(pCounter)) { SyncComputeBracketValues(pCounter); -- GitLab From c285798984c6bb99e454a33772cde23d394d3dcd Mon Sep 17 00:00:00 2001 From: Olivier Fourdan Date: Mon, 20 Jan 2025 17:10:31 +0100 Subject: [PATCH] sync: Apply changes last in SyncChangeAlarmAttributes() SyncChangeAlarmAttributes() would apply the various changes while checking for errors. If one of the changes triggers an error, the changes for the trigger, counter or delta value would remain, possibly leading to inconsistent changes. Postpone the actual changes until we're sure nothing else can go wrong. Related to CVE-2025-26601, ZDI-CAN-25870 Signed-off-by: Olivier Fourdan Reviewed-by: Peter Hutterer Part-of: --- Xext/sync.c | 42 +++++++++++++++++++++++++++--------------- 1 file changed, 27 insertions(+), 15 deletions(-) diff --git a/Xext/sync.c b/Xext/sync.c index 65f2d43780..cab73be927 100644 --- a/Xext/sync.c +++ b/Xext/sync.c @@ -830,8 +830,14 @@ SyncChangeAlarmAttributes(ClientPtr client, SyncAlarm * pAlarm, Mask mask, int status; XSyncCounter counter; Mask origmask = mask; + SyncTrigger trigger; + Bool select_events_changed = FALSE; + Bool select_events_value = FALSE; + int64_t delta; - counter = pAlarm->trigger.pSync ? pAlarm->trigger.pSync->id : None; + trigger = pAlarm->trigger; + delta = pAlarm->delta; + counter = trigger.pSync ? trigger.pSync->id : None; while (mask) { int index2 = lowbit(mask); @@ -847,24 +853,24 @@ SyncChangeAlarmAttributes(ClientPtr client, SyncAlarm * pAlarm, Mask mask, case XSyncCAValueType: mask &= ~XSyncCAValueType; /* sanity check in SyncInitTrigger */ - pAlarm->trigger.value_type = *values++; + trigger.value_type = *values++; break; case XSyncCAValue: mask &= ~XSyncCAValue; - pAlarm->trigger.wait_value = ((int64_t)values[0] << 32) | values[1]; + trigger.wait_value = ((int64_t)values[0] << 32) | values[1]; values += 2; break; case XSyncCATestType: mask &= ~XSyncCATestType; /* sanity check in SyncInitTrigger */ - pAlarm->trigger.test_type = *values++; + trigger.test_type = *values++; break; case XSyncCADelta: mask &= ~XSyncCADelta; - pAlarm->delta = ((int64_t)values[0] << 32) | values[1]; + delta = ((int64_t)values[0] << 32) | values[1]; values += 2; break; @@ -874,10 +880,8 @@ SyncChangeAlarmAttributes(ClientPtr client, SyncAlarm * pAlarm, Mask mask, client->errorValue = *values; return BadValue; } - status = SyncEventSelectForAlarm(pAlarm, client, - (Bool) (*values++)); - if (status != Success) - return status; + select_events_value = (Bool) (*values++); + select_events_changed = TRUE; break; default: @@ -886,25 +890,33 @@ SyncChangeAlarmAttributes(ClientPtr client, SyncAlarm * pAlarm, Mask mask, } } + if (select_events_changed) { + status = SyncEventSelectForAlarm(pAlarm, client, select_events_value); + if (status != Success) + return status; + } + /* "If the test-type is PositiveComparison or PositiveTransition * and delta is less than zero, or if the test-type is * NegativeComparison or NegativeTransition and delta is * greater than zero, a Match error is generated." */ if (origmask & (XSyncCADelta | XSyncCATestType)) { - if ((((pAlarm->trigger.test_type == XSyncPositiveComparison) || - (pAlarm->trigger.test_type == XSyncPositiveTransition)) - && pAlarm->delta < 0) + if ((((trigger.test_type == XSyncPositiveComparison) || + (trigger.test_type == XSyncPositiveTransition)) + && delta < 0) || - (((pAlarm->trigger.test_type == XSyncNegativeComparison) || - (pAlarm->trigger.test_type == XSyncNegativeTransition)) - && pAlarm->delta > 0) + (((trigger.test_type == XSyncNegativeComparison) || + (trigger.test_type == XSyncNegativeTransition)) + && delta > 0) ) { return BadMatch; } } /* postpone this until now, when we're sure nothing else can go wrong */ + pAlarm->delta = delta; + pAlarm->trigger = trigger; if ((status = SyncInitTrigger(client, &pAlarm->trigger, counter, RTCounter, origmask & XSyncCAAllTrigger)) != Success) return status; -- GitLab