[PATCH 1/4] HID: lamparray: read attribute reports synchronously

[Cristian Mazzotta]

lamparray_read_attributes_report() and lamparray_get_lamp_attributes()
use hid_hw_request(HID_REQ_GET_REPORT) followed by hid_hw_wait(), then
read the results out of field->value[]. hid_hw_request() is asynchronous
and hid_hw_wait() only clears the output queue, so the values are still
read before the transfer has completed.

On an Acer Predator PT14-52T (USB keyboard 05AF:767A), this returns zero
for LampCount, and then zero for red, green, blue, and intensity counts.
The last one is evil: max_brightness is passed to
led_mc_calc_color_components() as a divisor during
lamparray_register_led(), resulting in a divide by zero during probe:

    Oops: divide error: 0000 [#1] SMP NOPTI
    RIP: 0010:led_mc_calc_color_components+0x58/0x70
    Call Trace:
        lamparray_register_led+0x119/0x1e0
        lamparray_register+0x502/0x820
        hid_generic_probe+0x5e/0xc0

The fault kills the kworker running hub_event() while it holds the USB
and HID device locks, which stalls further probing on that bus.

Use hid_hw_raw_request() with a hid_report_len()-sized buffer and hand
the result to hid_report_raw_event() so the HID core parses it into
field->value[] before the values are read.

Validate the level counts after reading them and fail with -EINVAL if
any is zero, so a device reporting no levels cannot reach
led_mc_calc_color_components() at all. The four level fields are also
required to share one report, since a single GET_REPORT is used to
fetch them.

Signed-off-by: Cristian Mazzotta <cmmazzo@xxxxxxxxxx>
---
 drivers/hid/hid-lamparray.c | 73 ++++++++++++++++++++++++++++++-------
 1 file changed, 59 insertions(+), 14 deletions(-)

diff --git a/drivers/hid/hid-lamparray.c b/drivers/hid/hid-lamparray.c
index 9a438aa2d305..f169929aecd6 100644
--- a/drivers/hid/hid-lamparray.c
+++ b/drivers/hid/hid-lamparray.c
@@ -164,6 +164,9 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 {
 	struct hid_device *hdev = ldev->hdev;
 	struct hid_report *report;
+	int ret;
+	u8 *buf;
+	size_t len;
 
 	if (!ldev->lamp_count.field) {
 		hid_dbg(hdev, "No LampCount field found\n");
@@ -182,25 +185,37 @@ static int lamparray_read_attributes_report(struct lamparray_device *ldev)
 		return -ENODEV;
 	}
 
+	len = hid_report_len(report);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
+
 	mutex_lock(&ldev->dev_lock);
 
 	/* Update values */
-	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
-	hid_hw_wait(hdev);
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_dbg(hdev, "Failed to get LampCount value from device: %d\n", ret);
+		goto out;
+	}
 
-	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+	hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
 
-	if (ldev->lamp_count_value == 0) {
-		mutex_unlock(&ldev->dev_lock);
-		hid_dbg(hdev, "LampCount is %d (invalid)\n", ldev->lamp_count_value);
-		return -EINVAL;
+	ldev->lamp_count_value = get_field_value(&ldev->lamp_count);
+	if (!ldev->lamp_count_value) {
+		hid_dbg(hdev, "LampCount is 0 (invalid)\n");
+		ret = -EINVAL;
+		goto out;
 	}
 
 	ldev->lamparray_kind_value = get_field_value(&ldev->lamparray_kind);
+	ret = 0;
 
+out:
 	mutex_unlock(&ldev->dev_lock);
-
-	return 0;
+	kfree(buf);
+	return ret;
 }
 
 static int lamparray_parse_update_report(struct lamparray_device *ldev)
@@ -371,28 +386,58 @@ static int lamparray_get_lamp_attributes(struct lamparray_device *ldev)
 {
 	struct hid_device *hdev = ldev->hdev;
 	struct hid_report *report;
+	int ret;
+	u8 *buf;
+	size_t len;
 
 	if (!lamparray_color_fields_is_complete(&ldev->color_levels))
 		return -ENODEV;
 
 	/*
-	 * Get value of any lamp.
+	 * All four fields must share the same report since the
+	 * attributes are fetched with a single GET_REPORT below.
 	 */
 	report = ldev->color_levels.red.field->report;
+	if (!report ||
+		ldev->color_levels.green.field->report != report ||
+		ldev->color_levels.blue.field->report != report ||
+		ldev->color_levels.intensity.field->report != report)
+		return -ENODEV;
+
+	len = hid_report_len(report);
+	buf = kmalloc(len, GFP_KERNEL);
+	if (!buf)
+		return -ENOMEM;
 
 	mutex_lock(&ldev->dev_lock);
 
-	hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
-	hid_hw_wait(hdev);
+	/* Update values */
+	ret = hid_hw_raw_request(hdev, report->id, buf, len,
+		HID_FEATURE_REPORT, HID_REQ_GET_REPORT);
+	if (ret < 0) {
+		hid_dbg(hdev, "Failed to read LampAttributesResponseReport: %d\n", ret);
+		goto out;
+	}
+
+	hid_report_raw_event(hdev, HID_FEATURE_REPORT, buf, len, ret, 0);
 
 	ldev->max_r = get_field_value_as_u8(&ldev->color_levels.red);
 	ldev->max_g = get_field_value_as_u8(&ldev->color_levels.green);
 	ldev->max_b = get_field_value_as_u8(&ldev->color_levels.blue);
 	ldev->max_brightness = get_field_value_as_u8(&ldev->color_levels.intensity);
 
-	mutex_unlock(&ldev->dev_lock);
+	if (!ldev->max_r || !ldev->max_g || !ldev->max_b || !ldev->max_brightness) {
+		hid_dbg(hdev, "LampArray device has no color levels\n");
+		ret = -EINVAL;
+		goto out;
+	}
 
-	return 0;
+	ret = 0;
+
+out:
+	mutex_unlock(&ldev->dev_lock);
+	kfree(buf);
+	return ret;
 }
 
 /* Helper functions */
-- 
2.55.0