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>
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>
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>
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>
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>
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>
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>
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>
v2:
Remove can't-happen comparison for cmdlen < 0 (Michal Srb)
Reviewed-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Michal Srb <msrb@suse.com>
Reviewed-by: Andy Ritger <aritger@nvidia.com>
Signed-off-by: Julien Cristau <jcristau@debian.org>
Signed-off-by: Alan Coopersmith <alan.coopersmith@oracle.com>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
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>
drmmode_output_init() doesn't touch (the int*) num_dvi and num_hdmi.
Remove both parameters.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
We don't define HAVE_UDEV, that's a remnant from xf86-video-modesetting.
But, we have CONFIG_UDEV_KMS.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
If we don't glamor_egl_create_textured_screen_ext() in
drmmode_xf86crtc_resize() we end up with a black screen and no client
window(s) visible.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
Move the boolean glamor from struct modesetting into struct drmmode for
later re-use in drmmode_display.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
The message "removing GPU device ..." appeared even if the removal was
skipped (when path == NULL). Move it below the path check and make it a
LogMessage with config/udev prefix.
Signed-off-by: Daniel Martin <consume.noise@gmail.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
With the previous patches applied, we now have crash due to use-after-free when
a client exits without deleting all it's GL contexts
On client exit, CloseDownClient first calls glxClientCallback() with
ClientStateGone, which calls __glXFreeContext() directly.
Subsequently CloseDownClient() frees all the clients resources, which leads to
ContextGone() being called for a context resource where the context has already
been freed.
Fix this by modifiying glxClientCallback() to free the context resource.
Also make __glXFreeContext() static, as calling it directly leads to this
problem, instead the context resource should be released.
With the previous patches applied, this can be demonstrated with e.g. glxinfo,
which doesn't delete it's context before exit.
Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
Reviewed-by: Adam Jackson <ajax@redhat.com>
Some sequences of glean tests fail with GLXBadCurrentWindow when using indirect
rendering, e.g. glean -t 'fpexceptions getString'.
Flush a context which is being made non-current due to the drawable on which is
it is current going away. Waiting until another context is made current is too
late, as the drawable no longer exists.
v2: Rewrite for direct GL dispatch
v3: Inline FlushContext(), doesn't need to be a separate function
e.g. LIBGL_ALWAYS_INDIRECT=1 ./glean -r results -o --quick -t "fpexceptions
getString" fails with a BadContextTag error.
Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
Reviewed-by: Adam Jackson <ajax@redhat.com>
This reverts commit 7f5adf73a0.
This seems to miss the whole point of the idExists flag, as it makes the
lifetime of that being true the same as the lifetime of the Context resource.
The previously current context tag is always given in a MakeContextCurrent
request, even if that context tag is no longer valid (for example, the context
has been deleted), so this leads to BadContextTag errors.
See fd.o bug #30089 for the makecurrenttest.c testcase, and some discussion of
previous manifestations of this bug.
Signed-off-by: Jon TURNEY <jon.turney@dronecode.org.uk>
Reviewed-by: Adam Jackson <ajax@redhat.com>
When (long) is larger than (int), and when realloc succeeds with sizes
larger than INT_MAX, ConnectionOutput->size and ConnectionOutput->count
overflow and become negative.
When ConnectionOutput->count is negative, InsertIOV does not actually
insert an IOV, and FlushClient goes into an infinite loop of writev(fd,
iov, 0) [an empty list].
Avoid this situation by killing the client when it has more than INT_MAX
unread bytes of data.
Signed-off-by: Peter Harris <pharris@opentext.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
ChangePicture takes wire XIDs, but didn't do any Xinerama translation,
which meant setting a clip pixmap or a separate alpha picture would
result in those elements pointing at the instance of the pixmap on
screen 0. Which is, you know, bad.
v2: This one actually builds.
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49170
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
ErrorFSigSafe calls LogVMessageVerbSigSafe with the message type set to X_ERROR.
That generates this in the log:
(EE) Server terminated successfully (0). Closing log file.
People periodically report this as an error, sometimes quoting this "error"
rather than an earlier error that actually caused a problem.
v2: Use X_INFO instead of X_NOTICE
Signed-off-by: Aaron Plattner <aplattner@nvidia.com>
Reviewed-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Eric Anholt <eric@anholt.net>
Signed-off-by: Keith Packard <keithp@keithp.com>
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>
xts' XDrawArcs/15 regressed (turning into a server-side infinite loop)
after:
commit 7679afd4da
Author: Adam Jackson <ajax@redhat.com>
Date: Fri Sep 26 12:01:37 2014 -0400
mi: Fold mifpolycon.c into miarc.c
The reason is miarc.c provided its own definitions (sigh) of min/max,
that both accept int arguments and return an int. Since miFillSppPoly
uses a double (sigh) and some min-involving math for its loop index
variable, things do not go well.
Since the integer versions of min/max are redundant, nuke 'em.
Signed-off-by: Adam Jackson <ajax@redhat.com>
Reviewed-by: Aaron Plattner <aplattner@nvidia.com>
Tested-by: Keith Packard <keithp@keithp.com>
Signed-off-by: Keith Packard <keithp@keithp.com>
