Hi Kristian
On Wed, Apr 17, 2013 at 7:55 PM, David Herrmann <dh.herrmann@xxxxxxxxx> wrote:
> Hi Kristian
>
> On Tue, Apr 16, 2013 at 3:58 PM, Kristian Høgsberg <krh@xxxxxxxxxxxxx> wrote:
>> This commit adds a new ioctl to the evdev device: EVIOCMUTE. The
>> purpose of this ioctl it to temporarily block event delivery to a
>> specific evdev client and prevent access to most of the ioctls.
>>
>> The use case is a setuid helper process for a display server, which
>> opens input devices and passes the fds to the display server. On VT
>> switch away from the server, it should stop reading events from its
>> evdev input devices. However, if the display server runs as the user
>> it can be ptraced or if the server loads user defined modules, the
>> display server can no longer be trusted to not snoop on the evdev
>> devices.
>>
>> The mute ioctl allows the setuid helper to mute evdev devices when
>> switching away from the VT the server is running on. The idea is that
>> the helper listens for the VT switching signals and when VT switch happens
>> it notifies the display server, waits for the server to clean up and then
>> the helper drops drm master (which also requires root), mutes evdev
>> devices and the acks the VT switch.
>>
>> Why don't we just use revoke? The revoke syscall (when it's done) will
>> work on filenames and shut down all fds open to the device. This will
>> break all other processes that listen on evdev devices for legitimate
>> reasons. It's the same reason we don't use the EVIOCGRAB ioctl for input
>> devices. EVIOCMUTE lets the helper mute just the fd it gave to the
>> display server without affecting anything else potentially using the device.
>
> Other use-cases include SAK, forced VT switches and
> system-compositors. I'd really like seeing this feature.
>
>> Signed-off-by: Kristian Høgsberg <krh@xxxxxxxxxxxxx>
>> ---
>> drivers/input/evdev.c | 37 ++++++++++++++++++++++++++++++++++---
>> include/uapi/linux/input.h | 1 +
>> 2 files changed, 35 insertions(+), 3 deletions(-)
>>
>> This idea comes from work on Wayland and Weston [1], but the setuid helper
>> should work and is required for a non-root X server to function correctly
>> as well (ie, do proper vt switching).
>>
>> Kristian
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index f0f8928..cea6c35 100644
>> --- a/drivers/input/evdev.c
>> +++ b/drivers/input/evdev.c
>> @@ -48,6 +48,7 @@ struct evdev_client {
>> struct evdev *evdev;
>> struct list_head node;
>> int clkid;
>> + int muted;
>
> Use "bool" instead.
>
>> unsigned int bufsize;
>> struct input_event buffer[];
>> };
>> @@ -130,8 +131,9 @@ static void evdev_events(struct input_handle *handle,
>> evdev_pass_values(client, vals, count, time_mono, time_real);
>> else
>> list_for_each_entry_rcu(client, &evdev->client_list, node)
>> - evdev_pass_values(client, vals, count,
>> - time_mono, time_real);
>> + if (!client->muted)
>> + evdev_pass_values(client, vals, count,
>> + time_mono, time_real);
>
> Personally, I'd do this in evdev_pass_values(), but that's a matter of
> taste.. And this would also allow muting grabbed devices. Because
> currently input-grabs overwrite the mute flag which I think is odd. At
> least mention this in the commit message so people know how it works
> regarding input grabs.
>
>>
>> rcu_read_unlock();
>> }
>> @@ -674,6 +676,20 @@ static int evdev_handle_mt_request(struct input_dev *dev,
>> return 0;
>> }
>>
>> +static int evdev_mute(struct evdev *evdev, struct evdev_client *client)
>> +{
>> + client->muted = 1;
>> +
>> + return 0;
>> +}
>> +
>> +static int evdev_unmute(struct evdev *evdev, struct evdev_client *client)
>> +{
>> + client->muted = 0;
>> +
>> + return 0;
>> +}
>> +
>
> As I guess we want this feature to be atomic, we definitely need a
> lock here. We also want, after EVIOCMUTE returns, that no other ioctl
> will be allowed. Hence, we need a mutex that protects
> evdev_do_ioctl(). A spinlock around "muted" won't help..
> As we currently have no suitable lock in evdev_client I guess we need
> to add one. In this case you can also remove these helpers and do:
> client->muted = !!p;
> in evdev_do_ioctl().
>
> Note that a reader-writer lock would allow parallel ioctl execution if
> only the EVIOCMUTE command takes the write-lock and the other ioctls
> take a read lock.
As discussed on IRC, the evdev-mutex is definitely enough. I missed
that.. sorry.
Apart from that, I found another thing that we need to discuss:
Stopping force-feedback effects. If we mute a client, we should stop
all FF effects of that client that are currently ongoing. The kernel
already tracks them so this is quite easy to achieve.
On the other hand, clients need write access for FF. With write access
they can do all other kinds of stuff like grabbing the device or
calling EVIOCMUTE(0). Hence, it seems a bit stupid to allow clients
write access in scenarios where EVIOCMUTE is used. Same is true for
write()ing input_events to the device..
So is EVIOCMUTE intended for read-access only? Apart from FF it is
probably fine to tunnel all write-access through the compositor as
they are pretty rare. But for FF effects, clients need write access.
Of course, we can tunnel these through the compositor, too, but how
does that affect performance?
I think we should at least discuss this so we don't end up with
EVIOCMUTE but grant write access for gamepad/joystick devices and
basically render EVIOCMUTE uneffective..
I'm currently looking into a wl_keyboard/wl_mouse extension that
passes FDs optionally to clients so they can use this while being
fullscreen. I will post this here so we have a good example for this
feature.
Regards
David
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
