Clears warning from gcc 14.1:
../dix/resource.c: In function ‘HashResourceID’:
../dix/resource.c:691:44: warning: left shift of negative value
[-Wshift-negative-value]
691 | return (id ^ (id >> numBits)) & ~((~0) << numBits);
| ^~
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 26a7ab09ea)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1702>
No real harm, but clears warning from gcc 14.1:
../dix/property.c: In function ‘ProcListProperties’:
..//dix/property.c:605:27: warning: dereference of NULL ‘temppAtoms’
[CWE-476] [-Wanalyzer-null-dereference]
605 | *temppAtoms++ = pProp->propertyName;
| ~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 39f337fd49)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1702>
It shouldn't matter, since it would have a length of 0, but it
clears warnings from gcc 14.1:
../dix/property.c: In function ‘dixChangeWindowProperty’:
../dix/property.c:287:9: warning: use of possibly-NULL ‘data’ where
non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
287 | memcpy(data, value, totalSize);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../dix/property.c:324:13: warning: use of possibly-NULL ‘data’ where
non-null expected [CWE-690] [-Wanalyzer-possible-null-argument]
324 | memcpy(data, value, totalSize);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 10cafd0bbe)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1702>
Clears warning from gcc 14.1:
../dix/dixfonts.c: In function ‘SetFontPath’:
../dix/dixfonts.c:1697:28: warning: use of uninitialized value ‘bad’
[CWE-457] [-Wanalyzer-use-of-uninitialized-value]
1697 | client->errorValue = bad;
| ~~~~~~~~~~~~~~~~~~~^~~~~
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 1a86fba0d9)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1702>
Clears warning from gcc 14.1:
../dix/dixfonts.c:1352:15: warning: use of uninitialized value ‘*c.data’
[CWE-457] [-Wanalyzer-use-of-uninitialized-value]
1352 | free(c->data);
| ~^~~~~~
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit d78836a3a6)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1702>
Clears -Wcalloc-transposed-args warnings from gcc 14.1, such as:
../dix/main.c:165:42: warning: ‘calloc’ sizes specified with ‘sizeof’ in the
earlier argument and not in the later argument [-Wcalloc-transposed-args]
165 | serverClient = calloc(sizeof(ClientRec), 1);
| ^~~~~~~~~
../dix/main.c:165:42: note: earlier argument should specify number of
elements, later size of each element
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 522f469fe9)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1635>
Since the two DDX'es which had used this key (xnest and xfree86) now using
their own ones, this global key is obsolete and can be removed.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1342>
This header isn't installed, so no external modules could use the
functions declared there. Thus we can unexport it all.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1358>
These aren't used by any drivers/modules, just DDX'es, so no need to export.
Note: tigervnc does use it, but it has it's own DDX, therefore directly
linked in, just like the in-tree DDX'es which doesn't need exporting.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1349>
* this symbol is a server configuration flag (can be passed via cmdline)
for limiting the max size of big-requests. there shouldn't be any need
to use it outside the core X server (in server modules like drivers
or external extension) - therefore unexport it
* in order to reduce namespace pollution of public (server module API)
headers, create a new internal header for those tings (more to come)
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1275>
This breaks the xf86-input-synaptics driver:
synaptics.c: In function 'clickpad_guess_clickfingers':
synaptics.c:2638:5: error: implicit declaration of function 'BUG_RETURN_VAL' [-Werror=implicit-function-declaration]
2638 | BUG_RETURN_VAL(hw->num_mt_mask > sizeof(close_point) * 8, 0);
This reverts commit 442aec2219.
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1316>
The workQueue pointer is currently declared extern, so that WaitForSomething()
can check wether we've got something in the queue and call ProcessWorkQueue()
then.
But that's trivial to simplify: just let ProcessWorkQueue() return early if
workQueue == NULL. Gives us a better isolation of internal stuff as well as
ProcessWorkQueue() protecting itself from possible segfault.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1310>
Yet another step of uncluttering includes: move out the BUG_* macros
into a separate header, which then is included as-needed.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
CreateGC() allocates a new GC and then checks the resource access rights
with XaceHook().
If the call to XaceHook() fails (i.e. GC creation is not granted to the
client), CreateGC() exits early and calls FreeGC() to avoid leaking the
newly allocated GC.
If that happens, the screen's own CreateGC() has not yet been invoked,
and as a result the GC functions (GCfuncs) have not been set yet.
FreeGC() will invoke the funcs->DestroyClip() and the funcs->DestroyGC()
functions, but since those haven't been set, the Xserver will segfault
trying to call a NULL function.
To prevent that issue, make sure the GC's functions are initialized
prior to call them in FreeGC().
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1625
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
Disabling a master device floats all slave devices but we didn't do this
to already-disabled slave devices. As a result those devices kept their
reference to the master device resulting in access to already freed
memory if the master device was removed before the corresponding slave
device.
And to match this behavior, also forcibly reset that pointer during
CloseDownDevices().
Related to CVE-2024-21886, ZDI-CAN-22840
The `DisableDevice()` function is called whenever an enabled device
is disabled and it moves the device from the `inputInfo.devices` linked
list to the `inputInfo.off_devices` linked list.
However, its link/unlink operation has an issue during the recursive
call to `DisableDevice()` due to the `prev` pointer pointing to a
removed device.
This issue leads to a length mismatch between the total number of
devices and the number of device in the list, leading to a heap
overflow and, possibly, to local privilege escalation.
Simplify the code that checked whether the device passed to
`DisableDevice()` was in `inputInfo.devices` or not and find the
previous device after the recursion.
CVE-2024-21886, ZDI-CAN-22840
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
The previous code only made sense if one considers buttons and keys to
be mutually exclusive on a device. That is not necessarily true, causing
a number of issues.
This function allocates and fills in the number of xEvents we need to
send the device state down the wire. This is split across multiple
32-byte devices including one deviceStateNotify event and optional
deviceKeyStateNotify, deviceButtonStateNotify and (possibly multiple)
deviceValuator events.
The previous behavior would instead compose a sequence
of [state, buttonstate, state, keystate, valuator...]. This is not
protocol correct, and on top of that made the code extremely convoluted.
Fix this by streamlining: add both button and key into the deviceStateNotify
and then append the key state and button state, followed by the
valuators. Finally, the deviceValuator events contain up to 6 valuators
per event but we only ever sent through 3 at a time. Let's double that
troughput.
CVE-2024-0229, ZDI-CAN-22678
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
If a device has both a button class and a key class and numButtons is
zero, we can get an OOB write due to event under-allocation.
This function seems to assume a device has either keys or buttons, not
both. It has two virtually identical code paths, both of which assume
they're applying to the first event in the sequence.
A device with both a key and button class triggered a logic bug - only
one xEvent was allocated but the deviceStateNotify pointer was pushed on
once per type. So effectively this logic code:
int count = 1;
if (button && nbuttons > 32) count++;
if (key && nbuttons > 0) count++;
if (key && nkeys > 32) count++; // this is basically always true
// count is at 2 for our keys + zero button device
ev = alloc(count * sizeof(xEvent));
FixDeviceStateNotify(ev);
if (button)
FixDeviceStateNotify(ev++);
if (key)
FixDeviceStateNotify(ev++); // santa drops into the wrong chimney here
If the device has more than 3 valuators, the OOB is pushed back - we're
off by one so it will happen when the last deviceValuator event is
written instead.
Fix this by allocating the maximum number of events we may allocate.
Note that the current behavior is not protocol-correct anyway, this
patch fixes only the allocation issue.
Note that this issue does not trigger if the device has at least one
button. While the server does not prevent a button class with zero
buttons, it is very unlikely.
CVE-2024-0229, ZDI-CAN-22678
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
