diff mbox

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

Message ID 1378312645-27736-5-git-send-email-keescook@chromium.org (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Kees Cook Sept. 4, 2013, 4:37 p.m. UTC
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(+)

Comments

Benjamin Tissoires Sept. 9, 2013, 1:02 p.m. UTC | #1
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
--
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
Benjamin Tissoires Sept. 10, 2013, 11:45 a.m. UTC | #2
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
--
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
diff mbox

Patch

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");