AW: [PATCH v2] HID: roccat: add bounds checking in kone_sysfs_write_settings()

Commit Message

Walter Harms Aug. 24, 2020, 3:35 p.m. UTC

hello Dan, 

i notice that you can shorten the line to:
(line above checks for count==sizeof(struct kone_settings))

difference = memcmp(settings, &kone->settings, count);

nothing special just to shorten the line and make use of count.

and just to save one indent level and because its  readabel nicely:
    if ( ! difference ) 
          goto unlock;

hope that helps

re,
 wh

Comments

Dan Carpenter Aug. 25, 2020, 7:29 a.m. UTC | #1

On Mon, Aug 24, 2020 at 03:35:16PM +0000, Walter Harms wrote:
> hello Dan, 
> 
> i notice that you can shorten the line to:
> (line above checks for count==sizeof(struct kone_settings))
> 
> difference = memcmp(settings, &kone->settings, count);
> 
> nothing special just to shorten the line and make use of count.
> 
> and just to save one indent level and because its  readabel nicely:
>     if ( ! difference ) 
>           goto unlock;
> 
> hope that helps

Yeah.  I wrote that version and I wanted to send it, but then I decided
not to change the style too much.  I definitely agree with you, but I
figured I would keep the patch less intrusive.

regards,
dan carpenter

Walter Harms Aug. 25, 2020, 1:08 p.m. UTC | #2

lets hope the maintainer picks that up.

re,
 wh

Patch

diff --git a/drivers/hid/hid-roccat-kone.c b/drivers/hid/hid-roccat-kone.c
index 2ff4c8e366ff..1ca64481145e 100644
--- a/drivers/hid/hid-roccat-kone.c
+++ b/drivers/hid/hid-roccat-kone.c
@@ -294,31 +294,40 @@  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->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,