[PATCH v2 0/7] HID: validate report details

[PATCH v2 0/7] HID: validate report details

[Kees Cook]

These patches introduce a validation function for HID devices that do
direct report value accesses, solving a number of heap smashing flaws.

This version changes to using an field-index-based checker for the new
"hid_validate_values()" which requires callers to loop across fields if
they use more than one field.

-Kees

[PATCH 1/7] HID: provide a helper for validating hid reports

[Kees Cook]

Many drivers need to validate the characteristics of their HID report
during initialization to avoid misusing the reports. This adds a common
helper to perform validation of the report exisitng, the field existing,
and the expected number of values within the field.

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org

---
v2:
 - suggestions from Benjamin Tissoires:
  - check id too, just to be double-safe.
  - updated to check a specific field, moving the for loop to callers.
---
 drivers/hid/hid-core.c |   58 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/hid.h    |    4 ++++
 2 files changed, 62 insertions(+)

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 21e3b9d..dc23284 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -759,6 +759,64 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
 }
 EXPORT_SYMBOL_GPL(hid_parse_report);
 
+static const char * const hid_report_names[] = {
+	"HID_INPUT_REPORT",
+	"HID_OUTPUT_REPORT",
+	"HID_FEATURE_REPORT",
+};
+/**
+ * hid_validate_values - validate existing device report's value indexes
+ *
+ * @device: hid device
+ * @type: which report type to examine
+ * @id: which report ID to examine (0 for first)
+ * @field_index: which report field to examine
+ * @report_counts: expected number of values
+ *
+ * Validate the number of values in a given field of a given report, after
+ * parsing.
+ */
+struct hid_report *hid_validate_values(struct hid_device *hid,
+				       unsigned int type, unsigned int id,
+				       unsigned int field_index,
+				       unsigned int report_counts)
+{
+	struct hid_report *report;
+
+	if (type > HID_FEATURE_REPORT) {
+		hid_err(hid, "invalid HID report type %u\n", type);
+		return NULL;
+	}
+
+	if (id >= HID_MAX_IDS) {
+		hid_err(hid, "invalid HID report id %u\n", id);
+		return NULL;
+	}
+
+	/*
+	 * Explicitly not using hid_get_report() here since it depends on
+	 * ->numbered being checked, which may not always be the case when
+	 * drivers go to access report values.
+	 */
+	report = hid->report_enum[type].report_id_hash[id];
+	if (!report) {
+		hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
+		return NULL;
+	}
+	if (report->maxfield <= field_index) {
+		hid_err(hid, "not enough fields in %s %u\n",
+			hid_report_names[type], id);
+		return NULL;
+	}
+	if (report->field[field_index]->report_count < report_counts) {
+		hid_err(hid, "not enough values in %s %u field %u\n",
+			hid_report_names[type], id, field_index);
+		return NULL;
+	}
+	return report;
+}
+EXPORT_SYMBOL_GPL(hid_validate_values);
+
 /**
  * hid_open_report - open a driver-specific device report
  *
diff --git a/include/linux/hid.h b/include/linux/hid.h
index ff545cc..6e18550 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -749,6 +749,10 @@ void hid_output_report(struct hid_report *report, __u8 *data);
 struct hid_device *hid_allocate_device(void);
 struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
 int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
+struct hid_report *hid_validate_values(struct hid_device *hid,
+				       unsigned int type, unsigned int id,
+				       unsigned int field_index,
+				       unsigned int report_counts);
 int hid_open_report(struct hid_device *device);
 int hid_check_keys_pressed(struct hid_device *hid);
 int hid_connect(struct hid_device *hid, unsigned int connect_mask);
-- 
1.7.9.5

[PATCH 2/7] HID: zeroplus: validate output report details

[Kees Cook]

The zeroplus HID driver was not checking the size of allocated values
in fields it used. A HID device could send a malicious output report
that would cause the driver to write beyond the output report allocation
during initialization, causing a heap overflow:

[ 1442.728680] usb 1-1: New USB device found, idVendor=0c12, idProduct=0005
...
[ 1466.243173] BUG kmalloc-192 (Tainted: G        W   ): Redzone overwritten

CVE-2013-2889

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-zpff.c |   18 +++++-------------
 1 file changed, 5 insertions(+), 13 deletions(-)

diff --git a/drivers/hid/hid-zpff.c b/drivers/hid/hid-zpff.c
index 6ec28a3..a29756c 100644
--- a/drivers/hid/hid-zpff.c
+++ b/drivers/hid/hid-zpff.c
@@ -68,21 +68,13 @@ static int zpff_init(struct hid_device *hid)
 	struct hid_report *report;
 	struct hid_input *hidinput = list_entry(hid->inputs.next,
 						struct hid_input, list);
-	struct list_head *report_list =
-			&hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct input_dev *dev = hidinput->input;
-	int error;
+	int i, error;
 
-	if (list_empty(report_list)) {
-		hid_err(hid, "no output report found\n");
-		return -ENODEV;
-	}
-
-	report = list_entry(report_list->next, struct hid_report, list);
-
-	if (report->maxfield < 4) {
-		hid_err(hid, "not enough fields in report\n");
-		return -ENODEV;
+	for (i = 0; i < 4; i++) {
+		report = hid_validate_values(hid, HID_OUTPUT_REPORT, 0, i, 1);
+		if (!report)
+			return -ENODEV;
 	}
 
 	zpff = kzalloc(sizeof(struct zpff_device), GFP_KERNEL);
-- 
1.7.9.5

[PATCH 3/7] HID: sony: validate HID output report details

[Kees Cook]

This driver must validate the availability of the HID output report and
its size before it can write LED states via buzz_set_leds(). This stops
a heap overflow that is possible if a device provides a malicious HID
output report:

[  108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
...
[  117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten

CVE-2013-2890

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-sony.c |    4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
index 87fbe29..334a4b5 100644
--- a/drivers/hid/hid-sony.c
+++ b/drivers/hid/hid-sony.c
@@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
 	drv_data = hid_get_drvdata(hdev);
 	BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
 
+	/* Validate expected report characteristics. */
+	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 7))
+		return -ENODEV;
+
 	buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
 	if (!buzz) {
 		hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
-- 
1.7.9.5

[PATCH 4/7] HID: steelseries: validate output report details

[Kees Cook]

A HID device could send a malicious output report that would cause the
steelseries HID driver to write beyond the output report allocation
during initialization, causing a heap overflow:

[  167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
...
[  182.050547] BUG kmalloc-256 (Tainted: G        W   ): Redzone overwritten

CVE-2013-2891

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-steelseries.c |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
index d164911..29f328f 100644
--- a/drivers/hid/hid-steelseries.c
+++ b/drivers/hid/hid-steelseries.c
@@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
 		goto err_free;
 	}
 
+	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) {
+		ret = -ENODEV;
+		goto err_free;
+	}
+
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	if (ret) {
 		hid_err(hdev, "hw start failed\n");
-- 
1.7.9.5

[PATCH 5/7] HID: LG: validate HID output report details

[Kees Cook]

A HID device could send a malicious output report that would cause the
lg, lg3, and lg4 HID drivers to write beyond the output report allocation
during an event, causing a heap overflow:

[  325.245240] usb 1-1: New USB device found, idVendor=046d, idProduct=c287
...
[  414.518960] BUG kmalloc-4096 (Not tainted): Redzone overwritten

Additionally, while lg2 did correctly validate the report details, it was
cleaned up and shortened.

CVE-2013-2893

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-lg2ff.c |   19 +++----------------
 drivers/hid/hid-lg3ff.c |   29 ++++++-----------------------
 drivers/hid/hid-lg4ff.c |   20 +-------------------
 drivers/hid/hid-lgff.c  |   17 ++---------------
 4 files changed, 12 insertions(+), 73 deletions(-)

diff --git a/drivers/hid/hid-lg2ff.c b/drivers/hid/hid-lg2ff.c
index b3cd150..1a42eaa 100644
--- a/drivers/hid/hid-lg2ff.c
+++ b/drivers/hid/hid-lg2ff.c
@@ -64,26 +64,13 @@ int lg2ff_init(struct hid_device *hid)
 	struct hid_report *report;
 	struct hid_input *hidinput = list_entry(hid->inputs.next,
 						struct hid_input, list);
-	struct list_head *report_list =
-			&hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct input_dev *dev = hidinput->input;
 	int error;
 
-	if (list_empty(report_list)) {
-		hid_err(hid, "no output report found\n");
+	/* Check that the report looks ok */
+	report = hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 7);
+	if (!report)
 		return -ENODEV;
-	}
-
-	report = list_entry(report_list->next, struct hid_report, list);
-
-	if (report->maxfield < 1) {
-		hid_err(hid, "output report is empty\n");
-		return -ENODEV;
-	}
-	if (report->field[0]->report_count < 7) {
-		hid_err(hid, "not enough values in the field\n");
-		return -ENODEV;
-	}
 
 	lg2ff = kmalloc(sizeof(struct lg2ff_device), GFP_KERNEL);
 	if (!lg2ff)
diff --git a/drivers/hid/hid-lg3ff.c b/drivers/hid/hid-lg3ff.c
index e52f181..8c2da18 100644
--- a/drivers/hid/hid-lg3ff.c
+++ b/drivers/hid/hid-lg3ff.c
@@ -66,10 +66,11 @@ static int hid_lg3ff_play(struct input_dev *dev, void *data,
 	int x, y;
 
 /*
- * Maxusage should always be 63 (maximum fields)
- * likely a better way to ensure this data is clean
+ * Available values in the field should always be 63, but we only use up to
+ * 35. Instead, clear the entire area, however big it is.
  */
-	memset(report->field[0]->value, 0, sizeof(__s32)*report->field[0]->maxusage);
+	memset(report->field[0]->value, 0,
+	       sizeof(__s32) * report->field[0]->report_count);
 
 	switch (effect->type) {
 	case FF_CONSTANT:
@@ -129,32 +130,14 @@ static const signed short ff3_joystick_ac[] = {
 int lg3ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct input_dev *dev = hidinput->input;
-	struct hid_report *report;
-	struct hid_field *field;
 	const signed short *ff_bits = ff3_joystick_ac;
 	int error;
 	int i;
 
-	/* Find the report to use */
-	if (list_empty(report_list)) {
-		hid_err(hid, "No output report found\n");
-		return -1;
-	}
-
 	/* Check that the report looks ok */
-	report = list_entry(report_list->next, struct hid_report, list);
-	if (!report) {
-		hid_err(hid, "NULL output report\n");
-		return -1;
-	}
-
-	field = report->field[0];
-	if (!field) {
-		hid_err(hid, "NULL field\n");
-		return -1;
-	}
+	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 35))
+		return -ENODEV;
 
 	/* Assume single fixed device G940 */
 	for (i = 0; ff_bits[i] >= 0; i++)
diff --git a/drivers/hid/hid-lg4ff.c b/drivers/hid/hid-lg4ff.c
index 0ddae2a..8782fe1 100644
--- a/drivers/hid/hid-lg4ff.c
+++ b/drivers/hid/hid-lg4ff.c
@@ -484,34 +484,16 @@ static enum led_brightness lg4ff_led_get_brightness(struct led_classdev *led_cde
 int lg4ff_init(struct hid_device *hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct input_dev *dev = hidinput->input;
-	struct hid_report *report;
-	struct hid_field *field;
 	struct lg4ff_device_entry *entry;
 	struct lg_drv_data *drv_data;
 	struct usb_device_descriptor *udesc;
 	int error, i, j;
 	__u16 bcdDevice, rev_maj, rev_min;
 
-	/* Find the report to use */
-	if (list_empty(report_list)) {
-		hid_err(hid, "No output report found\n");
-		return -1;
-	}
-
 	/* Check that the report looks ok */
-	report = list_entry(report_list->next, struct hid_report, list);
-	if (!report) {
-		hid_err(hid, "NULL output report\n");
+	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 7))
 		return -1;
-	}
-
-	field = report->field[0];
-	if (!field) {
-		hid_err(hid, "NULL field\n");
-		return -1;
-	}
 
 	/* Check what wheel has been connected */
 	for (i = 0; i < ARRAY_SIZE(lg4ff_devices); i++) {
diff --git a/drivers/hid/hid-lgff.c b/drivers/hid/hid-lgff.c
index d7ea8c8..e1394af 100644
--- a/drivers/hid/hid-lgff.c
+++ b/drivers/hid/hid-lgff.c
@@ -128,27 +128,14 @@ static void hid_lgff_set_autocenter(struct input_dev *dev, u16 magnitude)
 int lgff_init(struct hid_device* hid)
 {
 	struct hid_input *hidinput = list_entry(hid->inputs.next, struct hid_input, list);
-	struct list_head *report_list = &hid->report_enum[HID_OUTPUT_REPORT].report_list;
 	struct input_dev *dev = hidinput->input;
-	struct hid_report *report;
-	struct hid_field *field;
 	const signed short *ff_bits = ff_joystick;
 	int error;
 	int i;
 
-	/* Find the report to use */
-	if (list_empty(report_list)) {
-		hid_err(hid, "No output report found\n");
-		return -1;
-	}
-
 	/* Check that the report looks ok */
-	report = list_entry(report_list->next, struct hid_report, list);
-	field = report->field[0];
-	if (!field) {
-		hid_err(hid, "NULL field\n");
-		return -1;
-	}
+	if (!hid_validate_values(hid, HID_OUTPUT_REPORT, 0, 0, 7))
+		return -ENODEV;
 
 	for (i = 0; i < ARRAY_SIZE(devices); i++) {
 		if (dev->id.vendor == devices[i].idVendor &&
-- 
1.7.9.5

[PATCH 6/7] HID: lenovo-tpkbd: validate output report details

[Kees Cook]

A HID device could send a malicious output report that would cause the
lenovo-tpkbd HID driver to write just beyond the output report allocation
during initialization, causing a heap overflow:

[   76.109807] usb 1-1: New USB device found, idVendor=17ef, idProduct=6009
...
[   80.462540] BUG kmalloc-192 (Tainted: G        W   ): Redzone overwritten

CVE-2013-2894

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-lenovo-tpkbd.c |   10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
index 07837f5..f458e76 100644
--- a/drivers/hid/hid-lenovo-tpkbd.c
+++ b/drivers/hid/hid-lenovo-tpkbd.c
@@ -339,7 +339,15 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
 	struct tpkbd_data_pointer *data_pointer;
 	size_t name_sz = strlen(dev_name(dev)) + 16;
 	char *name_mute, *name_micmute;
-	int ret;
+	int i, ret;
+
+	/* Validate required reports. */
+	for (i = 0; i < 4; i++) {
+		if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 4, i, 1))
+			return -ENODEV;
+	}
+	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
+		return -ENODEV;
 
 	if (sysfs_create_group(&hdev->dev.kobj,
 				&tpkbd_attr_group_pointer)) {
-- 
1.7.9.5

[PATCH 7/7] HID: logitech-dj: validate output report details

[Kees Cook]

A HID device could send a malicious output report that would cause the
logitech-dj HID driver to leak kernel memory contents to the device, or
trigger a NULL dereference during initialization:

[  304.424553] usb 1-1: New USB device found, idVendor=046d, idProduct=c52b
...
[  304.780467] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
[  304.781409] IP: [<ffffffff815d50aa>] logi_dj_recv_send_report.isra.11+0x1a/0x90

CVE-2013-2895

Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
 drivers/hid/hid-logitech-dj.c |   12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index cd33084..404f2d0 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -461,7 +461,7 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
 	struct hid_report *report;
 	struct hid_report_enum *output_report_enum;
 	u8 *data = (u8 *)(&dj_report->device_index);
-	int i;
+	unsigned int i, length;
 
 	output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
 	report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
@@ -471,7 +471,9 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
 		return -ENODEV;
 	}
 
-	for (i = 0; i < report->field[0]->report_count; i++)
+	length = min_t(size_t, sizeof(*dj_report) - 1,
+			       report->field[0]->report_count);
+	for (i = 0; i < length; i++)
 		report->field[0]->value[i] = data[i];
 
 	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
@@ -783,6 +785,12 @@ static int logi_dj_probe(struct hid_device *hdev,
 		goto hid_parse_fail;
 	}
 
+	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
+				 0, 3)) {
+		retval = -ENODEV;
+		goto hid_parse_fail;
+	}
+
 	/* Starts the usb device and connects to upper interfaces hiddev and
 	 * hidraw */
 	retval = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
-- 
1.7.9.5

Re: [PATCH 1/7] HID: provide a helper for validating hid reports

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> Many drivers need to validate the characteristics of their HID report
> during initialization to avoid misusing the reports. This adds a common
> helper to perform validation of the report exisitng, the field existing,
> and the expected number of values within the field.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
>
> ---

Thanks for the v2.

I'm happy with this one.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

Re: [PATCH 2/7] HID: zeroplus: validate output report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> The zeroplus HID driver was not checking the size of allocated values
> in fields it used. A HID device could send a malicious output report
> that would cause the driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [ 1442.728680] usb 1-1: New USB device found, idVendor=0c12, idProduct=0005
> ...
> [ 1466.243173] BUG kmalloc-192 (Tainted: G        W   ): Redzone overwritten
>
> CVE-2013-2889
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---

I don't have the report descriptor available, but the patch seems ok.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@gmail.com>

Cheers,
Benjamin

Re: [PATCH 3/7] HID: sony: validate HID output report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> This driver must validate the availability of the HID output report and
> its size before it can write LED states via buzz_set_leds(). This stops
> a heap overflow that is possible if a device provides a malicious HID
> output report:
>
> [  108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
> ...
> [  117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>
> CVE-2013-2890
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---

The tests shows that the device is still working with the fix :)

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

Re: [PATCH 4/7] HID: steelseries: validate output report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> steelseries HID driver to write beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [  167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
> ...
> [  182.050547] BUG kmalloc-256 (Tainted: G        W   ): Redzone overwritten
>
> CVE-2013-2891
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
>  drivers/hid/hid-steelseries.c |    5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
> index d164911..29f328f 100644
> --- a/drivers/hid/hid-steelseries.c
> +++ b/drivers/hid/hid-steelseries.c
> @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
>                 goto err_free;
>         }
>
> +       if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) {
> +               ret = -ENODEV;
> +               goto err_free;

There is a problem here in case of a failure:
hid_parse() allocates a lot of memory and sets the flag
HID_STAT_PARSED, but we are leaving the probe() without clearing all
this stuff.
In hid_parse(), when a failure is detected, we call
hid_close_report(), so we also should close the report in the same way
here (but hid_close_report() is a static function).

Cheers,
Benjamin

> +       }
> +
>         ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>         if (ret) {
>                 hid_err(hdev, "hw start failed\n");
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 5/7] HID: LG: validate HID output report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lg, lg3, and lg4 HID drivers to write beyond the output report allocation
> during an event, causing a heap overflow:
>
> [  325.245240] usb 1-1: New USB device found, idVendor=046d, idProduct=c287
> ...
> [  414.518960] BUG kmalloc-4096 (Not tainted): Redzone overwritten
>
> Additionally, while lg2 did correctly validate the report details, it was
> cleaned up and shortened.
>
> CVE-2013-2893
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---

To me, the patch looks good.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin

Re: [PATCH 6/7] HID: lenovo-tpkbd: validate output report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> A HID device could send a malicious output report that would cause the
> lenovo-tpkbd HID driver to write just beyond the output report allocation
> during initialization, causing a heap overflow:
>
> [   76.109807] usb 1-1: New USB device found, idVendor=17ef, idProduct=6009
> ...
> [   80.462540] BUG kmalloc-192 (Tainted: G        W   ): Redzone overwritten
>
> CVE-2013-2894
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---

As mentioned by Josh on the previous thread, this patch triggers a bug
for the users:
https://bugzilla.redhat.com/show_bug.cgi?id=1003998

I am writing the comments I have given the bug:

>  drivers/hid/hid-lenovo-tpkbd.c |   10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-lenovo-tpkbd.c b/drivers/hid/hid-lenovo-tpkbd.c
> index 07837f5..f458e76 100644
> --- a/drivers/hid/hid-lenovo-tpkbd.c
> +++ b/drivers/hid/hid-lenovo-tpkbd.c
> @@ -339,7 +339,15 @@ static int tpkbd_probe_tp(struct hid_device *hdev)
>         struct tpkbd_data_pointer *data_pointer;
>         size_t name_sz = strlen(dev_name(dev)) + 16;
>         char *name_mute, *name_micmute;
> -       int ret;
> +       int i, ret;
> +
> +       /* Validate required reports. */
> +       for (i = 0; i < 4; i++) {
> +               if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 4, i, 1))

The report descriptor shows that the report ID 4 is a FEATURE, not an OUTPUT.

> +                       return -ENODEV;

There is a problem in the probe() function when tpkbd_probe_tp()
fails: it directly bails out instead of calling hid_hw_stop(), thus,
the input devices are still there while nobody has a pointer to them.
This way, ODEBUG is triggered. But this should be address in an other
patch I would say.

Cheers,
Benjamin

> +       }
> +       if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 3, 0, 2))
> +               return -ENODEV;
>
>         if (sysfs_create_group(&hdev->dev.kobj,
>                                 &tpkbd_attr_group_pointer)) {
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 7/7] HID: logitech-dj: validate output report details

[Benjamin Tissoires]

On 04/09/13 18:37, Kees Cook wrote:
> A HID device could send a malicious output report that would cause the
> logitech-dj HID driver to leak kernel memory contents to the device, or
> trigger a NULL dereference during initialization:
> 
> [  304.424553] usb 1-1: New USB device found, idVendor=046d, idProduct=c52b
> ...
> [  304.780467] BUG: unable to handle kernel NULL pointer dereference at 0000000000000028
> [  304.781409] IP: [<ffffffff815d50aa>] logi_dj_recv_send_report.isra.11+0x1a/0x90
> 
> CVE-2013-2895
> 
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
>  drivers/hid/hid-logitech-dj.c |   12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
> index cd33084..404f2d0 100644
> --- a/drivers/hid/hid-logitech-dj.c
> +++ b/drivers/hid/hid-logitech-dj.c
> @@ -461,7 +461,7 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
>  	struct hid_report *report;
>  	struct hid_report_enum *output_report_enum;
>  	u8 *data = (u8 *)(&dj_report->device_index);
> -	int i;
> +	unsigned int i, length;
>  
>  	output_report_enum = &hdev->report_enum[HID_OUTPUT_REPORT];
>  	report = output_report_enum->report_id_hash[REPORT_ID_DJ_SHORT];
> @@ -471,7 +471,9 @@ static int logi_dj_recv_send_report(struct dj_receiver_dev *djrcv_dev,
>  		return -ENODEV;
>  	}
>  
> -	for (i = 0; i < report->field[0]->report_count; i++)
> +	length = min_t(size_t, sizeof(*dj_report) - 1,
> +			       report->field[0]->report_count);
> +	for (i = 0; i < length; i++)

I would rather simply do:
	for (i = 0; i < DJREPORT_SHORT_LENGTH - 1; i++)


>  		report->field[0]->value[i] = data[i];
>  
>  	hid_hw_request(hdev, report, HID_REQ_SET_REPORT);
> @@ -783,6 +785,12 @@ static int logi_dj_probe(struct hid_device *hdev,
>  		goto hid_parse_fail;
>  	}
>  
> +	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
> +				 0, 3)) {

and here:
	if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
				 0, DJREPORT_SHORT_LENGTH - 1)) {

3 seems to come from nowhere, and is also wrong because in logi_dj_recv_switch_to_dj_mode(), we write 4 bytes.

Besides this small nitpick, there is the same problem that I spotted in hid-steelseries, in case of a failure, the hid device is not cleaned correctly.

Cheers,
Benjamin

> +		retval = -ENODEV;
> +		goto hid_parse_fail;
> +	}
> +
>  	/* Starts the usb device and connects to upper interfaces hiddev and
>  	 * hidraw */
>  	retval = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> 

Re: [PATCH v2 0/7] HID: validate report details

[Benjamin Tissoires]

On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
> These patches introduce a validation function for HID devices that do
> direct report value accesses, solving a number of heap smashing flaws.
>
> This version changes to using an field-index-based checker for the new
> "hid_validate_values()" which requires callers to loop across fields if
> they use more than one field.

I am globally happy with the patch series.
I have some concerns about patches 4 6 and 7, but the other can be
applied right now.

Kees, if you want to switch to something else, I can handle the v3 for
these three patches: I have some logitech-dj devices and a tester for
the lenovo one.

Cheers,
Benjamin

>
> -Kees
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/7] HID: validate report details

[Kees Cook]

On Mon, Sep 9, 2013 at 6:48 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
>> These patches introduce a validation function for HID devices that do
>> direct report value accesses, solving a number of heap smashing flaws.
>>
>> This version changes to using an field-index-based checker for the new
>> "hid_validate_values()" which requires callers to loop across fields if
>> they use more than one field.
>
> I am globally happy with the patch series.
> I have some concerns about patches 4 6 and 7, but the other can be
> applied right now.
>
> Kees, if you want to switch to something else, I can handle the v3 for
> these three patches: I have some logitech-dj devices and a tester for
> the lenovo one.

That would be fantastic, thank you. I don't have any tools to test
these in the "expected" case. :)

-Kees

-- 
Kees Cook
Chrome OS Security

Re: [PATCH 4/7] HID: steelseries: validate output report details

[Benjamin Tissoires]

On Mon, Sep 9, 2013 at 3:02 PM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Sep 4, 2013 at 6:37 PM, Kees Cook <keescook@chromium.org> wrote:
>> A HID device could send a malicious output report that would cause the
>> steelseries HID driver to write beyond the output report allocation
>> during initialization, causing a heap overflow:
>>
>> [  167.981534] usb 1-1: New USB device found, idVendor=1038, idProduct=1410
>> ...
>> [  182.050547] BUG kmalloc-256 (Tainted: G        W   ): Redzone overwritten
>>
>> CVE-2013-2891
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@kernel.org
>> ---
>>  drivers/hid/hid-steelseries.c |    5 +++++
>>  1 file changed, 5 insertions(+)
>>
>> diff --git a/drivers/hid/hid-steelseries.c b/drivers/hid/hid-steelseries.c
>> index d164911..29f328f 100644
>> --- a/drivers/hid/hid-steelseries.c
>> +++ b/drivers/hid/hid-steelseries.c
>> @@ -249,6 +249,11 @@ static int steelseries_srws1_probe(struct hid_device *hdev,
>>                 goto err_free;
>>         }
>>
>> +       if (!hid_validate_values(hdev, HID_OUTPUT_REPORT, 0, 0, 16)) {
>> +               ret = -ENODEV;
>> +               goto err_free;
>
> There is a problem here in case of a failure:
> hid_parse() allocates a lot of memory and sets the flag
> HID_STAT_PARSED, but we are leaving the probe() without clearing all
> this stuff.
> In hid_parse(), when a failure is detected, we call
> hid_close_report(), so we also should close the report in the same way
> here (but hid_close_report() is a static function).
>

Sorry, my bad. hid_close_report() is called upon ->probe() failure. So
this is perfectly good.

Reviewed-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>

Cheers,
Benjamin