* [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl
@ 2022-04-21 6:57 Peter Hutterer
2022-04-21 7:41 ` Benjamin Tissoires
0 siblings, 1 reply; 3+ messages in thread
From: Peter Hutterer @ 2022-04-21 6:57 UTC (permalink / raw)
To: Jiri Kosina, Benjamin Tissoires; +Cc: linux-input, linux-kernel
There is a need for userspace applications to open HID devices directly.
Use-cases include configuration of gaming mice or direct access to
joystick devices. The latter is currently handled by the uaccess tag in
systemd, other devices include more custom/local configurations or just
sudo.
A better approach is what we already have for evdev devices: give the
application a file descriptor and revoke it when it may no longer access
that device.
This patch is the hidraw equivalent to the EVIOCREVOKE ioctl, see
commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details.
A draft MR for systemd-logind has been filed here:
https://github.com/systemd/systemd/pull/23140
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
Maybe noteworthy: even with logind support this is only the first step of
many. logind only hands the fd to whoever controls the session and the fd will
then have to be passed forward through portals to the application.
drivers/hid/hidraw.c | 34 ++++++++++++++++++++++++++++++----
include/linux/hidraw.h | 1 +
include/uapi/linux/hidraw.h | 1 +
3 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 681614a8302a..3449fe856090 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -42,6 +42,9 @@ static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count,
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
+ if (list->revoked)
+ return -ENODEV;
+
mutex_lock(&list->read_mutex);
while (ret == 0) {
@@ -159,9 +162,13 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
{
+ struct hidraw_list *list = file->private_data;
ssize_t ret;
down_read(&minors_rwsem);
- ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
+ if (list->revoked)
+ ret = -ENODEV;
+ else
+ ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
up_read(&minors_rwsem);
return ret;
}
@@ -254,7 +261,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
poll_wait(file, &list->hidraw->wait, wait);
if (list->head != list->tail)
mask |= EPOLLIN | EPOLLRDNORM;
- if (!list->hidraw->exist)
+ if (!list->hidraw->exist || list->revoked)
mask |= EPOLLERR | EPOLLHUP;
return mask;
}
@@ -313,6 +320,9 @@ static int hidraw_fasync(int fd, struct file *file, int on)
{
struct hidraw_list *list = file->private_data;
+ if (list->revoked)
+ return -ENODEV;
+
return fasync_helper(fd, file, on, &list->fasync);
}
@@ -360,6 +370,13 @@ static int hidraw_release(struct inode * inode, struct file * file)
return 0;
}
+static int hidraw_revoke(struct hidraw_list *list, struct file *file)
+{
+ list->revoked = true;
+
+ return 0;
+}
+
static long hidraw_ioctl(struct file *file, unsigned int cmd,
unsigned long arg)
{
@@ -367,11 +384,12 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
unsigned int minor = iminor(inode);
long ret = 0;
struct hidraw *dev;
+ struct hidraw_list *list = file->private_data;
void __user *user_arg = (void __user*) arg;
down_read(&minors_rwsem);
dev = hidraw_table[minor];
- if (!dev || !dev->exist) {
+ if (!dev || !dev->exist || list->revoked) {
ret = -ENODEV;
goto out;
}
@@ -409,6 +427,14 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
ret = -EFAULT;
break;
}
+ case HIDIOCREVOKE:
+ {
+ if (user_arg)
+ ret = -EINVAL;
+ else
+ ret = hidraw_revoke(list, file);
+ break;
+ }
default:
{
struct hid_device *hid = dev->hid;
@@ -515,7 +541,7 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
list_for_each_entry(list, &dev->list, node) {
int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
- if (new_head == list->tail)
+ if (list->revoked || new_head == list->tail)
continue;
if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
index cd67f4ca5599..18fd30a288de 100644
--- a/include/linux/hidraw.h
+++ b/include/linux/hidraw.h
@@ -32,6 +32,7 @@ struct hidraw_list {
struct hidraw *hidraw;
struct list_head node;
struct mutex read_mutex;
+ bool revoked;
};
#ifdef CONFIG_HIDRAW
diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
index 33ebad81720a..d0563f251da5 100644
--- a/include/uapi/linux/hidraw.h
+++ b/include/uapi/linux/hidraw.h
@@ -46,6 +46,7 @@ struct hidraw_devinfo {
/* The first byte of SOUTPUT and GOUTPUT is the report number */
#define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
#define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
+#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
#define HIDRAW_FIRST_MINOR 0
#define HIDRAW_MAX_DEVICES 64
--
2.36.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl
2022-04-21 6:57 [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl Peter Hutterer
@ 2022-04-21 7:41 ` Benjamin Tissoires
2022-04-21 8:09 ` Benjamin Tissoires
0 siblings, 1 reply; 3+ messages in thread
From: Benjamin Tissoires @ 2022-04-21 7:41 UTC (permalink / raw)
To: Peter Hutterer; +Cc: Jiri Kosina, linux-input, linux-kernel
On Thu, Apr 21, 2022 at 8:57 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
>
> There is a need for userspace applications to open HID devices directly.
> Use-cases include configuration of gaming mice or direct access to
> joystick devices. The latter is currently handled by the uaccess tag in
> systemd, other devices include more custom/local configurations or just
> sudo.
>
> A better approach is what we already have for evdev devices: give the
> application a file descriptor and revoke it when it may no longer access
> that device.
>
> This patch is the hidraw equivalent to the EVIOCREVOKE ioctl, see
> commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details.
>
> A draft MR for systemd-logind has been filed here:
> https://github.com/systemd/systemd/pull/23140
>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> ---
> Maybe noteworthy: even with logind support this is only the first step of
> many. logind only hands the fd to whoever controls the session and the fd will
> then have to be passed forward through portals to the application.
>
> drivers/hid/hidraw.c | 34 ++++++++++++++++++++++++++++++----
> include/linux/hidraw.h | 1 +
> include/uapi/linux/hidraw.h | 1 +
> 3 files changed, 32 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 681614a8302a..3449fe856090 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -42,6 +42,9 @@ static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count,
> int ret = 0, len;
> DECLARE_WAITQUEUE(wait, current);
>
> + if (list->revoked)
> + return -ENODEV;
> +
> mutex_lock(&list->read_mutex);
>
> while (ret == 0) {
> @@ -159,9 +162,13 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
>
> static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
> {
> + struct hidraw_list *list = file->private_data;
> ssize_t ret;
> down_read(&minors_rwsem);
> - ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
> + if (list->revoked)
> + ret = -ENODEV;
> + else
> + ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
> up_read(&minors_rwsem);
> return ret;
> }
> @@ -254,7 +261,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
> poll_wait(file, &list->hidraw->wait, wait);
> if (list->head != list->tail)
> mask |= EPOLLIN | EPOLLRDNORM;
> - if (!list->hidraw->exist)
> + if (!list->hidraw->exist || list->revoked)
> mask |= EPOLLERR | EPOLLHUP;
> return mask;
> }
> @@ -313,6 +320,9 @@ static int hidraw_fasync(int fd, struct file *file, int on)
> {
> struct hidraw_list *list = file->private_data;
>
> + if (list->revoked)
> + return -ENODEV;
> +
> return fasync_helper(fd, file, on, &list->fasync);
> }
>
> @@ -360,6 +370,13 @@ static int hidraw_release(struct inode * inode, struct file * file)
> return 0;
> }
>
> +static int hidraw_revoke(struct hidraw_list *list, struct file *file)
There is no use of *file here, we can drop the argument.
> +{
> + list->revoked = true;
> +
> + return 0;
> +}
> +
> static long hidraw_ioctl(struct file *file, unsigned int cmd,
> unsigned long arg)
> {
> @@ -367,11 +384,12 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> unsigned int minor = iminor(inode);
> long ret = 0;
> struct hidraw *dev;
> + struct hidraw_list *list = file->private_data;
> void __user *user_arg = (void __user*) arg;
>
> down_read(&minors_rwsem);
> dev = hidraw_table[minor];
> - if (!dev || !dev->exist) {
> + if (!dev || !dev->exist || list->revoked) {
> ret = -ENODEV;
> goto out;
> }
> @@ -409,6 +427,14 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> ret = -EFAULT;
> break;
> }
> + case HIDIOCREVOKE:
> + {
> + if (user_arg)
> + ret = -EINVAL;
> + else
> + ret = hidraw_revoke(list, file);
> + break;
> + }
> default:
> {
> struct hid_device *hid = dev->hid;
> @@ -515,7 +541,7 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
> list_for_each_entry(list, &dev->list, node) {
> int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
>
> - if (new_head == list->tail)
> + if (list->revoked || new_head == list->tail)
We had quite some discussions offline about that, and I wonder if you
should not squash the following patch into this one:
---
diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
index 3449fe856090..ee5e6fe33a4d 100644
--- a/drivers/hid/hidraw.c
+++ b/drivers/hid/hidraw.c
@@ -36,13 +36,19 @@ static struct class *hidraw_class;
static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES];
static DECLARE_RWSEM(minors_rwsem);
+__weak noinline bool hidraw_is_revoked(struct hidraw_list *list)
+{
+ return list->revoked;
+}
+ALLOW_ERROR_INJECTION(hidraw_is_revoked, TRUE);
+
static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
{
struct hidraw_list *list = file->private_data;
int ret = 0, len;
DECLARE_WAITQUEUE(wait, current);
- if (list->revoked)
+ if (hidraw_is_revoked(list))
return -ENODEV;
mutex_lock(&list->read_mutex);
@@ -165,7 +171,7 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t
struct hidraw_list *list = file->private_data;
ssize_t ret;
down_read(&minors_rwsem);
- if (list->revoked)
+ if (hidraw_is_revoked(list))
ret = -ENODEV;
else
ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
@@ -261,7 +267,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
poll_wait(file, &list->hidraw->wait, wait);
if (list->head != list->tail)
mask |= EPOLLIN | EPOLLRDNORM;
- if (!list->hidraw->exist || list->revoked)
+ if (!list->hidraw->exist || hidraw_is_revoked(list))
mask |= EPOLLERR | EPOLLHUP;
return mask;
}
@@ -320,7 +326,7 @@ static int hidraw_fasync(int fd, struct file *file, int on)
{
struct hidraw_list *list = file->private_data;
- if (list->revoked)
+ if (hidraw_is_revoked(list))
return -ENODEV;
return fasync_helper(fd, file, on, &list->fasync);
@@ -389,7 +395,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
down_read(&minors_rwsem);
dev = hidraw_table[minor];
- if (!dev || !dev->exist || list->revoked) {
+ if (!dev || !dev->exist || hidraw_is_revoked(list)) {
ret = -ENODEV;
goto out;
}
@@ -541,7 +547,8 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
list_for_each_entry(list, &dev->list, node) {
int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
- if (list->revoked || new_head == list->tail)
+ if (hidraw_is_revoked(list) ||
+ new_head == list->tail)
continue;
if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
---
The reasons are:
- we get one common helper for revoked
- we can then emulate with BPF the ioctl even if logind is not the owner
of the fd. This way, we can have the functionality without having to
change a single line in the client applications.
For an example such BPF program, see https://gitlab.freedesktop.org/bentiss/logind-hidraw
Cheers,
Benjamin
> continue;
>
> if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
> diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
> index cd67f4ca5599..18fd30a288de 100644
> --- a/include/linux/hidraw.h
> +++ b/include/linux/hidraw.h
> @@ -32,6 +32,7 @@ struct hidraw_list {
> struct hidraw *hidraw;
> struct list_head node;
> struct mutex read_mutex;
> + bool revoked;
> };
>
> #ifdef CONFIG_HIDRAW
> diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
> index 33ebad81720a..d0563f251da5 100644
> --- a/include/uapi/linux/hidraw.h
> +++ b/include/uapi/linux/hidraw.h
> @@ -46,6 +46,7 @@ struct hidraw_devinfo {
> /* The first byte of SOUTPUT and GOUTPUT is the report number */
> #define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
> #define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
> +#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
>
> #define HIDRAW_FIRST_MINOR 0
> #define HIDRAW_MAX_DEVICES 64
> --
> 2.36.0
>
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl
2022-04-21 7:41 ` Benjamin Tissoires
@ 2022-04-21 8:09 ` Benjamin Tissoires
0 siblings, 0 replies; 3+ messages in thread
From: Benjamin Tissoires @ 2022-04-21 8:09 UTC (permalink / raw)
To: Peter Hutterer; +Cc: Jiri Kosina, linux-input, linux-kernel
On Thu, Apr 21, 2022 at 9:41 AM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
>
>
> On Thu, Apr 21, 2022 at 8:57 AM Peter Hutterer <peter.hutterer@who-t.net> wrote:
> >
> > There is a need for userspace applications to open HID devices directly.
> > Use-cases include configuration of gaming mice or direct access to
> > joystick devices. The latter is currently handled by the uaccess tag in
> > systemd, other devices include more custom/local configurations or just
> > sudo.
> >
> > A better approach is what we already have for evdev devices: give the
> > application a file descriptor and revoke it when it may no longer access
> > that device.
> >
> > This patch is the hidraw equivalent to the EVIOCREVOKE ioctl, see
> > commit c7dc65737c9a607d3e6f8478659876074ad129b8 for full details.
> >
> > A draft MR for systemd-logind has been filed here:
> > https://github.com/systemd/systemd/pull/23140
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
> > ---
> > Maybe noteworthy: even with logind support this is only the first step of
> > many. logind only hands the fd to whoever controls the session and the fd will
> > then have to be passed forward through portals to the application.
> >
> > drivers/hid/hidraw.c | 34 ++++++++++++++++++++++++++++++----
> > include/linux/hidraw.h | 1 +
> > include/uapi/linux/hidraw.h | 1 +
> > 3 files changed, 32 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> > index 681614a8302a..3449fe856090 100644
> > --- a/drivers/hid/hidraw.c
> > +++ b/drivers/hid/hidraw.c
> > @@ -42,6 +42,9 @@ static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count,
> > int ret = 0, len;
> > DECLARE_WAITQUEUE(wait, current);
> >
> > + if (list->revoked)
> > + return -ENODEV;
> > +
> > mutex_lock(&list->read_mutex);
> >
> > while (ret == 0) {
> > @@ -159,9 +162,13 @@ static ssize_t hidraw_send_report(struct file *file, const char __user *buffer,
> >
> > static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t count, loff_t *ppos)
> > {
> > + struct hidraw_list *list = file->private_data;
> > ssize_t ret;
> > down_read(&minors_rwsem);
> > - ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
> > + if (list->revoked)
> > + ret = -ENODEV;
> > + else
> > + ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
> > up_read(&minors_rwsem);
> > return ret;
> > }
> > @@ -254,7 +261,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
> > poll_wait(file, &list->hidraw->wait, wait);
> > if (list->head != list->tail)
> > mask |= EPOLLIN | EPOLLRDNORM;
> > - if (!list->hidraw->exist)
> > + if (!list->hidraw->exist || list->revoked)
> > mask |= EPOLLERR | EPOLLHUP;
> > return mask;
> > }
> > @@ -313,6 +320,9 @@ static int hidraw_fasync(int fd, struct file *file, int on)
> > {
> > struct hidraw_list *list = file->private_data;
> >
> > + if (list->revoked)
> > + return -ENODEV;
> > +
> > return fasync_helper(fd, file, on, &list->fasync);
> > }
> >
> > @@ -360,6 +370,13 @@ static int hidraw_release(struct inode * inode, struct file * file)
> > return 0;
> > }
> >
> > +static int hidraw_revoke(struct hidraw_list *list, struct file *file)
>
> There is no use of *file here, we can drop the argument.
>
> > +{
> > + list->revoked = true;
> > +
> > + return 0;
> > +}
> > +
> > static long hidraw_ioctl(struct file *file, unsigned int cmd,
> > unsigned long arg)
> > {
> > @@ -367,11 +384,12 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> > unsigned int minor = iminor(inode);
> > long ret = 0;
> > struct hidraw *dev;
> > + struct hidraw_list *list = file->private_data;
> > void __user *user_arg = (void __user*) arg;
> >
> > down_read(&minors_rwsem);
> > dev = hidraw_table[minor];
> > - if (!dev || !dev->exist) {
> > + if (!dev || !dev->exist || list->revoked) {
> > ret = -ENODEV;
> > goto out;
> > }
> > @@ -409,6 +427,14 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> > ret = -EFAULT;
> > break;
> > }
> > + case HIDIOCREVOKE:
> > + {
> > + if (user_arg)
> > + ret = -EINVAL;
> > + else
> > + ret = hidraw_revoke(list, file);
> > + break;
> > + }
> > default:
> > {
> > struct hid_device *hid = dev->hid;
> > @@ -515,7 +541,7 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
> > list_for_each_entry(list, &dev->list, node) {
> > int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
> >
> > - if (new_head == list->tail)
> > + if (list->revoked || new_head == list->tail)
>
> We had quite some discussions offline about that, and I wonder if you
> should not squash the following patch into this one:
>
> ---
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index 3449fe856090..ee5e6fe33a4d 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -36,13 +36,19 @@ static struct class *hidraw_class;
> static struct hidraw *hidraw_table[HIDRAW_MAX_DEVICES];
> static DECLARE_RWSEM(minors_rwsem);
>
> +__weak noinline bool hidraw_is_revoked(struct hidraw_list *list)
> +{
> + return list->revoked;
> +}
> +ALLOW_ERROR_INJECTION(hidraw_is_revoked, TRUE);
> +
> static ssize_t hidraw_read(struct file *file, char __user *buffer, size_t count, loff_t *ppos)
> {
> struct hidraw_list *list = file->private_data;
> int ret = 0, len;
> DECLARE_WAITQUEUE(wait, current);
>
> - if (list->revoked)
> + if (hidraw_is_revoked(list))
> return -ENODEV;
>
> mutex_lock(&list->read_mutex);
> @@ -165,7 +171,7 @@ static ssize_t hidraw_write(struct file *file, const char __user *buffer, size_t
> struct hidraw_list *list = file->private_data;
> ssize_t ret;
> down_read(&minors_rwsem);
> - if (list->revoked)
> + if (hidraw_is_revoked(list))
> ret = -ENODEV;
> else
> ret = hidraw_send_report(file, buffer, count, HID_OUTPUT_REPORT);
> @@ -261,7 +267,7 @@ static __poll_t hidraw_poll(struct file *file, poll_table *wait)
> poll_wait(file, &list->hidraw->wait, wait);
> if (list->head != list->tail)
> mask |= EPOLLIN | EPOLLRDNORM;
> - if (!list->hidraw->exist || list->revoked)
> + if (!list->hidraw->exist || hidraw_is_revoked(list))
> mask |= EPOLLERR | EPOLLHUP;
> return mask;
> }
> @@ -320,7 +326,7 @@ static int hidraw_fasync(int fd, struct file *file, int on)
> {
> struct hidraw_list *list = file->private_data;
>
> - if (list->revoked)
> + if (hidraw_is_revoked(list))
> return -ENODEV;
>
> return fasync_helper(fd, file, on, &list->fasync);
> @@ -389,7 +395,7 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
>
> down_read(&minors_rwsem);
> dev = hidraw_table[minor];
> - if (!dev || !dev->exist || list->revoked) {
> + if (!dev || !dev->exist || hidraw_is_revoked(list)) {
> ret = -ENODEV;
> goto out;
> }
> @@ -541,7 +547,8 @@ int hidraw_report_event(struct hid_device *hid, u8 *data, int len)
> list_for_each_entry(list, &dev->list, node) {
> int new_head = (list->head + 1) & (HIDRAW_BUFFER_SIZE - 1);
>
> - if (list->revoked || new_head == list->tail)
> + if (hidraw_is_revoked(list) ||
> + new_head == list->tail)
> continue;
>
> if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
> ---
>
> The reasons are:
> - we get one common helper for revoked
> - we can then emulate with BPF the ioctl even if logind is not the owner
> of the fd. This way, we can have the functionality without having to
> change a single line in the client applications.
>
> For an example such BPF program, see https://gitlab.freedesktop.org/bentiss/logind-hidraw
Another quick thought: maybe we want stable to be added to this patch.
This code hasn't changed in a while and could easily be backported in
older kernel releases. Not sure if it matches stable criterias though
(but it seems we are more relaxed with those criterias).
Cheers,
Benjamin
>
> Cheers,
> Benjamin
>
> > continue;
> >
> > if (!(list->buffer[list->head].value = kmemdup(data, len, GFP_ATOMIC))) {
> > diff --git a/include/linux/hidraw.h b/include/linux/hidraw.h
> > index cd67f4ca5599..18fd30a288de 100644
> > --- a/include/linux/hidraw.h
> > +++ b/include/linux/hidraw.h
> > @@ -32,6 +32,7 @@ struct hidraw_list {
> > struct hidraw *hidraw;
> > struct list_head node;
> > struct mutex read_mutex;
> > + bool revoked;
> > };
> >
> > #ifdef CONFIG_HIDRAW
> > diff --git a/include/uapi/linux/hidraw.h b/include/uapi/linux/hidraw.h
> > index 33ebad81720a..d0563f251da5 100644
> > --- a/include/uapi/linux/hidraw.h
> > +++ b/include/uapi/linux/hidraw.h
> > @@ -46,6 +46,7 @@ struct hidraw_devinfo {
> > /* The first byte of SOUTPUT and GOUTPUT is the report number */
> > #define HIDIOCSOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0B, len)
> > #define HIDIOCGOUTPUT(len) _IOC(_IOC_WRITE|_IOC_READ, 'H', 0x0C, len)
> > +#define HIDIOCREVOKE _IOW('H', 0x0D, int) /* Revoke device access */
> >
> > #define HIDRAW_FIRST_MINOR 0
> > #define HIDRAW_MAX_DEVICES 64
> > --
> > 2.36.0
> >
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-04-21 8:09 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 6:57 [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl Peter Hutterer
2022-04-21 7:41 ` Benjamin Tissoires
2022-04-21 8:09 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).