| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 21 Apr 2022 10:09:15 +0200
From: Benjamin Tissoires <benjamin.tissoires@...hat.com>
To: Peter Hutterer <peter.hutterer@...-t.net>
Cc: Jiri Kosina <jikos@...nel.org>, linux-input@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: hidraw - add HIDIOCREVOKE ioctl
On Thu, Apr 21, 2022 at 9:41 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
>
>
> On Thu, Apr 21, 2022 at 8:57 AM Peter Hutterer <peter.hutterer@...-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@...-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
> >
Powered by blists - more mailing lists