The primary_ndx and approx_ndx fields from the XKM shape wire
description are used as indices into the shape->outlines[] array without
bounds checking against num_outlines.
Exploiting this (if it can be exploited) requires a malicious xkbcomp -
the path of which is built-in at compile time. There are lower-hanging
targets than trying to exploit through an XKM file.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2207>
CheckKeyActions() validates the per-key action count bytes individually
but does not verify that the computed total action data region falls
within the request buffer before advancing the wire pointer past it.
After the loop, the function calculates the final wire position as
wire + nActs * sizeof(XkbAnyAction), where nActs is the sum of per-key
action counts read from the request. The upstream length validation in
_XkbSetMapCheckLength() uses req->totalActs from the request header,
not the computed nActs. If a crafted request provides a totalActs value
that passes the length check but per-key action counts that sum to a
different nActs, the wire pointer could advance past the actual request
buffer.
The subsequent SetKeyActions() function uses memcpy to read from this
potentially out-of-bounds region, which could leak heap data or cause
a crash.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2208>
Off-by-one in rowUnder validation: the bounds check uses '>' instead
of '>=' when comparing rWire->rowUnder against section->num_rows.
Since num_rows is a count and valid indices are 0 to num_rows-1,
rowUnder == num_rows passes the check but is one past the valid range.
XkbAddGeomOverlayRow() uses this as an array index, causing an
out-of-bounds read on section->rows[].
And throw in two alloc checks while we're at it.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2208>
The bounds checks for baseColorNdx and labelColorNdx in _CheckSetGeom()
use '>' instead of '>=' when comparing against req->nColors. Since
nColors is a count and valid indices are 0 to nColors-1, an index equal
to nColors is one past the end of the array.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2208>
The primaryNdx and approxNdx fields in the shape wire description are
attacker-controlled CARD8 values from the client request. They are used
to index into the shape->outlines[] array, but were only checked against
XkbNoShape (0xff) and never validated against the actual number of
outlines (shapeWire->nOutlines).
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2208>
ProcXIPassiveUngrabDevice was missing XIGrabtypeGesturePinchBegin and
XIGrabtypeGestureSwipeBegin from its detail!=0 rejection check. The
corresponding ProcXIPassiveGrabDevice function correctly includes
these gesture types.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2197>
sproc_present_pixmap() was missing byte swaps the variable-length
xPresentNotify array after the fixed header was not
byte-swapped at all (each entry has window and serial CARD32 fields).
Fixes: a5ac3c8712 ("present: add missing byte swapping for various fields")
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2202>
Both SProcXChangeDeviceProperty() and SProcXIChangeProperty() swap the
fixed header fields (property, type, nUnits/num_items) but fail to
byte-swap the variable-length property data (CARD16 or CARD32, depending
on format) that follows the header.
Assisted-by: Claude:claude-claude-opus-4-6
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2202>
When the target device is disabled, ProcXIGrabDevice returns
AlreadyGrabbed directly as the request handler return value.
AlreadyGrabbed (1) is a grab status code, not an X error code. The
server dispatch loop interprets any non-zero return value as an X
protocol error, so the client receives BadRequest (error code 1)
instead of a proper XIGrabDevice reply with status=AlreadyGrabbed.
And use XIAlreadyGrabbed since this is an XI2 request. It's the same
value anyway.
This is the same class of bug that was fixed in ProcXIPassiveGrabDevice
by commit 'Xi: Fix XIPassiveGrab handling of keycodes > 255'
Fix by jumping to the reply path with status=AlreadyGrabbed instead of
returning the status code directly.
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2186>
This was fixed in commit 51eb63b0ee but woefully badly. Instead of returning
XIAlreadyGrabbed via the Reply, it simply returned the value from the
request handler - causing the server to interpret it as BadRequest.
Fix it and do what we intended to do instead.
Fixes: 51eb63b0ee ("Xi: disallow passive grabs with a detail > 255")
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2186>
Currently, when started fullscreen, Xwayland rootful would use a default
resolution of "640x480" and apply a viewport to match the actual output
resolution.
That's quite counter intuitive, because when started fullscreen, one
would expect the default Xwayland root size to match the logical size
of the output where it is placed, unless of course, a geometry is
explicitly specified from the command line.
Fix the default resolution to be driven from the window size instead,
even when started fullscreen, so that one can start Xwayland rootful
and fullscreen and get the optimal resolution by default.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2196>
Xwayland does not support indirect GLX contexts and enabling them will
crash the xserver.
Refuse to start if indirect GLX contexts are enabled on the command
line.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1745>
The design document hw/xfree86/doc/ddxDesign.xml states that:
| AddScreen() should only fail because of programming errors or
| failure to allocate resources (like memory).
| All configuration problems should be detected BEFORE this point.
Different command line options errors are detected in xwl_screen_init()
and can cause AddScreen() to fail, which is not compliant with the
specification.
Move all command line checks out of xwl_screen_init() in a separate
function that will take care of verifying the command line options and
bail out with meaningful error messages.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/1745>
If we fail to set up the default rules our keymap is likely going to end
up messed up, which means the client/user can't work correctly anyway.
And if we're that low on memory that we can't allocate these rules,
we're about to fall over anyway so why bother.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2184>
CHK_MASK_LEGAL expands to 'return BadValue' when the check fails and
doesn't clean up the already allocated names.keycodes, names.types, etc.
Move the check up so we don't need any cleanup code.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2184>
Use the XNF version for this and simply bail out if it fails. Clearly
this hasn't been a problem in over 20 years and I can't be bothered
finding the perfect cleanup path.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2184>
During extension init this makes sense, failing to assign a name to a
new device is more controversial but none of the paths handle
this situation correctly right now so we're just as likely to introduce
an exploit if the name remains NULL.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2184>
Add a new commmand line option to enable the Xwayland
clipboard selection bridge when running in rootful mode.
By default, clipboard selection bridge is disabled to keep the default
of having Xwayland rootful running isolated from the rest of the
applications.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2139>
To implement selection bridges, we need to be able to set the
SelectionOwner from the Xserver code.
Rather than duplicating the dix code for ProcSetSelectionOwner(), move
the code to its own dixSetSelectionOwner() function, and hook that from
the existing ProcSetSelectionOwner().
With that, a DDX can set the selection owner as intended.
This is preparation work for the following commits, no functional change
intended.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2139>
This is intended to be used to implement selection bridges in mixed
windowing systems such as Xwayland.
This adds a new SelectionBridgeCallback along with a new
SelectionBridgeInfoRec to convey the information from a selection
request so that a DDX such as Xwayland can bridge that to some other
clipboard implementation from another windowing system directly from the
DDX.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Assisted-by: Cursor AI
Assisted-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2139>
Commit 34934c37d6 restored calling register_damage() in
xwl_realize_window() before ensure_surface_for_window().
However if register_damage() succeeds and ensure_surface_for_window()
returns NULL, it would exit without "unregistering" the damage hook.
The X11 window, however, may still get damages reports, in which case
xwl_window_from_window() would return NULL, causing a NULL pointer
dereference in damage_report().
To avoid the issue, make sure we unregister the damage report if
ensure_surface_for_window() has failed, and add an early exit in
damage_report() if xwl_window is NULL.
v2: unregister_damage() unconditionally if ensure_surface_for_window()
failed (Michel Dänzer)
Fixes: commit 34934c37d6 ("revert: register damage before ensure_surface_for_window")
Closes: https://gitlab.freedesktop.org/xorg/xserver/-/work_items/1886
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2190>
SProcVidModeSwitchToMode() only byte-swapped the screen field (CARD32)
from the 52-byte xXF86VidModeSwitchToModeReq struct. All other fields
were passed to ProcVidModeSwitchToMode unswapped.
This implements full swapping, including the pre-v2 version because how
could we have lived without that for so long...
SwapRestL is not technically needed but added for consistency with other
request handlers.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2181>
The dispatch infrastructure already handles request length byte-swapping via
get_req_len() / client->req_len, so let's not double-swap the length
field back to the wrong byte order.
Co-Authored-by: Claude Code <noreply@anthropic.com>
Part-of: <https://gitlab.freedesktop.org/xorg/xserver/-/merge_requests/2181>
