Ensure that the given strings length in an XkbSetGeometry request remain
within the limits of the size of the request.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Reviewed-by: Peter Hutterer <peter.hutterer@who-t.net>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 20079c36cf)
(cherry picked from commit f160e72267)
Signed-off-by: Julien Cristau <jcristau@debian.org>
The XkbSetGeometry request embeds data which needs to be swapped when the
server and the client have different endianess.
_XkbSetGeometry() invokes functions that swap these data directly in the
input buffer.
However, ProcXkbSetGeometry() may call _XkbSetGeometry() more than once
(if there is more than one keyboard), thus causing on swapped clients the
same data to be swapped twice in memory, further causing a server crash
because the strings lengths on the second time are way off bounds.
To allow _XkbSetGeometry() to run reliably more than once with swapped
clients, do not swap the data in the buffer, use variables instead.
Signed-off-by: Olivier Fourdan <ofourdan@redhat.com>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
(cherry picked from commit 81c90dc8f0)
(cherry picked from commit 29be310c30)
Signed-off-by: Julien Cristau <jcristau@debian.org>
As a DDX may declare offload support without supporting DRI2
(because it is using an alternative acceleration mechanism like DRI3),
when iterating the list of offload_source Screens to find a matching
DRI2 provider we need to check before assuming it is DRI2 capable.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=88514
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Dave Airlie <airlied@redhat.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit 0829310148)
The CVE fix in:
commit 97015a07b9
Author: Alan Coopersmith <alan.coopersmith@oracle.com>
Date: Wed Jan 22 22:37:15 2014 -0800
dix: integer overflow in RegionSizeof() [CVE-2014-8092 3/4]
offended the C++ demons:
../../include/regionstr.h:147:45: error: invalid conversion from 'void*' to
'pixman_region16_data_t* {aka pixman_region16_data*}' [-fpermissive]
Normally this isn't a problem, because around here we have the sense and
common decency to not use C++, but this does make tigervnc fail to build,
which is a little rude of us.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Alan Coopersmith <alan.coopersmith@oracle.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
(cherry picked from commit bb23fbf5bb)
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>
