| 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: <cznzhmuizk6r3tv7ze6lqp3qoyfnu6ktmbwxbjpsf5phthsjoj@x46mthdr66l7>
Date: Fri, 11 Jul 2025 11:40:54 +0200
From: Benjamin Tissoires <bentiss@...nel.org>
To: Arnd Bergmann <arnd@...nel.org>
Cc: Jiri Kosina <jikos@...nel.org>, Arnd Bergmann <arnd@...db.de>,
Peter Hutterer <peter.hutterer@...-t.net>, linux-input@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] HID: tighten ioctl command parsing
Hi Arnd,
On Jul 11 2025, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.de>
>
> The handling for variable-length ioctl commands in hidraw_ioctl() is
> rather complex and the check for the data direction is incomplete.
>
> Simplify this code using a switch() statement with the size masked
> out, to ensure the rest of the command is correctly matched.
How much "urgent" you believe this patch is. I would say 6.17 material,
but I'm not sure if your analysis regarding "the check for the data
direction is incomplete." would justify a 6.16 late fix.
Also, lately I added a new tools/testing/selftests/hid/hidraw.c to test
the latest HIDIOCREVOKE addition. I wonder if we should not add a couple
of checks there to ensure we get the different kind of other ioctls
tested before and after this patch.
Cheers,
Benjamin
>
> Fixes: 9188e79ec3fd ("HID: add phys and name ioctls to hidraw")
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> ---
> drivers/hid/hidraw.c | 123 ++++++++++++++++++-------------------------
> 1 file changed, 50 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/hid/hidraw.c b/drivers/hid/hidraw.c
> index c887f48756f4..cc657d60d689 100644
> --- a/drivers/hid/hidraw.c
> +++ b/drivers/hid/hidraw.c
> @@ -403,6 +403,8 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> struct hidraw *dev;
> struct hidraw_list *list = file->private_data;
> void __user *user_arg = (void __user*) arg;
> + struct hid_device *hid;
> + int len;
>
> down_read(&minors_rwsem);
> dev = hidraw_table[minor];
> @@ -453,81 +455,56 @@ static long hidraw_ioctl(struct file *file, unsigned int cmd,
> break;
> }
> default:
> - {
> - struct hid_device *hid = dev->hid;
> - if (_IOC_TYPE(cmd) != 'H') {
> - ret = -EINVAL;
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSFEATURE(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
> - break;
> - }
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGFEATURE(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSINPUT(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_send_report(file, user_arg, len, HID_INPUT_REPORT);
> - break;
> - }
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGINPUT(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_get_report(file, user_arg, len, HID_INPUT_REPORT);
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCSOUTPUT(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_send_report(file, user_arg, len, HID_OUTPUT_REPORT);
> - break;
> - }
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGOUTPUT(0))) {
> - int len = _IOC_SIZE(cmd);
> - ret = hidraw_get_report(file, user_arg, len, HID_OUTPUT_REPORT);
> - break;
> - }
> -
> - /* Begin Read-only ioctls. */
> - if (_IOC_DIR(cmd) != _IOC_READ) {
> - ret = -EINVAL;
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWNAME(0))) {
> - int len = strlen(hid->name) + 1;
> - if (len > _IOC_SIZE(cmd))
> - len = _IOC_SIZE(cmd);
> - ret = copy_to_user(user_arg, hid->name, len) ?
> - -EFAULT : len;
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWPHYS(0))) {
> - int len = strlen(hid->phys) + 1;
> - if (len > _IOC_SIZE(cmd))
> - len = _IOC_SIZE(cmd);
> - ret = copy_to_user(user_arg, hid->phys, len) ?
> - -EFAULT : len;
> - break;
> - }
> -
> - if (_IOC_NR(cmd) == _IOC_NR(HIDIOCGRAWUNIQ(0))) {
> - int len = strlen(hid->uniq) + 1;
> - if (len > _IOC_SIZE(cmd))
> - len = _IOC_SIZE(cmd);
> - ret = copy_to_user(user_arg, hid->uniq, len) ?
> - -EFAULT : len;
> - break;
> - }
> - }
> + break;
> + }
>
> + hid = dev->hid;
> + switch (cmd & ~IOCSIZE_MASK) {
> + case HIDIOCSFEATURE(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_send_report(file, user_arg, len, HID_FEATURE_REPORT);
> + break;
> + case HIDIOCGFEATURE(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_get_report(file, user_arg, len, HID_FEATURE_REPORT);
> + break;
> + case HIDIOCSINPUT(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_send_report(file, user_arg, len, HID_INPUT_REPORT);
> + break;
> + case HIDIOCGINPUT(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_get_report(file, user_arg, len, HID_INPUT_REPORT);
> + break;
> + case HIDIOCSOUTPUT(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_send_report(file, user_arg, len, HID_OUTPUT_REPORT);
> + break;
> + case HIDIOCGOUTPUT(0):
> + len = _IOC_SIZE(cmd);
> + ret = hidraw_get_report(file, user_arg, len, HID_OUTPUT_REPORT);
> + break;
> + case HIDIOCGRAWNAME(0):
> + len = strlen(hid->name) + 1;
> + if (len > _IOC_SIZE(cmd))
> + len = _IOC_SIZE(cmd);
> + ret = copy_to_user(user_arg, hid->name, len) ? -EFAULT : len;
> + break;
> + case HIDIOCGRAWPHYS(0):
> + len = strlen(hid->phys) + 1;
> + if (len > _IOC_SIZE(cmd))
> + len = _IOC_SIZE(cmd);
> + ret = copy_to_user(user_arg, hid->phys, len) ? -EFAULT : len;
> + break;
> + case HIDIOCGRAWUNIQ(0):
> + len = strlen(hid->uniq) + 1;
> + if (len > _IOC_SIZE(cmd))
> + len = _IOC_SIZE(cmd);
> + ret = copy_to_user(user_arg, hid->uniq, len) ? -EFAULT : len;
> + break;
> + default:
> ret = -ENOTTY;
> + break;
> }
> out:
> up_read(&minors_rwsem);
> --
> 2.39.5
>
Powered by blists - more mailing lists