| 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
| ||
|
Message-ID: <99f54616-5464-be0d-9454-638352bc39eb@redhat.com>
Date: Thu, 21 Apr 2022 09:41:02 +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 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
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