| Message ID | 1366120697-25841-1-git-send-email-krh@bitplanet.net (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Kristian On Tue, Apr 16, 2013 at 3:58 PM, Kristian Høgsberg <krh@bitplanet.net> 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@bitplanet.net> > --- > 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. Regards David > static long evdev_do_ioctl(struct file *file, unsigned int cmd, > void __user *p, int compat_mode) > { > @@ -687,12 +703,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, > unsigned int size; > int error; > > - /* First we check for fixed-length commands */ > + /* Handle ioctls allowed in muted mode first */ > switch (cmd) { > + case EVIOCMUTE: > + if (!capable(CAP_SYS_ADMIN)) > + return -EACCES; > + > + if (p) > + return evdev_mute(evdev, client); > + else > + return evdev_unmute(evdev, client); > > case EVIOCGVERSION: > return put_user(EV_VERSION, ip); > > + default: > + if (client->muted) > + return -EACCES; > + } > + > + /* Now check for fixed-length commands */ > + switch (cmd) { > case EVIOCGID: > if (copy_to_user(p, &dev->id, sizeof(struct input_id))) > return -EFAULT; > diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h > index 935119c..75eda72 100644 > --- a/include/uapi/linux/input.h > +++ b/include/uapi/linux/input.h > @@ -154,6 +154,7 @@ struct input_keymap_entry { > #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */ > > #define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */ > +#define EVIOCMUTE _IOW('E', 0xb0, int) /* Mute event delivery */ > > /* > * Device properties and quirks > -- > 1.8.1.4 > > -- > To unsubscribe from this list: send the line "unsubscribe linux-input" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Kristian On Wed, Apr 17, 2013 at 7:55 PM, David Herrmann <dh.herrmann@gmail.com> wrote: > Hi Kristian > > On Tue, Apr 16, 2013 at 3:58 PM, Kristian Høgsberg <krh@bitplanet.net> 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@bitplanet.net> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; 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); 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; +} + static long evdev_do_ioctl(struct file *file, unsigned int cmd, void __user *p, int compat_mode) { @@ -687,12 +703,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd, unsigned int size; int error; - /* First we check for fixed-length commands */ + /* Handle ioctls allowed in muted mode first */ switch (cmd) { + case EVIOCMUTE: + if (!capable(CAP_SYS_ADMIN)) + return -EACCES; + + if (p) + return evdev_mute(evdev, client); + else + return evdev_unmute(evdev, client); case EVIOCGVERSION: return put_user(EV_VERSION, ip); + default: + if (client->muted) + return -EACCES; + } + + /* Now check for fixed-length commands */ + switch (cmd) { case EVIOCGID: if (copy_to_user(p, &dev->id, sizeof(struct input_id))) return -EFAULT; diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h index 935119c..75eda72 100644 --- a/include/uapi/linux/input.h +++ b/include/uapi/linux/input.h @@ -154,6 +154,7 @@ struct input_keymap_entry { #define EVIOCGRAB _IOW('E', 0x90, int) /* Grab/Release device */ #define EVIOCSCLOCKID _IOW('E', 0xa0, int) /* Set clockid to be used for timestamps */ +#define EVIOCMUTE _IOW('E', 0xb0, int) /* Mute event delivery */ /* * Device properties and quirks
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. Signed-off-by: Kristian Høgsberg <krh@bitplanet.net> --- 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