We alread have several of these calls, that aren't interested in result value,
explicitly casting to void. Fixing this up for the remaining ones.
This is helpful for the human reader as well as quality analysis tools.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1648>
(cherry picked from commit f26f17c66a)
This has been nothing but an alias for two decades now (somewhere in R6.6),
so there doesn't seem to be any practical need for this indirection.
The macro still needs to remain, as long as (external) drivers still using it.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1529>
(cherry picked from commit a1fd7e7ba5)
The xnfreallocarray was added along (and just as an alias to) XNFreallocarray
back a decade ago. It's just used in a few places and it's only saves us from
passing the first parameter (NULL), so the actual benefit isn't really huge.
No (known) driver is using it, so the macro can be dropped entirely.
Fixes: ae75d50395
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1529>
(cherry picked from commit 61233adbca)
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>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1606>
(cherry picked from commit 522f469fe9)
For now, new selection objects are only created in ProcSetSelectionOwner()
when dixLookupSelection() can't find the requested one (returns BadMatch).
When somebody's trying to listen on a not-yet existing selection, via
XFixesSelectSelectionInput() -- XFIXES:SelectSelectionInput message -- he's
also getting BadMatch. The spec isn't explicitly clear on the exact behaviour
in those specific situations: it doesn't tell anything about selection's
lifetimes (when are they actually *created* or *destroyed*), just about their
ownership.
But there are real-world clients not expecting an error here and crashing
with a BadMatch error.
Since the spec doesn't mandate any Selection lifetime, it's safe to assume,
they can be created as-needed (as other related code paths already do).
Doing so ensures such an error cannot happen anymore.
XACE consumers get properly notified by the new Selection object creation
(eg. SElinux is attaching it's private data to it). And all callers already
prepared to get a cleared Selection object, because that's always been a
perfectly normal situation - Selection objects never get removed again,
just cleared.
Fixes: 601fd0fd8 - xfixes/xace: fix pointer type mismatch on XFixesSelectSelectionInput()
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1574>
(cherry picked from commit 6a1d730006)
As soon as winapi headers are included, we're running into a name clash
on UpdateColors(), since winapi has a function by the same name.
Trivial fix simply renaming our own UpdateColors() function.
../dix/colormap.c:110:13: error: conflicting types for ‘UpdateColors’
110 | static void UpdateColors(ColormapPtr /*pmap */
| ^~~~~~~~~~~~
In file included from /usr/share/mingw-w64/include/windows.h:71,
from /usr/share/mingw-w64/include/winsock2.h:23,
from /usr/i686-w64-mingw32/include/X11/Xwinsock.h:57,
from ../os/osdep.h:138,
from ../dix/colormap.c:57:
/usr/share/mingw-w64/include/wingdi.h:3202:28: note: previous declaration of ‘UpdateColors’ was here
3202 | WINGDIAPI WINBOOL WINAPI UpdateColors(HDC hdc);
| ^~~~~~~~~~~~
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1351>
(cherry picked from commit d8cbcfd601)
he generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 305f2d59d8)
he generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 51d8bcfc0d)
he generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 47d6c3ad75)
he generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 0f6bb23bc2)
The generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 922b7685dc)
The generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 3dfe00d5e0)
The generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 67e468c8bd)
The generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit 9524ffee89)
The generic XaceHook() call isn't typesafe (und unnecessarily slow).
Better add an explicit function, just like we already have for others.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1556>
(cherry picked from commit ae3c573337)
Since we already had to rename some of them, in order to fix name clashes
on win32, it's now time to rename all the remaining ones.
The old ones are still present as define's to the new ones, just for
backwards compatibility.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1355>
(cherry picked from commit 85d4bd0dba)
The problem is, ming32 headers unconditionally define CreateWindow() to
alias CreateWindowA() in winuser.h, which is included by windows.h, which
is included by *a lot* common headers. So it highly depends on the exact
include order, whether it works. (also weird things could happen, e.g.
the Xserver's CreateWindow() ending up renamed to CreateWindowA(), ...)
Until we've found a really clean solution to this problem (which might
involve fixing mingw32 first), just add a little workaround by undef'ing
CreateWindow symbol whereever necessary.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1355>
(cherry picked from commit 1ca48d0a48)
Windows' native headers using some our RT_* define's names for other things.
Since the naming isn't very nice anyways, introducing some new ones
(X11_RESTYPE_NONE, X11_RESTYPE_FONT, X11_RESTYPE_CURSOR) and define the old
ones as an alias to them, in case some out-of-tree code still uses them.
With thins change, we don't need to be so extremely careful about include
ordering and have explicit #undef's in order to prevent name clashes on
Win32 targets.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1355>
(cherry picked from commit 232cad9ec3)
It's only used by dix internally (actually just dixfonts.c - since at
least 25 years now), thus no need to keep it in global include directory.
Signed-off-by: Enrico Weigelt, metux IT consult <info@metux.net>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1388>
(cherry picked from commit f789dca4e9)
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>
(cherry picked from commit d84fd3bf19)
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>
(cherry picked from commit af9f70013a)
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
Both DeviceFocusEvent and the XIQueryPointer reply contain a bit for
each logical button currently down. Since buttons can be arbitrarily mapped
to anything up to 255 make sure we have enough bits for the maximum mapping.
CVE-2023-6816, ZDI-CAN-22664, ZDI-CAN-22665
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
XTest requests lets the client specify a device ID, only if none
is specified do we fall back to the XTEST special device.
As of commit
aa4074251 input: Add new hook DeviceSendEventsProc for XTEST
regular devices are no longer able to send XTest events because they
have no sendEventsProc set.
This caused issue #1574 and the crash was fixed with commit
e820030de xtest: Check whether there is a sendEventsProc to call
but we still cannot send XTest events through a specific device.
Fix this by defaulting every device to the XTest send function and
punting it to the DDX (i.e. Xwayland) to override the devices as
necessary.
Fixes e820030de2
Fixes aa4074251f
button->xkb_acts is supposed to be an array sufficiently large for all
our buttons, not just a single XkbActions struct. Allocating
insufficient memory here means when we memcpy() later in
XkbSetDeviceInfo we write into memory that wasn't ours to begin with,
leading to the usual security ooopsiedaisies.
CVE-2023-6377, ZDI-CAN-22412, ZDI-CAN-22413
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
PointerWindows[] keeps a reference to the last window our sprite
entered - changes are usually handled by CheckMotion().
If we switch between screens via XWarpPointer our
dev->spriteInfo->sprite->win is set to the new screen's root window.
If there's another window at the cursor location CheckMotion() will
trigger the right enter/leave events later. If there is not, it skips
that process and we never trigger LeaveWindow() - PointerWindows[] for
the device still refers to the previous window.
If that window is destroyed we have a dangling reference that will
eventually cause a use-after-free bug when checking the window hierarchy
later.
To trigger this, we require:
- two protocol screens
- XWarpPointer to the other screen's root window
- XDestroyWindow before entering any other window
This is a niche bug so we hack around it by making sure we reset the
PointerWindows[] entry so we cannot have a dangling pointer. This
doesn't handle Enter/Leave events correctly but the previous code didn't
either.
CVE-2023-5380, ZDI-CAN-21608
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>
This is to make sure the hardware gets the device states regardless
whether the internal state has changed or not, to overcome situations
that device LEDs are out of sync e.g. switching between VTs.
Signed-off-by: Yao Wei (魏銘廷) <yao.wei@canonical.com>
The increment sign wasn't taking into account when checking if the next
value is past our current value. The result was that for negative
increments, we kept looping indefinitely, locking up the server.
Easiest to reproduce with the evdev driver which has a negative
increment on the y axis.
Fixes 0a22502c34
dix: switch scroll button emulation to multiples of increment
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
