[PATCH RFC] Xorg: Add a suid root wrapper

[Mark Kettenis]

> From: Hans de Goede <hdegoede at redhat.com>
> Date: Wed,  5 Mar 2014 16:51:52 +0100

If you end up going with this wrapper approach anyway despite my
previous message, here are some comments.

Oh, and it's good that this is optional.

> diff --git a/hw/xfree86/Xorg.sh b/hw/xfree86/Xorg.sh
> new file mode 100644
> index 0000000..3a2b34b
> --- /dev/null
> +++ b/hw/xfree86/Xorg.sh
> @@ -0,0 +1,13 @@
> +#!/bin/sh
> +#
> +# See if Xorg.wrap is installed and if it is execute it, otherwise execute
> +# Xorg.bin directly. This allows distros to put the suid wrapper in a
> +# separate package.
> +
> +canonical=$(readlink -f "$0")

POSIX doesn't specify readlink(1), so it might not be available on all
systems.  Solaris doesn't seem to have it.  Perhaps just have
configure substitute the installation path here?

> +basedir=$(dirname "$canonical")
> +if [ -x "$basedir"/Xorg.wrap ]; then
> +	exec "$basedir"/Xorg.wrap "$@"
> +else
> +	exec "$basedir"/Xorg.bin "$@"
> +fi

> diff --git a/hw/xfree86/xorg-wrapper.c b/hw/xfree86/xorg-wrapper.c
> new file mode 100644
> index 0000000..93a5f15
> --- /dev/null
> +++ b/hw/xfree86/xorg-wrapper.c
> @@ -0,0 +1,82 @@
> +/*
> + * 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 <hdegoede at redhat.com>
> + */
> +
> +#include <fcntl.h>
> +#include <limits.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/ioctl.h>
> +#include <sys/stat.h>
> +#include <sys/types.h>
> +#include <unistd.h>
> +#include <drm/drm.h>
> +
> +#include "dix-config.h" /* For PROJECTROOT */
> +
> +int main(int argc, char *argv[])
> +{
> +    struct drm_mode_card_res res;
> +    char buf[PATH_MAX];
> +    int i, r, fd;
> +    int kms_cards = 0;
> +    int total_cards = 0;
> +
> +    for (i = 0; i < 16; i++) {
> +        snprintf(buf, PATH_MAX, "/dev/dri/card%d", i);

Hardcoding paths like this is a bad idea.  We use "/dev/drm%d" on
OpenBSD for example.  Other systems might use different names yet
again.  Probably better to use libdrm.

> +        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 (total_cards && kms_cards == total_cards) {

I think total_cards only includes devices that have a kernel drm
driver loaded.  So what you're really checking here is how many of
those provide KMS support.  So you're missing any true legacy device.