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

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

[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,
-- 
2.27.0

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

[Jiri Kosina]

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,

-- 
Jiri Kosina
SUSE Labs

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

[Stefan Achatz]

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,

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

[Dan Carpenter]

This code doesn't check if "settings->startup_profile" is within bounds
and that could result in an out of bounds array access.  What the code
does do is it checks if the settings can be written to the firmware, so
it's possible that the firmware has a bounds check?  It's safer and
easier to verify when the bounds checking is done in the kernel.

Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  In the v1 patch I added a check against settings->size but that's
potentially too strict so it was removed.

 drivers/hid/hid-roccat-kone.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

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,
-- 
2.28.0

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

[Walter Harms]

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
________________________________________
Von: kernel-janitors-owner@vger.kernel.org [kernel-janitors-owner@vger.kernel.org] im Auftrag von Dan Carpenter [dan.carpenter@oracle.com]
Gesendet: Montag, 24. August 2020 10:57
An: Stefan Achatz
Cc: Jiri Kosina; Benjamin Tissoires; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: [PATCH v2] HID: roccat: add bounds checking in kone_sysfs_write_settings()

This code doesn't check if "settings->startup_profile" is within bounds
and that could result in an out of bounds array access.  What the code
does do is it checks if the settings can be written to the firmware, so
it's possible that the firmware has a bounds check?  It's safer and
easier to verify when the bounds checking is done in the kernel.

Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse")
Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
---
v2:  In the v1 patch I added a check against settings->size but that's
potentially too strict so it was removed.

 drivers/hid/hid-roccat-kone.c | 23 ++++++++++++++++-------
 1 file changed, 16 insertions(+), 7 deletions(-)

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,
--
2.28.0

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

[Dan Carpenter]

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

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

[Walter Harms]

lets hope the maintainer picks that up.

re,
 wh

________________________________________
Von: Dan Carpenter [dan.carpenter@oracle.com]
Gesendet: Dienstag, 25. August 2020 09:29
An: Walter Harms
Cc: Stefan Achatz; Jiri Kosina; Benjamin Tissoires; linux-input@vger.kernel.org; linux-kernel@vger.kernel.org; kernel-janitors@vger.kernel.org
Betreff: Re: [PATCH v2] HID: roccat: add bounds checking in kone_sysfs_write_settings()

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

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

[Jiri Kosina]

On Mon, 24 Aug 2020, Dan Carpenter wrote:

> This code doesn't check if "settings->startup_profile" is within bounds
> and that could result in an out of bounds array access.  What the code
> does do is it checks if the settings can be written to the firmware, so
> it's possible that the firmware has a bounds check?  It's safer and
> easier to verify when the bounds checking is done in the kernel.
> 
> Fixes: 14bf62cde794 ("HID: add driver for Roccat Kone gaming mouse")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
> ---
> v2:  In the v1 patch I added a check against settings->size but that's
> potentially too strict so it was removed.

Applied, thanks Dan.

-- 
Jiri Kosina
SUSE Labs