[PATCH RFC] Xorg: Add a suid root wrapper

[Mark Kettenis]

> Date: Thu, 06 Mar 2014 13:57:32 +0100
> From: Hans de Goede <hdegoede at redhat.com>

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

Well, there's always a tradeoff between running external code and the
risk of making mistakes reimplementing such functionality.

Anyway, what you could do here is include xf86drm.h but not link
against libdrm.h.  Then you could use DRM_DEV_NAME instead of
hardcoding /dev/dri/card%d.

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

Sounds reasonable.