* [PATCH] Input: evdev - add EVIOCREVOKE ioctl
From: David Herrmann @ 2013-08-27 11:25 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, Kristian Høgsberg, 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
^ permalink raw reply related
* [PATCH v2] Input: evdev - add EVIOCREVOKE ioctl
From: David Herrmann @ 2013-08-27 11:39 UTC (permalink / raw)
To: linux-input; +Cc: Dmitry Torokhov, David Herrmann
In-Reply-To: <1377602704-23301-1-git-send-email-dh.herrmann@gmail.com>
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
^ permalink raw reply related
* Re: [PATCH v2] Input: evdev - add EVIOCREVOKE ioctl
From: Dmitry Torokhov @ 2013-08-27 22:17 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input
In-Reply-To: <1377603589-25501-1-git-send-email-dh.herrmann@gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2] Input: evdev - add EVIOCREVOKE ioctl
From: David Herrmann @ 2013-08-27 22:35 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: open list:HID CORE LAYER
In-Reply-To: <20130827221733.GA20726@core.coreip.homeip.net>
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
^ permalink raw reply
* [PATCH] iio: am335x-iio-adc: select triggered buffer
From: Sebastian Andrzej Siewior @ 2013-08-28 10:31 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1377470724-15710-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
"triggered buffer" isn't selected so we end up with:
|ERROR: "iio_triggered_buffer_setup" [drivers/iio/adc/ti_am335x_adc.ko] undefined!
Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
---
Please merge this into _this_ patch, it is still required.
drivers/iio/adc/Kconfig | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
index 09371cb..cfa2039 100644
--- a/drivers/iio/adc/Kconfig
+++ b/drivers/iio/adc/Kconfig
@@ -167,6 +167,7 @@ config TI_ADC081C
config TI_AM335X_ADC
tristate "TI's AM335X ADC driver"
depends on MFD_TI_AM335X_TSCADC
+ select IIO_TRIGGERED_BUFFER
help
Say yes here to build support for Texas Instruments ADC
driver which is also a MFD client.
--
1.8.4.rc3
^ permalink raw reply related
* Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Sebastian Andrzej Siewior @ 2013-08-28 10:42 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1377470724-15710-2-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:
>diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
>index e1c5300..4124e580 100644
>--- a/drivers/input/touchscreen/ti_am335x_tsc.c
>+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
>@@ -315,11 +321,17 @@ static irqreturn_t titsc_irq(int irq, void *dev)
> }
>
> if (irqclr) {
>- titsc_writel(ts_dev, REG_IRQSTATUS, irqclr);
>- am335x_tsc_se_update(ts_dev->mfd_tscadc);
>- return IRQ_HANDLED;
>+ titsc_writel(ts_dev, REG_IRQSTATUS, (status | irqclr));
>+ am335x_tsc_se_set(ts_dev->mfd_tscadc, ts_dev->step_mask);
> }
>- return IRQ_NONE;
>+
>+ /* If any IRQ flags left, return none. So ADC can handle its IRQs */
>+ status = titsc_readl(ts_dev, REG_IRQSTATUS);
>+ if (status == false)
>+ return IRQ_HANDLED;
>+ else
>+ return IRQ_NONE;
If I understand this correctly you return IRQ_NONE the TSC interrupt has
been handled and no ADC interrupt is outstanding.
This is bad because if you received 1k _only_ TSC interrupts you return
always IRQ_NONE and the irq-core will disable the interrupt line. You
don't want this.
Now, if you handle an interrupt at the TSC level and return IRQ_HANDLED
then the second handler, the ADC in your case, is also invoked.
So you can drop this and return IRQ_HANDLED if you *did* something here
and NONE if didn't do anything.
Basides that, I'm fine with it.
>+
> }
Sebastian
^ permalink raw reply
* Re: kernel Oops: 0003 on usbhid_submit_report
From: Jiri Kosina @ 2013-08-28 12:42 UTC (permalink / raw)
To: Andreas Lillebø Holm
Cc: Benjamin Tissoires, Greg KH, linux-usb-u79uwXL29TY76Z2rM5mHXA,
linux-input
In-Reply-To: <7851429DDA1F498899F023F32CC33483-nr7O5T6ryCTQT0dZR+AlfA@public.gmane.org>
On Mon, 26 Aug 2013, Andreas Lillebø Holm wrote:
> You are my hero Jiri :-) These two patches fixes the issues I was seeing
> and I can no longer reproduce them.
Thanks a lot for confirmation.
This has been a very long-standing bug, but for some reasons I have
started to see devices actually triggering it only now.
My plan is to push it to Linus in the upcoming merge window, and once it
proves to be stable, I'll do a -stable backport.
Thanks again,
--
Jiri Kosina
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Sebastian Andrzej Siewior @ 2013-08-28 13:01 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23, lee.jones, linux-iio, linux-input, linux-kernel, gregkh
In-Reply-To: <1377470724-15710-3-git-send-email-zubair.lutfullah@gmail.com>
* Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
I am mostly happy with it. There are just two things I pointed out.
Besides that, it looks good from my side.
>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
>@@ -85,7 +96,175 @@ static void tiadc_step_config(struct tiadc_device *adc_dev)
> adc_dev->channel_step[i] = steps;
> steps++;
> }
>+}
>+
>+static irqreturn_t tiadc_irq(int irq, void *private)
>+{
>+ struct iio_dev *indio_dev = private;
>+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
>+ unsigned int status, config;
>+ status = tiadc_readl(adc_dev, REG_IRQSTATUS);
>+
>+ /*
>+ * ADC and touchscreen share the IRQ line.
>+ * FIFO0 interrupts are used by TSC. Handle FIFO1 IRQs here only
>+ */
>+ if (status & IRQENB_FIFO1OVRRUN) {
>+ /* FIFO Overrun. Clear flag. Disable/Enable ADC to recover */
>+ config = tiadc_readl(adc_dev, REG_CTRL);
>+ config &= ~(CNTRLREG_TSCSSENB);
>+ tiadc_writel(adc_dev, REG_CTRL, config);
>+ tiadc_writel(adc_dev, REG_IRQSTATUS, IRQENB_FIFO1OVRRUN
>+ | IRQENB_FIFO1UNDRFLW | IRQENB_FIFO1THRES);
>+ tiadc_writel(adc_dev, REG_CTRL, (config | CNTRLREG_TSCSSENB));
>+ } else if (status & IRQENB_FIFO1THRES) {
>+ /* Trigger to push FIFO data to iio buffer */
>+ tiadc_writel(adc_dev, REG_IRQCLR, IRQENB_FIFO1THRES);
>+ iio_trigger_poll(indio_dev->trig, iio_get_time_ns());
>+ } else
>+ return IRQ_NONE;
>+
>+ /* If any IRQ flags left, return none. So TSC can handle its IRQs */
>+ status = tiadc_readl(adc_dev, REG_IRQSTATUS);
>+ if (status == false)
>+ return IRQ_HANDLED;
>+ else
>+ return IRQ_NONE;
As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
shared line are invoked once an interrupt is detected.
…
>+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
>+{
>+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
>+ struct iio_buffer *buffer = indio_dev->buffer;
>+ unsigned int enb = 0, stepnum;
>+ u8 bit;
>+
>+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
>+ if (adc_dev->data == NULL)
>+ return -ENOMEM;
>+
>+ tiadc_step_config(indio_dev);
>+ for_each_set_bit(bit, buffer->scan_mask,
>+ adc_dev->channels) {
>+ struct iio_chan_spec const *chan = indio_dev->channels + bit;
>+ /*
>+ * There are a total of 16 steps available
>+ * that are shared between ADC and touchscreen.
>+ * We start configuring from step 16 to 0 incase of
>+ * ADC. Hence the relation between input channel
>+ * and step for ADC would be as below.
>+ */
>+ stepnum = chan->channel + 9;
>+ enb |= (1 << stepnum);
Hmm. This looks odd.
In tiadc_step_config() we do:
| steps = TOTAL_STEPS - adc_dev->channels;
|…
| for (i = 0; i < adc_dev->channels; i++) {
| chan = adc_dev->channel_line[i];
| tiadc_writel(adc_dev,
| REG_STEPCONFIG(steps),
| stepconfig
| |
| STEPCONFIG_INP(chan));
| tiadc_writel(adc_dev,
| REG_STEPDELAY(steps),
| STEPCONFIG_OPENDLY);
| adc_dev->channel_step[i]
| =
| steps;
| steps++;
| }
That means if we have only one channel we enable the last step bit /
topmost that is bit 16. For the "default" four channels we get 0x1e000
as mask which means bit 13 to 16.
If you do have only one channel with the number 4 you then
get_adc_step_mask() will compute the correct step mask but here enable
step 14 instead of step 16.
What about the following as replacement?
index 08681d3..3bfcf1b 100644
--- a/drivers/iio/adc/ti_am335x_adc.c
+++ b/drivers/iio/adc/ti_am335x_adc.c
@@ -65,6 +65,11 @@ static u32 get_adc_step_mask(struct tiadc_device *adc_dev)
return step_en;
}
+static u32 get_adc_step_bit(struct tiadc_device *adc_dev, int chan)
+{
+ return 1 << adc_dev->channel_step[chan];
+}
+
static void tiadc_step_config(struct iio_dev *indio_dev)
{
struct tiadc_device *adc_dev = iio_priv(indio_dev);
@@ -179,7 +187,7 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
{
struct tiadc_device *adc_dev = iio_priv(indio_dev);
struct iio_buffer *buffer = indio_dev->buffer;
- unsigned int enb = 0, stepnum;
+ unsigned int enb = 0;
u8 bit;
adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
@@ -187,19 +195,8 @@ static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
return -ENOMEM;
tiadc_step_config(indio_dev);
- for_each_set_bit(bit, buffer->scan_mask,
- adc_dev->channels) {
- struct iio_chan_spec const *chan = indio_dev->channels + bit;
- /*
- * There are a total of 16 steps available
- * that are shared between ADC and touchscreen.
- * We start configuring from step 16 to 0 incase of
- * ADC. Hence the relation between input channel
- * and step for ADC would be as below.
- */
- stepnum = chan->channel + 9;
- enb |= (1 << stepnum);
- }
+ for_each_set_bit(bit, buffer->scan_mask, adc_dev->channels)
+ enb |= get_adc_step_bit(adc_dev, bit);
adc_dev->buffer_en_ch_steps = enb;
am335x_tsc_se_set(adc_dev->mfd_tscadc, enb);
Would it work? This is untested but it could work :)
Sebastian
^ permalink raw reply related
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Sebastian Andrzej Siewior @ 2013-08-28 14:18 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1377470724-15710-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
…
>+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
>+{
>+ struct iio_trigger *trig;
>+ int ret;
>+
>+ trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
>+ if (trig == NULL)
>+ return NULL;
>+
>+ trig->dev.parent = indio_dev->dev.parent;
>+ trig->ops = &tiadc_trigger_ops;
>+ iio_trigger_set_drvdata(trig, indio_dev);
>+
>+ ret = iio_trigger_register(trig);
>+ if (ret)
shouldn't you free the trigger / undo iio_trigger_alloc() here?
>+ return NULL;
>+
>+ return trig;
> }
>
> static const char * const chan_name_ain[] = {
>@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
> sizeof(struct tiadc_device));
> if (indio_dev == NULL) {
> dev_err(&pdev->dev, "failed to allocate iio device\n");
>- return -ENOMEM;
>+ err = -ENOMEM;
>+ goto err_ret;
Why the jump instead of leave right away?
> }
> adc_dev = iio_priv(indio_dev);
>
>@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> u32 step_en;
>
>+ free_irq(adc_dev->irq, indio_dev);
> iio_device_unregister(indio_dev);
>+ iio_buffer_unregister(indio_dev);
> tiadc_channels_remove(indio_dev);
I think you need also to
iio_trigger_unregister(adc_dev->trig);
iio_trigger_free(adc_dev->trig);
here.
> step_en = get_adc_step_mask(adc_dev);
> am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>+ iio_device_free(indio_dev);
>
> return 0;
> }
Sebastian
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Sebastian Andrzej Siewior @ 2013-08-28 16:43 UTC (permalink / raw)
To: Zubair Lutfullah
Cc: jic23-KWPb1pKIrIJaa/9Udqfwiw, lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <1377470724-15710-3-git-send-email-zubair.lutfullah-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
* Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
>index a952538..ae2202b 100644
>--- a/drivers/iio/adc/ti_am335x_adc.c
>+++ b/drivers/iio/adc/ti_am335x_adc.c
>@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
…
>+err_free_device:
>+ iio_device_free(indio_dev);
I am not sure about this one.
>+err_ret:
> return err;
> }
>
>@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> struct tiadc_device *adc_dev = iio_priv(indio_dev);
> u32 step_en;
>
>+ free_irq(adc_dev->irq, indio_dev);
> iio_device_unregister(indio_dev);
>+ iio_buffer_unregister(indio_dev);
> tiadc_channels_remove(indio_dev);
>
> step_en = get_adc_step_mask(adc_dev);
> am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
>+ iio_device_free(indio_dev);
But this one is wrong. The will be removed via dev_res() and if you do
it here as well then dev_res() will decrement the reference of an unused
object.
>
> return 0;
> }
Sebastian
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-08-28 18:19 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Zubair Lutfullah, jic23-KWPb1pKIrIJaa/9Udqfwiw,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130828164308.GE14111-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
On Wed, Aug 28, 2013 at 06:43:08PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >index a952538..ae2202b 100644
> >--- a/drivers/iio/adc/ti_am335x_adc.c
> >+++ b/drivers/iio/adc/ti_am335x_adc.c
> >@@ -231,28 +406,56 @@ static int tiadc_probe(struct platform_device *pdev)
> …
> >+err_free_device:
> >+ iio_device_free(indio_dev);
>
> I am not sure about this one.
If I understand correctly, if devm_iio_device_alloc
is successful earlier in the code and subsequent stuff fails.
Then the code jumps to err_free_device and this is needed.
>
> >+err_ret:
> > return err;
> > }
> >
> >@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> > struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > u32 step_en;
> >
> >+ free_irq(adc_dev->irq, indio_dev);
> > iio_device_unregister(indio_dev);
> >+ iio_buffer_unregister(indio_dev);
> > tiadc_channels_remove(indio_dev);
> >
> > step_en = get_adc_step_mask(adc_dev);
> > am335x_tsc_se_clr(adc_dev->mfd_tscadc, step_en);
> >+ iio_device_free(indio_dev);
>
> But this one is wrong. The will be removed via dev_res() and if you do
> it here as well then dev_res() will decrement the reference of an unused
> object.
I was unaware of this aspect.
*but*
iio_simple_dummy.c has it in its remove function.
And I just checked and found it in several drivers as well.
I'll leave this to the more experienced folks on the list..
> >
> > return 0;
> > }
>
> Sebastian
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-08-28 18:22 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Zubair Lutfullah, jic23-KWPb1pKIrIJaa/9Udqfwiw,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130828141835.GD14111-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
On Wed, Aug 28, 2013 at 04:18:35PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>
> >diff --git a/drivers/iio/adc/ti_am335x_adc.c b/drivers/iio/adc/ti_am335x_adc.c
> >index a952538..ae2202b 100644
> >--- a/drivers/iio/adc/ti_am335x_adc.c
> >+++ b/drivers/iio/adc/ti_am335x_adc.c
> …
>
> >+static struct iio_trigger *tiadc_iio_allocate_trigger(struct iio_dev *indio_dev)
...
> >+ trig = iio_trigger_alloc("%s-dev%d", indio_dev->name, indio_dev->id);
> >+ if (trig == NULL)
> >+ return NULL;
> >+
...
> >+ ret = iio_trigger_register(trig);
> >+ if (ret)
>
> shouldn't you free the trigger / undo iio_trigger_alloc() here?
>
> >+ return NULL;
> >+
...
Possible. It's done in the probe section just
to keep error handling bunched there.
>
> >
> > static const char * const chan_name_ain[] = {
> >@@ -220,7 +394,8 @@ static int tiadc_probe(struct platform_device *pdev)
> > sizeof(struct tiadc_device));
> > if (indio_dev == NULL) {
> > dev_err(&pdev->dev, "failed to allocate iio device\n");
> >- return -ENOMEM;
> >+ err = -ENOMEM;
> >+ goto err_ret;
> Why the jump instead of leave right away?
>
> > }
Again. Error handling all in one place.
> >
> >@@ -262,11 +465,14 @@ static int tiadc_remove(struct platform_device *pdev)
> > struct tiadc_device *adc_dev = iio_priv(indio_dev);
> > u32 step_en;
> >
> >+ free_irq(adc_dev->irq, indio_dev);
> > iio_device_unregister(indio_dev);
> >+ iio_buffer_unregister(indio_dev);
> > tiadc_channels_remove(indio_dev);
>
> I think you need also to
>
> iio_trigger_unregister(adc_dev->trig);
> iio_trigger_free(adc_dev->trig);
>
> here.
Indeed.
Thanks for pointing it out.
Zubair
^ permalink raw reply
* Re: [PATCH] iio: am335x-iio-adc: select triggered buffer
From: Zubair Lutfullah : @ 2013-08-28 18:32 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Zubair Lutfullah, jic23-KWPb1pKIrIJaa/9Udqfwiw,
lee.jones-QSEj5FYQhm4dnm+yROfE0A,
linux-iio-u79uwXL29TY76Z2rM5mHXA,
linux-input-u79uwXL29TY76Z2rM5mHXA,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r
In-Reply-To: <20130828103134.GA14111-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
On Wed, Aug 28, 2013 at 12:31:34PM +0200, Sebastian Andrzej Siewior wrote:
> "triggered buffer" isn't selected so we end up with:
> |ERROR: "iio_triggered_buffer_setup" [drivers/iio/adc/ti_am335x_adc.ko] undefined!
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
> ---
> Please merge this into _this_ patch, it is still required.
>
> drivers/iio/adc/Kconfig | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/iio/adc/Kconfig b/drivers/iio/adc/Kconfig
> index 09371cb..cfa2039 100644
> --- a/drivers/iio/adc/Kconfig
> +++ b/drivers/iio/adc/Kconfig
> @@ -167,6 +167,7 @@ config TI_ADC081C
> config TI_AM335X_ADC
> tristate "TI's AM335X ADC driver"
> depends on MFD_TI_AM335X_TSCADC
> + select IIO_TRIGGERED_BUFFER
Thanks for pointing it out. Added.
ZubairLK
^ permalink raw reply
* Re: [PATCH 2/2] iio: ti_am335x_adc: Add continuous sampling support
From: Zubair Lutfullah : @ 2013-08-28 18:43 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Zubair Lutfullah, jic23, lee.jones, linux-iio, linux-input,
linux-kernel, gregkh
In-Reply-To: <20130828130124.GC14111@linutronix.de>
On Wed, Aug 28, 2013 at 03:01:24PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:24 [+0100]:
>
> I am mostly happy with it. There are just two things I pointed out.
> Besides that, it looks good from my side.
Sign-off?
> >+static irqreturn_t tiadc_irq(int irq, void *private)
> >+{
...
> >+ /* If any IRQ flags left, return none. So TSC can handle its IRQs */
> >+ status = tiadc_readl(adc_dev, REG_IRQSTATUS);
> >+ if (status == false)
> >+ return IRQ_HANDLED;
> >+ else
> >+ return IRQ_NONE;
>
> As I said in 1/2 of this series, you shouldn't do this. Both handlers of a
> shared line are invoked once an interrupt is detected.
>
> …
I somehow missed sending these patches to Dmitry..
I'll do it next series.
He originally suggested this way.
An alternative was to handle irqs in mfd core which would
seriously complicate code and patches..
Whenever I change the handlers to the way you suggest.
i.e. Handle what each irq handler does and return IRQ_HANDLED
if the irq handler handled nothing then return IRQ_NONE.
The driver hangs when I'm brutal while touching the TSC and
continuously sampling the ADC. Not always. But happens.
When I check /proc/interrupts the hardware irqs seem to be firing.
But for some reason the whole thing messes up..
> >+static int tiadc_buffer_postenable(struct iio_dev *indio_dev)
> >+{
> >+ struct tiadc_device *adc_dev = iio_priv(indio_dev);
> >+ struct iio_buffer *buffer = indio_dev->buffer;
> >+ unsigned int enb = 0, stepnum;
> >+ u8 bit;
> >+
> >+ adc_dev->data = kmalloc(indio_dev->scan_bytes, GFP_KERNEL);
> >+ if (adc_dev->data == NULL)
> >+ return -ENOMEM;
> >+
> >+ tiadc_step_config(indio_dev);
> >+ for_each_set_bit(bit, buffer->scan_mask,
> >+ adc_dev->channels) {
> >+ struct iio_chan_spec const *chan = indio_dev->channels + bit;
> >+ /*
> >+ * There are a total of 16 steps available
> >+ * that are shared between ADC and touchscreen.
> >+ * We start configuring from step 16 to 0 incase of
> >+ * ADC. Hence the relation between input channel
> >+ * and step for ADC would be as below.
> >+ */
> >+ stepnum = chan->channel + 9;
> >+ enb |= (1 << stepnum);
>
> Hmm. This looks odd.
>
> In tiadc_step_config() we do:
> | steps = TOTAL_STEPS - adc_dev->channels;
> |…
> | for (i = 0; i < adc_dev->channels; i++) {
> | chan = adc_dev->channel_line[i];
> | tiadc_writel(adc_dev,
> | REG_STEPCONFIG(steps),
> | stepconfig
> | |
> | STEPCONFIG_INP(chan));
> | tiadc_writel(adc_dev,
> | REG_STEPDELAY(steps),
> | STEPCONFIG_OPENDLY);
> | adc_dev->channel_step[i]
> | =
> | steps;
> | steps++;
> | }
>
> That means if we have only one channel we enable the last step bit /
> topmost that is bit 16. For the "default" four channels we get 0x1e000
> as mask which means bit 13 to 16.
> If you do have only one channel with the number 4 you then
> get_adc_step_mask() will compute the correct step mask but here enable
> step 14 instead of step 16.
>
> What about the following as replacement?
I sense a mismatch. Probably because of different styles of coding
these bit handling :).
I'll look into it and the alternative way you suggested as well.
Thanks
ZubairLK
^ permalink raw reply
* [PATCH] input/serio: disable i8042 PC keyboard ctrl for ARC
From: Mischa Jonker @ 2013-08-28 18:46 UTC (permalink / raw)
To: Dmitry Torokhov, Heiko Carstens, Greg Kroah-Hartman, linux-input,
linux-kernel
Cc: Mischa Jonker
It causes crashes when enabled, and we don't have such a peripheral
anyway on ARC platforms.
Signed-off-by: Mischa Jonker <mjonker@synopsys.com>
---
drivers/input/serio/Kconfig | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
index 94c17c2..1e691a3 100644
--- a/drivers/input/serio/Kconfig
+++ b/drivers/input/serio/Kconfig
@@ -22,7 +22,8 @@ config SERIO_I8042
tristate "i8042 PC Keyboard controller" if EXPERT || !X86
default y
depends on !PARISC && (!ARM || ARCH_SHARK || FOOTBRIDGE_HOST) && \
- (!SUPERH || SH_CAYMAN) && !M68K && !BLACKFIN && !S390
+ (!SUPERH || SH_CAYMAN) && !M68K && !BLACKFIN && !S390 && \
+ !ARC
help
i8042 is the chip over which the standard AT keyboard and PS/2
mouse are connected to the computer. If you use these devices,
--
1.7.9.5
^ permalink raw reply related
* Re: [PATCH 1/2] input: ti_am335x_tsc: Enable shared IRQ for TSC
From: Zubair Lutfullah : @ 2013-08-28 18:49 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: Zubair Lutfullah, jic23, lee.jones, linux-iio, linux-input,
linux-kernel, gregkh
In-Reply-To: <20130828104211.GB14111@linutronix.de>
On Wed, Aug 28, 2013 at 12:42:11PM +0200, Sebastian Andrzej Siewior wrote:
> * Zubair Lutfullah | 2013-08-25 23:45:23 [+0100]:
>
> >diff --git a/drivers/input/touchscreen/ti_am335x_tsc.c b/drivers/input/touchscreen/ti_am335x_tsc.c
> >index e1c5300..4124e580 100644
> >--- a/drivers/input/touchscreen/ti_am335x_tsc.c
> >+++ b/drivers/input/touchscreen/ti_am335x_tsc.c
> >@@ -315,11 +321,17 @@ static irqreturn_t titsc_irq(int irq, void *dev)
...
> >+ /* If any IRQ flags left, return none. So ADC can handle its IRQs */
> >+ status = titsc_readl(ts_dev, REG_IRQSTATUS);
> >+ if (status == false)
> >+ return IRQ_HANDLED;
> >+ else
> >+ return IRQ_NONE;
>
> If I understand this correctly you return IRQ_NONE the TSC interrupt has
> been handled and no ADC interrupt is outstanding.
Its actually the opposite. TSC handler checks if there are any ADC IRQ flags
outstanding.
If there is no outstanding, then IRQ_HANDLED is returned.
If there is ADC IRQ outstanding, then IRQ_NONE is returned.
ZubairLK
^ permalink raw reply
* [PATCH 01/14] HID: validate HID report id size
From: Jiri Kosina @ 2013-08-28 20:29 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
The "Report ID" field of a HID report is used to build indexes of
reports. The kernel's index of these is limited to 256 entries, so any
malicious device that sets a Report ID greater than 255 will trigger
memory corruption on the host:
[ 1347.156239] BUG: unable to handle kernel paging request at ffff88094958a878
[ 1347.156261] IP: [<ffffffff813e4da0>] hid_register_report+0x2a/0x8b
CVE-2013-2888
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-core.c | 10 +++++++---
include/linux/hid.h | 4 +++-
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 36668d1..5ea7d51 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -63,6 +63,8 @@ struct hid_report *hid_register_report(struct hid_device *device, unsigned type,
struct hid_report_enum *report_enum = device->report_enum + type;
struct hid_report *report;
+ if (id >= HID_MAX_IDS)
+ return NULL;
if (report_enum->report_id_hash[id])
return report_enum->report_id_hash[id];
@@ -404,8 +406,10 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item)
case HID_GLOBAL_ITEM_TAG_REPORT_ID:
parser->global.report_id = item_udata(item);
- if (parser->global.report_id == 0) {
- hid_err(parser->device, "report_id 0 is invalid\n");
+ if (parser->global.report_id == 0 ||
+ parser->global.report_id >= HID_MAX_IDS) {
+ hid_err(parser->device, "report_id %u is invalid\n",
+ parser->global.report_id);
return -1;
}
return 0;
@@ -575,7 +579,7 @@ static void hid_close_report(struct hid_device *device)
for (i = 0; i < HID_REPORT_TYPES; i++) {
struct hid_report_enum *report_enum = device->report_enum + i;
- for (j = 0; j < 256; j++) {
+ for (j = 0; j < HID_MAX_IDS; j++) {
struct hid_report *report = report_enum->report_id_hash[j];
if (report)
hid_free_report(report);
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 0c48991..ff545cc 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -393,10 +393,12 @@ struct hid_report {
struct hid_device *device; /* associated device */
};
+#define HID_MAX_IDS 256
+
struct hid_report_enum {
unsigned numbered;
struct list_head report_list;
- struct hid_report *report_id_hash[256];
+ struct hid_report *report_id_hash[HID_MAX_IDS];
};
#define HID_REPORT_TYPES 3
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 02/14] HID: provide a helper for validating hid reports
From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
Many drivers need to validate the characteristics of their HID report
during initialization to avoid misusing the reports. This adds a common
helper to perform validation of the report, its field count, and the
value count within the fields.
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
include/linux/hid.h | 4 ++++
2 files changed, 54 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 5ea7d51..55798b2 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -759,6 +759,56 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
}
EXPORT_SYMBOL_GPL(hid_parse_report);
+static const char * const hid_report_names[] = {
+ "HID_INPUT_REPORT",
+ "HID_OUTPUT_REPORT",
+ "HID_FEATURE_REPORT",
+};
+/**
+ * hid_validate_report - validate existing device report
+ *
+ * @device: hid device
+ * @type: which report type to examine
+ * @id: which report ID to examine (0 for first)
+ * @fields: expected number of fields
+ * @report_counts: expected number of values per field
+ *
+ * Validate the report details after parsing.
+ */
+struct hid_report *hid_validate_report(struct hid_device *hid,
+ unsigned int type, unsigned int id,
+ unsigned int fields,
+ unsigned int report_counts)
+{
+ struct hid_report *report;
+ unsigned int i;
+
+ if (type > HID_FEATURE_REPORT) {
+ hid_err(hid, "invalid HID report %u\n", type);
+ return NULL;
+ }
+
+ report = hid->report_enum[type].report_id_hash[id];
+ if (!report) {
+ hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
+ return NULL;
+ }
+ if (report->maxfield < fields) {
+ hid_err(hid, "not enough fields in %s %u\n",
+ hid_report_names[type], id);
+ return NULL;
+ }
+ for (i = 0; i < fields; i++) {
+ if (report->field[i]->report_count < report_counts) {
+ hid_err(hid, "not enough values in %s %u fields\n",
+ hid_report_names[type], id);
+ return NULL;
+ }
+ }
+ return report;
+}
+EXPORT_SYMBOL_GPL(hid_validate_report);
+
/**
* hid_open_report - open a driver-specific device report
*
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ff545cc..76e41d8 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -749,6 +749,10 @@ void hid_output_report(struct hid_report *report, __u8 *data);
struct hid_device *hid_allocate_device(void);
struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
+struct hid_report *hid_validate_report(struct hid_device *hid,
+ unsigned int type, unsigned int id,
+ unsigned int fields,
+ unsigned int report_counts);
int hid_open_report(struct hid_device *device);
int hid_check_keys_pressed(struct hid_device *hid);
int hid_connect(struct hid_device *hid, unsigned int connect_mask);
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 03/14] HID: zeroplus: validate output report details
From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
The zeroplus HID driver was not checking the size of allocated values
in fields it used. A HID device could send a malicious output report
that would cause the driver to write beyond the output report allocation
during initialization, causing a heap overflow:
[ 1442.728680] usb 1-1: New USB device found, idVendor=0c12, idProduct=0005
...
[ 1466.243173] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
CVE-2013-2889
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-zpff.c | 14 ++------------
1 file changed, 2 insertions(+), 12 deletions(-)
diff --git a/drivers/hid/hid-zpff.c b/drivers/hid/hid-zpff.c
index 6ec28a3..b124991 100644
--- a/drivers/hid/hid-zpff.c
+++ b/drivers/hid/hid-zpff.c
@@ -68,22 +68,12 @@ static int zpff_init(struct hid_device *hid)
struct hid_report *report;
struct hid_input *hidinput = list_entry(hid->inputs.next,
struct hid_input, list);
- struct list_head *report_list =
- &hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct input_dev *dev = hidinput->input;
int error;
- if (list_empty(report_list)) {
- hid_err(hid, "no output report found\n");
+ report = hid_validate_report(hid, HID_OUTPUT_REPORT, 0, 4, 1);
+ if (!report)
return -ENODEV;
- }
-
- report = list_entry(report_list->next, struct hid_report, list);
-
- if (report->maxfield < 4) {
- hid_err(hid, "not enough fields in report\n");
- return -ENODEV;
- }
zpff = kzalloc(sizeof(struct zpff_device), GFP_KERNEL);
if (!zpff)
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 04/14] HID: sony: validate HID output report details
From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
This driver must validate the availability of the HID output report and
its size before it can write LED states via buzz_set_leds(). This stops
a heap overflow that is possible if a device provides a malicious HID
output report:
[ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
...
[ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
CVE-2013-2890
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-sony.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 87fbe29..b987926 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
drv_data = hid_get_drvdata(hdev);
BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
+ /* Validate expected report characteristics. */
+ if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
+ return -ENODEV;
+
buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
if (!buzz) {
hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 05/14] HID: steelseries: validate output report details
From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Simon Wood
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
steelseries HID driver to write beyond the output report allocation
during initialization, causing a heap overflow:
[ 167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
...
[ 182.050547] BUG kmalloc-256 (Tainted: G W ): Redzone overwritten
CVE-2013-2891
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-steelseries.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index d164911..ef42e86 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
goto err_free;
}
+ if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 16)) {
+ ret = -ENODEV;
+ goto err_free;
+ }
+
ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
if (ret) {
hid_err(hdev, "hw start failed\n");
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 06/14] HID: pantherlord: validate output report details
From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
pantherlord HID driver to write beyond the output report allocation
during initialization, causing a heap overflow:
[ 310.939483] usb 1-1: New USB device found, idVendor=0e8f, idProduct=0003
...
[ 315.980774] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
CVE-2013-2892
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-pl.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-pl.c b/drivers/hid/hid-pl.c
index d29112f..2dcd7d9 100644
--- a/drivers/hid/hid-pl.c
+++ b/drivers/hid/hid-pl.c
@@ -132,8 +132,14 @@ static int plff_init(struct hid_device *hid)
strong = &report->field[0]->value[2];
weak = &report->field[0]->value[3];
debug("detected single-field device");
- } else if (report->maxfield >= 4 && report->field[0]->maxusage == 1 &&
- report->field[0]->usage[0].hid == (HID_UP_LED | 0x43)) {
+ } else if (report->field[0]->maxusage == 1 &&
+ report->field[0]->usage[0].hid ==
+ (HID_UP_LED | 0x43) &&
+ report->maxfield >= 4 &&
+ report->field[0]->report_count >= 1 &&
+ report->field[1]->report_count >= 1 &&
+ report->field[2]->report_count >= 1 &&
+ report->field[3]->report_count >= 1) {
report->field[0]->value[0] = 0x00;
report->field[1]->value[0] = 0x00;
strong = &report->field[2]->value[0];
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 07/14] HID: LG: validate HID output report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
lg, lg3, and lg4 HID drivers to write beyond the output report allocation
during an event, causing a heap overflow:
[ 325.245240] usb 1-1: New USB device found, idVendor=046d, idProduct=c287
...
[ 414.518960] BUG kmalloc-4096 (Not tainted): Redzone overwritten
Additionally, while lg2 did correctly validate the report details, it was
cleaned up and shortened.
CVE-2013-2893
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-lg2ff.c | 19 +++----------------
drivers/hid/hid-lg3ff.c | 29 ++++++-----------------------
drivers/hid/hid-lg4ff.c | 20 +-------------------
drivers/hid/hid-lgff.c | 17 ++---------------
4 files changed, 12 insertions(+), 73 deletions(-)
diff --git a/drivers/hid/hid-lg2ff.c b/drivers/hid/hid-lg2ff.c
index b3cd150..9805197 100644
--- a/drivers/hid/hid-lg2ff.c
+++ b/drivers/hid/hid-lg2ff.c
@@ -64,26 +64,13 @@ int lg2ff_init(struct hid_device *hid)
struct hid_report *report;
struct hid_input *hidinput = list_entry(hid->inputs.next,
struct hid_input, list);
- struct list_head *report_list =
- &hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct input_dev *dev = hidinput->input;
int error;
- if (list_empty(report_list)) {
- hid_err(hid, "no output report found\n");
+ /* Check that the report looks ok */
+ report = hid_validate_report(hid, HID_OUTPUT_REPORT, 0, 1, 7);
+ if (!report)
return -ENODEV;
- }
-
- report = list_entry(report_list->next, struct hid_report, list);
-
- if (report->maxfield < 1) {
- hid_err(hid, "output report is empty\n");
- return -ENODEV;
- }
- if (report->field[0]->report_count < 7) {
- hid_err(hid, "not enough values in the field\n");
- return -ENODEV;
- }
lg2ff = kmalloc(sizeof(struct lg2ff_device), GFP_KERNEL);
if (!lg2ff)
diff --git a/drivers/hid/hid-lg3ff.c b/drivers/hid/hid-lg3ff.c
index e52f181..53ac79b 100644
--- a/drivers/hid/hid-lg3ff.c
+++ b/drivers/hid/hid-lg3ff.c
@@ -66,10 +66,11 @@ static int hid_lg3ff_play(struct input_dev *dev, void *data,
int x, y;
/*
- * Maxusage should always be 63 (maximum fields)
- * likely a better way to ensure this data is clean
+ * Available values in the field should always be 63, but we only use up to
+ * 35. Instead, clear the entire area, however big it is.
*/
- memset(report->field[0]->value, 0, sizeof(__s32)*report->field[0]->maxusage);
+ memset(report->field[0]->value, 0,
+ sizeof(__s32) * report->field[0]->report_count);
switch (effect->type) {
case FF_CONSTANT:
@@ -129,32 +130,14 @@ static const signed short ff3_joystick_ac[] = {
int lg3ff_init(struct hid_device *hid)
{
struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
- struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct input_dev *dev = hidinput->input;
- struct hid_report *report;
- struct hid_field *field;
const signed short *ff_bits = ff3_joystick_ac;
int error;
int i;
- /* Find the report to use */
- if (list_empty(report_list)) {
- hid_err(hid, "No output report found\n");
- return -1;
- }
-
/* Check that the report looks ok */
- report = list_entry(report_list->next, struct hid_report, list);
- if (!report) {
- hid_err(hid, "NULL output report\n");
- return -1;
- }
-
- field = report->field[0];
- if (!field) {
- hid_err(hid, "NULL field\n");
- return -1;
- }
+ if (!hid_validate_report(hid, HID_OUTPUT_REPORT, 0, 1, 35))
+ return -ENODEV;
/* Assume single fixed device G940 */
for (i = 0; ff_bits[i] >= 0; i++)
diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 0ddae2a..8b89f0f 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -484,34 +484,16 @@ static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cde
int lg4ff_init(struct hid_device *hid)
{
struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
- struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct input_dev *dev = hidinput->input;
- struct hid_report *report;
- struct hid_field *field;
struct lg4ff_device_entry *entry;
struct lg_drv_data *drv_data;
struct usb_device_descriptor *udesc;
int error, i, j;
__u16 bcdDevice, rev_maj, rev_min;
- /* Find the report to use */
- if (list_empty(report_list)) {
- hid_err(hid, "No output report found\n");
- return -1;
- }
-
/* Check that the report looks ok */
- report = list_entry(report_list->next, struct hid_report, list);
- if (!report) {
- hid_err(hid, "NULL output report\n");
+ if (!hid_validate_report(hid, HID_OUTPUT_REPORT, 0, 1, 7))
return -1;
- }
-
- field = report->field[0];
- if (!field) {
- hid_err(hid, "NULL field\n");
- return -1;
- }
/* Check what wheel has been connected */
for (i = 0; i < ARRAY_SIZE(lg4ff_devices); i++) {
diff --git a/drivers/hid/hid-lgff.c b/drivers/hid/hid-lgff.c
index d7ea8c8..a84fb40 100644
--- a/drivers/hid/hid-lgff.c
+++ b/drivers/hid/hid-lgff.c
@@ -128,27 +128,14 @@ static void hid_lgff_set_autocenter(struct input_dev *dev, u16 magnitude)
int lgff_init(struct hid_device* hid)
{
struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
- struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
struct input_dev *dev = hidinput->input;
- struct hid_report *report;
- struct hid_field *field;
const signed short *ff_bits = ff_joystick;
int error;
int i;
- /* Find the report to use */
- if (list_empty(report_list)) {
- hid_err(hid, "No output report found\n");
- return -1;
- }
-
/* Check that the report looks ok */
- report = list_entry(report_list->next, struct hid_report, list);
- field = report->field[0];
- if (!field) {
- hid_err(hid, "NULL field\n");
- return -1;
- }
+ if (!hid_validate_report(hid, HID_OUTPUT_REPORT, 0, 1, 7))
+ return -ENODEV;
for (i = 0; i < ARRAY_SIZE(devices); i++) {
if (dev->id.vendor == devices[i].idVendor &&
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 08/14] HID: lenovo-tpkbd: validate output report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Bernhard Seibold
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
lenovo-tpkbd HID driver to write just beyond the output report allocation
during initialization, causing a heap overflow:
[ 76.109807] usb 1-1: New USB device found, idVendor=17ef, idProduct=6009
...
[ 80.462540] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
CVE-2013-2894
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-lenovo-tpkbd.c | 5 +++++
1 file changed, 5 insertions(+)
diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
index 07837f5..b697ada 100644
--- a/drivers/hid/hid-lenovo-tpkbd.c
+++ b/drivers/hid/hid-lenovo-tpkbd.c
@@ -341,6 +341,11 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
char *name_mute, *name_micmute;
int ret;
+ /* Validate required reports. */
+ if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 4, 4, 1) ||
+ !hid_validate_report(hdev, HID_OUTPUT_REPORT, 3, 1, 2))
+ return -ENODEV;
+
if (sysfs_create_group(&hdev->dev.kobj,
&tpkbd_attr_group_pointer)) {
hid_warn(hdev, "Could not create sysfs group\n");
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 09/14] HID: logitech-dj: validate output report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Nestor Lopez Casado
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
logitech-dj HID driver to leak kernel memory contents to the device, or
trigger a NULL dereference during initialization:
[ 304.424553] usb 1-1: New USB device found, idVendor=046d, idProduct=c52b
...
[ 304.780467] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
[ 304.781409] IP: [<ffffffff815d50aa>] logi_dj_recv_send_report.isra.11+0x1a/0x90
CVE-2013-2895
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-logitech-dj.c | 12 ++++++++++--
1 file changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index cd33084..7b99c2a 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -461,7 +461,7 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
struct hid_report *report;
struct hid_report_enum *output_report_enum;
u8 *data = (u8 *)(&dj_report->device_index);
- int i;
+ unsigned int i, length;
output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
@@ -471,7 +471,9 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
return -ENODEV;
}
- for (i = 0; i < report->field[0]->report_count; i++)
+ length = min_t(size_t, sizeof(*dj_report) - 1,
+ report->field[0]->report_count);
+ for (i = 0; i < length; i++)
report->field[0]->value[i] = data[i];
hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
@@ -783,6 +785,12 @@ static int logi_dj_probe(struct hid_device *hdev,
goto hid_parse_fail;
}
+ if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
+ 1, 3)) {
+ retval = -ENODEV;
+ goto hid_parse_fail;
+ }
+
/* Starts the usb device and connects to upper interfaces hiddev and
* hidraw */
retval = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox