From e7b84ca46944895971a8f048c7e34869b7de01c0 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 5 Mar 2014 16:41:14 +0100 Subject: [PATCH 1/8] Xorg: Add a suid root wrapper With the recent systemd-logind changes it is possible to install the Xorg binary without suid root rights and still have everything working as it should *if* the user only has cards which are supported by kms. This commit adds a little suid root wrapper, which is a bit weird, first we strip the suid-root bit of the Xorg binary, and then we add a wrapper ? The function of this wrapper is to see if a system still needs root-rights, if it does not (it supports kms and the kms drivers are properly loaded), then it will immediately drop all elevated rights before executing the real Xorg binary. If it finds (some) cards which don't support kms, or no cards at all, then it will execute the Xorg server with elevated rights so that ie the nvidia binary driver and the vesa driver can keep working normally. To make it possible for security concious users who don't need the root rights to completely remove the wrapper, Xorg is started in a 3 step process when the wrapper is enabled during build time: 1) A simple shell script which checks if the wrapper is there, if it is it executes the wrapper, if not it directly executes the real Xorg binary 2) The wrapper gets executed, does its checks, normally drops all elevated rights and then executes the real Xorg binary 3) The real Xorg binary does its thing This allows distributions to put the wrapper binary in a separate package, and will allow users to remove this package. IE the plan with Fedora is to make "legacy" drivers depend on the wrapper pkg, and since our default install contains some legacy drivers it will be part of the default install, but users can later yum remove it (which will also automatically remove the legacy driver packages as those won't work without it anyways). The wrapper is loosely modelled after the existing Debian Xwrapper, it uses the same config-file + config-file format, and also allows restricting Xserver execution (through the wrapper) to console users only. There also is a new needs_root_rights config file directive, which can be used to override the auto-detection the wrapper does. Hopefully this will allow Debian to replace their own wrapper with this upstream one. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- configure.ac | 14 +- hw/xfree86/Makefile.am | 15 +- hw/xfree86/Xorg.sh.in | 11 ++ hw/xfree86/man/Makefile.am | 5 + hw/xfree86/man/Xorg.wrap.man | 67 +++++++++ hw/xfree86/man/Xwrapper.config.man | 1 + hw/xfree86/xorg-wrapper.c | 231 +++++++++++++++++++++++++++++ include/dix-config.h.in | 6 + include/xwin-config.h.in | 3 - manpages.am | 1 + 10 files changed, 348 insertions(+), 6 deletions(-) create mode 100644 hw/xfree86/Xorg.sh.in create mode 100644 hw/xfree86/man/Xorg.wrap.man create mode 100644 hw/xfree86/man/Xwrapper.config.man create mode 100644 hw/xfree86/xorg-wrapper.c diff --git a/configure.ac b/configure.ac index 162c0cf3c..749fa84f0 100644 --- a/configure.ac +++ b/configure.ac @@ -627,6 +627,7 @@ AC_ARG_ENABLE(pciaccess, AS_HELP_STRING([--enable-pciaccess], [Build Xorg with p AC_ARG_ENABLE(linux_acpi, AS_HELP_STRING([--disable-linux-acpi], [Disable building ACPI support on Linux (if available).]), [enable_linux_acpi=$enableval], [enable_linux_acpi=yes]) AC_ARG_ENABLE(linux_apm, AS_HELP_STRING([--disable-linux-apm], [Disable building APM support on Linux (if available).]), [enable_linux_apm=$enableval], [enable_linux_apm=yes]) AC_ARG_ENABLE(systemd-logind, AC_HELP_STRING([--enable-systemd-logind], [Build systemd-logind support (default: auto)]), [SYSTEMD_LOGIND=$enableval], [SYSTEMD_LOGIND=auto]) +AC_ARG_ENABLE(suid-wrapper, AC_HELP_STRING([--enable-suid-wrapper], [Build suid-root wrapper for legacy driver support on rootless xserver systems (default: no)]), [SUID_WRAPPER=$enableval], [SUID_WRAPPER=no]) dnl DDXes. AC_ARG_ENABLE(xorg, AS_HELP_STRING([--enable-xorg], [Build Xorg server (default: auto)]), [XORG=$enableval], [XORG=auto]) @@ -924,6 +925,16 @@ if test "x$SYSTEMD_LOGIND" = xyes; then fi AM_CONDITIONAL(SYSTEMD_LOGIND, [test "x$SYSTEMD_LOGIND" = xyes]) +if test "x$SUID_WRAPPER" = xyes; then + dnl The wrapper uses libdrm headers, so ensure they are available + PKG_CHECK_MODULES([LIBDRM], $LIBDRM) + dnl This is a define so that if some platforms want to put the wrapper + dnl somewhere else this can be easily changed + AC_DEFINE_DIR(SUID_WRAPPER_DIR, libexecdir, [Where to install Xorg.bin and Xorg.wrap]) + SETUID="no" +fi +AM_CONDITIONAL(SUID_WRAPPER, [test "x$SUID_WRAPPER" = xyes]) + if test "x$NEED_DBUS" = xyes; then AC_DEFINE(NEED_DBUS, 1, [Enable D-Bus core]) fi @@ -2116,7 +2127,6 @@ fi AC_MSG_RESULT([$XWIN]) if test "x$XWIN" = xyes; then - AC_DEFINE_DIR(SYSCONFDIR, sysconfdir, [Location of system.XWinrc]) AC_DEFINE_DIR(DEFAULT_LOGDIR, logdir, [Default log location]) AC_DEFINE_UNQUOTED(XORG_VERSION_CURRENT, [$VENDOR_RELEASE], [Current Xorg version]) AC_DEFINE_UNQUOTED(__VENDORDWEBSUPPORT__, ["$VENDOR_WEB"], [Vendor web address for support]) @@ -2440,6 +2450,7 @@ AC_COMPILE_IFELSE([AC_LANG_PROGRAM([[ [ Enable GNU and other extensions to the C environment for glibc])]) AC_DEFINE_DIR(PROJECTROOT, prefix, [Overall prefix]) +AC_DEFINE_DIR(SYSCONFDIR, sysconfdir, [sysconfdir]) AC_SUBST([RELEASE_DATE]) BUILD_DATE="`date +'%Y%m%d'`" @@ -2499,6 +2510,7 @@ dri3/Makefile present/Makefile hw/Makefile hw/xfree86/Makefile +hw/xfree86/Xorg.sh hw/xfree86/common/Makefile hw/xfree86/common/xf86Build.h hw/xfree86/ddc/Makefile diff --git a/hw/xfree86/Makefile.am b/hw/xfree86/Makefile.am index 73e1b4c1f..a315bbc17 100644 --- a/hw/xfree86/Makefile.am +++ b/hw/xfree86/Makefile.am @@ -81,9 +81,15 @@ Xorg_DEPENDENCIES = $(LOCAL_LIBS) Xorg_LDFLAGS = $(LD_EXPORT_SYMBOLS_FLAG) +if SUID_WRAPPER +wrapdir = $(SUID_WRAPPER_DIR) +wrap_PROGRAMS = Xorg.wrap +Xorg_wrap_SOURCES = xorg-wrapper.c +endif + BUILT_SOURCES = xorg.conf.example DISTCLEANFILES = xorg.conf.example -EXTRA_DIST = xorgconf.cpp +EXTRA_DIST = xorgconf.cpp Xorg.sh.in # Without logdir, X will post an error on the terminal and will not start install-data-local: @@ -98,6 +104,11 @@ if INSTALL_SETUID chown root $(DESTDIR)$(bindir)/Xorg chmod u+s $(DESTDIR)$(bindir)/Xorg endif +if SUID_WRAPPER + mv $(DESTDIR)$(bindir)/Xorg $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.bin + ${INSTALL} -m 755 Xorg.sh $(DESTDIR)$(bindir)/Xorg + -chown root $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.wrap && chmod u+s $(DESTDIR)$(SUID_WRAPPER_DIR)/Xorg.wrap +endif uninstall-local: if CYGWIN @@ -119,7 +130,7 @@ xorg.conf.example: xorgconf.cpp relink: $(AM_V_at)rm -f Xorg$(EXEEXT) && $(MAKE) Xorg$(EXEEXT) -CLEANFILES = sdksyms.c sdksyms.dep +CLEANFILES = sdksyms.c sdksyms.dep Xorg.sh EXTRA_DIST += sdksyms.sh sdksyms.dep sdksyms.c: sdksyms.sh diff --git a/hw/xfree86/Xorg.sh.in b/hw/xfree86/Xorg.sh.in new file mode 100644 index 000000000..cef4859c8 --- /dev/null +++ b/hw/xfree86/Xorg.sh.in @@ -0,0 +1,11 @@ +#!/bin/sh +# +# Execute Xorg.wrap if it exists otherwise execute Xorg.bin directly. +# This allows distros to put the suid wrapper in a separate package. + +basedir=@SUID_WRAPPER_DIR@ +if [ -x "$basedir"/Xorg.wrap ]; then + exec "$basedir"/Xorg.wrap "$@" +else + exec "$basedir"/Xorg.bin "$@" +fi diff --git a/hw/xfree86/man/Makefile.am b/hw/xfree86/man/Makefile.am index 80e22cbab..f41d26c4e 100644 --- a/hw/xfree86/man/Makefile.am +++ b/hw/xfree86/man/Makefile.am @@ -1,3 +1,8 @@ include $(top_srcdir)/manpages.am appman_PRE = Xorg.man fileman_PRE = xorg.conf.man xorg.conf.d.man + +if SUID_WRAPPER +appman_PRE += Xorg.wrap.man +fileman_PRE += Xwrapper.config.man +endif diff --git a/hw/xfree86/man/Xorg.wrap.man b/hw/xfree86/man/Xorg.wrap.man new file mode 100644 index 000000000..f2153e35b --- /dev/null +++ b/hw/xfree86/man/Xorg.wrap.man @@ -0,0 +1,67 @@ +.\" Xwrapper.wrap.1 +.\" +.\" Copyright 2014 Red Hat, Inc. +.\" +.\" Permission to use, copy, modify, distribute, and sell this software and its +.\" documentation for any purpose is hereby granted without fee, provided that +.\" the above copyright notice appear in all copies and that both that +.\" copyright notice and this permission notice appear in supporting +.\" documentation. +.\" +.\" The above copyright notice and this permission notice shall be included +.\" in all copies or substantial portions of the Software. +.\" +.\" THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS +.\" OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF +.\" MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. +.\" IN NO EVENT SHALL THE OPEN GROUP BE LIABLE FOR ANY CLAIM, DAMAGES OR +.\" OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, +.\" ARISING FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR +.\" OTHER DEALINGS IN THE SOFTWARE. +.\" +.\" Except as contained in this notice, the name of The Open Group shall +.\" not be used in advertising or otherwise to promote the sale, use or +.\" other dealings in this Software without prior written authorization +.\" from The Open Group. +.\" +.\" shorthand for double quote that works everywhere. +.ds q \N'34' +.TH Xorg.wrap 1 __xorgversion__ +.SH NAME +Xorg.wrap \- Xorg X server binary wrapper +.SH DESCRIPTION +The Xorg X server may need root rights to function properly. To start the +Xorg X server with these rights your system is using a suid root wrapper +installed as __suid_wrapper_dir__/Xorg.wrap which will execute the real +X server which is installed as __suid_wrapper_dir__/Xorg.bin . +.PP +By default Xorg.wrap will autodetect if root rights are necessary, and +if not it will drop its elevated rights before starting the real X server. +By default Xorg.wrap will only allow executing the real X server from login +sessions on a physical console. + +.SH CONFIG FILE +Xorg.wrap's default behavior can be overridden from the +\fI__sysconfdir__/X11/Xwrapper.config\fP config file. Lines starting with a +\fB#\fP in Xwrapper.config are considered comments and will be ignored. Any +other non empty lines must take the form of \fBkey\fP = \fIvalue\fP. +.TP 8 +\fBallowed_users\fP = \fIrootonly\fP|\fIconsole\fP|\fIanybody\fP +Specify which users may start the X server through the wrapper. Use +\fIrootonly\fP to only allow root, use \fIconsole\fP to only allow users +logged into a physical console, and use \fIanybody\fP to allow anybody. +The default is \fIconsole\fP. +.TP 8 +\fBneeds_root_rights\fP = \fIyes\fP|\fIno\fP|\fIauto\fP +Configure if the wrapper should drop its elevated (root) rights before starting +the X server. Use \fIyes\fP to force execution as root, \fIno\fP to force +execution with all suid rights dropped, and \fIauto\fP to letter the wrapper +auto-detect. The default is \fIauto\fP. +.PP +When auto-detecting the wrapper will drop rights if kms graphics are available +and not drop them if no kms graphics are detected. If a system has multiple +graphics cards and some are not kms capable auto-detection may fail, +in this case manual configuration should be used. + +.SH "SEE ALSO" +Xorg X server information: \fIXorg\fP(1) diff --git a/hw/xfree86/man/Xwrapper.config.man b/hw/xfree86/man/Xwrapper.config.man new file mode 100644 index 000000000..800947c55 --- /dev/null +++ b/hw/xfree86/man/Xwrapper.config.man @@ -0,0 +1 @@ +.so man1/Xorg.wrap.1 diff --git a/hw/xfree86/xorg-wrapper.c b/hw/xfree86/xorg-wrapper.c new file mode 100644 index 000000000..90c8c11ef --- /dev/null +++ b/hw/xfree86/xorg-wrapper.c @@ -0,0 +1,231 @@ +/* + * Copyright © 2014 Red Hat, Inc. + * + * Permission is hereby granted, free of charge, to any person obtaining a + * copy of this software and associated documentation files (the "Software"), + * to deal in the Software without restriction, including without limitation + * the rights to use, copy, modify, merge, publish, distribute, sublicense, + * and/or sell copies of the Software, and to permit persons to whom the + * Software is furnished to do so, subject to the following conditions: + * + * The above copyright notice and this permission notice (including the next + * paragraph) shall be included in all copies or substantial portions of the + * Software. + * + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER + * DEALINGS IN THE SOFTWARE. + * + * Author: Hans de Goede + */ + +#include "dix-config.h" + +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include +#include /* For DRM_DEV_NAME */ + +#define CONFIG_FILE SYSCONFDIR "/X11/Xwrapper.config" + +enum { ROOT_ONLY, CONSOLE_ONLY, ANYBODY }; + +/* KISS non locale / LANG parsing isspace version */ +static int is_space(char c) +{ + return c == ' ' || c == '\t' || c == '\n'; +} + +static char *strip(char *s) +{ + int i; + + /* Strip leading whitespace */ + while (s[0] && is_space(s[0])) + s++; + + /* Strip trailing whitespace */ + i = strlen(s) - 1; + while (i >= 0 && is_space(s[i])) { + s[i] = 0; + i--; + } + + return s; +} + +static void parse_config(int *allowed, int *needs_root_rights) +{ + FILE *f; + char buf[1024]; + char *stripped, *equals, *key, *value; + int line = 0; + + f = fopen(CONFIG_FILE, "r"); + if (!f) + return; + + while (fgets(buf, sizeof(buf), f)) { + line++; + + /* Skip comments and empty lines */ + stripped = strip(buf); + if (stripped[0] == '#' || stripped[0] == 0) + continue; + + /* Split in a key + value pair */ + equals = strchr(stripped, '='); + if (!equals) { + fprintf(stderr, "Syntax error at %s line %d\n", CONFIG_FILE, line); + exit(1); + } + *equals = 0; + key = strip(stripped); /* To remove trailing whitespace from key */ + value = strip(equals + 1); /* To remove leading whitespace from val */ + if (!key[0]) { + fprintf(stderr, "Missing key at %s line %d\n", CONFIG_FILE, line); + exit(1); + } + if (!value[0]) { + fprintf(stderr, "Missing value at %s line %d\n", CONFIG_FILE, line); + exit(1); + } + + /* And finally process */ + if (strcmp(key, "allowed_users") == 0) { + if (strcmp(value, "rootonly") == 0) + *allowed = ROOT_ONLY; + else if (strcmp(value, "console") == 0) + *allowed = CONSOLE_ONLY; + else if (strcmp(value, "anybody") == 0) + *allowed = ANYBODY; + else { + fprintf(stderr, + "Invalid value '%s' for 'allowed_users' at %s line %d\n", + value, CONFIG_FILE, line); + exit(1); + } + } + else if (strcmp(key, "needs_root_rights") == 0) { + if (strcmp(value, "yes") == 0) + *needs_root_rights = 1; + else if (strcmp(value, "no") == 0) + *needs_root_rights = 0; + else if (strcmp(value, "auto") == 0) + *needs_root_rights = -1; + else { + fprintf(stderr, + "Invalid value '%s' for 'needs_root_rights' at %s line %d\n", + value, CONFIG_FILE, line); + exit(1); + } + } + else if (strcmp(key, "nice_value") == 0) { + /* Backward compatibility with older Debian Xwrapper, ignore */ + } + else { + fprintf(stderr, "Invalid key '%s' at %s line %d\n", key, + CONFIG_FILE, line); + exit(1); + } + } + fclose(f); +} + +int main(int argc, char *argv[]) +{ + struct drm_mode_card_res res; + struct stat st; + char buf[PATH_MAX]; + int i, r, fd; + int kms_cards = 0; + int total_cards = 0; + int allowed = CONSOLE_ONLY; + int needs_root_rights = -1; + + parse_config(&allowed, &needs_root_rights); + + /* For non root users check if they are allowed to run the X server */ + if (getuid() != 0) { + switch (allowed) { + case ROOT_ONLY: + /* Already checked above */ + fprintf(stderr, "%s: Only root is allowed to run the X server\n", argv[0]); + exit(1); + break; + case CONSOLE_ONLY: + /* Some of stdin / stdout / stderr maybe redirected to a file */ + for (i = STDIN_FILENO; i <= STDERR_FILENO; i++) { + r = fstat(i, &st); + if (r == 0 && S_ISCHR(st.st_mode) && major(st.st_rdev) == 4) + break; + } + if (i > STDERR_FILENO) { + fprintf(stderr, "%s: Only console users are allowed to run the X server\n", argv[0]); + exit(1); + } + break; + case ANYBODY: + break; + } + } + + /* Detect if we need root rights, except when overriden by the config */ + if (needs_root_rights == -1) { + for (i = 0; i < 16; i++) { + snprintf(buf, sizeof(buf), DRM_DEV_NAME, DRM_DIR_NAME, i); + fd = open(buf, O_RDWR); + if (fd == -1) + continue; + + total_cards++; + + memset(&res, 0, sizeof(struct drm_mode_card_res)); + r = ioctl(fd, DRM_IOCTL_MODE_GETRESOURCES, &res); + if (r == 0 && res.count_connectors > 0) + kms_cards++; + + close(fd); + } + } + + /* If we've found cards, and all cards support kms, drop root rights */ + if (needs_root_rights == 0 || (total_cards && kms_cards == total_cards)) { + gid_t realgid = getgid(); + uid_t realuid = getuid(); + + if (setresgid(-1, realgid, realgid) != 0) { + perror("Could not drop setgid privileges"); + exit(1); + } + if (setresuid(-1, realuid, realuid) != 0) { + perror("Could not drop setuid privileges"); + exit(1); + } + } + + snprintf(buf, sizeof(buf), "%s/Xorg.bin", SUID_WRAPPER_DIR); + + /* Check if the server is executable by our real uid */ + if (access(buf, X_OK) != 0) { + perror("Missing execute permissions for " SUID_WRAPPER_DIR "Xorg.bin"); + exit(1); + } + + argv[0] = buf; + (void) execv(argv[0], argv); + perror("Failed to execute " SUID_WRAPPER_DIR "/Xorg.bin"); + exit(1); +} diff --git a/include/dix-config.h.in b/include/dix-config.h.in index 06455a837..f980a3d37 100644 --- a/include/dix-config.h.in +++ b/include/dix-config.h.in @@ -288,9 +288,15 @@ /* Support SHAPE extension */ #undef SHAPE +/* Where to install Xorg.bin and Xorg.wrap */ +#undef SUID_WRAPPER_DIR + /* Define to 1 on systems derived from System V Release 4 */ #undef SVR4 +/* sysconfdir */ +#undef SYSCONFDIR + /* Support TCP socket connections */ #undef TCPCONN diff --git a/include/xwin-config.h.in b/include/xwin-config.h.in index c5119f268..176c01980 100644 --- a/include/xwin-config.h.in +++ b/include/xwin-config.h.in @@ -26,9 +26,6 @@ /* Vendor web address for support */ #undef __VENDORDWEBSUPPORT__ -/* Location of system.XWinrc */ -#undef SYSCONFDIR - /* Default log location */ #undef DEFAULT_LOGDIR diff --git a/manpages.am b/manpages.am index dfd671915..648210b4e 100644 --- a/manpages.am +++ b/manpages.am @@ -31,6 +31,7 @@ MAN_SUBSTS += -e 's|__logdir__|$(logdir)|g' \ -e 's|__XKB_DFLT_OPTIONS__|$(XKB_DFLT_OPTIONS)|g' \ -e 's|__bundle_id_prefix__|$(BUNDLE_ID_PREFIX)|g' \ -e 's|__modulepath__|$(DEFAULT_MODULE_PATH)|g' \ + -e 's|__suid_wrapper_dir__|$(SUID_WRAPPER_DIR)|g' \ -e 's|__default_font_path__|$(COMPILEDDEFAULTFONTPATH)|g' \ -e '\|$(COMPILEDDEFAULTFONTPATH)| s|/,|/, |g' From 92ff79f1a804d63d2f2bb59dfbf3a2869627609a Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 11 Mar 2014 11:28:15 +0100 Subject: [PATCH 2/8] config_odev*: Use XNF alloc functions config_odev* functions are called in code-paths were we already use XNF* functions in other places, so which are not oom safe already. Besides that oom is something which should simply never happen, so aborting when it does is as good a response as any other. While switching to XNF functions also fixup an unchecked strdup case. Note the function prototypes are kept unchanged, as they are part of the server ABI. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- config/config.c | 18 +++--------------- hw/xfree86/common/xf86platformBus.h | 5 +++-- include/hotplug.h | 3 +++ 3 files changed, 9 insertions(+), 17 deletions(-) diff --git a/config/config.c b/config/config.c index 46f2532ec..def7f16ca 100644 --- a/config/config.c +++ b/config/config.c @@ -132,10 +132,7 @@ config_odev_allocate_attribute_list(void) { struct OdevAttributes *attriblist; - attriblist = malloc(sizeof(struct OdevAttributes)); - if (!attriblist) - return NULL; - + attriblist = XNFalloc(sizeof(struct OdevAttributes)); xorg_list_init(&attriblist->list); return attriblist; } @@ -168,10 +165,7 @@ config_odev_find_or_add_attribute(struct OdevAttributes *attribs, int attrib) if (oa) return oa; - oa = calloc(1, sizeof(struct OdevAttribute)); - if (!oa) - return oa; - + oa = XNFcalloc(sizeof(struct OdevAttribute)); oa->attrib_id = attrib; xorg_list_append(&oa->member, &attribs->list); @@ -185,11 +179,8 @@ config_odev_add_attribute(struct OdevAttributes *attribs, int attrib, struct OdevAttribute *oa; oa = config_odev_find_or_add_attribute(attribs, attrib); - if (!oa) - return FALSE; - free(oa->attrib_name); - oa->attrib_name = strdup(attrib_name); + oa->attrib_name = XNFstrdup(attrib_name); oa->attrib_type = ODEV_ATTRIB_STRING; return TRUE; } @@ -201,9 +192,6 @@ config_odev_add_int_attribute(struct OdevAttributes *attribs, int attrib, struct OdevAttribute *oa; oa = config_odev_find_or_add_attribute(attribs, attrib); - if (!oa) - return FALSE; - oa->attrib_value = attrib_value; oa->attrib_type = ODEV_ATTRIB_INT; return TRUE; diff --git a/hw/xfree86/common/xf86platformBus.h b/hw/xfree86/common/xf86platformBus.h index 78b5a5bea..5dee4e0e0 100644 --- a/hw/xfree86/common/xf86platformBus.h +++ b/hw/xfree86/common/xf86platformBus.h @@ -54,11 +54,12 @@ xf86_add_platform_device(struct OdevAttributes *attribs, Bool unowned); extern int xf86_remove_platform_device(int dev_index); extern Bool +xf86_get_platform_device_unowned(int index); +/* Note starting with xserver 1.16 these 2 functions never fail */ +extern Bool xf86_add_platform_device_attrib(int index, int attrib_id, char *attrib_str); extern Bool xf86_add_platform_device_int_attrib(int index, int attrib_id, int attrib_value); -extern Bool -xf86_get_platform_device_unowned(int index); extern int xf86platformAddDevice(int index); diff --git a/include/hotplug.h b/include/hotplug.h index 1d9364eee..cefc164ae 100644 --- a/include/hotplug.h +++ b/include/hotplug.h @@ -48,12 +48,14 @@ struct OdevAttributes { struct xorg_list list; }; +/* Note starting with xserver 1.16 this function never fails */ struct OdevAttributes * config_odev_allocate_attribute_list(void); void config_odev_free_attribute_list(struct OdevAttributes *attribs); +/* Note starting with xserver 1.16 this function never fails */ Bool config_odev_add_attribute(struct OdevAttributes *attribs, int attrib, const char *attrib_name); @@ -61,6 +63,7 @@ config_odev_add_attribute(struct OdevAttributes *attribs, int attrib, char * config_odev_get_attribute(struct OdevAttributes *attribs, int attrib_id); +/* Note starting with xserver 1.16 this function never fails */ Bool config_odev_add_int_attribute(struct OdevAttributes *attribs, int attrib, int attrib_value); From 40e3c79a591909ab64822cc86fd07513317bf19b Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 11 Mar 2014 11:38:09 +0100 Subject: [PATCH 3/8] Remove config_odev_add_*attribute checks in various places Note that there are more callers but those were already not doing any error checking. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- config/udev.c | 32 ++++------------------ hw/xfree86/os-support/linux/lnx_platform.c | 5 +--- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/config/udev.c b/config/udev.c index d88abaaa1..a1b72c13b 100644 --- a/config/udev.c +++ b/config/udev.c @@ -55,7 +55,7 @@ static struct udev_monitor *udev_monitor; #ifdef CONFIG_UDEV_KMS -static Bool +static void config_udev_odev_setup_attribs(const char *path, const char *syspath, int major, int minor, config_odev_probe_proc_ptr probe_callback); @@ -457,40 +457,20 @@ config_udev_fini(void) #ifdef CONFIG_UDEV_KMS -static Bool +static void config_udev_odev_setup_attribs(const char *path, const char *syspath, int major, int minor, config_odev_probe_proc_ptr probe_callback) { struct OdevAttributes *attribs = config_odev_allocate_attribute_list(); - int ret; - if (!attribs) - return FALSE; - - ret = config_odev_add_attribute(attribs, ODEV_ATTRIB_PATH, path); - if (ret == FALSE) - goto fail; - - ret = config_odev_add_attribute(attribs, ODEV_ATTRIB_SYSPATH, syspath); - if (ret == FALSE) - goto fail; - - ret = config_odev_add_int_attribute(attribs, ODEV_ATTRIB_MAJOR, major); - if (ret == FALSE) - goto fail; - - ret = config_odev_add_int_attribute(attribs, ODEV_ATTRIB_MINOR, minor); - if (ret == FALSE) - goto fail; + config_odev_add_attribute(attribs, ODEV_ATTRIB_PATH, path); + config_odev_add_attribute(attribs, ODEV_ATTRIB_SYSPATH, syspath); + config_odev_add_int_attribute(attribs, ODEV_ATTRIB_MAJOR, major); + config_odev_add_int_attribute(attribs, ODEV_ATTRIB_MINOR, minor); /* ownership of attribs is passed to probe layer */ probe_callback(attribs); - return TRUE; -fail: - config_odev_free_attributes(attribs); - free(attribs); - return FALSE; } void diff --git a/hw/xfree86/os-support/linux/lnx_platform.c b/hw/xfree86/os-support/linux/lnx_platform.c index 109a9a774..dbd7aa0aa 100644 --- a/hw/xfree86/os-support/linux/lnx_platform.c +++ b/hw/xfree86/os-support/linux/lnx_platform.c @@ -40,10 +40,7 @@ get_drm_info(struct OdevAttributes *attribs, char *path, int delayed_index) systemd_logind_release_fd(major, minor); return FALSE; } - if (!config_odev_add_int_attribute(attribs, ODEV_ATTRIB_FD, fd)) { - systemd_logind_release_fd(major, minor); - return FALSE; - } + config_odev_add_int_attribute(attribs, ODEV_ATTRIB_FD, fd); server_fd = TRUE; } From ddc3888bbaaddc47706a9cb96459738683d72cb3 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Mar 2014 12:45:40 +0100 Subject: [PATCH 4/8] systemd_logind_find_info_ptr_by_devnum: Add a start argument Modify systemd_logind_find_info_ptr_by_devnum to take a start argument, so that it can be used to find all occurences of a devnum in an InputInfo list, rather then just the first. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- hw/xfree86/os-support/linux/systemd-logind.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/hw/xfree86/os-support/linux/systemd-logind.c b/hw/xfree86/os-support/linux/systemd-logind.c index a8406d8be..09db33633 100644 --- a/hw/xfree86/os-support/linux/systemd-logind.c +++ b/hw/xfree86/os-support/linux/systemd-logind.c @@ -204,11 +204,12 @@ systemd_logind_vtenter(void) } static InputInfoPtr -systemd_logind_find_info_ptr_by_devnum(int major, int minor) +systemd_logind_find_info_ptr_by_devnum(InputInfoPtr start, + int major, int minor) { InputInfoPtr pInfo; - for (pInfo = xf86InputDevs; pInfo; pInfo = pInfo->next) + for (pInfo = start; pInfo; pInfo = pInfo->next) if (pInfo->major == major && pInfo->minor == minor && (pInfo->flags & XI86_SERVER_FD)) return pInfo; @@ -320,7 +321,8 @@ message_filter(DBusConnection * connection, DBusMessage * message, void *data) pdev = xf86_find_platform_device_by_devnum(major, minor); if (!pdev) - pInfo = systemd_logind_find_info_ptr_by_devnum(major, minor); + pInfo = systemd_logind_find_info_ptr_by_devnum(xf86InputDevs, + major, minor); if (!pdev && !pInfo) { LogMessage(X_WARNING, "systemd-logind: could not find dev %u:%u\n", major, minor); From 4e3d9690e1868d286dcb766b429f9c99313f2401 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Mar 2014 13:19:32 +0100 Subject: [PATCH 5/8] systemd_logind_find_info_ptr_by_devnum: Move to higher inside the file This is a preparation patch for adding support for server managed fds for InputDevices where multiple input devices share the same device node (and thus also their major and minor). Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- hw/xfree86/os-support/linux/systemd-logind.c | 28 ++++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/hw/xfree86/os-support/linux/systemd-logind.c b/hw/xfree86/os-support/linux/systemd-logind.c index 09db33633..012a9204e 100644 --- a/hw/xfree86/os-support/linux/systemd-logind.c +++ b/hw/xfree86/os-support/linux/systemd-logind.c @@ -51,6 +51,20 @@ struct systemd_logind_info { static struct systemd_logind_info logind_info; +static InputInfoPtr +systemd_logind_find_info_ptr_by_devnum(InputInfoPtr start, + int major, int minor) +{ + InputInfoPtr pInfo; + + for (pInfo = start; pInfo; pInfo = pInfo->next) + if (pInfo->major == major && pInfo->minor == minor && + (pInfo->flags & XI86_SERVER_FD)) + return pInfo; + + return NULL; +} + int systemd_logind_take_fd(int _major, int _minor, const char *path, Bool *paused_ret) @@ -203,20 +217,6 @@ systemd_logind_vtenter(void) xf86InputEnableVTProbe(); } -static InputInfoPtr -systemd_logind_find_info_ptr_by_devnum(InputInfoPtr start, - int major, int minor) -{ - InputInfoPtr pInfo; - - for (pInfo = start; pInfo; pInfo = pInfo->next) - if (pInfo->major == major && pInfo->minor == minor && - (pInfo->flags & XI86_SERVER_FD)) - return pInfo; - - return NULL; -} - static void systemd_logind_ack_pause(struct systemd_logind_info *info, dbus_int32_t minor, dbus_int32_t major) From 8d3f63dbe9bfd816beb6475fd0e00df4dbba269f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Mar 2014 12:58:22 +0100 Subject: [PATCH 6/8] systemd-logind: Add a systemd_logind_set_input_fd_for_all_devs helper And use it where appropriate. Setting the fd for all matching InputDevices is necessary when we've multiple InputDevices sharing a single device-node, such as happens with Wacom tablets. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- hw/xfree86/os-support/linux/systemd-logind.c | 33 ++++++++++++++------ 1 file changed, 23 insertions(+), 10 deletions(-) diff --git a/hw/xfree86/os-support/linux/systemd-logind.c b/hw/xfree86/os-support/linux/systemd-logind.c index 012a9204e..a2ef7afba 100644 --- a/hw/xfree86/os-support/linux/systemd-logind.c +++ b/hw/xfree86/os-support/linux/systemd-logind.c @@ -65,6 +65,23 @@ systemd_logind_find_info_ptr_by_devnum(InputInfoPtr start, return NULL; } +static void +systemd_logind_set_input_fd_for_all_devs(int major, int minor, int fd, + Bool enable) +{ + InputInfoPtr pInfo; + + pInfo = systemd_logind_find_info_ptr_by_devnum(xf86InputDevs, major, minor); + while (pInfo) { + pInfo->fd = fd; + pInfo->options = xf86ReplaceIntOption(pInfo->options, "fd", fd); + if (enable) + xf86EnableInputDeviceForVTSwitch(pInfo); + + pInfo = systemd_logind_find_info_ptr_by_devnum(pInfo->next, major, minor); + } +} + int systemd_logind_take_fd(int _major, int _minor, const char *path, Bool *paused_ret) @@ -337,8 +354,7 @@ message_filter(DBusConnection * connection, DBusMessage * message, void *data) pdev->flags |= XF86_PDEV_PAUSED; else { close(pInfo->fd); - pInfo->fd = -1; - pInfo->options = xf86ReplaceIntOption(pInfo->options, "fd", -1); + systemd_logind_set_input_fd_for_all_devs(major, minor, -1, FALSE); } if (ack) systemd_logind_ack_pause(info, major, minor); @@ -347,15 +363,12 @@ message_filter(DBusConnection * connection, DBusMessage * message, void *data) /* info->vt_active gets set by systemd_logind_vtenter() */ info->active = TRUE; - if (pdev) { + if (pdev) pdev->flags &= ~XF86_PDEV_PAUSED; - } - else { - pInfo->fd = fd; - pInfo->options = xf86ReplaceIntOption(pInfo->options, "fd", fd); - if (info->vt_active) - xf86EnableInputDeviceForVTSwitch(pInfo); - } + else + systemd_logind_set_input_fd_for_all_devs(major, minor, fd, + info->vt_active); + /* Always call vtenter(), in case there are only legacy video devs */ systemd_logind_vtenter(); } From 0e972b6037d3709c13d46adef9d14b702f477abc Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Mar 2014 14:57:24 +0100 Subject: [PATCH 7/8] systemd-logind: Correctly deal with InputDevs sharing a device-node InputDevices may share a single device-node, this happens ie with Wacom tablets. This patch makes take_fd and release_fd properly deal with this, together with the earlier patch for updating the fd in all matching xf86InputDevs on pause / resume this completes support for such shared device-nodes. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- hw/xfree86/os-support/linux/systemd-logind.c | 27 ++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/hw/xfree86/os-support/linux/systemd-logind.c b/hw/xfree86/os-support/linux/systemd-logind.c index a2ef7afba..62858b062 100644 --- a/hw/xfree86/os-support/linux/systemd-logind.c +++ b/hw/xfree86/os-support/linux/systemd-logind.c @@ -87,6 +87,7 @@ systemd_logind_take_fd(int _major, int _minor, const char *path, Bool *paused_ret) { struct systemd_logind_info *info = &logind_info; + InputInfoPtr pInfo; DBusError error; DBusMessage *msg = NULL; DBusMessage *reply = NULL; @@ -102,6 +103,16 @@ systemd_logind_take_fd(int _major, int _minor, const char *path, if (strstr(path, "mouse")) return -1; + /* Check if we already have an InputInfo entry with this major, minor + * (shared device-nodes happen ie with Wacom tablets). */ + pInfo = systemd_logind_find_info_ptr_by_devnum(xf86InputDevs, major, minor); + if (pInfo) { + LogMessage(X_INFO, "systemd-logind: returning pre-existing fd for %s %u:%u\n", + path, major, minor); + *paused_ret = FALSE; + return pInfo->fd; + } + dbus_error_init(&error); msg = dbus_message_new_method_call("org.freedesktop.login1", info->session, @@ -154,15 +165,31 @@ void systemd_logind_release_fd(int _major, int _minor) { struct systemd_logind_info *info = &logind_info; + InputInfoPtr pInfo; DBusError error; DBusMessage *msg = NULL; DBusMessage *reply = NULL; dbus_int32_t major = _major; dbus_int32_t minor = _minor; + int matches = 0; if (!info->session || major == 0) return; + /* Only release the fd if there is only 1 InputInfo left for this major + * and minor, otherwise other InputInfo's are still referencing the fd. */ + pInfo = systemd_logind_find_info_ptr_by_devnum(xf86InputDevs, major, minor); + while (pInfo) { + matches++; + pInfo = systemd_logind_find_info_ptr_by_devnum(pInfo->next, major, minor); + } + if (matches > 1) { + LogMessage(X_INFO, "systemd-logind: not releasing fd for %u:%u, still in use\n", major, minor); + return; + } + + LogMessage(X_INFO, "systemd-logind: releasing fd for %u:%u\n", major, minor); + dbus_error_init(&error); msg = dbus_message_new_method_call("org.freedesktop.login1", info->session, From 2b77b208daf9402472ba7fb709156a14eb487299 Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Wed, 12 Mar 2014 16:00:23 +0100 Subject: [PATCH 8/8] xf86Xinput: release server managed fd before removing the device from the list So that the fd in use test in systemd_logind_release_fd works properly. Note we cannot change the test inside systemd_logind_release_fd as it must work for devices which were never added to the xf86InputDevs too. Signed-off-by: Hans de Goede Reviewed-by: Peter Hutterer --- hw/xfree86/common/xf86Xinput.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c index 36b92a9f7..a367ae3cb 100644 --- a/hw/xfree86/common/xf86Xinput.c +++ b/hw/xfree86/common/xf86Xinput.c @@ -766,6 +766,11 @@ xf86DeleteInput(InputInfoPtr pInp, int flags) FreeInputAttributes(pInp->attrs); + if (pInp->flags & XI86_SERVER_FD) { + systemd_logind_release_fd(pInp->major, pInp->minor); + close(pInp->fd); + } + /* Remove the entry from the list. */ if (pInp == xf86InputDevs) xf86InputDevs = pInp->next; @@ -779,11 +784,6 @@ xf86DeleteInput(InputInfoPtr pInp, int flags) /* Else the entry wasn't in the xf86InputDevs list (ignore this). */ } - if (pInp->flags & XI86_SERVER_FD) { - systemd_logind_release_fd(pInp->major, pInp->minor); - close(pInp->fd); - } - free((void *) pInp->driver); free((void *) pInp->name); xf86optionListFree(pInp->options);