Skip to contentSkip to conversation

hid: thingm: reorder calls in thingm_probe

PATCH Applied Heiner Kallweit 2 participants 2 messages Mar 10, 2016 – Mar 15, 2016

Subsystems (2)
HID LED
lore
Share & tools
lore

Heiner Kallweit

author
#0 lore raw

PATCH v2 1/1 hid: thingm: reorder calls in thingm_probe

When reviewing another thingm patch Benjamin Tissoires pointed out the following: "The problem here is that hid_hw_start() is called before thingm_version() which allows user space to briefly introduce races between thingm_version() and any hidraw requests.
The mutex will not help here as it is initialized after hid_hw_start() and only used for protecting the concurrent access of the rgb."

Avoid this possible issue by calling hid_hw_start() later in the probe function.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- make commit message more verbose

---
 drivers/hid/hid-thingm.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)
drivers/hid/hid-thingm.c +13 -18
index f4196ac..847a497 100644
@@ -216,17 +216,13 @@ static int thingm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	err = hid_parse(hdev);
 	if (err)
-		goto error;
-
-	err = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
-	if (err)
-		goto error;
+		return err;
 
 	mutex_init(&tdev->lock);
 
 	err = thingm_version(tdev);
 	if (err)
-		goto stop;
+		return err;
 
 	hid_dbg(hdev, "firmware version: %c.%c\n",
 			tdev->version.major, tdev->version.minor);

@@ -237,17 +233,18 @@ static int thingm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	if (!tdev->fwinfo) {
 		hid_err(hdev, "unsupported firmware %c\n", tdev->version.major);
-		err = -ENODEV;
-		goto stop;
+		return -ENODEV;
 	}
 
 	tdev->rgb = devm_kzalloc(&hdev->dev,
 			sizeof(struct thingm_rgb) * tdev->fwinfo->numrgb,
 			GFP_KERNEL);
-	if (!tdev->rgb) {
-		err = -ENOMEM;
-		goto stop;
-	}
+	if (!tdev->rgb)
+		return -ENOMEM;
+
+	err = hid_hw_start(hdev, HID_CONNECT_HIDRAW);
+	if (err)
+		return err;
 
 	for (i = 0; i < tdev->fwinfo->numrgb; ++i) {
 		struct thingm_rgb *rgb = tdev->rgb + i;

@@ -255,15 +252,13 @@ static int thingm_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		rgb->tdev = tdev;
 		rgb->num = tdev->fwinfo->first + i;
 		err = thingm_init_rgb(rgb);
-		if (err)
-			goto stop;
+		if (err) {
+			hid_hw_stop(hdev);
+			return err;
+		}
 	}
 
 	return 0;
-stop:
-	hid_hw_stop(hdev);
-error:
-	return err;
 }
 
 static const struct hid_device_id thingm_table[] = {
Signature · 1 line
2.7.1
maintainer Applied
#1 lore raw
Quoted 7 lines · Heiner Kallweit

When reviewing another thingm patch Benjamin Tissoires pointed out the following: "The problem here is that hid_hw_start() is called before thingm_version() which allows user space to briefly introduce races between thingm_version() and any hidraw requests.
The mutex will not help here as it is initialized after hid_hw_start() and only used for protecting the concurrent access of the rgb."

Quoted 7 lines

Avoid this possible issue by calling hid_hw_start() later in the probe function.

Signed-off-by Heiner Kallweit <[email protected]>

---
v2:
- make commit message more verbose

Applied to for-4.6/thingm.

Signature · 2 lines
Jiri Kosina
SUSE Labs