* [PATCH 1/7] HID: provide a helper for validating hid reports
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 12:33 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 2/7] HID: zeroplus: validate output report details Kees Cook
` (6 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 1/7] HID: provide a helper for validating hid reports
2013-09-04 16:37 ` [PATCH 1/7] HID: provide a helper for validating hid reports Kees Cook
@ 2013-09-09 12:33 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 12:33 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 2/7] HID: zeroplus: validate output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
2013-09-04 16:37 ` [PATCH 1/7] HID: provide a helper for validating hid reports Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 12:36 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 3/7] HID: sony: validate HID " Kees Cook
` (5 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 2/7] HID: zeroplus: validate output report details
2013-09-04 16:37 ` [PATCH 2/7] HID: zeroplus: validate output report details Kees Cook
@ 2013-09-09 12:36 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 12:36 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 3/7] HID: sony: validate HID output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
2013-09-04 16:37 ` [PATCH 1/7] HID: provide a helper for validating hid reports Kees Cook
2013-09-04 16:37 ` [PATCH 2/7] HID: zeroplus: validate output report details Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 12:39 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 4/7] HID: steelseries: validate " Kees Cook
` (4 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 3/7] HID: sony: validate HID output report details
2013-09-04 16:37 ` [PATCH 3/7] HID: sony: validate HID " Kees Cook
@ 2013-09-09 12:39 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 12:39 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 4/7] HID: steelseries: validate output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
` (2 preceding siblings ...)
2013-09-04 16:37 ` [PATCH 3/7] HID: sony: validate HID " Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 13:02 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 5/7] HID: LG: validate HID " Kees Cook
` (3 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 4/7] HID: steelseries: validate output report details
2013-09-04 16:37 ` [PATCH 4/7] HID: steelseries: validate " Kees Cook
@ 2013-09-09 13:02 ` Benjamin Tissoires
2013-09-10 11:45 ` Benjamin Tissoires
0 siblings, 1 reply; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 13:02 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH 4/7] HID: steelseries: validate output report details
2013-09-09 13:02 ` Benjamin Tissoires
@ 2013-09-10 11:45 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-10 11:45 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 5/7] HID: LG: validate HID output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
` (3 preceding siblings ...)
2013-09-04 16:37 ` [PATCH 4/7] HID: steelseries: validate " Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 13:22 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 6/7] HID: lenovo-tpkbd: validate " Kees Cook
` (2 subsequent siblings)
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 5/7] HID: LG: validate HID output report details
2013-09-04 16:37 ` [PATCH 5/7] HID: LG: validate HID " Kees Cook
@ 2013-09-09 13:22 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 13:22 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 6/7] HID: lenovo-tpkbd: validate output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
` (4 preceding siblings ...)
2013-09-04 16:37 ` [PATCH 5/7] HID: LG: validate HID " Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 13:28 ` Benjamin Tissoires
2013-09-04 16:37 ` [PATCH 7/7] HID: logitech-dj: " Kees Cook
2013-09-09 13:48 ` [PATCH v2 0/7] HID: validate " Benjamin Tissoires
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 6/7] HID: lenovo-tpkbd: validate output report details
2013-09-04 16:37 ` [PATCH 6/7] HID: lenovo-tpkbd: validate " Kees Cook
@ 2013-09-09 13:28 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 13:28 UTC (permalink / raw)
To: Kees Cook
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg,
stable
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
^ permalink raw reply [flat|nested] 18+ messages in thread
* [PATCH 7/7] HID: logitech-dj: validate output report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
` (5 preceding siblings ...)
2013-09-04 16:37 ` [PATCH 6/7] HID: lenovo-tpkbd: validate " Kees Cook
@ 2013-09-04 16:37 ` Kees Cook
2013-09-09 13:44 ` Benjamin Tissoires
2013-09-09 13:48 ` [PATCH v2 0/7] HID: validate " Benjamin Tissoires
7 siblings, 1 reply; 18+ messages in thread
From: Kees Cook @ 2013-09-04 16:37 UTC (permalink / raw)
To: linux-input
Cc: Benjamin Tissoires, Jiri Kosina, Henrik Rydberg, Kees Cook,
stable
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
^ permalink raw reply related [flat|nested] 18+ messages in thread* Re: [PATCH 7/7] HID: logitech-dj: validate output report details
2013-09-04 16:37 ` [PATCH 7/7] HID: logitech-dj: " Kees Cook
@ 2013-09-09 13:44 ` Benjamin Tissoires
0 siblings, 0 replies; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 13:44 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Jiri Kosina, Henrik Rydberg
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);
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH v2 0/7] HID: validate report details
2013-09-04 16:37 [PATCH v2 0/7] HID: validate report details Kees Cook
` (6 preceding siblings ...)
2013-09-04 16:37 ` [PATCH 7/7] HID: logitech-dj: " Kees Cook
@ 2013-09-09 13:48 ` Benjamin Tissoires
2013-09-09 21:48 ` Kees Cook
7 siblings, 1 reply; 18+ messages in thread
From: Benjamin Tissoires @ 2013-09-09 13:48 UTC (permalink / raw)
To: Kees Cook; +Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [PATCH v2 0/7] HID: validate report details
2013-09-09 13:48 ` [PATCH v2 0/7] HID: validate " Benjamin Tissoires
@ 2013-09-09 21:48 ` Kees Cook
0 siblings, 0 replies; 18+ messages in thread
From: Kees Cook @ 2013-09-09 21:48 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: linux-input, Benjamin Tissoires, Jiri Kosina, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 18+ messages in thread