On Thu, Apr 21, 2022 at 9:41 AM Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx> wrote: > > > > On Thu, Apr 21, 2022 at 8:57 AM Peter Hutterer <peter.hutterer@xxxxxxxxx> 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@xxxxxxxxx> > > --- > > 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 > >
