[PATCH] Input: evdev - add EVIOCREVOKE ioctl

[PATCH] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

If we have multiple sessions on a system, we normally don't want
background sessions to read input events. Otherwise, it could capture
passwords and more entered by the user on the foreground session. This is
a real world problem as the recent XMir development showed:
  http://mjg59.dreamwidth.org/27327.html

We currently rely on sessions to release input devices when being
deactivated. This relies on trust across sessions. But that's not given on
usual systems. We therefore need a way to control which processes have
access to input devices.

With VTs the kernel simply routed them through the active /dev/ttyX. This
is not possible with evdev devices, though. Moreover, we want to avoid
routing input-devices through some dispatcher-daemon in userspace (which
would add some latency).

This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
device-access irrecoverably for that *single* open-file. Hence, once you
call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
rather useless now (but still valid compared to close()!). This allows us
to pass fds directly to session-processes from a trusted source. The
source keeps a dup()ed fd and revokes access once the session-process is
no longer active.
Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
restriction now as there is no way to revive the fd again. Hence, a user
is free to call EVIOCREVOKE themself to kill the fd.

Additionally, this ioctl allows multi-layer access-control (again compared
to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
layer can simply request a new open-file from the layer above and pass it
to the layer below. Now each layer can call EVIOCREVOKE on the fds to
revoke access for all layers below, at the expense of one fd per layer.

There's already ongoing experimental user-space work which demonstrates
how it can be used:
  http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 drivers/input/evdev.c      | 30 +++++++++++++++++++++++++++++-
 include/uapi/linux/input.h |  1 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..2ea70ec 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;
+	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
 	struct input_event event;
 	bool wakeup = false;
 
+	if (client->revoked)
+		return;
+
 	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
 				      mono : real);
 
@@ -795,6 +799,15 @@ static int evdev_handle_mt_request(struct input_dev *dev,
 	return 0;
 }
 
+static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
+			struct file *file)
+{
+	client->revoked = true;
+	input_flush_device(&evdev->handle, file);
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -808,12 +821,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	unsigned int size;
 	int error;
 
-	/* First we check for fixed-length commands */
+	/* First check for ioctls allowed while revoked */
 	switch (cmd) {
 
 	case EVIOCGVERSION:
 		return put_user(EV_VERSION, ip);
 
+	case EVIOCREVOKE:
+		if (p)
+			return -EINVAL;
+		else
+			return evdev_revoke(evdev, client, file);
+
+	default:
+		if (client->revoked)
+			return -EACCES;
+		break;
+	}
+
+	/* Then 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 2fb6fae..d61c61c 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -152,6 +152,7 @@ struct input_keymap_entry {
 #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
+#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
-- 
1.8.4

[PATCH v2] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

If we have multiple sessions on a system, we normally don't want
background sessions to read input events. Otherwise, it could capture
passwords and more entered by the user on the foreground session. This is
a real world problem as the recent XMir development showed:
  http://mjg59.dreamwidth.org/27327.html

We currently rely on sessions to release input devices when being
deactivated. This relies on trust across sessions. But that's not given on
usual systems. We therefore need a way to control which processes have
access to input devices.

With VTs the kernel simply routed them through the active /dev/ttyX. This
is not possible with evdev devices, though. Moreover, we want to avoid
routing input-devices through some dispatcher-daemon in userspace (which
would add some latency).

This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
device-access irrecoverably for that *single* open-file. Hence, once you
call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
rather useless now (but still valid compared to close()!). This allows us
to pass fds directly to session-processes from a trusted source. The
source keeps a dup()ed fd and revokes access once the session-process is
no longer active.
Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
restriction now as there is no way to revive the fd again. Hence, a user
is free to call EVIOCREVOKE themself to kill the fd.

Additionally, this ioctl allows multi-layer access-control (again compared
to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
layer can simply request a new open-file from the layer above and pass it
to the layer below. Now each layer can call EVIOCREVOKE on the fds to
revoke access for all layers below, at the expense of one fd per layer.

There's already ongoing experimental user-space work which demonstrates
how it can be used:
  http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
 - ungrab device during revoke
 - wake-up blocking read()s of the client
 - return -EACCES from write() for revoked clients
 - signal POLLHUP | POLLERR for revoked clients
 - don't signal POLLOUT for revoked clients

 drivers/input/evdev.c      | 43 ++++++++++++++++++++++++++++++++++++++++---
 include/uapi/linux/input.h |  1 +
 2 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..f2abd76 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;
+	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
 	struct input_event event;
 	bool wakeup = false;
 
+	if (client->revoked)
+		return;
+
 	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
 				      mono : real);
 
@@ -432,6 +436,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 	if (!evdev->exist) {
 		retval = -ENODEV;
 		goto out;
+	} else if (client->revoked) {
+		retval = -EACCES;
+		goto out;
 	}
 
 	while (retval + input_event_size() <= count) {
@@ -511,7 +518,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
 		if (!(file->f_flags & O_NONBLOCK)) {
 			error = wait_event_interruptible(evdev->wait,
 					client->packet_head != client->tail ||
-					!evdev->exist);
+					!evdev->exist || client->revoked);
 			if (error)
 				return error;
 		}
@@ -529,7 +536,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &evdev->wait, wait);
 
-	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
+	if (evdev->exist && !client->revoked)
+		mask = POLLOUT | POLLWRNORM;
+	else
+		mask = POLLHUP | POLLERR;
+
 	if (client->packet_head != client->tail)
 		mask |= POLLIN | POLLRDNORM;
 
@@ -795,6 +806,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
 	return 0;
 }
 
+static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
+			struct file *file)
+{
+	client->revoked = true;
+	evdev_ungrab(evdev, client);
+	input_flush_device(&evdev->handle, file);
+	wake_up_interruptible(&evdev->wait);
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -808,12 +830,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 	unsigned int size;
 	int error;
 
-	/* First we check for fixed-length commands */
+	/* First check for ioctls allowed while revoked */
 	switch (cmd) {
 
 	case EVIOCGVERSION:
 		return put_user(EV_VERSION, ip);
 
+	case EVIOCREVOKE:
+		if (p)
+			return -EINVAL;
+		else
+			return evdev_revoke(evdev, client, file);
+
+	default:
+		if (client->revoked)
+			return -EACCES;
+		break;
+	}
+
+	/* Then 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 2fb6fae..d61c61c 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -152,6 +152,7 @@ struct input_keymap_entry {
 #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
+#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
-- 
1.8.4

Re: [PATCH v2] Input: evdev - add EVIOCREVOKE ioctl

[Dmitry Torokhov]

Hi David,

On Tue, Aug 27, 2013 at 01:39:49PM +0200, David Herrmann wrote:
> If we have multiple sessions on a system, we normally don't want
> background sessions to read input events. Otherwise, it could capture
> passwords and more entered by the user on the foreground session. This is
> a real world problem as the recent XMir development showed:
>   http://mjg59.dreamwidth.org/27327.html
> 
> We currently rely on sessions to release input devices when being
> deactivated. This relies on trust across sessions. But that's not given on
> usual systems. We therefore need a way to control which processes have
> access to input devices.
> 
> With VTs the kernel simply routed them through the active /dev/ttyX. This
> is not possible with evdev devices, though. Moreover, we want to avoid
> routing input-devices through some dispatcher-daemon in userspace (which
> would add some latency).
> 
> This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
> device-access irrecoverably for that *single* open-file. Hence, once you
> call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
> rather useless now (but still valid compared to close()!). This allows us
> to pass fds directly to session-processes from a trusted source. The
> source keeps a dup()ed fd and revokes access once the session-process is
> no longer active.
> Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
> restriction now as there is no way to revive the fd again. Hence, a user
> is free to call EVIOCREVOKE themself to kill the fd.
> 
> Additionally, this ioctl allows multi-layer access-control (again compared
> to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
> layer can simply request a new open-file from the layer above and pass it
> to the layer below. Now each layer can call EVIOCREVOKE on the fds to
> revoke access for all layers below, at the expense of one fd per layer.
> 
> There's already ongoing experimental user-space work which demonstrates
> how it can be used:
>   http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
> 
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
>  - ungrab device during revoke
>  - wake-up blocking read()s of the client
>  - return -EACCES from write() for revoked clients
>  - signal POLLHUP | POLLERR for revoked clients
>  - don't signal POLLOUT for revoked clients
> 
>  drivers/input/evdev.c      | 43 ++++++++++++++++++++++++++++++++++++++++---
>  include/uapi/linux/input.h |  1 +
>  2 files changed, 41 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index d2b34fb..f2abd76 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;
> +	bool revoked;
>  	unsigned int bufsize;
>  	struct input_event buffer[];
>  };
> @@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
>  	struct input_event event;
>  	bool wakeup = false;
>  
> +	if (client->revoked)
> +		return;
> +
>  	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>  				      mono : real);
>  
> @@ -432,6 +436,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>  	if (!evdev->exist) {
>  		retval = -ENODEV;
>  		goto out;
> +	} else if (client->revoked) {
> +		retval = -EACCES;
> +		goto out;

Why do we treat revoke differently form device going away? I'd return
-ENODEV in both cases.

>  	}
>  
>  	while (retval + input_event_size() <= count) {
> @@ -511,7 +518,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>  		if (!(file->f_flags & O_NONBLOCK)) {
>  			error = wait_event_interruptible(evdev->wait,
>  					client->packet_head != client->tail ||
> -					!evdev->exist);
> +					!evdev->exist || client->revoked);
>  			if (error)
>  				return error;
>  		}
> @@ -529,7 +536,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
>  
>  	poll_wait(file, &evdev->wait, wait);
>  
> -	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
> +	if (evdev->exist && !client->revoked)
> +		mask = POLLOUT | POLLWRNORM;
> +	else
> +		mask = POLLHUP | POLLERR;
> +
>  	if (client->packet_head != client->tail)
>  		mask |= POLLIN | POLLRDNORM;
>  
> @@ -795,6 +806,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
>  	return 0;
>  }
>  
> +static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
> +			struct file *file)
> +{
> +	client->revoked = true;
> +	evdev_ungrab(evdev, client);
> +	input_flush_device(&evdev->handle, file);
> +	wake_up_interruptible(&evdev->wait);
> +
> +	return 0;
> +}
> +
>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  			   void __user *p, int compat_mode)
>  {
> @@ -808,12 +830,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>  	unsigned int size;
>  	int error;
>  
> -	/* First we check for fixed-length commands */
> +	/* First check for ioctls allowed while revoked */
>  	switch (cmd) {
>  
>  	case EVIOCGVERSION:
>  		return put_user(EV_VERSION, ip);
>  
> +	case EVIOCREVOKE:
> +		if (p)
> +			return -EINVAL;
> +		else
> +			return evdev_revoke(evdev, client, file);
> +
> +	default:
> +		if (client->revoked)
> +			return -EACCES;
> +		break;
> +	}

Here as well, I'd check revoked in the same place where we check exist
and bail if device gone away or our access was revoked.

> +
> +	/* Then 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 2fb6fae..d61c61c 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -152,6 +152,7 @@ struct input_keymap_entry {
>  #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
>  
>  #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
> +#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
>  
>  #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
>  
> -- 
> 1.8.4
> 

Thanks.

-- 
Dmitry

Re: [PATCH v2] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

Hi

On Wed, Aug 28, 2013 at 12:17 AM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> Hi David,
>
> On Tue, Aug 27, 2013 at 01:39:49PM +0200, David Herrmann wrote:
>> If we have multiple sessions on a system, we normally don't want
>> background sessions to read input events. Otherwise, it could capture
>> passwords and more entered by the user on the foreground session. This is
>> a real world problem as the recent XMir development showed:
>>   http://mjg59.dreamwidth.org/27327.html
>>
>> We currently rely on sessions to release input devices when being
>> deactivated. This relies on trust across sessions. But that's not given on
>> usual systems. We therefore need a way to control which processes have
>> access to input devices.
>>
>> With VTs the kernel simply routed them through the active /dev/ttyX. This
>> is not possible with evdev devices, though. Moreover, we want to avoid
>> routing input-devices through some dispatcher-daemon in userspace (which
>> would add some latency).
>>
>> This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
>> device-access irrecoverably for that *single* open-file. Hence, once you
>> call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
>> rather useless now (but still valid compared to close()!). This allows us
>> to pass fds directly to session-processes from a trusted source. The
>> source keeps a dup()ed fd and revokes access once the session-process is
>> no longer active.
>> Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
>> restriction now as there is no way to revive the fd again. Hence, a user
>> is free to call EVIOCREVOKE themself to kill the fd.
>>
>> Additionally, this ioctl allows multi-layer access-control (again compared
>> to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
>> layer can simply request a new open-file from the layer above and pass it
>> to the layer below. Now each layer can call EVIOCREVOKE on the fds to
>> revoke access for all layers below, at the expense of one fd per layer.
>>
>> There's already ongoing experimental user-space work which demonstrates
>> how it can be used:
>>   http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
>>
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> v2:
>>  - ungrab device during revoke
>>  - wake-up blocking read()s of the client
>>  - return -EACCES from write() for revoked clients
>>  - signal POLLHUP | POLLERR for revoked clients
>>  - don't signal POLLOUT for revoked clients
>>
>>  drivers/input/evdev.c      | 43 ++++++++++++++++++++++++++++++++++++++++---
>>  include/uapi/linux/input.h |  1 +
>>  2 files changed, 41 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index d2b34fb..f2abd76 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;
>> +     bool revoked;
>>       unsigned int bufsize;
>>       struct input_event buffer[];
>>  };
>> @@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
>>       struct input_event event;
>>       bool wakeup = false;
>>
>> +     if (client->revoked)
>> +             return;
>> +
>>       event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>>                                     mono : real);
>>
>> @@ -432,6 +436,9 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>>       if (!evdev->exist) {
>>               retval = -ENODEV;
>>               goto out;
>> +     } else if (client->revoked) {
>> +             retval = -EACCES;
>> +             goto out;
>
> Why do we treat revoke differently form device going away? I'd return
> -ENODEV in both cases.

My plan was to allow applications to react to revoke. So if someone
remotely revokes their fd, they notice it via EACCES, not via ENODEV.
They could just close the fd but keep contexts around (they may get a
new fd soon). If they receive ENODEV (or some other error), they could
drop all their contexts and just remove the device.

However, I just noticed that it is very unlikely that they're revoked
during write(). It's more likely during poll() (which returns POLLHUP
for both). So I guess you're right. Will rethink it again, but if I
don't come up with some idea I'll just drop it for v3.

Thanks!
David

>>       }
>>
>>       while (retval + input_event_size() <= count) {
>> @@ -511,7 +518,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>>               if (!(file->f_flags & O_NONBLOCK)) {
>>                       error = wait_event_interruptible(evdev->wait,
>>                                       client->packet_head != client->tail ||
>> -                                     !evdev->exist);
>> +                                     !evdev->exist || client->revoked);
>>                       if (error)
>>                               return error;
>>               }
>> @@ -529,7 +536,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
>>
>>       poll_wait(file, &evdev->wait, wait);
>>
>> -     mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
>> +     if (evdev->exist && !client->revoked)
>> +             mask = POLLOUT | POLLWRNORM;
>> +     else
>> +             mask = POLLHUP | POLLERR;
>> +
>>       if (client->packet_head != client->tail)
>>               mask |= POLLIN | POLLRDNORM;
>>
>> @@ -795,6 +806,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
>>       return 0;
>>  }
>>
>> +static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>> +                     struct file *file)
>> +{
>> +     client->revoked = true;
>> +     evdev_ungrab(evdev, client);
>> +     input_flush_device(&evdev->handle, file);
>> +     wake_up_interruptible(&evdev->wait);
>> +
>> +     return 0;
>> +}
>> +
>>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>                          void __user *p, int compat_mode)
>>  {
>> @@ -808,12 +830,27 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>       unsigned int size;
>>       int error;
>>
>> -     /* First we check for fixed-length commands */
>> +     /* First check for ioctls allowed while revoked */
>>       switch (cmd) {
>>
>>       case EVIOCGVERSION:
>>               return put_user(EV_VERSION, ip);
>>
>> +     case EVIOCREVOKE:
>> +             if (p)
>> +                     return -EINVAL;
>> +             else
>> +                     return evdev_revoke(evdev, client, file);
>> +
>> +     default:
>> +             if (client->revoked)
>> +                     return -EACCES;
>> +             break;
>> +     }
>
> Here as well, I'd check revoked in the same place where we check exist
> and bail if device gone away or our access was revoked.
>
>> +
>> +     /* Then 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 2fb6fae..d61c61c 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
>> @@ -152,6 +152,7 @@ struct input_keymap_entry {
>>  #define EVIOCGEFFECTS                _IOR('E', 0x84, int)                    /* Report number of effects playable at the same time */
>>
>>  #define EVIOCGRAB            _IOW('E', 0x90, int)                    /* Grab/Release device */
>> +#define EVIOCREVOKE          _IOW('E', 0x91, int)                    /* Revoke device access */
>>
>>  #define EVIOCSCLOCKID                _IOW('E', 0xa0, int)                    /* Set clockid to be used for timestamps */
>>
>> --
>> 1.8.4
>>
>
> Thanks.
>
> --
> Dmitry

[PATCH v3] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

If we have multiple sessions on a system, we normally don't want
background sessions to read input events. Otherwise, it could capture
passwords and more entered by the user on the foreground session. This is
a real world problem as the recent XMir development showed:
  http://mjg59.dreamwidth.org/27327.html

We currently rely on sessions to release input devices when being
deactivated. This relies on trust across sessions. But that's not given on
usual systems. We therefore need a way to control which processes have
access to input devices.

With VTs the kernel simply routed them through the active /dev/ttyX. This
is not possible with evdev devices, though. Moreover, we want to avoid
routing input-devices through some dispatcher-daemon in userspace (which
would add some latency).

This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
device-access irrecoverably for that *single* open-file. Hence, once you
call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
rather useless now (but still valid compared to close()!). This allows us
to pass fds directly to session-processes from a trusted source. The
source keeps a dup()ed fd and revokes access once the session-process is
no longer active.
Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
restriction now as there is no way to revive the fd again. Hence, a user
is free to call EVIOCREVOKE themself to kill the fd.

Additionally, this ioctl allows multi-layer access-control (again compared
to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
layer can simply request a new open-file from the layer above and pass it
to the layer below. Now each layer can call EVIOCREVOKE on the fds to
revoke access for all layers below, at the expense of one fd per layer.

There's already ongoing experimental user-space work which demonstrates
how it can be used:
  http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Changes from v2:
 - Handle client->revoked like evdev->exist to avoid special-casing "revoked"

 drivers/input/evdev.c      | 33 +++++++++++++++++++++++++++++----
 include/uapi/linux/input.h |  1 +
 2 files changed, 30 insertions(+), 4 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..cf6bdff 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;
+	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
 	struct input_event event;
 	bool wakeup = false;
 
+	if (client->revoked)
+		return;
+
 	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
 				      mono : real);
 
@@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 	if (retval)
 		return retval;
 
-	if (!evdev->exist) {
+	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
 		if (!(file->f_flags & O_NONBLOCK)) {
 			error = wait_event_interruptible(evdev->wait,
 					client->packet_head != client->tail ||
-					!evdev->exist);
+					!evdev->exist || client->revoked);
 			if (error)
 				return error;
 		}
@@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &evdev->wait, wait);
 
-	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
+	if (evdev->exist && !client->revoked)
+		mask = POLLOUT | POLLWRNORM;
+	else
+		mask = POLLHUP | POLLERR;
+
 	if (client->packet_head != client->tail)
 		mask |= POLLIN | POLLRDNORM;
 
@@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
 	return 0;
 }
 
+static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
+			struct file *file)
+{
+	client->revoked = true;
+	evdev_ungrab(evdev, client);
+	input_flush_device(&evdev->handle, file);
+	wake_up_interruptible(&evdev->wait);
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCREVOKE:
+		if (p)
+			return -EINVAL;
+		else
+			return evdev_revoke(evdev, client, file);
+
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
@@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
 	if (retval)
 		return retval;
 
-	if (!evdev->exist) {
+	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
 		goto out;
 	}
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 2fb6fae..d61c61c 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -152,6 +152,7 @@ struct input_keymap_entry {
 #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
+#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
-- 
1.8.4

Re: [PATCH v3] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

Hi Dmitry

On Sun, Sep 1, 2013 at 4:07 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
> If we have multiple sessions on a system, we normally don't want
> background sessions to read input events. Otherwise, it could capture
> passwords and more entered by the user on the foreground session. This is
> a real world problem as the recent XMir development showed:
>   http://mjg59.dreamwidth.org/27327.html
>
> We currently rely on sessions to release input devices when being
> deactivated. This relies on trust across sessions. But that's not given on
> usual systems. We therefore need a way to control which processes have
> access to input devices.
>
> With VTs the kernel simply routed them through the active /dev/ttyX. This
> is not possible with evdev devices, though. Moreover, we want to avoid
> routing input-devices through some dispatcher-daemon in userspace (which
> would add some latency).
>
> This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
> device-access irrecoverably for that *single* open-file. Hence, once you
> call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
> rather useless now (but still valid compared to close()!). This allows us
> to pass fds directly to session-processes from a trusted source. The
> source keeps a dup()ed fd and revokes access once the session-process is
> no longer active.
> Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
> restriction now as there is no way to revive the fd again. Hence, a user
> is free to call EVIOCREVOKE themself to kill the fd.
>
> Additionally, this ioctl allows multi-layer access-control (again compared
> to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
> layer can simply request a new open-file from the layer above and pass it
> to the layer below. Now each layer can call EVIOCREVOKE on the fds to
> revoke access for all layers below, at the expense of one fd per layer.
>
> There's already ongoing experimental user-space work which demonstrates
> how it can be used:
>   http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

I've been running this logind-feature for some time on my machine now.
I haven't seen any problems. Could you let us know what your plans are
for this patch?
3.12 would be very nice so we could continue pushing the user-space parts.
There are even existing programs (like weston-launch) which could
already make great use of that with just a <10 line patch.

Thanks
David

> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> Changes from v2:
>  - Handle client->revoked like evdev->exist to avoid special-casing "revoked"
>
>  drivers/input/evdev.c      | 33 +++++++++++++++++++++++++++++----
>  include/uapi/linux/input.h |  1 +
>  2 files changed, 30 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
> index d2b34fb..cf6bdff 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;
> +       bool revoked;
>         unsigned int bufsize;
>         struct input_event buffer[];
>  };
> @@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
>         struct input_event event;
>         bool wakeup = false;
>
> +       if (client->revoked)
> +               return;
> +
>         event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>                                       mono : real);
>
> @@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>         if (retval)
>                 return retval;
>
> -       if (!evdev->exist) {
> +       if (!evdev->exist || client->revoked) {
>                 retval = -ENODEV;
>                 goto out;
>         }
> @@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>                 if (!(file->f_flags & O_NONBLOCK)) {
>                         error = wait_event_interruptible(evdev->wait,
>                                         client->packet_head != client->tail ||
> -                                       !evdev->exist);
> +                                       !evdev->exist || client->revoked);
>                         if (error)
>                                 return error;
>                 }
> @@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
>
>         poll_wait(file, &evdev->wait, wait);
>
> -       mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
> +       if (evdev->exist && !client->revoked)
> +               mask = POLLOUT | POLLWRNORM;
> +       else
> +               mask = POLLHUP | POLLERR;
> +
>         if (client->packet_head != client->tail)
>                 mask |= POLLIN | POLLRDNORM;
>
> @@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
>         return 0;
>  }
>
> +static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
> +                       struct file *file)
> +{
> +       client->revoked = true;
> +       evdev_ungrab(evdev, client);
> +       input_flush_device(&evdev->handle, file);
> +       wake_up_interruptible(&evdev->wait);
> +
> +       return 0;
> +}
> +
>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                            void __user *p, int compat_mode)
>  {
> @@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>                 else
>                         return evdev_ungrab(evdev, client);
>
> +       case EVIOCREVOKE:
> +               if (p)
> +                       return -EINVAL;
> +               else
> +                       return evdev_revoke(evdev, client, file);
> +
>         case EVIOCSCLOCKID:
>                 if (copy_from_user(&i, p, sizeof(unsigned int)))
>                         return -EFAULT;
> @@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
>         if (retval)
>                 return retval;
>
> -       if (!evdev->exist) {
> +       if (!evdev->exist || client->revoked) {
>                 retval = -ENODEV;
>                 goto out;
>         }
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 2fb6fae..d61c61c 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -152,6 +152,7 @@ struct input_keymap_entry {
>  #define EVIOCGEFFECTS          _IOR('E', 0x84, int)                    /* Report number of effects playable at the same time */
>
>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
> +#define EVIOCREVOKE            _IOW('E', 0x91, int)                    /* Revoke device access */
>
>  #define EVIOCSCLOCKID          _IOW('E', 0xa0, int)                    /* Set clockid to be used for timestamps */
>
> --
> 1.8.4
>

Re: [PATCH v3] Input: evdev - add EVIOCREVOKE ioctl

[Kristian Høgsberg]

On Fri, Sep 6, 2013 at 10:12 AM, David Herrmann <dh.herrmann@gmail.com> wrote:
> Hi Dmitry
>
> On Sun, Sep 1, 2013 at 4:07 PM, David Herrmann <dh.herrmann@gmail.com> wrote:
>> If we have multiple sessions on a system, we normally don't want
>> background sessions to read input events. Otherwise, it could capture
>> passwords and more entered by the user on the foreground session. This is
>> a real world problem as the recent XMir development showed:
>>   http://mjg59.dreamwidth.org/27327.html
>>
>> We currently rely on sessions to release input devices when being
>> deactivated. This relies on trust across sessions. But that's not given on
>> usual systems. We therefore need a way to control which processes have
>> access to input devices.
>>
>> With VTs the kernel simply routed them through the active /dev/ttyX. This
>> is not possible with evdev devices, though. Moreover, we want to avoid
>> routing input-devices through some dispatcher-daemon in userspace (which
>> would add some latency).
>>
>> This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
>> device-access irrecoverably for that *single* open-file. Hence, once you
>> call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
>> rather useless now (but still valid compared to close()!). This allows us
>> to pass fds directly to session-processes from a trusted source. The
>> source keeps a dup()ed fd and revokes access once the session-process is
>> no longer active.
>> Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
>> restriction now as there is no way to revive the fd again. Hence, a user
>> is free to call EVIOCREVOKE themself to kill the fd.
>>
>> Additionally, this ioctl allows multi-layer access-control (again compared
>> to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
>> layer can simply request a new open-file from the layer above and pass it
>> to the layer below. Now each layer can call EVIOCREVOKE on the fds to
>> revoke access for all layers below, at the expense of one fd per layer.
>>
>> There's already ongoing experimental user-space work which demonstrates
>> how it can be used:
>>   http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
>
> I've been running this logind-feature for some time on my machine now.
> I haven't seen any problems. Could you let us know what your plans are
> for this patch?
> 3.12 would be very nice so we could continue pushing the user-space parts.
> There are even existing programs (like weston-launch) which could
> already make great use of that with just a <10 line patch.

I think we have good use cases for this - with revoke we can give
input fds to a non-trusted, non-root display-severs and standalone
KMS+evdev applications and reliably revoke them when their session is
deactivated (vt switched away typically).  EVIOCREVOKE is usable in
logind and standalone setuid helpers such as weston-launch,.
Everything other priviledge operation a display server needs can be
done in logind or weston-launch, this ioctl is the final piece that
lets us run X without root priviledges.

Reviewed-by: Kristian Høgsberg <krh@bitplanet.net>

>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>> Changes from v2:
>>  - Handle client->revoked like evdev->exist to avoid special-casing "revoked"
>>
>>  drivers/input/evdev.c      | 33 +++++++++++++++++++++++++++++----
>>  include/uapi/linux/input.h |  1 +
>>  2 files changed, 30 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>> index d2b34fb..cf6bdff 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;
>> +       bool revoked;
>>         unsigned int bufsize;
>>         struct input_event buffer[];
>>  };
>> @@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
>>         struct input_event event;
>>         bool wakeup = false;
>>
>> +       if (client->revoked)
>> +               return;
>> +
>>         event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
>>                                       mono : real);
>>
>> @@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
>>         if (retval)
>>                 return retval;
>>
>> -       if (!evdev->exist) {
>> +       if (!evdev->exist || client->revoked) {
>>                 retval = -ENODEV;
>>                 goto out;
>>         }
>> @@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
>>                 if (!(file->f_flags & O_NONBLOCK)) {
>>                         error = wait_event_interruptible(evdev->wait,
>>                                         client->packet_head != client->tail ||
>> -                                       !evdev->exist);
>> +                                       !evdev->exist || client->revoked);
>>                         if (error)
>>                                 return error;
>>                 }
>> @@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
>>
>>         poll_wait(file, &evdev->wait, wait);
>>
>> -       mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
>> +       if (evdev->exist && !client->revoked)
>> +               mask = POLLOUT | POLLWRNORM;
>> +       else
>> +               mask = POLLHUP | POLLERR;
>> +
>>         if (client->packet_head != client->tail)
>>                 mask |= POLLIN | POLLRDNORM;
>>
>> @@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
>>         return 0;
>>  }
>>
>> +static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
>> +                       struct file *file)
>> +{
>> +       client->revoked = true;
>> +       evdev_ungrab(evdev, client);
>> +       input_flush_device(&evdev->handle, file);
>> +       wake_up_interruptible(&evdev->wait);
>> +
>> +       return 0;
>> +}
>> +
>>  static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>                            void __user *p, int compat_mode)
>>  {
>> @@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
>>                 else
>>                         return evdev_ungrab(evdev, client);
>>
>> +       case EVIOCREVOKE:
>> +               if (p)
>> +                       return -EINVAL;
>> +               else
>> +                       return evdev_revoke(evdev, client, file);
>> +
>>         case EVIOCSCLOCKID:
>>                 if (copy_from_user(&i, p, sizeof(unsigned int)))
>>                         return -EFAULT;
>> @@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
>>         if (retval)
>>                 return retval;
>>
>> -       if (!evdev->exist) {
>> +       if (!evdev->exist || client->revoked) {
>>                 retval = -ENODEV;
>>                 goto out;
>>         }
>> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>> index 2fb6fae..d61c61c 100644
>> --- a/include/uapi/linux/input.h
>> +++ b/include/uapi/linux/input.h
>> @@ -152,6 +152,7 @@ struct input_keymap_entry {
>>  #define EVIOCGEFFECTS          _IOR('E', 0x84, int)                    /* Report number of effects playable at the same time */
>>
>>  #define EVIOCGRAB              _IOW('E', 0x90, int)                    /* Grab/Release device */
>> +#define EVIOCREVOKE            _IOW('E', 0x91, int)                    /* Revoke device access */
>>
>>  #define EVIOCSCLOCKID          _IOW('E', 0xa0, int)                    /* Set clockid to be used for timestamps */
>>
>> --
>> 1.8.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

[PATCH v4] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

If we have multiple sessions on a system, we normally don't want
background sessions to read input events. Otherwise, it could capture
passwords and more entered by the user on the foreground session. This is
a real world problem as the recent XMir development showed:
  http://mjg59.dreamwidth.org/27327.html

We currently rely on sessions to release input devices when being
deactivated. This relies on trust across sessions. But that's not given on
usual systems. We therefore need a way to control which processes have
access to input devices.

With VTs the kernel simply routed them through the active /dev/ttyX. This
is not possible with evdev devices, though. Moreover, we want to avoid
routing input-devices through some dispatcher-daemon in userspace (which
would add some latency).

This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
device-access irrecoverably for that *single* open-file. Hence, once you
call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
rather useless now (but still valid compared to close()!). This allows us
to pass fds directly to session-processes from a trusted source. The
source keeps a dup()ed fd and revokes access once the session-process is
no longer active.
Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
restriction now as there is no way to revive the fd again. Hence, a user
is free to call EVIOCREVOKE themself to kill the fd.

Additionally, this ioctl allows multi-layer access-control (again compared
to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
layer can simply request a new open-file from the layer above and pass it
to the layer below. Now each layer can call EVIOCREVOKE on the fds to
revoke access for all layers below, at the expense of one fd per layer.

There's already ongoing experimental user-space work which demonstrates
how it can be used:
  http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
Hi Dmitry

Given the recent ABS_*-regression, I wrote a bunch of more aggressive
stress-tests for this. I didn't found any regressions if EVIOCREVOKE is not
used, but one with revoke on an empty queue in evdev_read(). Now fixed. Please
let me know what your plans for this patch are (-next or -rc1?) so we can
schedule accordingly. As a side note, will you attend LPC this year? We have a
bunch of fancy stuff I'd like your opinion on (including
device-properties, device-detection, ABS_* bit extension).

Thanks and cheers
David

 drivers/input/evdev.c      | 37 +++++++++++++++++++++++++++++++------
 include/uapi/linux/input.h |  1 +
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
index d2b34fb..b6ded17 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;
+	bool revoked;
 	unsigned int bufsize;
 	struct input_event buffer[];
 };
@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client *client,
 	struct input_event event;
 	bool wakeup = false;
 
+	if (client->revoked)
+		return;
+
 	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
 				      mono : real);
 
@@ -240,7 +244,7 @@ static int evdev_flush(struct file *file, fl_owner_t id)
 	if (retval)
 		return retval;
 
-	if (!evdev->exist)
+	if (!evdev->exist || client->revoked)
 		retval = -ENODEV;
 	else
 		retval = input_flush_device(&evdev->handle, file);
@@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const char __user *buffer,
 	if (retval)
 		return retval;
 
-	if (!evdev->exist) {
+	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
 		goto out;
 	}
@@ -482,7 +486,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
 		return -EINVAL;
 
 	for (;;) {
-		if (!evdev->exist)
+		if (!evdev->exist || client->revoked)
 			return -ENODEV;
 
 		if (client->packet_head == client->tail &&
@@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char __user *buffer,
 		if (!(file->f_flags & O_NONBLOCK)) {
 			error = wait_event_interruptible(evdev->wait,
 					client->packet_head != client->tail ||
-					!evdev->exist);
+					!evdev->exist || client->revoked);
 			if (error)
 				return error;
 		}
@@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file, poll_table *wait)
 
 	poll_wait(file, &evdev->wait, wait);
 
-	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
+	if (evdev->exist && !client->revoked)
+		mask = POLLOUT | POLLWRNORM;
+	else
+		mask = POLLHUP | POLLERR;
+
 	if (client->packet_head != client->tail)
 		mask |= POLLIN | POLLRDNORM;
 
@@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct input_dev *dev,
 	return 0;
 }
 
+static int evdev_revoke(struct evdev *evdev, struct evdev_client *client,
+			struct file *file)
+{
+	client->revoked = true;
+	evdev_ungrab(evdev, client);
+	input_flush_device(&evdev->handle, file);
+	wake_up_interruptible(&evdev->wait);
+
+	return 0;
+}
+
 static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 			   void __user *p, int compat_mode)
 {
@@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file, unsigned int cmd,
 		else
 			return evdev_ungrab(evdev, client);
 
+	case EVIOCREVOKE:
+		if (p)
+			return -EINVAL;
+		else
+			return evdev_revoke(evdev, client, file);
+
 	case EVIOCSCLOCKID:
 		if (copy_from_user(&i, p, sizeof(unsigned int)))
 			return -EFAULT;
@@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file *file, unsigned int cmd,
 	if (retval)
 		return retval;
 
-	if (!evdev->exist) {
+	if (!evdev->exist || client->revoked) {
 		retval = -ENODEV;
 		goto out;
 	}
diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
index 2fb6fae..d61c61c 100644
--- a/include/uapi/linux/input.h
+++ b/include/uapi/linux/input.h
@@ -152,6 +152,7 @@ struct input_keymap_entry {
 #define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of effects playable at the same time */
 
 #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
+#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
 
 #define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used for timestamps */
 
-- 
1.8.4

Re: [PATCH v4] Input: evdev - add EVIOCREVOKE ioctl

[Dmitry Torokhov]

David Herrmann <dh.herrmann@gmail.com> wrote:
>If we have multiple sessions on a system, we normally don't want
>background sessions to read input events. Otherwise, it could capture
>passwords and more entered by the user on the foreground session. This
>is
>a real world problem as the recent XMir development showed:
>  http://mjg59.dreamwidth.org/27327.html
>
>We currently rely on sessions to release input devices when being
>deactivated. This relies on trust across sessions. But that's not given
>on
>usual systems. We therefore need a way to control which processes have
>access to input devices.
>
>With VTs the kernel simply routed them through the active /dev/ttyX.
>This
>is not possible with evdev devices, though. Moreover, we want to avoid
>routing input-devices through some dispatcher-daemon in userspace
>(which
>would add some latency).
>
>This patch introduces EVIOCREVOKE. If called on an evdev fd, this
>revokes
>device-access irrecoverably for that *single* open-file. Hence, once
>you
>call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
>rather useless now (but still valid compared to close()!). This allows
>us
>to pass fds directly to session-processes from a trusted source. The
>source keeps a dup()ed fd and revokes access once the session-process
>is
>no longer active.
>Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
>restriction now as there is no way to revive the fd again. Hence, a
>user
>is free to call EVIOCREVOKE themself to kill the fd.
>
>Additionally, this ioctl allows multi-layer access-control (again
>compared
>to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A
>middle
>layer can simply request a new open-file from the layer above and pass
>it
>to the layer below. Now each layer can call EVIOCREVOKE on the fds to
>revoke access for all layers below, at the expense of one fd per layer.
>
>There's already ongoing experimental user-space work which demonstrates
>how it can be used:
>http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html
>
>Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>---
>Hi Dmitry
>
>Given the recent ABS_*-regression, I wrote a bunch of more aggressive
>stress-tests for this. I didn't found any regressions if EVIOCREVOKE is
>not
>used, but one with revoke on an empty queue in evdev_read(). Now fixed.
>Please
>let me know what your plans for this patch are (-next or -rc1?) so we
>can
>schedule accordingly.

I think this one is safer than extending axis numbers as there is no concerns about breaking stuff - it's brand new. So I think we can work it in -rc1.

> As a side note, will you attend LPC this year? We
>have a
>bunch of fancy stuff I'd like your opinion on (including
>device-properties, device-detection, ABS_* bit extension).

Yes. I'll get to new Orleans a couple days earlier (Sunday night) so if you are in town we could meet for drinks or such.

>
>Thanks and cheers
>David
>
> drivers/input/evdev.c      | 37 +++++++++++++++++++++++++++++++------
> include/uapi/linux/input.h |  1 +
> 2 files changed, 32 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/input/evdev.c b/drivers/input/evdev.c
>index d2b34fb..b6ded17 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;
>+	bool revoked;
> 	unsigned int bufsize;
> 	struct input_event buffer[];
> };
>@@ -164,6 +165,9 @@ static void evdev_pass_values(struct evdev_client
>*client,
> 	struct input_event event;
> 	bool wakeup = false;
> 
>+	if (client->revoked)
>+		return;
>+
> 	event.time = ktime_to_timeval(client->clkid == CLOCK_MONOTONIC ?
> 				      mono : real);
> 
>@@ -240,7 +244,7 @@ static int evdev_flush(struct file *file,
>fl_owner_t id)
> 	if (retval)
> 		return retval;
> 
>-	if (!evdev->exist)
>+	if (!evdev->exist || client->revoked)
> 		retval = -ENODEV;
> 	else
> 		retval = input_flush_device(&evdev->handle, file);
>@@ -429,7 +433,7 @@ static ssize_t evdev_write(struct file *file, const
>char __user *buffer,
> 	if (retval)
> 		return retval;
> 
>-	if (!evdev->exist) {
>+	if (!evdev->exist || client->revoked) {
> 		retval = -ENODEV;
> 		goto out;
> 	}
>@@ -482,7 +486,7 @@ static ssize_t evdev_read(struct file *file, char
>__user *buffer,
> 		return -EINVAL;
> 
> 	for (;;) {
>-		if (!evdev->exist)
>+		if (!evdev->exist || client->revoked)
> 			return -ENODEV;
> 
> 		if (client->packet_head == client->tail &&
>@@ -511,7 +515,7 @@ static ssize_t evdev_read(struct file *file, char
>__user *buffer,
> 		if (!(file->f_flags & O_NONBLOCK)) {
> 			error = wait_event_interruptible(evdev->wait,
> 					client->packet_head != client->tail ||
>-					!evdev->exist);
>+					!evdev->exist || client->revoked);
> 			if (error)
> 				return error;
> 		}
>@@ -529,7 +533,11 @@ static unsigned int evdev_poll(struct file *file,
>poll_table *wait)
> 
> 	poll_wait(file, &evdev->wait, wait);
> 
>-	mask = evdev->exist ? POLLOUT | POLLWRNORM : POLLHUP | POLLERR;
>+	if (evdev->exist && !client->revoked)
>+		mask = POLLOUT | POLLWRNORM;
>+	else
>+		mask = POLLHUP | POLLERR;
>+
> 	if (client->packet_head != client->tail)
> 		mask |= POLLIN | POLLRDNORM;
> 
>@@ -795,6 +803,17 @@ static int evdev_handle_mt_request(struct
>input_dev *dev,
> 	return 0;
> }
> 
>+static int evdev_revoke(struct evdev *evdev, struct evdev_client
>*client,
>+			struct file *file)
>+{
>+	client->revoked = true;
>+	evdev_ungrab(evdev, client);
>+	input_flush_device(&evdev->handle, file);
>+	wake_up_interruptible(&evdev->wait);
>+
>+	return 0;
>+}
>+
> static long evdev_do_ioctl(struct file *file, unsigned int cmd,
> 			   void __user *p, int compat_mode)
> {
>@@ -857,6 +876,12 @@ static long evdev_do_ioctl(struct file *file,
>unsigned int cmd,
> 		else
> 			return evdev_ungrab(evdev, client);
> 
>+	case EVIOCREVOKE:
>+		if (p)
>+			return -EINVAL;
>+		else
>+			return evdev_revoke(evdev, client, file);
>+
> 	case EVIOCSCLOCKID:
> 		if (copy_from_user(&i, p, sizeof(unsigned int)))
> 			return -EFAULT;
>@@ -1002,7 +1027,7 @@ static long evdev_ioctl_handler(struct file
>*file, unsigned int cmd,
> 	if (retval)
> 		return retval;
> 
>-	if (!evdev->exist) {
>+	if (!evdev->exist || client->revoked) {
> 		retval = -ENODEV;
> 		goto out;
> 	}
>diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
>index 2fb6fae..d61c61c 100644
>--- a/include/uapi/linux/input.h
>+++ b/include/uapi/linux/input.h
>@@ -152,6 +152,7 @@ struct input_keymap_entry {
>#define EVIOCGEFFECTS		_IOR('E', 0x84, int)			/* Report number of
>effects playable at the same time */
> 
> #define EVIOCGRAB		_IOW('E', 0x90, int)			/* Grab/Release device */
>+#define EVIOCREVOKE		_IOW('E', 0x91, int)			/* Revoke device access */
> 
>#define EVIOCSCLOCKID		_IOW('E', 0xa0, int)			/* Set clockid to be used
>for timestamps */
> 

Hi David,
Thanks.

-- 
Dmitry

Re: [PATCH v4] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

Hi Dmitry

On Sat, Sep 7, 2013 at 7:16 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> David Herrmann <dh.herrmann@gmail.com> wrote:
>>Given the recent ABS_*-regression, I wrote a bunch of more aggressive
>>stress-tests for this. I didn't found any regressions if EVIOCREVOKE is
>>not
>>used, but one with revoke on an empty queue in evdev_read(). Now fixed.
>>Please
>>let me know what your plans for this patch are (-next or -rc1?) so we
>>can
>>schedule accordingly.
>
> I think this one is safer than extending axis numbers as there is no concerns about breaking stuff - it's brand new. So I think we can work it in -rc1.

Sounds good, thanks.
The ABS-fixes will take some time, yepp. We can retry these with the
guitar/drums ABS bits for 3.13. EVIOC[SG]ABS2 is probably the way to
go.

>> As a side note, will you attend LPC this year? We
>>have a
>>bunch of fancy stuff I'd like your opinion on (including
>>device-properties, device-detection, ABS_* bit extension).
>
> Yes. I'll get to new Orleans a couple days earlier (Sunday night) so if you are in town we could meet for drinks or such.

I'll arrive there on Saturday-evening, will drop you a message. See
you in New Orleans.

Thanks
David

Re: [PATCH] Input: evdev - add EVIOCREVOKE ioctl

[Bruno Prémont]

Hi David,

Sorry for reviving this old thread (I didn't find more recent patch series
at first glance or have not been using the proper keyword while searching).

At FOSDEM 2015 last Sunday Hans presented libinput X input driver and mentioned
evdev FD revoking.

A question I raise was how are input devices to be put in a reasonable
state on FD revoking?
The specific case of force-feedback game-pads/wheels and the like that libinput
is expected to pass through to games was the main trigger.

Assume:
- Game triggers some force-feedback event like vibrating device
- Game looses focus and gets its evdev FD revoked
- Newly focused application does not care about the game-pad/wheel

How should the force-feedback activity get stopped on that focus change
and thus FD revoking?
Is the game expected to react before the FD being revoked (how long to
wait?) or should the kernel somehow reset the device to a sane state on
revoke (and if so, under which conditions?).

Should some other evdev devices also receive a special treatment to reset
them into a known/idle state (eventually LEDs on keyboards, beep, ...)?

Bruno

On Tue, 27 August 2013 David Herrmann <dh.herrmann@gmail.com> wrote:
> If we have multiple sessions on a system, we normally don't want
> background sessions to read input events. Otherwise, it could capture
> passwords and more entered by the user on the foreground session. This is
> a real world problem as the recent XMir development showed:
>   http://mjg59.dreamwidth.org/27327.html
> 
> We currently rely on sessions to release input devices when being
> deactivated. This relies on trust across sessions. But that's not given on
> usual systems. We therefore need a way to control which processes have
> access to input devices.
> 
> With VTs the kernel simply routed them through the active /dev/ttyX. This
> is not possible with evdev devices, though. Moreover, we want to avoid
> routing input-devices through some dispatcher-daemon in userspace (which
> would add some latency).
> 
> This patch introduces EVIOCREVOKE. If called on an evdev fd, this revokes
> device-access irrecoverably for that *single* open-file. Hence, once you
> call EVIOCREVOKE on any dup()ed fd, all fds for that open-file will be
> rather useless now (but still valid compared to close()!). This allows us
> to pass fds directly to session-processes from a trusted source. The
> source keeps a dup()ed fd and revokes access once the session-process is
> no longer active.
> Compared to the EVIOCMUTE proposal, we can avoid the CAP_SYS_ADMIN
> restriction now as there is no way to revive the fd again. Hence, a user
> is free to call EVIOCREVOKE themself to kill the fd.
> 
> Additionally, this ioctl allows multi-layer access-control (again compared
> to EVIOCMUTE which was limited to one layer via CAP_SYS_ADMIN). A middle
> layer can simply request a new open-file from the layer above and pass it
> to the layer below. Now each layer can call EVIOCREVOKE on the fds to
> revoke access for all layers below, at the expense of one fd per layer.
> 
> There's already ongoing experimental user-space work which demonstrates
> how it can be used:
>   http://lists.freedesktop.org/archives/systemd-devel/2013-August/012897.html

Re: [PATCH] Input: evdev - add EVIOCREVOKE ioctl

[David Herrmann]

Hi

On Wed, Feb 4, 2015 at 2:12 PM, Bruno Prémont <bonbons@linux-vserver.org> wrote:
> Hi David,
>
> Sorry for reviving this old thread (I didn't find more recent patch series
> at first glance or have not been using the proper keyword while searching).
>
> At FOSDEM 2015 last Sunday Hans presented libinput X input driver and mentioned
> evdev FD revoking.
>
> A question I raise was how are input devices to be put in a reasonable
> state on FD revoking?
> The specific case of force-feedback game-pads/wheels and the like that libinput
> is expected to pass through to games was the main trigger.
>
> Assume:
> - Game triggers some force-feedback event like vibrating device
> - Game looses focus and gets its evdev FD revoked
> - Newly focused application does not care about the game-pad/wheel
>
> How should the force-feedback activity get stopped on that focus change
> and thus FD revoking?
> Is the game expected to react before the FD being revoked (how long to
> wait?) or should the kernel somehow reset the device to a sane state on
> revoke (and if so, under which conditions?).
>
> Should some other evdev devices also receive a special treatment to reset
> them into a known/idle state (eventually LEDs on keyboards, beep, ...)?

We call input_device_flush() on EVIOCREVOKE, which stops any ongoing
FF owned by this handle. Same should be done for any per-handle state.
However, LEDs are not associated with a handle, so it will stay the
same. Applications are expected to re-sync their LEDs after they
revoked a file-descriptor of someone else.

Thanks
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

Re: [PATCH] Input: evdev - add EVIOCREVOKE ioctl

[Bruno Prémont]

Hi David,

On Wed, 04 February 2015 David Herrmann wrote:
> On Wed, Feb 4, 2015 at 2:12 PM, Bruno Prémont wrote:
> > Hi David,
> >
> > Sorry for reviving this old thread (I didn't find more recent patch series
> > at first glance or have not been using the proper keyword while searching).
> >
> > At FOSDEM 2015 last Sunday Hans presented libinput X input driver and mentioned
> > evdev FD revoking.
> >
> > A question I raise was how are input devices to be put in a reasonable
> > state on FD revoking?
> > The specific case of force-feedback game-pads/wheels and the like that libinput
> > is expected to pass through to games was the main trigger.
> >
> > Assume:
> > - Game triggers some force-feedback event like vibrating device
> > - Game looses focus and gets its evdev FD revoked
> > - Newly focused application does not care about the game-pad/wheel
> >
> > How should the force-feedback activity get stopped on that focus change
> > and thus FD revoking?
> > Is the game expected to react before the FD being revoked (how long to
> > wait?) or should the kernel somehow reset the device to a sane state on
> > revoke (and if so, under which conditions?).
> >
> > Should some other evdev devices also receive a special treatment to reset
> > them into a known/idle state (eventually LEDs on keyboards, beep, ...)?
> 
> We call input_device_flush() on EVIOCREVOKE, which stops any ongoing
> FF owned by this handle. Same should be done for any per-handle state.
> However, LEDs are not associated with a handle, so it will stay the
> same. Applications are expected to re-sync their LEDs after they
> revoked a file-descriptor of someone else.

Thanks for the explanation!
It answers my question.

So all that's needed should be there unless a specific kernel-side driver
does not properly have state associated to handles.

> Thanks
> David

Bruno
--
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