| Message ID | 20200805095501.GD483832@mwanda (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: roccat: add bounds checking in kone_sysfs_write_settings() | expand |
On Wed, 5 Aug 2020, Dan Carpenter wrote: > In the original code we didn't check if the new value for > "settings->startup_profile" was within bounds that could possibly > result in an out of bounds array acccess. What we did was we checked if > we could write the data to the firmware and it's possibly the firmware > checks these values but there is no way to know. It's safer and easier > to read if we check it in the kernel as well. > > I also added a check to ensure that "settings->size" was correct. The > comments say that the only valid value is 36 which is sizeof(struct > kone_settings). > > Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> Stefan, could I please get your Reviewed-by and/or Tested-by, to make sure this doesn't unintentionally somehow break userspace? Thanks,
Please remove the settings->size check. The driver sometimes just writes a modified version of the read settings data. This would fail if stored size is invalid. This value of size is not solely in my hands, I can't guarantee Windows driver does a sanity check, also cases of flash data corruption are known. My general design agenda is that in my userspace tools I make sure only valid data (to the best of knowledge) is prepared for the device. In the kernel driver I don't (or didn't) do additional checks and eventually let the hardware reject things it doesn't like. This way the kernel driver doesn't need an update if firmware updates change the binary interface. This happend in the past, but won't for this device anymore because it's oop for years. Am Mittwoch, den 05.08.2020, 12:55 +0300 schrieb Dan Carpenter: > In the original code we didn't check if the new value for > "settings->startup_profile" was within bounds that could possibly > result in an out of bounds array acccess. What we did was we checked > if > we could write the data to the firmware and it's possibly the > firmware > checks these values but there is no way to know. It's safer and > easier > to read if we check it in the kernel as well. > > I also added a check to ensure that "settings->size" was > correct. The > comments say that the only valid value is 36 which is sizeof(struct > kone_settings). > > Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse") > Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> > --- > This isn't tested. Checking settings->size seems like a good idea, > but > there is a slight risky because maybe invalid values used to be > allowed > and I don't want to break user space. > > drivers/hid/hid-roccat-kone.c | 24 +++++++++++++++++------- > 1 file changed, 17 insertions(+), 7 deletions(-) > > diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat- > kone.c > index 1a6e600197d0..5e8e1517f23c 100644 > --- a/drivers/hid/hid-roccat-kone.c > +++ b/drivers/hid/hid-roccat-kone.c > @@ -294,31 +294,41 @@ static ssize_t kone_sysfs_write_settings(struct > file *fp, struct kobject *kobj, > struct kone_device *kone = > hid_get_drvdata(dev_get_drvdata(dev)); > struct usb_device *usb_dev = > interface_to_usbdev(to_usb_interface(dev)); > int retval = 0, difference, old_profile; > + struct kone_settings *settings = (struct kone_settings > *)buf; > > /* I need to get my data in one piece */ > if (off != 0 || count != sizeof(struct kone_settings)) > return -EINVAL; > > mutex_lock(&kone->kone_lock); > - difference = memcmp(buf, &kone->settings, sizeof(struct > kone_settings)); > + difference = memcmp(settings, &kone->settings, > + sizeof(struct kone_settings)); > if (difference) { > - retval = kone_set_settings(usb_dev, > - (struct kone_settings const *)buf); > - if (retval) { > - mutex_unlock(&kone->kone_lock); > - return retval; > + if (settings->size != sizeof(struct kone_settings) > || > + settings->startup_profile < 1 || > + settings->startup_profile > 5) { > + retval = -EINVAL; > + goto unlock; > } > > + retval = kone_set_settings(usb_dev, settings); > + if (retval) > + goto unlock; > + > old_profile = kone->settings.startup_profile; > - memcpy(&kone->settings, buf, sizeof(struct > kone_settings)); > + memcpy(&kone->settings, settings, sizeof(struct > kone_settings)); > > kone_profile_activated(kone, kone- > >settings.startup_profile); > > if (kone->settings.startup_profile != old_profile) > kone_profile_report(kone, kone- > >settings.startup_profile); > } > +unlock: > mutex_unlock(&kone->kone_lock); > > + if (retval) > + return retval; > + > return sizeof(struct kone_settings); > } > static BIN_ATTR(settings, 0660, kone_sysfs_read_settings,
diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c index 1a6e600197d0..5e8e1517f23c 100644 --- a/drivers/hid/hid-roccat-kone.c +++ b/drivers/hid/hid-roccat-kone.c @@ -294,31 +294,41 @@ static ssize_t kone_sysfs_write_settings(struct file *fp, struct kobject *kobj, struct kone_device *kone = hid_get_drvdata(dev_get_drvdata(dev)); struct usb_device *usb_dev = interface_to_usbdev(to_usb_interface(dev)); int retval = 0, difference, old_profile; + struct kone_settings *settings = (struct kone_settings *)buf; /* I need to get my data in one piece */ if (off != 0 || count != sizeof(struct kone_settings)) return -EINVAL; mutex_lock(&kone->kone_lock); - difference = memcmp(buf, &kone->settings, sizeof(struct kone_settings)); + difference = memcmp(settings, &kone->settings, + sizeof(struct kone_settings)); if (difference) { - retval = kone_set_settings(usb_dev, - (struct kone_settings const *)buf); - if (retval) { - mutex_unlock(&kone->kone_lock); - return retval; + if (settings->size != sizeof(struct kone_settings) || + settings->startup_profile < 1 || + settings->startup_profile > 5) { + retval = -EINVAL; + goto unlock; } + retval = kone_set_settings(usb_dev, settings); + if (retval) + goto unlock; + old_profile = kone->settings.startup_profile; - memcpy(&kone->settings, buf, sizeof(struct kone_settings)); + memcpy(&kone->settings, settings, sizeof(struct kone_settings)); kone_profile_activated(kone, kone->settings.startup_profile); if (kone->settings.startup_profile != old_profile) kone_profile_report(kone, kone->settings.startup_profile); } +unlock: mutex_unlock(&kone->kone_lock); + if (retval) + return retval; + return sizeof(struct kone_settings); } static BIN_ATTR(settings, 0660, kone_sysfs_read_settings,
In the original code we didn't check if the new value for "settings->startup_profile" was within bounds that could possibly result in an out of bounds array acccess. What we did was we checked if we could write the data to the firmware and it's possibly the firmware checks these values but there is no way to know. It's safer and easier to read if we check it in the kernel as well. I also added a check to ensure that "settings->size" was correct. The comments say that the only valid value is 36 which is sizeof(struct kone_settings). Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse") Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com> --- This isn't tested. Checking settings->size seems like a good idea, but there is a slight risky because maybe invalid values used to be allowed and I don't want to break user space. drivers/hid/hid-roccat-kone.c | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-)