Hi, Thanks for the review!
On 03/06/2014 01:46 PM, Mark Kettenis wrote: >> From: Hans de Goede <hdego...@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 <hdego...@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 _______________________________________________ xorg-devel@lists.x.org: X.Org development Archives: http://lists.x.org/archives/xorg-devel Info: http://lists.x.org/mailman/listinfo/xorg-devel