What to do with xf86-video-intel backlight control when running Xorg as non root

What to do with xf86-video-intel backlight control when running Xorg as non root

[Hans de Goede]

Hi All,

Currently xf86-video-intel is unique in that it is the only video driver
which does backlight control inside the driver rather then letting
something else (ie the desktop environment) deal with it.

This is a problem when running the xserver without root rights because
writing /sys/class/backlight/foo/brightness requires root rights.

There are 2 possible (short term) solutions for this:

1) Detect that the xserver is not running as root, and don't register the
backlight property on the connectors, let something else deal with it,
as is done or other xf86-video-* drivers already.

2) Add a little xf86-video-intel-brightness helper on Linux which the driver
execs (through pkexec) each time it needs to set the brightness.


1) of course is very KISS, so I like. 2) is not that hard either, and
1) might cause some regressions in cases where ie gsd-brightness-helper
does not do the right thing, where as the intel driver does. OTOH it seems
that the intel video code is mostly there to deal with older kernels, and
rootless xorg will be used with newer kernels only anyways.

So before I start writing a patch for this I would like to hear which
approach would be acceptable for inclusion into xf86-video-intel.

Regards,

Hans
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: What to do with xf86-video-intel backlight control when running Xorg as non root

[Chris Wilson]

On Thu, Feb 13, 2014 at 04:52:59PM +0100, Hans de Goede wrote:
> Hi All,
> 
> Currently xf86-video-intel is unique in that it is the only video driver
> which does backlight control inside the driver rather then letting
> something else (ie the desktop environment) deal with it.
> 
> This is a problem when running the xserver without root rights because
> writing /sys/class/backlight/foo/brightness requires root rights.
> 
> There are 2 possible (short term) solutions for this:
> 
> 1) Detect that the xserver is not running as root, and don't register the
> backlight property on the connectors, let something else deal with it,
> as is done or other xf86-video-* drivers already.
> 
> 2) Add a little xf86-video-intel-brightness helper on Linux which the driver
> execs (through pkexec) each time it needs to set the brightness.
> 
> 
> 1) of course is very KISS, so I like. 2) is not that hard either, and
> 1) might cause some regressions in cases where ie gsd-brightness-helper
> does not do the right thing, where as the intel driver does. OTOH it seems
> that the intel video code is mostly there to deal with older kernels, and
> rootless xorg will be used with newer kernels only anyways.

Not registering a property that is broken seems like the fundamental first
step. Would it be possible to use udev to set the access mode on the
backlight properties such that the display controller does have
permission to write to those files? Otherwise, it seems like we need the
proxy in order to keep the xrandr property available to users and
prevent those who rely upon it in scripts from seeing regressions.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] What to do with xf86-video-intel backlight control when running Xorg as non root

[Hans de Goede]

Hi,

On 02/13/2014 05:40 PM, Chris Wilson wrote:
> On Thu, Feb 13, 2014 at 04:52:59PM +0100, Hans de Goede wrote:
>> Hi All,
>>
>> Currently xf86-video-intel is unique in that it is the only video driver
>> which does backlight control inside the driver rather then letting
>> something else (ie the desktop environment) deal with it.
>>
>> This is a problem when running the xserver without root rights because
>> writing /sys/class/backlight/foo/brightness requires root rights.
>>
>> There are 2 possible (short term) solutions for this:
>>
>> 1) Detect that the xserver is not running as root, and don't register the
>> backlight property on the connectors, let something else deal with it,
>> as is done or other xf86-video-* drivers already.
>>
>> 2) Add a little xf86-video-intel-brightness helper on Linux which the driver
>> execs (through pkexec) each time it needs to set the brightness.
>>
>>
>> 1) of course is very KISS, so I like. 2) is not that hard either, and
>> 1) might cause some regressions in cases where ie gsd-brightness-helper
>> does not do the right thing, where as the intel driver does. OTOH it seems
>> that the intel video code is mostly there to deal with older kernels, and
>> rootless xorg will be used with newer kernels only anyways.
> 
> Not registering a property that is broken seems like the fundamental first
> step. Would it be possible to use udev to set the access mode on the
> backlight properties such that the display controller does have
> permission to write to those files?

When we were discussing this at Fosdem Kay Sievers was in the room, and I
can summarize his response to that suggestion as: *NO*.

> Otherwise, it seems like we need the
> proxy in order to keep the xrandr property available to users and
> prevent those who rely upon it in scripts from seeing regressions.

Right, that is what I was thinking too, so the question then becomes how
hard you will scream at me if I add something like this to xf86-video-intel
linux specific backlight code:

    if (geteuid() == 0) {
        /* Old write directly to /sys/class/backlight/... code */
    {
    else {
        /* The & is to avoid the xserver blocking */
        snprintf(command, sizeof(command), "pkexec %s/libexec/xf86-video-intel-backlight-helper %s %d&",
                 PREFIX, sna_output->backlight_iface, level);
        r = system(command);
        if (r) {
            /* complain */
        }
    }

If you won't scream too much, and more importantly, if you will accept such
a patch (including code for the helper), then I'll try to cook up something
like this tomorrow.

In case you're wondering what pkexec is, it stands for policy-kit-exec, it
is a little helper which will ask policykit if it is ok to run argv[1] as
root (for which there needs to be policy-file saying so), and if it is ok,
it will execute argv[1 .. x] as root (in a sanitized environment).

This will more or less give us the "display controller permission" you
were suggesting before without trying to get udev / logind to manage acls
on sysfs files.

To be specific we would need a policy file with aprox. this in there:

<policyconfig>
  <action id="org.x.xf86-video-intel.backlight-helper>
    <defaults>
      <allow_any>no</allow_any>
      <allow_inactive>no</allow_inactive>
      <allow_active>yes</allow_active>
    </defaults>
    <annotate key="org.freedesktop.policykit.exec.path">@PREFIX@/libexec/xf86-video-intel-backlight-helper
  </action>
</policyconfig>

Regards,

Hans
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: What to do with xf86-video-intel backlight control when running Xorg as non root

[Mark Kettenis]

> Date: Thu, 13 Feb 2014 20:37:47 +0100
> From: Hans de Goede <hdegoede@redhat.com>
> 
> Hi,

Hi Hans,

Apologies in advance for jumping into the discussion at a somewhat
random point.

> On 02/13/2014 05:40 PM, Chris Wilson wrote: > On Thu, Feb 13, 2014
> at 04:52:59PM +0100, Hans de Goede wrote: >> Hi All,
> >>
> >> Currently xf86-video-intel is unique in that it is the only video driver
> >> which does backlight control inside the driver rather then letting
> >> something else (ie the desktop environment) deal with it.
> >>
> >> This is a problem when running the xserver without root rights because
> >> writing /sys/class/backlight/foo/brightness requires root rights.
> >>
> >> There are 2 possible (short term) solutions for this:
> >>
> >> 1) Detect that the xserver is not running as root, and don't register the
> >> backlight property on the connectors, let something else deal with it,
> >> as is done or other xf86-video-* drivers already.
> >>
> >> 2) Add a little xf86-video-intel-brightness helper on Linux which the driver
> >> execs (through pkexec) each time it needs to set the brightness.
> >>
> >>
> >> 1) of course is very KISS, so I like. 2) is not that hard either, and
> >> 1) might cause some regressions in cases where ie gsd-brightness-helper
> >> does not do the right thing, where as the intel driver does. OTOH it seems
> >> that the intel video code is mostly there to deal with older kernels, and
> >> rootless xorg will be used with newer kernels only anyways.
> > 
> > Not registering a property that is broken seems like the fundamental first
> > step. Would it be possible to use udev to set the access mode on the
> > backlight properties such that the display controller does have
> > permission to write to those files?
> 
> When we were discussing this at Fosdem Kay Sievers was in the room, and I
> can summarize his response to that suggestion as: *NO*.
> 
> > Otherwise, it seems like we need the
> > proxy in order to keep the xrandr property available to users and
> > prevent those who rely upon it in scripts from seeing regressions.
> 
> Right, that is what I was thinking too, so the question then becomes how
> hard you will scream at me if I add something like this to xf86-video-intel
> linux specific backlight code:
> 
>     if (geteuid() == 0) {
>         /* Old write directly to /sys/class/backlight/... code */
>     {
>     else {
>         /* The & is to avoid the xserver blocking */
>         snprintf(command, sizeof(command), "pkexec %s/libexec/xf86-video-intel-backlight-helper %s %d&",
>                  PREFIX, sna_output->backlight_iface, level);
>         r = system(command);
>         if (r) {
>             /* complain */
>         }
>     }
> 
> If you won't scream too much, and more importantly, if you will accept such
> a patch (including code for the helper), then I'll try to cook up something
> like this tomorrow.

I don't really care as long as this is #ifdef __linux__, but yeah,
that's fairly gross.  Does the Linux community really think it is a
good idea that the xserver starts to depend on policykit.  What will
happen when the next framework for doing things with elevated
priviliges comes along?  And do you realize that you'll end up adding
similar code to many other drivers as well?

You've probably scrolled past the #ifdef __OpenBSD__ code in the
xf86-video-intel driver, and perhaps noticed it is quite a bit
simpler.  On OpenBSD we've had an ioctl-based backlight control
interface for ages.  It works without root priviliges because the
appropriate file descriptor is already open.  (Our xserver is still
setuid root, but it is privilige seperated, and virtually all code
runs in the part that has dropped root priviliges).  It has the major
benefit that we can have an OS-provided tool to control the backlight
that works even when X isn't running.  The backlight policy is
determined by the kernel, and currently rather basic.  For x86 it
decides between some vendor-defined mechanisms, acpi and what the KMS
drivers provide.  And a simple policy seems to cover most of the use
cases.

Conceptually our approach is very similar to having drm provide an
interface for this.  And it would be fairly trivial for us to adapt
such an approach.  But the pkexec or backlightd approach really isn't
compatible with our goals of keeping things simple and providing a
basic X11 environment that is well-integrated into our OS and works
out-of-the-box for most (if not all) people.

Re: [Intel-gfx] What to do with xf86-video-intel backlight control when running Xorg as non root

[Hans de Goede]

Hi,

On 02/13/2014 09:24 PM, Mark Kettenis wrote:
>> Date: Thu, 13 Feb 2014 20:37:47 +0100
>> From: Hans de Goede <hdegoede-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

<snip>

>> Right, that is what I was thinking too, so the question then becomes how
>> hard you will scream at me if I add something like this to xf86-video-intel
>> linux specific backlight code:
>>
>>     if (geteuid() == 0) {
>>         /* Old write directly to /sys/class/backlight/... code */
>>     {
>>     else {
>>         /* The & is to avoid the xserver blocking */
>>         snprintf(command, sizeof(command), "pkexec %s/libexec/xf86-video-intel-backlight-helper %s %d&",
>>                  PREFIX, sna_output->backlight_iface, level);
>>         r = system(command);
>>         if (r) {
>>             /* complain */
>>         }
>>     }
>>
>> If you won't scream too much, and more importantly, if you will accept such
>> a patch (including code for the helper), then I'll try to cook up something
>> like this tomorrow.
> 
> I don't really care as long as this is #ifdef __linux__, but yeah,
> that's fairly gross.  Does the Linux community really think it is a
> good idea that the xserver starts to depend on policykit.

It does not look like policykit will be going away anytime soon.

Also note that this is intended as an interim solution until we sort out the
backlight mess in a better way, see the "Fixing the kernels backlight API"
thread on xorg-devel.

Regards,

Hans
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel

Re: [Intel-gfx] What to do with xf86-video-intel backlight control when running Xorg as non root

[Chris Wilson]

On Thu, Feb 13, 2014 at 08:37:47PM +0100, Hans de Goede wrote:
> Hi,
> 
> On 02/13/2014 05:40 PM, Chris Wilson wrote:
> > Otherwise, it seems like we need the
> > proxy in order to keep the xrandr property available to users and
> > prevent those who rely upon it in scripts from seeing regressions.
> 
> Right, that is what I was thinking too, so the question then becomes how
> hard you will scream at me if I add something like this to xf86-video-intel
> linux specific backlight code:
> 
>     if (geteuid() == 0) {
>         /* Old write directly to /sys/class/backlight/... code */
>     {
>     else {
>         /* The & is to avoid the xserver blocking */
>         snprintf(command, sizeof(command), "pkexec %s/libexec/xf86-video-intel-backlight-helper %s %d&",
>                  PREFIX, sna_output->backlight_iface, level);
>         r = system(command);
>         if (r) {
>             /* complain */
>         }
>     }
> 
> If you won't scream too much, and more importantly, if you will accept such
> a patch (including code for the helper), then I'll try to cook up something
> like this tomorrow.

I feel comfortable enough with that approach, and have nothing better to
suggest. (I would just employ the fallback case if open(O_RDWR) fails.)
And limiting the helper to only write its value into
BACKLIGHT_CLASS/%{iface}/brightness should help reduce its attack surface.
Thinking about it, it would be useful if we verified that BACKLIGHT_CLASS
(both the ddx and its helper)was a sysfs directory in the first place.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
xorg-devel-go0+a7rfsptAfugRpC6u6w@public.gmane.org: X.Org development
Archives: http://lists.x.org/archives/xorg-devel
Info: http://lists.x.org/mailman/listinfo/xorg-devel