Jonathan Dieter posted a few patches to do this inside the Xorg
server but it makes no sense to do it there, just have the code
we use to probe the device list at startup check seat assignments
using the same code we check at hotplug time.
Bugilla: https://bugzilla.redhat.com/show_bug.cgi?id=1183654
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Acked-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Jonathan Dieter <jdieter@lesbg.com>
Signed-off-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit 697b696e5e)
If the user wants to set one of the slave devices as
the primary output, we shouldn't fail to do so,
we were returning BadMatch which was tripping up
gnome-settings-daemon and bad things ensues.
Fix all the places we use primaryOutput to work
out primaryCrtc and take it into a/c when slave
gpus are in use.
v2: review from Aaron, fix indent, unhide has_primary from
macro. I left the int vs Bool alone to be consistent with
code below, a future patch could fix both.
Signed-off-by: Dave Airlie <airlied@redhat.com>
Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit df1b401f57)
The length checking code validates PutImage height and byte width by
making sure that byte-width >= INT32_MAX / height. If height is zero,
this generates a divide by zero exception. Allow zero height requests
explicitly, bypassing the INT32_MAX check.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit dc777c346d)
Signed-off-by: Julien Cristau <jcristau@debian.org>
GetHosts saves the pointer to allocated memory in *data, and then
wants to bounds-check writes to that region, but was mistakenly using
a bare 'data' instead of '*data'. Also, data is declared as void **,
so we need a cast to turn it into a byte pointer so we can actually do
pointer comparisons.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 1559a94395)
Signed-off-by: Julien Cristau <jcristau@debian.org>
The 'n' parameter must be surrounded by parens in both places to
prevent precedence from mis-computing things.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 9802a0162f)
Signed-off-by: Julien Cristau <jcristau@debian.org>
We're using compiler compatibility settings which generate warnings
when a variable is declared after the first statement.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 61b17c0f10)
Signed-off-by: Julien Cristau <jcristau@debian.org>
When the local types used to walk the DBE request were changed, this
changed the type of the parameter passed to the DDX SwapBuffers API,
but there wasn't a matching change in the API definition.
At this point, with the API frozen, I just stuck a new variable in
with the correct type. Because we've already bounds-checked nStuff to
be smaller than UINT32_MAX / sizeof(DbeSwapInfoRec), we know it will
fit in a signed int without overflow.
Signed-off-by: Keith Packard <keithp@keithp.com
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit b20912c3d4)
Signed-off-by: Julien Cristau <jcristau@debian.org>
On a system where sizeof(unsigned) != sizeof(intptr_t), the unary
bitwise not operation will result in a mask that clears all high bits
from temp_buf in the expression:
temp_buf = (temp_buf + mask) & ~mask;
Signed-off-by: Robert Morell <rmorell@nvidia.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 7e7630bbb7)
Signed-off-by: Julien Cristau <jcristau@debian.org>
v2: Handle more multiplies in indirect_reqsize.c (Julien Cristau)
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit e883c170c1)
Signed-off-by: Julien Cristau <jcristau@debian.org>
v2:
Fix single versus vendor-private length checking for ARB_imaging subset
extensions. (Julien Cristau)
v3:
Fix single versus vendor-private length checking for ARB_imaging subset
extensions. (Julien Cristau)
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 984583a497)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 44ba149f28)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit afe177020d)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit c91e4abc3b)
Signed-off-by: Julien Cristau <jcristau@debian.org>
This is a half-measure until we start passing request length into the
varsize function, but it's better than the nothing we had before.
v2: Verify that there's at least a large render header's worth of
dataBytes (Julien Cristau)
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit a33a939e6a)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Conflicts:
glx/glxcmds.c
v2:
Fix constants in __glXMap2fReqSize (Michal Srb)
Validate w/h/d for proxy targets too (Keith Packard)
v3:
Fix Map[12]Size to correctly reject order == 0 (Julien Cristau)
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 698888e667)
Signed-off-by: Julien Cristau <jcristau@debian.org>
These are paranoid about integer overflow, and will return -1 if their
operation would overflow a (signed) integer or if either argument is
negative.
Note that RenderLarge requests are sized with a uint32_t so in principle
this could be sketchy there, but dix limits bigreqs to 128M so you
shouldn't ever notice, and honestly if you're sending more than 2G of
rendering commands you're already doing something very wrong.
v2: Use INT_MAX for consistency with the rest of the server (jcristau)
v3: Reject negative arguments (anholt)
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 2a5cbc17fc)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Without this we'd reject the request with BadLength. Note that some old
versions of Mesa had a bug in the same place, and would _send_ zero
bytes of image data; these will now be rejected, correctly.
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 13d36923e0)
Signed-off-by: Julien Cristau <jcristau@debian.org>
If the computed reply size is negative, something went wrong, treat it
as an error.
v2: Be more careful about size_t being unsigned (Matthieu Herrb)
v3: SIZE_MAX not SIZE_T_MAX (Alan Coopersmith)
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 717a1b3776)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Before this we'd just clamp the image size to 0, which was just
hideously stupid; if the parameters were such that they'd overflow an
integer, you'd allocate a small buffer, then pass huge values into (say)
ReadPixels, and now you're scribbling over arbitrary server memory.
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit ab2ba9338a)
Signed-off-by: Julien Cristau <jcristau@debian.org>
If the size computation routine returns -1 we should just reject the
request outright. Clamping it to zero could give an attacker the
opportunity to also mangle cmdlen in such a way that the subsequent
length check passes, and the request would get executed, thus passing
data we wanted to reject to the renderer.
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit 23fe7718bb)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Otherwise we may be reading outside of the client request.
Signed-off-by: Julien Cristau <jcristau@debian.org>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
(cherry picked from commit b5f9ef03df)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Multiple functions in the Xinput extension handling of requests from
clients failed to check that the length of the request sent by the
client was large enough to perform all the required operations and
thus could read or write to memory outside the bounds of the request
buffer.
This commit includes the creation of a new REQUEST_AT_LEAST_EXTRA_SIZE
macro in include/dix.h for the common case of needing to ensure a
request is large enough to include both the request itself and a
minimum amount of extra data following the request header.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 73c63afb93)
Signed-off-by: Julien Cristau <jcristau@debian.org>
ProcDbeSwapBuffers() has a 32bit (n) length value that it uses to read
from a buffer. The length is never validated, which can lead to out of
bound reads, and possibly returning the data read from out of bounds to
the misbehaving client via an X Error packet.
SProcDbeSwapBuffers() swaps data (for correct endianness) before
handing it off to the real proc. While doing the swapping, the
length field is not validated, which can cause memory corruption.
v2: reorder checks to avoid compilers optimizing out checks for overflow
that happen after we'd already have done the overflowing multiplications.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 2ef42519c4)
Signed-off-by: Julien Cristau <jcristau@debian.org>
ProcDRI2GetBuffers() tries to validate a length field (count).
There is an integer overflow in the validation. This can cause
out of bound reads and memory corruption later on.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Julien Cristau <jcristau@debian.org>
(cherry picked from commit 6692670fde)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Force use of 64-bit integers when evaluating data provided by clients
in 32-bit fields which can overflow when added or multiplied during
checks.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit e0e1164462)
Signed-off-by: Julien Cristau <jcristau@debian.org>
RegionSizeof contains several integer overflows if a large length
value is passed in. Once we fix it to return 0 on overflow, we
also have to fix the callers to handle this error condition
v2: Fixed limit calculation in RegionSizeof as pointed out by jcristau.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Reviewed-by: Julien Cristau <jcristau@debian.org>
(cherry picked from commit 97015a07b9)
Signed-off-by: Julien Cristau <jcristau@debian.org>
GetHosts() iterates over all the hosts it has in memory, and copies
them to a buffer. The buffer length is calculated by iterating over
all the hosts and adding up all of their combined length. There is a
potential integer overflow, if there are lots and lots of hosts (with
a combined length of > ~4 gig). This should be possible by repeatedly
calling ProcChangeHosts() on 64bit machines with enough memory.
This patch caps the list at 1mb, because multi-megabyte hostname
lists for X access control are insane.
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit bc8e20430b)
Signed-off-by: Julien Cristau <jcristau@debian.org>
ProcPutImage() calculates a length field from a width, left pad and depth
specified by the client (if the specified format is XYPixmap).
The calculations for the total amount of memory the server needs for the
pixmap can overflow a 32-bit number, causing out-of-bounds memory writes
on 32-bit systems (since the length is stored in a long int variable).
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit eeae42d60b)
Signed-off-by: Julien Cristau <jcristau@debian.org>
authdes_ezdecode() calls malloc() using a length provided by the
connection handshake sent by a newly connected client in order
to authenticate to the server, so should be treated as untrusted.
It didn't check if malloc() failed before writing to the newly
allocated buffer, so could lead to a server crash if the server
fails to allocate memory (up to UINT16_MAX bytes, since the len
field is a CARD16 in the X protocol).
Reported-by: Ilja Van Sprundel <ivansprundel@ioactive.com>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 90cc925c59)
Signed-off-by: Julien Cristau <jcristau@debian.org>
This function can return NULL; make sure every caller tests for that.
Reviewed-by: Adam Jackson <ajax@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit 61a292adf4)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Pageflips for Pixmap presents were not synchronized to vblank on
drivers with support for PresentCapabilityAsync, due to some
missing init for vblank->sync_flips. The PresentOptionAsync
flag was completely ignored for pageflipped presents.
Vsynced flips only worked by accident on the intel-ddx, as that
driver doesn't have PresentCapabilityAsync support.
On nouveau-ddx, which supports PresentCapabilityAsync, this
always caused non-vsynced pageflips with pretty ugly tearing.
This patch fixes the problem, as tested on top of XOrg 1.16.2
on nouveau and intel.
v4: Add additional PresentCapabilityAsync caps check, as
suggested by Eric Anholt.
Please also apply to XOrg 1.17 and XOrg 1.16.2 stable.
Applying on top of XOrg 1.16.2 requires cherry-picking
commit 2051514652
which trivially fixes lack of support for protocol option
PresentOptionCopy - get two bug fixes for the price of one!
Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit aae6460694)
Signed-off-by: Julien Cristau <jcristau@debian.org>
DebugPresent() crashed the server when a dri3 drawable
was closed while a pageflipped present was still pending,
due to vblank->window-> Null-Ptr deref, so debug builds
caused new problems to debug.
E.g.,
glXSwapBuffers(...);
glXDestroyWindow(...);
-> Pageflip for non-existent window completes -> boom.
Also often happens when switching desktop compositor on/off
due to Present unflips, or when logging out of session.
Also add info if a Present is queued for copyswap or pageflip,
if the present is vsynced, and the serial no of the Present
request, to aid debugging of pageflip and vsync issues. The
serial number is useful as Mesa's dri3/present backend encodes
its sendSBC in the serial number, so one can easily correlate
server debug output with Mesa and with the SBC values returned
to actual OpenGL client applications via OML_sync_control and
INTEL_swap_events extension, makes debugging quite a bit more
easy.
Please also cherry-pick this for a 1.16.x stable update.
Signed-off-by: Mario Kleiner <mario.kleiner.de@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit 32d3100bd7)
Signed-off-by: Julien Cristau <jcristau@debian.org>
We added this option to the present protocol before 1.0 but somehow
never implemented it in the server. It's pretty simple; just don't
ever do flips if the application specifies Copy.
Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Adam Jackson <ajax@redhat.com>
(cherry picked from commit 2051514652)
Signed-off-by: Julien Cristau <jcristau@debian.org>
Covers the current state after commits 99f0365b1f,
d0da0e9c3b, & e3aa13b8d6 were all applied.
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Reviewed-by: James Jones <jajones@nvidia.com>
Reviewed-by: Robert Morell <rmorell@nvidia.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit b09d593428)
[alanc: Modified for server-1.16-branch to show +iglx as default instead of
-iglx, to match code in os/utils.c in server-1.16-branch.]
Signed-off-by: Julien Cristau <jcristau@debian.org>
Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=54168
Fix errors introducted in 863d528a9f. Said
patch does indeed remove the problematic writes to bad memory, however
it also introduces errors in the algoritm. This patch has the effect of
reverting said patch and adding an if in the proper location to catch
the out of bounds memory write without causing problems to the overall
algorithm.
Signed-off-by: Alex Orange <crazycasta@gmail.com>
Reviewed-by: Peter Harris <pharris@opentext.com>
Tested-by: Peter Harris <pharris@opentext.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit 1b94fd7779)
