[PATCH xserver] xwayland: do not set checkRepeat on master kbd

[Olivier Fourdan]

If a key event is sent programmatically, the virtual core keyboard does
not have its dev->public.devicePrivate set and we can't get the Xwayland
seat, in which case we end up crashing in keyboard_check_repeat().

This is the case with "antimicro" which sends key events based on the
joystick buttons.

Adding the checkRepeat handler on the VCK is useless anyway, so remove
it and avoid the crash in keyboard_check_repeat() when trying to get the
Xwayland seat.

Bugzilla: https://bugzilla.redhat.com/1416244
Signed-off-by: Olivier Fourdan 
---
 v2: Avoid the crash by not setting the checkRepeat handler on the master
 keyboard - For some reason, I was convinced it was needed when I sent
 the patch for commit 239705a (xwayland: add a server sync before
 repeating keys) but on further consideration, I don't see how it 
 could be, considering we cannot get to the xwl seat anyway.
 Checked that removing it does not affect the key repeat mechanism
 when the compositor is busy, it works equally well without this so
 all is fine... My bad, thanks Peter!

 hw/xwayland/xwayland-input.c | 5 -
 1 file changed, 5 deletions(-)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 2ca99d9..1f5d323 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -1027,8 +1027,6 @@ release_relative_pointer(struct xwl_seat *xwl_seat)
 static void
 init_keyboard(struct xwl_seat *xwl_seat)
 {
-DeviceIntPtr master;
-
 xwl_seat->wl_keyboard = wl_seat_get_keyboard(xwl_seat->seat);
 wl_keyboard_add_listener(xwl_seat->wl_keyboard,
  _listener, xwl_seat);
@@ -1040,9 +1038,6 @@ init_keyboard(struct xwl_seat *xwl_seat)
 }
 EnableDevice(xwl_seat->keyboard, TRUE);
 xwl_seat->keyboard->key->xkbInfo->checkRepeat = keyboard_check_repeat;
-master = GetMaster(xwl_seat->keyboard, MASTER_KEYBOARD);
-if (master)
-master->key->xkbInfo->checkRepeat = keyboard_check_repeat;
 }
 
 static void
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xwayland: Make sure we have a focus window

[Peter Hutterer]

On Tue, Feb 28, 2017 at 02:27:52PM +0100, Olivier Fourdan wrote:
> During the InitInput() phase, the wayland events get dequeued so we
> can possibly end up calling dispatch_pointer_motion_event().
> 
> If this occurs before xwl_seat->focus_window is set, it leads to a NULL
> pointer derefence and a segfault.
> 
> Check for xwl_seat->focus_window in both pointer_handle_frame() and
> relative_pointer_handle_relative_motion() prior to calling
> dispatch_pointer_motion_event()  like it's done in
> pointer_handle_motion().
> 
> Bugzilla: https://bugzilla.redhat.com/1410804
> Signed-off-by: Olivier Fourdan 

remote: Updating patchwork state for 
https://patchwork.freedesktop.org/project/Xorg/list/
remote: I: patch #141238 updated using rev 
8c9909a99292b2fb4a86de694bb0029f61e35662.
remote: I: 1 patch(es) updated to state Accepted.
To git+ssh://git.freedesktop.org/git/xorg/xserver
   c9cbdad..8c9909a  master -> master


Cheers,
   Peter

> ---
>  hw/xwayland/xwayland-input.c | 6 ++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
> index 8435da0..9c1581f 100644
> --- a/hw/xwayland/xwayland-input.c
> +++ b/hw/xwayland/xwayland-input.c
> @@ -510,6 +510,9 @@ pointer_handle_frame(void *data, struct wl_pointer 
> *wl_pointer)
>  {
>  struct xwl_seat *xwl_seat = data;
>  
> +if (!xwl_seat->focus_window)
> +return;
> +
>  dispatch_pointer_motion_event(xwl_seat);
>  }
>  
> @@ -560,6 +563,9 @@ relative_pointer_handle_relative_motion(void *data,
>  xwl_seat->pending_pointer_event.dx_unaccel = 
> wl_fixed_to_double(dx_unaccelf);
>  xwl_seat->pending_pointer_event.dy_unaccel = 
> wl_fixed_to_double(dy_unaccelf);
>  
> +if (!xwl_seat->focus_window)
> +return;
> +
>  if (wl_proxy_get_version((struct wl_proxy *) xwl_seat->wl_pointer) < 5)
>  dispatch_pointer_motion_event(xwl_seat);
>  }
> -- 
> 2.9.3
> 
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xwayland: Do not assume we have a seat

[Peter Hutterer]

On Tue, Feb 28, 2017 at 03:46:55AM -0500, Olivier Fourdan wrote:
> Hi Peter,
> 
> > what device is this called for? quick skim of the xwayland sources indicates
> > that a device is only created if we have a seat, so I wonder if the repeat 
> > is
> > called for the wrong device here?
> 
> The keyboard_check_repeat() handler is called directly from 
> AccessXRepeatKeyExpire() [1].
> 
> It's set in init_keyboard() [2] on the seat's keyboard and on the master 
> keyboard as well.
> 
> IIRC that was needed otherwise the keyboard_check_repeat() would not avoid
> spurious key repeats in all cases when the compositor becomes "busy".
> 
> The device that causes the issue is the virtual core keyboard:

ok, but in that case we have another bug:
* init_keyboard() is called from seat_handle_capabilities()
* seat_handle_capabilities() is part of the seat listener, which rather
* strongly implies we have a seat :)

But the function pointer is set up for the master device. So from what I can
tell, this situation can only happen if we had a seat and don't have one
anymore, and the event is generated elsewhere (i.e. not through the input
device). Is this what's happening here? because if so, that suggests that
the cleanup isn't quite correct, or that the initialization isn't correct
because this shouldn't depend on whether we had a seat in the past.

I'm assuming we need keyboard_check_repeat() even if we don't have a seat,
so this suggests moving it to the right init place. Replacing it with a
ddx-specific stub that is assigned during InitKeyboardDeviceStructInternal()
seems like the correct solution.

We'll still need this patch, but only this patch alone would just paper over
the real issue.

Cheers,
   Peter

> 
> (gdb) bt
> #0  keyboard_check_repeat (dev=0x3092040, xkbi=0x30932b0, key=111)
> at xwayland-input.c:751
> #1  0x005245e7 in AccessXRepeatKeyExpire (
> timer=, now=, arg=0x3092040)
> at xkbAccessX.c:321
> #2  0x0058aa40 in DoTimer (timer=0x35fc3c0, 
> now=now@entry=68761551) at WaitFor.c:294
> #3  0x0058aab8 in DoTimers (now=68761551) at WaitFor.c:308
> #4  0x0058adf7 in check_timers () at WaitFor.c:151
> #5  WaitForSomething (are_ready=) at WaitFor.c:217
> #6  0x0055660a in Dispatch () at dispatch.c:422
> #7  0x0055a858 in dix_main (argc=10, argv=0x7fffc3408528, 
> envp=) at main.c:287
> #8  0x7fda223c7401 in __libc_start_main (main=0x423d80 , 
> argc=10, argv=0x7fffc3408528, init=, 
> fini=, rtld_fini=, 
> stack_end=0x7fffc3408518) at ../csu/libc-start.c:289
> #9  0x00423dba in _start ()
> 
> (gdb) p *dev
> $2 = {public = {devicePrivate = 0x0, 
> processInputProc = 0x52c870 , 
> realInputProc = 0x52c870 , 
> enqueueInputProc = 0x55fa10 , on = 0}, 
>   next = 0x309fff0, startup = 1, 
>   deviceProc = 0x54a820 , inited = 1, enabled = 1, 
>   coreEvents = 1, deviceGrab = {grabTime = {months = 0, 
>   milliseconds = 86921}, fromPassiveGrab = 0, implicitGrab = 0, 
> unused = 0x0, grab = 0x0, activatingKey = 0 '\000', 
> ActivateGrab = 0x566bd0 , 
> DeactivateGrab = 0x566f90 , sync = {
>   frozen = 0, state = 0, other = 0x0, event = 0x3092400}}, 
>   type = 2, xinput_type = 0, 
>   name = 0x30926a0 "Virtual core keyboard", id = 3, key = 0x30931c0, 
>   valuator = 0x0, touch = 0x0, button = 0x0, focus = 0x309f380, 
>   proximity = 0x0, kbdfeed = 0x3093240, ptrfeed = 0x0, intfeed = 0x0, 
>   stringfeed = 0x0, bell = 0x0, leds = 0x0, xkb_interest = 0x3714540, 
>   config_info = 0x0, unused_classes = 0x30926c0, saved_master_id = 0, 
>   devPrivates = 0x30923d0, unwrapProc = 0x52ae60 , 
>   spriteInfo = 0x3092398, master = 0x0, lastSlave = 0x30a0650, 
>   last = {valuators = {0 }, numValuators = 0, 
> slave = 0x30a0650, scroll = 0x0, num_touches = 0, touches = 0x0}, 
>   properties = {properties = 0x3092610, handlers = 0x3092670}, 
>   relative_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}}, 
>   scale_and_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}}, 
>   xtest_master_id = 0, idle_counter = 0x309f5d0}
> 
> Cheers,
> Olivier
> 
> 
> [1] https://cgit.freedesktop.org/xorg/xserver/tree/xkb/xkbAccessX.c#n312
> [2] 
> https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland-input.c#n1021
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [RFC] Visual Class for On-Screen HDR Drawables

[Alex Goins]

Thanks Adam. Inline -

On Fri, 10 Feb 2017, Adam Jackson wrote:

> On Thu, 2017-02-09 at 19:06 -0800, Alex Goins wrote:
> > Any input?
> 
> Inline below...
> 
> > > Would it be a major problem for automatic redirection to destroy HDR?
> > > Compositors could disable automatic redirection for DeepColor windows, and
> > > redirect themselves with preserved HDR.
> 
> It's not a problem in the sense that the actual HDR window content
> would still exist somewhere; we just need to be able to do _something_
> when an HDR window ends up in Automatic redirection. We could skip
> painting it to the parent, we could clamp to some range...

Right, as long as it's acceptable for it to be done on a best-effort basis I
think that makes sense, and if we can get away with skipping painting it
altogether in the worst case that gives us some flexibility to add new formats
later without breaking compatibility with older versions.

> > > One of the reasons why we initially went with the idea of a visual opaque 
> > > to
> > > core/Render operations is that there are other HDR encodings that have 
> > > different
> > > transfer functions from the HDR format to sRGB. It sounds like the 
> > > current train
> > > of thought with clamping to the sRGB range assumes that we are operating 
> > > only on
> > > the linear FP16 encoding of scRGB, which is highly limiting. scRGB is 
> > > useful if
> > > we are trying to composite HDR and SDR content together, but there are 
> > > other
> > > formats as well. Even the scRGB color space has support for a 12-bit 
> > > nonlinear
> > > encoding in addition to the 16-bit linear one. Android and Windows both 
> > > have
> > > support for multiple HDR formats.
> 
> Sure. Picking sRGB here is just a plausible default. What we're
> trying to cover by exposing HDR content through existing visual classes
> and imaging ops is just "do something reasonable for compatibility with
> non-HDR-aware apps". If there's some other transformation to unorm8
> that would produce better visual results along those paths then great,
> we just need to figure out how to expose that to a) application control
> b) compositor visibility. There are options there, it could vary by
> visual (although I think that's not how GLX visuals want to work), it
> could be some new bit of per-window state that we add to Fixes or XC-
> MISC or a new extension, it could be a new NETWM property...

I don't have much of an opinion on whether we should define a whole new
extension or extend an existing one, although from reading the description of
the XFIXES extension it doesn't seem like the right scope, in that we aren't
trying to eliminate a problem caused by a workaround. I hadn't considered NETWM,
that could make sense to put pressure on window managers to composite HDR
content correctly rather than relying on automatic redirection, even if most of
the info is queryable through an extension.

More on this topic below.

> > > One concern that came up internally is handling pixmaps, which specify a 
> > > depth
> > > but not a Visual. I looked a bit into Mutter and Compton, and they seem 
> > > to deal
> > > with this by querying visual properties from the root window, and 
> > > assuming there
> > > aren't any pixmap-specific parameters that would affect the texture 
> > > target.
> > > However, since the Pixmap is queried from the Window, they should be able 
> > > to get
> > > the parameters from the Visual of the Window backing the Pixmap.
> 
> I feel comfortable saying these compositors are wrong and should be
> using glXQueryDrawable to figure out the fbconfig for the window-
> pixmap; though, having said that, I'm not sure Xorg's glx would return
> something sensible.
> 
> But again, I think the colorspace is more properly a property of the
> drawable than of the fbconfig. The EGL HDR specs seem to be written in
> terms of per-drawable state rather than new configs. So we might need
> to add a new attribute for QueryDrawable; that's fine, the set of
> people involved in new GLX extension specs are all already on this
> list, if not this thread.

FYI - it's only the texture_from_pixmap texture target attributes that they base
off of the root window, e.g. GLX_TEXTURE_2D_EXT, GLX_TEXTURE_RECTANGLE_EXT,
GLX_TEXTURE_FORMAT_RBGA_EXT. They get the fbconfig from the X pixmap's depth,
along with a few other static attributes, which is still a problem for us since
with HDR we'll need more info than just depth. The texture target derivation is
still wrong behavior, however, as it should be done using FBConfig attributes.
If they want to avoid looking up FBconfig attributes every time a window is
mapped, a better implementation suggested internally would be to cache FBconfig
capabilities in a hash map or something based on the visual ID, due to the
variability of texture format attributes between different visuals/fbconfigs.

Thanks, after going back over the EGL HDR specs I see what you're getting at.
With GLX we don't really address 

Re: [RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD

[Daniel Vetter]

On Thu, Feb 02, 2017 at 09:57:20AM -0800, Eric Anholt wrote:
> Daniel Vetter  writes:
> 
> > On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote:
> >> Jani Nikula  writes:
> >> 
> >> > On Tue, 31 Jan 2017, Eric Anholt  wrote:
> >> >> Martin Peres  writes:
> >> >>
> >> >>> Despite all the careful planing of the kernel, a link may become
> >> >>> insufficient to handle the currently-set mode. At this point, the
> >> >>> kernel should mark this particular configuration as being broken
> >> >>> and potentially prune the mode before setting the offending connector's
> >> >>> link-status to BAD and send the userspace a hotplug event. This may
> >> >>> happen right after a modeset or later on.
> >> >>>
> >> >>> When available, we should use the link-status information to reset
> >> >>> the wanted mode.
> >> >>>
> >> >>> Signed-off-by: Martin Peres 
> >> >>
> >> >> If I understand this right, there are two failure modes being handled:
> >> >>
> >> >> 1) A mode that won't actually work because the link isn't good enough.
> >> >>
> >> >> 2) A mode that should work, but link parameters were too optimistic and
> >> >> if we just ask the kernel to set the mode again it'll use more
> >> >> conservative parameters that work.
> >> >>
> >> >> This patch seems good for 2).  For 1), the drmmode_set_mode_major is
> >> >> going to set our old mode back.  Won't the modeset then fail to link
> >> >> train again, and bring us back into this loop?  The only escape that I
> >> >> see would be some other userspace responding to the advertised mode list
> >> >> changing, and then asking X to modeset to something new.
> >> >>
> >> >> To avoid that failure busy loop, should we re-fetching modes at this
> >> >> point, and only re-setting if our mode still exists?
> >> >
> >> > Disclaimer: I don't know anything about the internals of the modesetting
> >> > driver.
> >> >
> >> > Perhaps we can identify the two cases now, but I'd put this more
> >> > generally: if the link status has gone bad, it's an indicator to
> >> > userspace that the circumstances may have changed, and userspace should
> >> > query the kernel for the list of available modes again. It should no
> >> > longer trust information obtained prior to getting the bad link status,
> >> > including the current mode.
> >> >
> >> > But specifically, I think you're right, and AFAICT asking for the list
> >> > of modes again is the only way for the userspace to distinguish between
> >> > the two cases. I don't think there's a shortcut for deciding the current
> >> > mode is still valid.
> >> 
> >> To avoid the busy-loop problem, I think I'd like this patch to re-query
> >> the kernel to ask about the current mode list, and only try to re-set
> >> the mode if our mode is still there.
> >
> > Yeah, I guess that sounds like a reasonable heuristics. More integrated
> > compositors (aka the wayland ones) might be able to make more useful
> > decisions, but for -modesetting that's probably the best we can do.
> >  
> >> If the mode isn't there, then it's up to the DE to take action in
> >> response to the notification of new modes.  If you don't have a DE to
> >> take appropriate action, you're kind of out of luck.
> >> 
> >> As far as the ABI goes, this seems fine to me.  The only concern I had
> >> about ABI was having to walk all the connectors on every uevent to see
> >> if any had gone bad -- couldn't we have a flag of some sort about what
> >> the uevent indicates?  But uevents should be super rare, so I'd say the
> >> kernel could go ahead with the current plan.
> >
> > We've discussed finer-grained uevent singalling a few times, and I'd like
> > that to be an orthogonal abi extension. Right now we just don't have the
> > required tracking even within the kernel to know which connector changed
> > (and the tracking we do have missed a few things, too). My idea is to push
> > the tracking into the leaves of the callchains with a function that
> > increases some per-connector epoch counter. Then we'd just have to expose
> > that somehow cheaply to userspace (could probably just send it along in
> > the uevent). Plus expose it as a read-only property so that userspace can
> > re-synchronize.
> >
> > But again, that should be an orthogonal thing imo.
> 
> Yeah, that was roughly my thought process, too.

I merged the kernel side of this new uapi:

commit 40ee6fbef75fe6452dc9e69e6f9f1a2c7808ed67
Author: Manasi Navare 
Date:   Fri Dec 16 12:29:06 2016 +0200

drm: Add a new connector atomic property for link status

Feel free to go ahead with the userspace side merging. (Yes the i915 side
isn't merged yet, but purely for "in which branch should I put it?"
technicality).

Cheers, Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
___

Re: [RFC PATCH xserver] modesetting: re-set the crtc's mode when link-status goes BAD

[Daniel Vetter]

On Tue, Feb 28, 2017 at 04:07:02AM +, Navare, Manasi D wrote:
> 
> On Fri, Feb 24, 2017 at 12:09:51PM -0800, Manasi Navare wrote:
> > Hi Daniel,
> > 
> > We have ACKs on the userspace design from both Adams and Eric.
> > Is this enough to merge the kernel patches?
> > 
> > I spoke to Eric briefly about this and he gave me a verbal r-b as well.
> > He said the userspace patches cant get merged unless DRM patches are merged.
> > 
> > So what should be some of our next steps here?
> 
> Still needs review on the kernel patches themselves, iirc Jani still had some 
> opens on that. But I was out of the loop for 2  weeks :-) -Daniel
> 
> Thanks for merging the 1st patch in the kernel series. Are there any opens on 
> the 2nd patch (the i915 patch)?
> I wanted to actually respin the 2nd patch to add reset for 
> intel_dp->lane_count  on the link training failure so that it doesn't 
> accidently retrain the link with the stale/failed values. 

Didn't look like that, and we can do this as a follow-up. I only asked
where we should merge it for best results. Let's continue that discussion
in the other thread.
-Daniel

> 
> Regards
> Manasi
> 
> > 
> > Regards
> > Manasi
> > 
> > 
> > On Mon, Feb 13, 2017 at 01:05:17PM -0800, Eric Anholt wrote:
> > > Martin Peres  writes:
> > > 
> > > > On 06/02/17 17:50, Martin Peres wrote:
> > > >> On 03/02/17 10:04, Daniel Vetter wrote:
> > > >>> On Fri, Feb 03, 2017 at 01:30:14AM +0200, Martin Peres wrote:
> > >  On 01/02/17 22:05, Manasi Navare wrote:
> > > > On Wed, Feb 01, 2017 at 11:58:16AM -0800, Eric Anholt wrote:
> > > >> Jani Nikula  writes:
> > > >>
> > > >>> On Tue, 31 Jan 2017, Eric Anholt  wrote:
> > >  Martin Peres  writes:
> > > 
> > > > Despite all the careful planing of the kernel, a link may 
> > > > become insufficient to handle the currently-set mode. At 
> > > > this point, the kernel should mark this particular 
> > > > configuration as being broken and potentially prune the 
> > > > mode before setting the offending connector's link-status 
> > > > to BAD and send the userspace a hotplug event. This may 
> > > > happen right after a modeset or later on.
> > > >
> > > > When available, we should use the link-status information 
> > > > to reset the wanted mode.
> > > >
> > > > Signed-off-by: Martin Peres 
> > >  If I understand this right, there are two failure modes 
> > >  being
> > >  handled:
> > > 
> > >  1) A mode that won't actually work because the link isn't 
> > >  good enough.
> > > 
> > >  2) A mode that should work, but link parameters were too 
> > >  optimistic and if we just ask the kernel to set the mode 
> > >  again it'll use more conservative parameters that work.
> > > 
> > >  This patch seems good for 2).  For 1), the 
> > >  drmmode_set_mode_major is going to set our old mode back.  
> > >  Won't the modeset then fail to link train again, and bring 
> > >  us back into this loop?  The only escape that I see would 
> > >  be some other userspace responding to the advertised mode 
> > >  list changing, and then asking X to modeset to something 
> > >  new.
> > > 
> > >  To avoid that failure busy loop, should we re-fetching 
> > >  modes at this point, and only re-setting if our mode still 
> > >  exists?
> > > >>> Disclaimer: I don't know anything about the internals of the 
> > > >>> modesetting driver.
> > > >>>
> > > >>> Perhaps we can identify the two cases now, but I'd put this 
> > > >>> more
> > > >>> generally: if the link status has gone bad, it's an 
> > > >>> indicator to userspace that the circumstances may have 
> > > >>> changed, and userspace should query the kernel for the list 
> > > >>> of available modes again. It should no longer trust 
> > > >>> information obtained prior to getting the bad link status, 
> > > >>> including the current mode.
> > > >>>
> > > >>> But specifically, I think you're right, and AFAICT asking 
> > > >>> for the list of modes again is the only way for the 
> > > >>> userspace to distinguish between the two cases. I don't 
> > > >>> think there's a shortcut for deciding the current mode is 
> > > >>> still valid.
> > > >> To avoid the busy-loop problem, I think I'd like this patch 
> > > >> to
> > > >> re-query
> > > >> the kernel to ask about the current mode list, and only try to 
> > > >> re-set
> > > >> the mode if our mode is still there.
> > > >>
> > > >> If the mode isn't there, then it's up to the DE to take action in
> > > >> response to the 

Re: [PATCH xserver] dispatch: Mark swapped dispatch as _X_COLD

[Eric Anholt]

Adam Jackson  writes:

> This touches everything that ends up in the Xorg binary; the big missing
> part is GLX since that's all generated code. Cuts about 14k from the
> binary on amd64.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] dix: Move {Change, Copy, Destroy}Clip from GCFuncs to Screen

[Eric Anholt]

Adam Jackson  writes:

> There are only two behaviors here. Xnest and Xdmx forward the clip list
> to the backing server, other DDXes use mi. But a layer like damage has
> to wrap everything in the GCFuncs vtable even though it really only
> cares about ValidateGC.
>
> Since none of this varies within a screen, we can move the clip
> management funcs up to the ScreenRec and eliminate the empty wrappers.

I tried to come up with a reason I would want a layer to be able to
change the funcs per-gc at runtime, and failed.  Yay for killing
boilerplate.

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

On Wed, Mar 01, 2017 at 12:15:00AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 28-02-17 23:52, Matthieu Herrb wrote:
> > 
> > On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:
> > > Hi Matthieu,
> > > 
> > > On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> > > > Provide the function definition for systems that don't have it.
> > > > 
> > > > Signed-off-by: Matthieu Herrb 
> > > > Reviewed-by: Alan Coopersmith 
> > > > ---
> > > >  configure.ac|  3 ++-
> > > >  include/dix-config.h.in |  3 +++
> > > >  include/os.h|  5 +
> > > >  os/mitauth.c|  2 +-
> > > >  os/timingsafe_memcmp.c  | 45 
> > > > +
> > > >  5 files changed, 56 insertions(+), 2 deletions(-)
> > > 
> > > > --- /dev/null
> > > > +++ b/os/timingsafe_memcmp.c
> > > Shouldn't we add this new file to Makefile.am somewhere ?
> > 
> > Hi,
> > 
> > No; AC_REPLACE_FUNCS() takes completely care of it.
> > 
> > In os/Makefile.am you have :
> > 
> > libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)
> > 
> > and LTLIBOBJS is expanded to the list of filenames corresponding to
> > functions that need to be provided in the AC_REPLACE_FUNC() macro.
> 
> What about make dist for making the source tarbals ?
> 

Check the generated makefile. It's all autotools magic too :)

COMMON_DIST = ...

-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Hans de Goede]

Hi,

On 28-02-17 23:52, Matthieu Herrb wrote:


On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:

Hi Matthieu,

On 28 February 2017 at 18:18, Matthieu Herrb  wrote:

Provide the function definition for systems that don't have it.

Signed-off-by: Matthieu Herrb 
Reviewed-by: Alan Coopersmith 
---
 configure.ac|  3 ++-
 include/dix-config.h.in |  3 +++
 include/os.h|  5 +
 os/mitauth.c|  2 +-
 os/timingsafe_memcmp.c  | 45 +
 5 files changed, 56 insertions(+), 2 deletions(-)



--- /dev/null
+++ b/os/timingsafe_memcmp.c

Shouldn't we add this new file to Makefile.am somewhere ?


Hi,

No; AC_REPLACE_FUNCS() takes completely care of it.

In os/Makefile.am you have :

libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)

and LTLIBOBJS is expanded to the list of filenames corresponding to
functions that need to be provided in the AC_REPLACE_FUNC() macro.


What about make dist for making the source tarbals ?

Regards,

Hans
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

On Tue, Feb 28, 2017 at 10:41:29PM +, Emil Velikov wrote:
> Hi Matthieu,
> 
> On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> > Provide the function definition for systems that don't have it.
> >
> > Signed-off-by: Matthieu Herrb 
> > Reviewed-by: Alan Coopersmith 
> > ---
> >  configure.ac|  3 ++-
> >  include/dix-config.h.in |  3 +++
> >  include/os.h|  5 +
> >  os/mitauth.c|  2 +-
> >  os/timingsafe_memcmp.c  | 45 +
> >  5 files changed, 56 insertions(+), 2 deletions(-)
> 
> > --- /dev/null
> > +++ b/os/timingsafe_memcmp.c
> Shouldn't we add this new file to Makefile.am somewhere ?

Hi,

No; AC_REPLACE_FUNCS() takes completely care of it.

In os/Makefile.am you have :

libos_la_LIBADD = @SHA1_LIBS@ $(DLOPEN_LIBS) $(LTLIBOBJS)

and LTLIBOBJS is expanded to the list of filenames corresponding to
functions that need to be provided in the AC_REPLACE_FUNC() macro.
-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Emil Velikov]

Hi Matthieu,

On 28 February 2017 at 18:18, Matthieu Herrb  wrote:
> Provide the function definition for systems that don't have it.
>
> Signed-off-by: Matthieu Herrb 
> Reviewed-by: Alan Coopersmith 
> ---
>  configure.ac|  3 ++-
>  include/dix-config.h.in |  3 +++
>  include/os.h|  5 +
>  os/mitauth.c|  2 +-
>  os/timingsafe_memcmp.c  | 45 +
>  5 files changed, 56 insertions(+), 2 deletions(-)

> --- /dev/null
> +++ b/os/timingsafe_memcmp.c
Shouldn't we add this new file to Makefile.am somewhere ?

-Emil
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/3] patches for Advisory X41-2017-001

[Matthieu Herrb]

On Tue, Feb 28, 2017 at 02:17:39PM -0500, Adam Jackson wrote:
> On Tue, 2017-02-28 at 19:18 +0100, Matthieu Herrb wrote:
> > Hi,
> > 
> > this patches series, already sent (and partially reviewed by) to
> > xorg-security address the X server side of Advisory X41-2017-001
> > http://marc.info/?l=oss-security=148787083023082=2
> > 
> > I've only implemented random number generation for Linux using
> > arc4random_buf() from libbsd since getting a proper, error-prone
> > wrapper around the new getrandom() is too complex for me. I'm leaving
> > this open until people get a consensus on how to implement this on
> > non-OpenBSD systems.
> 
> Totally fair. I'll spin something up for getrandom(), in the meantime
> people can install libbsd.
> 
> > Concerning the libXdmcp and libICE issues, I've double checked on my
> > Linux system that "official" X.Org builds (with build.sh) do depend
> > on libbsd and use arc4random_buf() here to generate the cookies. I
> > don't know why the versions shipped by some distros seem to still use
> > the fallback code.
> 
> If I had to guess, it's because we _have_ fallback code.

Makes sense yes.

> Typical Linux
> binary distributions will build packages in chroots that contain only
> specified dependencies, and since we allow things to build without
> libbsd, they can and do.
> 
> Once again: choice is a false virtue.
> 
> I've merged this series to master, with a trivial fix to 3/3 to also
> delete the function definitions not just their declarations. I'll
> cherry-pick them back to 1.19 shortly so they'll be included in 1.19.2.
> 
> remote: I: patch #141306 updated using rev 
> d7ac755f0b618eb1259d93c8a16ec6e39a18627c.
> remote: I: patch #141307 updated using rev 
> 957e8db38f27932d353e86e9aa69cf16778b18f1.
> remote: E: failed to find patch for rev 
> 2855f759b1e7bf7f5e57cac36c1f0d0e5ac1a683.
> remote: I: 2 patch(es) updated to state Accepted.
> To ssh://git.freedesktop.org/git/xorg/xserver
>1b12249..2855f75  master -> master

Thanks.

-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] vfb: Bump default depth to 24

[Julien Cristau]

On Mon, Feb 20, 2017 at 10:24:14 -0500, Adam Jackson wrote:

> Signed-off-by: Adam Jackson 
> ---
>  hw/vfb/InitOutput.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
Acked-by: Julien Cristau 

Cheers,
Julien
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] vfb: Bump default depth to 24

[Eric Anholt]

Adam Jackson  writes:

> On Mon, 2017-02-20 at 10:24 -0500, Adam Jackson wrote:
>> Signed-off-by: Adam Jackson 
>
> Pretty please? Nobody on earth thinks 8bpp is a reasonable default
> anymore. Should I just assume I have carte blanche to push trivial
> things like this? I'm happy to have literally anybody revert anything I
> push that causes problems if we go that route.
>
> For that matter, I'm happy to have pretty much anybody pushing code if
> they're confident in it, with the same "anybody can revert too" caveat.

I would be pretty OK with that plan, but for this patch, at least:

Reviewed-by: Eric Anholt 


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] vfb: Bump default depth to 24

[Keith Packard]

Adam Jackson  writes:

> On Mon, 2017-02-20 at 10:24 -0500, Adam Jackson wrote:
>> Signed-off-by: Adam Jackson 
>
> Pretty please? Nobody on earth thinks 8bpp is a reasonable default
> anymore.

I guess the only question is whether this will break some existing
usage, but at some point...

Acked-by: Keith Packard 

> For that matter, I'm happy to have pretty much anybody pushing code if
> they're confident in it, with the same "anybody can revert too" caveat.

Agreed -- ask forgiveness rather than permission. I like having
review/ack on patches, but for trivial stuff, that's can be a lot of
overhead.

-- 
-keith


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH 0/3] patches for Advisory X41-2017-001

[Adam Jackson]

On Tue, 2017-02-28 at 19:18 +0100, Matthieu Herrb wrote:
> Hi,
> 
> this patches series, already sent (and partially reviewed by) to
> xorg-security address the X server side of Advisory X41-2017-001
> http://marc.info/?l=oss-security=148787083023082=2
> 
> I've only implemented random number generation for Linux using
> arc4random_buf() from libbsd since getting a proper, error-prone
> wrapper around the new getrandom() is too complex for me. I'm leaving
> this open until people get a consensus on how to implement this on
> non-OpenBSD systems.

Totally fair. I'll spin something up for getrandom(), in the meantime
people can install libbsd.

> Concerning the libXdmcp and libICE issues, I've double checked on my
> Linux system that "official" X.Org builds (with build.sh) do depend
> on libbsd and use arc4random_buf() here to generate the cookies. I
> don't know why the versions shipped by some distros seem to still use
> the fallback code.

If I had to guess, it's because we _have_ fallback code. Typical Linux
binary distributions will build packages in chroots that contain only
specified dependencies, and since we allow things to build without
libbsd, they can and do.

Once again: choice is a false virtue.

I've merged this series to master, with a trivial fix to 3/3 to also
delete the function definitions not just their declarations. I'll
cherry-pick them back to 1.19 shortly so they'll be included in 1.19.2.

remote: I: patch #141306 updated using rev 
d7ac755f0b618eb1259d93c8a16ec6e39a18627c.
remote: I: patch #141307 updated using rev 
957e8db38f27932d353e86e9aa69cf16778b18f1.
remote: E: failed to find patch for rev 
2855f759b1e7bf7f5e57cac36c1f0d0e5ac1a683.
remote: I: 2 patch(es) updated to state Accepted.
To ssh://git.freedesktop.org/git/xorg/xserver
   1b12249..2855f75  master -> master

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: (timeout in ms vs. XSyncValueSubtract) Frozen client, found cause, need advise for fix

[Adam Jackson]

On Mon, 2017-02-20 at 10:55 +0100, Daniel Martin wrote:

> What happens is:
> - ServertimeBlockHandler() forwards a _big_ unsigned timeout to
> AdjustWaitForDelay()
> - AdjustWaitForDelay() takes this _big_ unsigned as int and it becomes
> a negative value

This, I think, is where things start to go wrong. There's no sense in
treating the timeout as a negative value, we should just fix that.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] vfb: Bump default depth to 24

[Adam Jackson]

On Mon, 2017-02-20 at 10:24 -0500, Adam Jackson wrote:
> Signed-off-by: Adam Jackson 

Pretty please? Nobody on earth thinks 8bpp is a reasonable default
anymore. Should I just assume I have carte blanche to push trivial
things like this? I'm happy to have literally anybody revert anything I
push that causes problems if we go that route.

For that matter, I'm happy to have pretty much anybody pushing code if
they're confident in it, with the same "anybody can revert too" caveat.

- ajax
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 3/3] auth: remove AuthToIDFunc and associated functions. Not used anymore.

[Matthieu Herrb]

And the current code for MitToId has a use-after-free() issue.

Signed-off-by: Matthieu Herrb 
Reviewed-by: Alan Coopersmith 
---
 os/auth.c  | 7 +++
 os/osdep.h | 6 --
 2 files changed, 3 insertions(+), 10 deletions(-)

diff --git a/os/auth.c b/os/auth.c
index 81164a2f5..41b625db5 100644
--- a/os/auth.c
+++ b/os/auth.c
@@ -55,7 +55,6 @@ struct protocol {
 AuthAddCFunc Add;   /* new authorization data */
 AuthCheckFunc Check;/* verify client authorization data */
 AuthRstCFunc Reset; /* delete all authorization data entries */
-AuthToIDFunc ToID;  /* convert cookie to ID */
 AuthFromIDFunc FromID;  /* convert ID to cookie */
 AuthRemCFunc Remove;/* remove a specific cookie */
 #ifdef XCSECURITY
@@ -66,7 +65,7 @@ struct protocol {
 static struct protocol protocols[] = {
 {(unsigned short) 18, "MIT-MAGIC-COOKIE-1",
  MitAddCookie, MitCheckCookie, MitResetCookie,
- MitToID, MitFromID, MitRemoveCookie,
+ MitFromID, MitRemoveCookie,
 #ifdef XCSECURITY
  MitGenerateCookie
 #endif
@@ -74,7 +73,7 @@ static struct protocol protocols[] = {
 #ifdef HASXDMAUTH
 {(unsigned short) 19, "XDM-AUTHORIZATION-1",
  XdmAddCookie, XdmCheckCookie, XdmResetCookie,
- XdmToID, XdmFromID, XdmRemoveCookie,
+ XdmFromID, XdmRemoveCookie,
 #ifdef XCSECURITY
  NULL
 #endif
@@ -83,7 +82,7 @@ static struct protocol protocols[] = {
 #ifdef SECURE_RPC
 {(unsigned short) 9, "SUN-DES-1",
  SecureRPCAdd, SecureRPCCheck, SecureRPCReset,
- SecureRPCToID, SecureRPCFromID, SecureRPCRemove,
+ SecureRPCFromID, SecureRPCRemove,
 #ifdef XCSECURITY
  NULL
 #endif
diff --git a/os/osdep.h b/os/osdep.h
index 90a247fab..a0d57b8db 100644
--- a/os/osdep.h
+++ b/os/osdep.h
@@ -113,9 +113,6 @@ typedef int (*AuthRemCFunc) (AuthRemCArgs);
 #define AuthRstCArgs void
 typedef int (*AuthRstCFunc) (AuthRstCArgs);
 
-#define AuthToIDArgs unsigned short data_length, char *data
-typedef XID (*AuthToIDFunc) (AuthToIDArgs);
-
 typedef void (*OsCloseFunc) (ClientPtr);
 
 typedef int (*OsFlushFunc) (ClientPtr who, struct _osComm * oc, char *extraBuf,
@@ -185,7 +182,6 @@ extern void GenerateRandomData(int len, char *buf);
 /* in mitauth.c */
 extern XID MitCheckCookie(AuthCheckArgs);
 extern XID MitGenerateCookie(AuthGenCArgs);
-extern XID MitToID(AuthToIDArgs);
 extern int MitAddCookie(AuthAddCArgs);
 extern int MitFromID(AuthFromIDArgs);
 extern int MitRemoveCookie(AuthRemCArgs);
@@ -194,7 +190,6 @@ extern int MitResetCookie(AuthRstCArgs);
 /* in xdmauth.c */
 #ifdef HASXDMAUTH
 extern XID XdmCheckCookie(AuthCheckArgs);
-extern XID XdmToID(AuthToIDArgs);
 extern int XdmAddCookie(AuthAddCArgs);
 extern int XdmFromID(AuthFromIDArgs);
 extern int XdmRemoveCookie(AuthRemCArgs);
@@ -205,7 +200,6 @@ extern int XdmResetCookie(AuthRstCArgs);
 #ifdef SECURE_RPC
 extern void SecureRPCInit(AuthInitArgs);
 extern XID SecureRPCCheck(AuthCheckArgs);
-extern XID SecureRPCToID(AuthToIDArgs);
 extern int SecureRPCAdd(AuthAddCArgs);
 extern int SecureRPCFromID(AuthFromIDArgs);
 extern int SecureRPCRemove(AuthRemCArgs);
-- 
2.11.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 2/3] Use arc4random_buf(3) if available to generate cookies.

[Matthieu Herrb]

Signed-off-by: Matthieu Herrb 
---
 configure.ac| 2 ++
 include/dix-config.h.in | 6 ++
 os/auth.c   | 7 +++
 3 files changed, 15 insertions(+)

diff --git a/configure.ac b/configure.ac
index f6a49302f..6a7c4cc6f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -225,6 +225,8 @@ AC_REPLACE_FUNCS([reallocarray strcasecmp strcasestr 
strlcat strlcpy strndup\
timingsafe_memcmp])
 AM_CONDITIONAL(POLL, [test "x$ac_cv_func_poll" = "xyes"])
 
+AC_CHECK_LIB([bsd], [arc4random_buf])
+
 AC_CHECK_DECLS([program_invocation_short_name], [], [], [[#include ]])
 
 dnl Check for SO_PEERCRED #define
diff --git a/include/dix-config.h.in b/include/dix-config.h.in
index 4b86c1a3c..d357910a6 100644
--- a/include/dix-config.h.in
+++ b/include/dix-config.h.in
@@ -125,6 +125,9 @@
 /* Build a standalone xpbproxy */
 #undef STANDALONE_XPBPROXY
 
+/* Define to 1 if you have the `bsd' library (-lbsd). */
+#undef HAVE_LIBBSD
+
 /* Define to 1 if you have the `m' library (-lm). */
 #undef HAVE_LIBM
 
@@ -161,6 +164,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_RPCSVC_DBM_H
 
+/* Define to 1 if you have the `arc4random_buf' function. */
+#undef HAVE_ARC4RANDOM_BUF
+
 /* Define to use libc SHA1 functions */
 #undef HAVE_SHA1_IN_LIBC
 
diff --git a/os/auth.c b/os/auth.c
index 7da6fc6ed..81164a2f5 100644
--- a/os/auth.c
+++ b/os/auth.c
@@ -45,6 +45,9 @@ from The Open Group.
 #ifdef WIN32
 #include
 #endif
+#ifdef HAVE_LIBBSD
+#include  /* for arc4random_buf() */
+#endif
 
 struct protocol {
 unsigned short name_length;
@@ -303,11 +306,15 @@ GenerateAuthorization(unsigned name_length,
 void
 GenerateRandomData(int len, char *buf)
 {
+#ifdef HAVE_ARC4RANDOMBUF
+arc4random_buf(buf, len);
+#else
 int fd;
 
 fd = open("/dev/urandom", O_RDONLY);
 read(fd, buf, len);
 close(fd);
+#endif
 }
 
 #endif  /* XCSECURITY */
-- 
2.11.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 1/3] Use timingsafe_memcmp() to compare MIT-MAGIC-COOKIES CVE-2017-2624

[Matthieu Herrb]

Provide the function definition for systems that don't have it.

Signed-off-by: Matthieu Herrb 
Reviewed-by: Alan Coopersmith 
---
 configure.ac|  3 ++-
 include/dix-config.h.in |  3 +++
 include/os.h|  5 +
 os/mitauth.c|  2 +-
 os/timingsafe_memcmp.c  | 45 +
 5 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/configure.ac b/configure.ac
index 4dcf8b5c2..f6a49302f 100644
--- a/configure.ac
+++ b/configure.ac
@@ -221,7 +221,8 @@ AC_CHECK_FUNCS([backtrace ffs geteuid getuid issetugid 
getresuid \
mmap posix_fallocate seteuid shmctl64 strncasecmp vasprintf vsnprintf \
walkcontext setitimer poll epoll_create1])
 AC_CONFIG_LIBOBJ_DIR([os])
-AC_REPLACE_FUNCS([reallocarray strcasecmp strcasestr strlcat strlcpy strndup])
+AC_REPLACE_FUNCS([reallocarray strcasecmp strcasestr strlcat strlcpy strndup\
+   timingsafe_memcmp])
 AM_CONDITIONAL(POLL, [test "x$ac_cv_func_poll" = "xyes"])
 
 AC_CHECK_DECLS([program_invocation_short_name], [], [], [[#include ]])
diff --git a/include/dix-config.h.in b/include/dix-config.h.in
index 4f020e5d8..4b86c1a3c 100644
--- a/include/dix-config.h.in
+++ b/include/dix-config.h.in
@@ -238,6 +238,9 @@
 /* Define to 1 if you have the  header file. */
 #undef HAVE_SYS_UTSNAME_H
 
+/* Define to 1 if you have the `timingsafe_memcmp' function. */
+#undef HAVE_TIMINGSAFE_MEMCMP
+
 /* Define to 1 if you have the  header file. */
 #undef HAVE_TSLIB_H
 
diff --git a/include/os.h b/include/os.h
index d2c41b402..aa231f550 100644
--- a/include/os.h
+++ b/include/os.h
@@ -590,6 +590,11 @@ extern _X_EXPORT char *
 strndup(const char *str, size_t n);
 #endif
 
+#ifndef HAVE_TIMINGSAFE_MEMCMP
+extern _X_EXPORT int
+timingsafe_memcmp(const void *b1, const void *b2, size_t len);
+#endif
+
 /* Logging. */
 typedef enum _LogParameter {
 XLOG_FLUSH,
diff --git a/os/mitauth.c b/os/mitauth.c
index 768a52a22..efae4404c 100644
--- a/os/mitauth.c
+++ b/os/mitauth.c
@@ -76,7 +76,7 @@ MitCheckCookie(unsigned short data_length,
 
 for (auth = mit_auth; auth; auth = auth->next) {
 if (data_length == auth->len &&
-memcmp(data, auth->data, (int) data_length) == 0)
+timingsafe_memcmp(data, auth->data, (int) data_length) == 0)
 return auth->id;
 }
 *reason = "Invalid MIT-MAGIC-COOKIE-1 key";
diff --git a/os/timingsafe_memcmp.c b/os/timingsafe_memcmp.c
new file mode 100644
index 0..36ab362a7
--- /dev/null
+++ b/os/timingsafe_memcmp.c
@@ -0,0 +1,45 @@
+/*
+ * Copyright (c) 2014 Google Inc.
+ *
+ * Permission to use, copy, modify, and distribute this software for any
+ * purpose with or without fee is hereby granted, provided that the above
+ * copyright notice and this permission notice appear in all copies.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES
+ * WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF
+ * MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR
+ * ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES
+ * WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN
+ * ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF
+ * OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.
+ */
+
+#include 
+#include 
+
+int
+timingsafe_memcmp(const void *b1, const void *b2, size_t len)
+{
+const unsigned char *p1 = b1, *p2 = b2;
+size_t i;
+int res = 0, done = 0;
+
+for (i = 0; i < len; i++) {
+/* lt is -1 if p1[i] < p2[i]; else 0. */
+int lt = (p1[i] - p2[i]) >> CHAR_BIT;
+
+/* gt is -1 if p1[i] > p2[i]; else 0. */
+int gt = (p2[i] - p1[i]) >> CHAR_BIT;
+
+/* cmp is 1 if p1[i] > p2[i]; -1 if p1[i] < p2[i]; else 0. */
+int cmp = lt - gt;
+
+/* set res = cmp if !done. */
+res |= cmp & ~done;
+
+/* set done if p1[i] != p2[i]. */
+done |= lt | gt;
+}
+
+return (res);
+}
-- 
2.11.1

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

[PATCH 0/3] patches for Advisory X41-2017-001

[Matthieu Herrb]

Hi,

this patches series, already sent (and partially reviewed by) to
xorg-security address the X server side of Advisory X41-2017-001
http://marc.info/?l=oss-security=148787083023082=2

I've only implemented random number generation for Linux using
arc4random_buf() from libbsd since getting a proper, error-prone
wrapper around the new getrandom() is too complex for me. I'm leaving
this open until people get a consensus on how to implement this on
non-OpenBSD systems.

Concerning the libXdmcp and libICE issues, I've double checked on my
Linux system that "official" X.Org builds (with build.sh) do depend
on libbsd and use arc4random_buf() here to generate the cookies. I
don't know why the versions shipped by some distros seem to still use
the fallback code.


-- 
Matthieu Herrb


signature.asc
Description: PGP signature
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Fwd: [oss-security] Advisory X41-2017-001: Multiple Vulnerabilities in X.org

[Alan Coopersmith]

FYI, since this has gone public now, after several weeks of discussion
on the xorg-security list.

-alan-


 Forwarded Message 
Subject: [oss-security] Advisory X41-2017-001: Multiple Vulnerabilities in X.org
Date: Tue, 28 Feb 2017 14:39:18 +0100
From: X41 D-Sec GmbH Advisories 
Reply-To: oss-secur...@lists.openwall.com
Organization: X41 D-Sec GmbH
To: oss-secur...@lists.openwall.com


X41 D-Sec GmbH Security Advisory: X41-2017-001

Multiple Vulnerabilities in X.org
=

Overview

Vendor: X.org/Freedesktop.org
Vendor URL: https://www.x.org/wiki/
Credit: X41 D-Sec GmbH, Eric Sesterhenn
Advisory-URL: https://www.x41-dsec.de/lab/advisories/x41-2017-001-xorg/
Status: Public


Timing attack against MIT Cookie

Vulnerability Type: Other
Affected Products: Xorg Server
Attack Type: Local
Impact: Escalation of Privileges
Severity Rating: low
Confirmed Affected Version: 1.19.0 and lower
Confirmed Patched Version: -
Vector: local
CVE: CVE-2017-2624
CVSS Score: 5.9
CVSS Vector: CVSS:3.0/AV:L/AC:H/PR:N/UI:N/S:C/C:H/I:N/A:N


Summary and Impact
--
The xorg-server uses memcmp() to check the received MIT cookie against a
series of valid cookies. If the cookie is correct, it is allowed to
attach to the Xorg session:

XID
MitCheckCookie(unsigned short data_length,
   const char *data, ClientPtr client, const char **reason)
{
struct auth *auth;

for (auth = mit_auth; auth; auth = auth->next) {
if (data_length == auth->len &&
memcmp(data, auth->data, (int) data_length) == 0)
return auth->id;
}
*reason = "Invalid MIT-MAGIC-COOKIE-1 key";
return (XID) -1;
}

Since most memcmp() implementations return after an invalid byte is
seen, this causes a time difference between a valid and invalid byte,
which in theory could allow an efficient brute force attack[1].

Analysis

X41 was not able to measure a significant difference using the optimised
memcmp() version of a standard Linux system, but for a naive
implementation consisting of a loop comparing the bytes. Since timing
attacks against memcmp() have been successful in the past [2] and fixed
elsewhere [3][4] X41 would consider this an issue. If this would be
exploited, it would allow a local attacker to run code in the Xorg
session of another user.

In order to prevent this, MIT-COOKIES should be removed or a memcmp()
similar to timingsafe_memcmp()[5] used. Other projects (e.g. openssl)
use timing safe memcmp() implementations to compare cookies retrieved
via the network[6].

Workaround
--

None

References
--

[1]
https://cryptocoding.net/index.php/Coding_rules#Compare_secret_strings_in_constant_time
[2]
http://de.slideshare.net/cisoplatform7/defcon-22paulmcmillanattackingtheiotusingtimingattac
[3] http://seb.dbzteam.org/crypto/python-oauth-timing-hmac.pdf
[4] https://bugs.ruby-lang.org/issues/10098
[5]
http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/lib/libc/string/timingsafe_memcmp.c
[6] https://github.com/openssl/openssl/blob/master/ssl/t1_lib.c#L1249





Potential Use after Free in Xorg Server
===
Vulnerability Type: Other
Affected Products: Xorg Server
Attack Type: Local
Impact: -   
Severity Rating: none
Confirmed Affected Version: 1.19.0 and lower
Confirmed Patched Version:
Vector: local
CVE: -
CVSS Score: -
CVSS Vector: -

Summary and Impact
--

In XDM is a (currently non security) issue, regarding a potential use
after free.

The ToID() function in os/auth.c is not used anywhere, just defined in
the struct and filled by the protocols, but there are no users.

AuthToIDFunc ToID;  /* convert cookie to ID */

X41 noticed that, XdmToID() frees the cookie argument in case it can
resolve the ID or on failure, but not if it can't allocate memory for plain:

XdmToID(unsigned short cookie_length, char *cookie)
{
XdmAuthorizationPtr auth;
XdmClientAuthPtr client;
unsigned char *plain;

plain = malloc(cookie_length);
if (!plain)
return (XID) -1;
for (auth = xdmAuth; auth; auth = auth->next) {
XdmcpUnwrap((unsigned char *) cookie, (unsigned char *) 
>key,
plain, cookie_length);
if ((client =
 XdmAuthorizationValidate(plain, cookie_length, >rho,
NULL,
  NULL)) != NULL) {
free(client);
free(cookie);
free(plain);
return auth->id;
}
}
free(cookie);
free(plain);
return (XID) -1;
}

The same return value is 

[PATCH xserver] xwayland: Make sure we have a focus window

[Olivier Fourdan]

During the InitInput() phase, the wayland events get dequeued so we
can possibly end up calling dispatch_pointer_motion_event().

If this occurs before xwl_seat->focus_window is set, it leads to a NULL
pointer derefence and a segfault.

Check for xwl_seat->focus_window in both pointer_handle_frame() and
relative_pointer_handle_relative_motion() prior to calling
dispatch_pointer_motion_event()  like it's done in
pointer_handle_motion().

Bugzilla: https://bugzilla.redhat.com/1410804
Signed-off-by: Olivier Fourdan 
---
 hw/xwayland/xwayland-input.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/hw/xwayland/xwayland-input.c b/hw/xwayland/xwayland-input.c
index 8435da0..9c1581f 100644
--- a/hw/xwayland/xwayland-input.c
+++ b/hw/xwayland/xwayland-input.c
@@ -510,6 +510,9 @@ pointer_handle_frame(void *data, struct wl_pointer 
*wl_pointer)
 {
 struct xwl_seat *xwl_seat = data;
 
+if (!xwl_seat->focus_window)
+return;
+
 dispatch_pointer_motion_event(xwl_seat);
 }
 
@@ -560,6 +563,9 @@ relative_pointer_handle_relative_motion(void *data,
 xwl_seat->pending_pointer_event.dx_unaccel = 
wl_fixed_to_double(dx_unaccelf);
 xwl_seat->pending_pointer_event.dy_unaccel = 
wl_fixed_to_double(dy_unaccelf);
 
+if (!xwl_seat->focus_window)
+return;
+
 if (wl_proxy_get_version((struct wl_proxy *) xwl_seat->wl_pointer) < 5)
 dispatch_pointer_motion_event(xwl_seat);
 }
-- 
2.9.3

___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel

Re: [PATCH xserver] xwayland: Do not assume we have a seat

[Olivier Fourdan]

Hi Peter,

> what device is this called for? quick skim of the xwayland sources indicates
> that a device is only created if we have a seat, so I wonder if the repeat is
> called for the wrong device here?

The keyboard_check_repeat() handler is called directly from 
AccessXRepeatKeyExpire() [1].

It's set in init_keyboard() [2] on the seat's keyboard and on the master 
keyboard as well.

IIRC that was needed otherwise the keyboard_check_repeat() would not avoid 
spurious key repeats in all cases when the compositor becomes "busy".

The device that causes the issue is the virtual core keyboard:

(gdb) bt
#0  keyboard_check_repeat (dev=0x3092040, xkbi=0x30932b0, key=111)
at xwayland-input.c:751
#1  0x005245e7 in AccessXRepeatKeyExpire (
timer=, now=, arg=0x3092040)
at xkbAccessX.c:321
#2  0x0058aa40 in DoTimer (timer=0x35fc3c0, 
now=now@entry=68761551) at WaitFor.c:294
#3  0x0058aab8 in DoTimers (now=68761551) at WaitFor.c:308
#4  0x0058adf7 in check_timers () at WaitFor.c:151
#5  WaitForSomething (are_ready=) at WaitFor.c:217
#6  0x0055660a in Dispatch () at dispatch.c:422
#7  0x0055a858 in dix_main (argc=10, argv=0x7fffc3408528, 
envp=) at main.c:287
#8  0x7fda223c7401 in __libc_start_main (main=0x423d80 , 
argc=10, argv=0x7fffc3408528, init=, 
fini=, rtld_fini=, 
stack_end=0x7fffc3408518) at ../csu/libc-start.c:289
#9  0x00423dba in _start ()

(gdb) p *dev
$2 = {public = {devicePrivate = 0x0, 
processInputProc = 0x52c870 , 
realInputProc = 0x52c870 , 
enqueueInputProc = 0x55fa10 , on = 0}, 
  next = 0x309fff0, startup = 1, 
  deviceProc = 0x54a820 , inited = 1, enabled = 1, 
  coreEvents = 1, deviceGrab = {grabTime = {months = 0, 
  milliseconds = 86921}, fromPassiveGrab = 0, implicitGrab = 0, 
unused = 0x0, grab = 0x0, activatingKey = 0 '\000', 
ActivateGrab = 0x566bd0 , 
DeactivateGrab = 0x566f90 , sync = {
  frozen = 0, state = 0, other = 0x0, event = 0x3092400}}, 
  type = 2, xinput_type = 0, 
  name = 0x30926a0 "Virtual core keyboard", id = 3, key = 0x30931c0, 
  valuator = 0x0, touch = 0x0, button = 0x0, focus = 0x309f380, 
  proximity = 0x0, kbdfeed = 0x3093240, ptrfeed = 0x0, intfeed = 0x0, 
  stringfeed = 0x0, bell = 0x0, leds = 0x0, xkb_interest = 0x3714540, 
  config_info = 0x0, unused_classes = 0x30926c0, saved_master_id = 0, 
  devPrivates = 0x30923d0, unwrapProc = 0x52ae60 , 
  spriteInfo = 0x3092398, master = 0x0, lastSlave = 0x30a0650, 
  last = {valuators = {0 }, numValuators = 0, 
slave = 0x30a0650, scroll = 0x0, num_touches = 0, touches = 0x0}, 
  properties = {properties = 0x3092610, handlers = 0x3092670}, 
  relative_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}}, 
  scale_and_transform = {m = {{1, 0, 0}, {0, 1, 0}, {0, 0, 1}}}, 
  xtest_master_id = 0, idle_counter = 0x309f5d0}

Cheers,
Olivier


[1] https://cgit.freedesktop.org/xorg/xserver/tree/xkb/xkbAccessX.c#n312
[2] 
https://cgit.freedesktop.org/xorg/xserver/tree/hw/xwayland/xwayland-input.c#n1021
___
xorg-devel@lists.x.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: https://lists.x.org/mailman/listinfo/xorg-devel