[PATCH RFC] Xorg: Add a suid root wrapper

[Hans de Goede]

Hi,

Thanks for the review!

On 03/06/2014 01:46 PM, Mark Kettenis wrote:
>> 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?

Yeah that seems like a good idea.

>> +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.

We may end up putting a define in some config.h for this, but ...

> Probably better to use libdrm.

Ugh no, this is a suid-root wrapper, less code is more here. Yes
I know libdrm is designed to be safe to run as root, but it is still
better to not run it as root at all.

>> +        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.

That is why the check is there for if we've found any cards at all, so
on a machine with only a legacy card the wrapper will keep the root rights.

I admit that on a machine with 2 cards, one legacy one kms it may end up doing
the wrong thing. That is why the non RFC version of this patch is going to have
a /etc/X11/Xwrapper.config config file, so that people can simply put a

needs_root_rights = 1

In there, given that this is rather a corner case which will likely require
manual config anyways that seems like a better idea then complicating the
wrapper a lot for this corner case.

Regards,

Hans