Reported in #1817:
xwayland-24.1.6/redhat-linux-build/../dix/property.c:584:5:
warning[-Wanalyzer-malloc-leak]: leak of ‘pAtoms’
Can happen if, and only if, XaceHookPropertyAccess() blocks access
to all properties, so that numProps ends up zero on the second loop.
Fixes: 39f337fd4 ("dix: ProcListProperties: skip unneeded work if numProps is 0")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit ed92eae84d)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2077>
If we're swapping 4-byte integers or 2-byte integers, make sure the size
of the buffer doesn't have any bytes left over, since we won't correctly
handle those bytes.
Reported in #1817:
xwayland-24.1.6/redhat-linux-build/../dix/swaprep.c:99:22:
warning[-Wanalyzer-allocation-size]:
allocated buffer size is not a multiple of the pointee's size
xwayland-24.1.6/redhat-linux-build/../dix/swaprep.c:146:22:
warning[-Wanalyzer-allocation-size]:
allocated buffer size is not a multiple of the pointee's size
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit cf49354b60)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2077>
The wOtherInputMasks(win) macro will return NULL if
win->optional is NULL.
Reported in #1817:
xwayland-24.1.6/redhat-linux-build/../dix/gestures.c:242:9:
warning[-Wanalyzer-null-dereference]: dereference of NULL ‘inputMasks’
xwayland-24.1.6/redhat-linux-build/../dix/touch.c:765:9:
warning[-Wanalyzer-null-dereference]: dereference of NULL ‘inputMasks’
xwayland-24.1.6/redhat-linux-build/../dix/touch.c:782:9:
warning[-Wanalyzer-null-dereference]: dereference of NULL ‘inputMasks’
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 15496a5e3d)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2077>
In the doListFontsWithInfo function in dixfonts.c, when a font alias is
encountered (err == FontNameAlias), the code saves the current state
and allocates memory for c->savedName.
If the malloc(namelen + 1) call fails, c->savedName remains NULL,
but c->haveSaved is still set to TRUE. Later, when a font is
successfully resolved (err == Successful), the code uses c->savedName
without checking if it is NULL, so there is potential null ptr
dereference. XNFalloc will check result of malloc and stop
program execution if allocation was failed.
Found by Linux Verification Center (linuxtesting.org) with SVACE.
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1842
Signed-off-by: Mikhail Dmitrichenko <m.dmitrichenko222@gmail.com>
(cherry picked from commit dd5c2595a4)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2077>
The BigRequest extension allows requests larger than the 16-bit length
limit.
It uses integers for the request length and checks for the size not to
exceed the maxBigRequestSize limit, but does so after translating the
length to integer by multiplying the given size in bytes by 4.
In doing so, it might overflow the integer size limit before actually
checking for the overflow, defeating the purpose of the test.
To avoid the issue, make sure to check that the request size does not
overflow the maxBigRequestSize limit prior to any conversion.
The caller Dispatch() function however expects the return value to be in
bytes, so we cannot just return the converted value in case of error, as
that would also overflow the integer size.
To preserve the existing API, we use a negative value for the X11 error
code BadLength as the function only return positive values, 0 or -1 and
update the caller Dispatch() function to take that case into account to
return the error code to the offending client.
CVE-2025-49176
This issue was discovered by Nils Emmerich <nemmerich@ernw.de> and
reported by Julian Suleder via ERNW Vulnerability Disclosure.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Michel Dänzer <mdaenzer@redhat.com>
(cherry picked from commit 03731b326a)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2025>
This fixes a crash when we try to send focus events and dereference
FollowKeyboardWin (0x3) as WindowPtr.
A device set to XSetDeviceFocus(FollowKeyboard) is supposed to follow
the focus of the corresponding master device. During ActivateKeyboard
a slave device is detached from the master for the duration for the grab
so we don't actually have a master to follow - leaving our oldWin set to
the FollowKeyboardWin constant. This later crashes when we try to
dereference it.
Fix this by getting the current master (if any), or the saved master (if
temporarily detached due to a grab). And if failing that, use the VCK
as fallback device - that is technically wrong but it's such a niche use
case that it shouldn't matter.
Reproducer:
window = XCreateSimpleWindow(...)
deviceid = any device that is IsXExtensionKeyboard device
XSetDeviceFocus(deviceid, FollowKeyboard, ...)
XGrabDevice(deviceid, window, ...)
Fixes: f01ee198ff ("dix: don't use inputInfo.keyboard to get the focus window in ActivateKbdGrab")
Found-by: Olivier Fourdan <ofourdan@redhat.com>
Acked-by: Olivier Fourdan <ofourdan@redhat.com>
Tested-by: Olivier Fourdan <ofourdan@redhat.com>
(cherry picked from commit cab9017485)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1894>
Fix the following race:
Possible data race during read of size 8 at 0xA112510 by thread #6
Locks held: 1, at address 0x366B40
at 0x14C8B9: GetMaster (devices.c:2691)
by 0x15CFC5: IsFloating (events.c:346)
by 0x2B9554: miPointerGetScreen (mipointer.c:527)
by 0x1A5136: xf86PostButtonEventM (xf86Xinput.c:1379)
by 0x1A52BD: xf86PostButtonEvent (xf86Xinput.c:1345)
by 0x485F45B: EvdevProcessEvent (in /usr/lib64/xorg/modules/input/evdev_drv.so)
by 0x485FDAC: EvdevReadInput (in /usr/lib64/xorg/modules/input/evdev_drv.so)
by 0x195427: xf86ReadInput (xf86Events.c:247)
by 0x2CC113: InputReady (inputthread.c:180)
by 0x2CE4EA: ospoll_wait (ospoll.c:657)
by 0x2CC077: InputThreadDoWork (inputthread.c:369)
by 0x484A336: mythread_wrapper (hg_intercepts.c:406)
This conflicts with a previous write of size 8 by thread #1
Locks held: none
at 0x14D2C6: AttachDevice (devices.c:2609)
by 0x15CF85: ReattachToOldMaster (events.c:1457)
by 0x1647DD: DeactivateKeyboardGrab (events.c:1700)
by 0x25D7F1: ProcXIUngrabDevice (xigrabdev.c:169)
by 0x2552AD: ProcIDispatch (extinit.c:398)
by 0x155291: Dispatch (dispatch.c:479)
by 0x158CBA: dix_main (main.c:276)
by 0x143A3D: main (stubmain.c:34)
Address 0xa112510 is 336 bytes inside a block of size 904 alloc'd
at 0x4846571: calloc (vg_replace_malloc.c:1328)
by 0x14A0B3: AddInputDevice (devices.c:260)
by 0x1A31A0: xf86ActivateDevice (xf86Xinput.c:365)
by 0x1A4549: xf86NewInputDevice (xf86Xinput.c:948)
by 0x1A4B44: NewInputDeviceRequest (xf86Xinput.c:1090)
by 0x1B81FE: device_added (udev.c:282)
by 0x1B8516: config_udev_init (udev.c:439)
by 0x1B7091: config_init (config.c:50)
by 0x197970: InitInput (xf86Init.c:814)
by 0x158C6B: dix_main (main.c:250)
by 0x143A3D: main (stubmain.c:34)
Block was alloc'd by thread #1
The steps to trigger the race are:
1. Main thread does cleanup at mipointer.c:360 setting the slave device's
miPointerPtr to null.
2. Input thread use MIPOINTER in mipointer.c and get the slave's
miPointerPtr = null.
3. Main thread updates dev->master at devices.c:2609.
4. MIPOINTER would now return the master's miPointerPtr but the input
thread already got the slave's miPointerPtr in step 2 and segfaults by
null ptr deref.
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1260
Signed-off-by: Thomas Lindroth <thomas.lindroth@gmail.com>
(cherry picked from commit dc7cb45482)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1874>
When a device is removed while still frozen, the events queued for that
device remain while the device itself is freed.
As a result, replaying the events will cause a use after free.
To avoid the issue, make sure to dequeue and free any pending events on
a frozen device when removed.
CVE-2025-26600, ZDI-CAN-25871
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 6e0f332ba4)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1830>
CreateCursor returns a cursor with refcount 1 - that refcount is used by
the resource system, any caller needs to call RefCursor to get their own
reference. That happens correctly for normal cursors but for our
rootCursor we keep a variable to the cursor despite not having a ref for
ourselves.
Fix this by reffing/unreffing the rootCursor to ensure our pointer is
valid.
Related to CVE-2025-26594, ZDI-CAN-25544
Reviewed-by: Olivier Fourdan <ofourdan@redhat.com>
(cherry picked from commit b0a09ba602)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1830>
If a cursor reference count drops to 0, the cursor is freed.
The root cursor however is referenced with a specific global variable,
and when the root cursor is freed, the global variable may still point
to freed memory.
Make sure to prevent the rootCursor from being explicitly freed by a
client.
CVE-2025-26594, ZDI-CAN-25544
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
v2: Explicitly forbid XFreeCursor() on the root cursor (Peter Hutterer
<peter.hutterer@who-t.net>)
v3: Return BadCursor instead of BadValue (Michel Dänzer
<michel@daenzer.net>)
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Suggested-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 01642f263f)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1830>
Previously, it was looping through sizeof(ev->valuators.mask) * 8
valuators, where valuators.mask is defined as an array of
(MAX_VALUATORS + 7) / 8 entries. Since MAX_VALUATORS is defined as 36,
this made it actually loop through 40 entries. The last 4 bits in this
array should never be set, so we should never access memory outside the
bounds of the arrays defined to be exactly MAX_VALUATORS in length, but
we can make the static analyzer happier and not waste time checking bits
that should never be set.
Found by Oracle Parfait 13.3 static analyzer:
Read outside array bounds [read-outside-array-bounds]:
In array dereference of ev->valuators.data[i] with index i
Array size is 36 elements (of 8 bytes each), index >= 0 and index <= 39
at line 741 of dix/eventconvert.c in function 'eventToDeviceEvent'.
Read outside array bounds [read-outside-array-bounds]:
In array dereference of ev->valuators.data[i] with index i
Array size is 36 elements (of 8 bytes each), index >= 0 and index <= 39
at line 808 of dix/eventconvert.c in function 'eventToRawEvent'.
Read outside array bounds [read-outside-array-bounds]:
In array dereference of ev->valuators.data_raw[i] with index i
Array size is 36 elements (of 8 bytes each), index >= 0 and index <= 39
at line 809 of dix/eventconvert.c in function 'eventToRawEvent'.
Fixes: b2ba77bac ("dix: add EventToXI2 and GetXI2Type.")
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit b65eea43dd)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1767>
Found by Oracle Parfait 13.3 static analyzer:
Buffer Overflow in STD C function [buffer-overflow-call-stdc]:
Buffer overflow in call to memcpy. Buffer &bev->buttons[4] of
size 24 is written at an offset of 28
Array size is 28 bytes, index is 32
at line 743 of dix/enterleave.c in function
'DeliverStateNotifyEvent'.
Fixes: a85f0d6b9 ("Xi: fix use of button->down - bitflags instead of int arrays.")
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 4b073d65bb)
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1767>
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/1717>
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/1717>
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/1717>
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/1717>
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/1717>
The X server swapping code is a huge attack surface, much of this code
is untested and prone to security issues. The use-case of byte-swapped
clients is very niche, so allow users to disable this if they don't
need it, using either a config option or commandline flag.
For Xorg, this adds the ServerFlag "AllowByteSwappedClients" "off".
For all DDX, this adds the commandline options +byteswappedclients and
-byteswappedclients to enable or disable, respectively.
Fixes#1201
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
(cherry picked from commit 412777664a)
(cherry picked from commit af5cd5acc9012e527ee869f8e98bf6c2e9a02ca4)
Backport to server-21.1-branch modified to keep byte-swapping enabled
by default but easy to disable by users or admins (or even by distros
shipping an xorg.conf.d fragment in their packages).
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1440>
This fixes access to freed heap memory via dev->master. E.g. when
running BarrierNotify.ReceivesNotifyEvents/7 test from
xorg-integration-tests:
==24736==ERROR: AddressSanitizer: heap-use-after-free on address
0x619000065020 at pc 0x55c450e2b9cf bp 0x7fffc532fd20 sp 0x7fffc532fd10
READ of size 4 at 0x619000065020 thread T0
#0 0x55c450e2b9ce in GetMaster ../../../dix/devices.c:2722
#1 0x55c450e9d035 in IsFloating ../../../dix/events.c:346
#2 0x55c4513209c6 in GetDeviceUse ../../../Xi/xiquerydevice.c:525
../../../Xi/xichangehierarchy.c:95
#4 0x55c450e3455c in RemoveDevice ../../../dix/devices.c:1204
../../../hw/xfree86/common/xf86Xinput.c:1142
#6 0x55c450e17b04 in CloseDeviceList ../../../dix/devices.c:1038
#7 0x55c450e1de85 in CloseDownDevices ../../../dix/devices.c:1068
#8 0x55c450e837ef in dix_main ../../../dix/main.c:302
#9 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)
#11 0x55c450d0113d in _start (/usr/lib/xorg/Xorg+0x117713d)
0x619000065020 is located 160 bytes inside of 912-byte region
[0x619000064f80,0x619000065310)
freed by thread T0 here:
(/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10d7cf)
#1 0x55c450e19f1c in CloseDevice ../../../dix/devices.c:1014
#2 0x55c450e343a4 in RemoveDevice ../../../dix/devices.c:1186
../../../hw/xfree86/common/xf86Xinput.c:1142
#4 0x55c450e17b04 in CloseDeviceList ../../../dix/devices.c:1038
#5 0x55c450e1de85 in CloseDownDevices ../../../dix/devices.c:1068
#6 0x55c450e837ef in dix_main ../../../dix/main.c:302
#7 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)
previously allocated by thread T0 here:
(/usr/lib/x86_64-linux-gnu/libasan.so.5+0x10ddc6)
#1 0x55c450e1c57b in AddInputDevice ../../../dix/devices.c:259
#2 0x55c450e34840 in AllocDevicePair ../../../dix/devices.c:2755
#3 0x55c45130318f in add_master ../../../Xi/xichangehierarchy.c:152
../../../Xi/xichangehierarchy.c:465
#5 0x55c4512cb9f5 in ProcIDispatch ../../../Xi/extinit.c:390
#6 0x55c450e6a92b in Dispatch ../../../dix/dispatch.c:551
#7 0x55c450e834b7 in dix_main ../../../dix/main.c:272
#8 0x55c4517a8d93 in main ../../../dix/stubmain.c:34
(/lib/x86_64-linux-gnu/libc.so.6+0x28564)
The problem is caused by dev->master being not reset when disabling the
device, which then causes dangling pointer when the master device itself
is being deleted when exiting whole server.
Note that RecalculateMasterButtons() requires dev->master to be still
valid, so we can reset it only at the end of function.
Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
(cherry picked from commit 1801fe0ac3)
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
(cherry picked from commit 26769aa71f)
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
(cherry picked from commit bc1fdbe465)
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
(cherry picked from commit 219c54b8a3)
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
(cherry picked from commit ece23be888)
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
(cherry picked from commit 9e2ecb2af8)
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
(cherry picked from commit 0c1a93d319)
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>
(cherry picked from commit 564ccf2ce9)
The event might be a DeviceEvent allocated on the stack, in
AccessXKeyboardEvent for instance. Fixes out-of-bounds read.
Signed-off-by: Mike Gorse <mgorse@suse.com>
(cherry picked from commit 2ef5ef57bd)
Commit c7311654 cached the value of ResourceClientBits(), but that value
depends on the `MaxClients` value set either from the command line or
from the configuration file.
For the latter, a call to ResourceClientBits() is issued before the
configuration file is read, meaning that the cached value is from the
default, not from the maximum number of clients set in the configuration
file.
That obviously causes all sort of issues, including memory corruption
and crashes of the Xserver when reaching the default limit value.
To avoid that issue, also keep the LimitClient value, and recompute the
ilog2() value if that changes, as on startup when the value is set from
the the xorg.conf ServerFlags section.
v2: Drop the `cache == 0` test
Rename cache vars
Fixes: c7311654 - dix: cache ResourceClientBits() value
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1310
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
(cherry picked from commit 2efa6d6595)
This fixes an OOB read and the resulting information disclosure.
Length calculation for the request was clipped to a 32-bit integer. With
the correct stuff->num_items value the expected request size was
truncated, passing the REQUEST_FIXED_SIZE check.
The server then proceeded with reading at least stuff->num_items bytes
(depending on stuff->format) from the request and stuffing whatever it
finds into the property. In the process it would also allocate at least
stuff->num_items bytes, i.e. 4GB.
The same bug exists in ProcChangeProperty and ProcXChangeDeviceProperty,
so let's fix that too.
CVE-2022-46344, ZDI-CAN 19405
This vulnerability was discovered by:
Jan-Niklas Sohn working with Trend Micro Zero Day Initiative
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Acked-by: Olivier Fourdan <ofourdan@redhat.com>
(cherry picked from commit 8f454b793e)
GTK3 menu widget creates a selection for touch and other events and
after receiving touch events creates an async grab that excludes touch
events. Unfortunately it relies on X server not sending the touch end
event in order to function properly. Sending touch end event will cause
it to think that the initiating touch ended and when it actually ends,
the ButtonRelease event will make it think that the menu should be
closed. As a result, the menu will be open only for the duration of the
touch making it useless.
This commit reverts f682e0563f.
Fixes: https://gitlab.freedesktop.org/xorg/xserver/-/issues/1255
Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
(cherry picked from commit 43e934a19f)
When processing events we operate on InternalEvent pointers. They may
actually refer to a an instance of DeviceEvent, GestureEvent or any
other event that comprises the InternalEvent union. This works well in
practice because we always look into event type before doing anything,
except in the case of copying the event.
*dst_event = *src_event would copy whole InternalEvent event and would
cause out of bounds read in case the pointed to event was not
InternalEvent but e.g. DeviceEvent.
This regression has been introduced in
23a8b62d34.
Fixes https://gitlab.freedesktop.org/xorg/xserver/-/issues/1261
Signed-off-by: Povilas Kanapickas <povilas@radix.lt>
(cherry picked from commit 6ef5c05728)
This fixes a crash when a DeviceEvent struct converted to
InteralEvent was beeing copied as InternalEvent (and thus
causing out of bounds reads) in ActivateGrabNoDelivery()
in events.c: 3876 *grabinfo->sync.event = *real_event;
Possible fix for https://gitlab.freedesktop.org/xorg/xserver/-/issues/1253
Signed-off-by: Matthieu Herrb <matthieu@herrb.eu>
(cherry picked from commit 5b8817a019)
Adding the offset between the realloc result and the old allocation to
update pointers into the new allocation is undefined behaviour: the
old pointers are no longer valid after realloc() according to the C
standard. While this works on almost all architectures and compilers,
it causes problems on architectures that track pointer bounds (e.g.
CHERI or Arm's Morello): the DevPrivateKey pointers will still have the
bounds of the previous allocation and therefore any dereference will
result in a run-time trap.
I found this due to a crash (dereferencing an invalid capability) while
trying to run `XVnc` on a CHERI-RISC-V system. With this commit I can
successfully connect to the XVnc instance running inside a QEMU with a
VNC viewer on my host.
This also changes the check whether the allocation was moved to use
uintptr_t instead of a pointer since according to the C standard:
"The value of a pointer becomes indeterminate when the object it
points to (or just past) reaches the end of its lifetime." Casting to an
integer type avoids this undefined behaviour.
Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
(cherry picked from commit f9f705bf3c)
In some scenarios, the Wayland compositor might have more knowledge
than the X11 server and may be able to perform pointer emulation for
touch events better. Add a command-line switch to allow compositors
to turn Xwayland pointer emulation off.
Signed-off-by: Simon Ser <contact@emersion.fr>
It is copied using memcpy() and not modified so we can add const. This
fixes a -Wincompatible-pointer-types-discards-qualifiers compiler warning
that was failing a -Werror XVnc build for me.
Signed-off-by: Alex Richardson <Alexander.Richardson@cl.cam.ac.uk>
When the command line option "-terminate" is used, it could be
interesting to give it an optional grace period to let the Xserver
running for a little longer in case a new connection occurs.
This adds an optional parameter to the "-terminate" command line option
for this purpose.
v2: Use a delay in seconds instead of milliseconds
(Martin Peres <martin.peres@mupuf.org>)
v3: Clarify man page entry, ensure terminateDelay is always >= 0,
simplify TimerFree(). (Peter Hutterer <peter.hutterer@who-t.net>)
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
With Wayland compositors now being able to start Xwayland on demand, the
next logical step is to be able to stop Xwayland when there is no more
need for it.
The Xserver itself is capable of terminating itself once all X11 clients
are gone, yet in a typical full session, there are a number of X11
clients running continuously (e.g. the Xsettings daemon, IBus, etc.).
Those always-running clients will prevent the Xserver from terminating,
because the actual number of X11 clients will never drop to 0. Worse,
the X11 window manager of a Wayland compositor also counts as an X11
client, hence also preventing Xwayland from stopping.
Some compositors such as mutter use the XRes extension to query the X11
clients connected, match their PID with the actual executable name and
compare those with a list of executables that can be ignored when
deciding to kill the Xserver.
But that's not just clumsy, it is also racy, because a new X11 client
might initiate a connection the X11 server right when the compositor is
about to kill it.
To solve this issue directly at the Xserver level, this add new entries
to the XFixes extension to let the X11 clients themselves specify the
disconnect mode they expect.
Typically, those X11 daemon clients would specify the disconnect mode
XFixesClientDisconnectFlagTerminate to let the Xserver know that they
should not be accounted for when checking the remaining clients prior
to terminate.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
