* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Kees Cook @ 2013-08-29 14:40 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAN+gG=F8Y2aoEMr-Zy=JLgT3VT_QGTBi60Hem299p8Sdc-icVg@mail.gmail.com>
On Thu, Aug 29, 2013 at 2:48 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> From: Kees Cook <keescook@chromium.org>
>>
>> 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..b987926 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_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
>
> I don't have access to the device anymore, but I still kept the report
> descriptors (this is the interesting part):
>
> 0xa1, 0x02, // Collection (Logical) 60
> 0x75, 0x08, // Report Size (8) 62
> 0x95, 0x07, // Report Count (7) 64
> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
> 0x09, 0x02, // Usage (Vendor Usage 2) 72
> 0x91, 0x02, // Output (Data,Var,Abs) 74
> 0xc0, // End Collection 76
>
> So with the current implementation of hid_validate_report(), it works,
> but if another Buzz controller show up at some point with extras
> fields in this output report... we will be screwed. So please, amend
> hid_validate_report(), or specifically test here that the LED output
> report is 7 bytes.
hid_validate_report() checks for "at least" 7 in this call, so it
should be fine, unless I've misunderstood something.
-Kees
>
> Cheers,
> Benjamin
>
>> + return -ENODEV;
>> +
>> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
>> if (!buzz) {
>> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>>
>> --
>> Jiri Kosina
>> SUSE Labs
>> --
>> 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
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* [PATCH] HID: roccat: Added support for KonePureOptical
From: Stefan Achatz @ 2013-08-29 14:30 UTC (permalink / raw)
To: Jiri Kosina, linux-input, linux-kernel
KonePureOptical is a KonePure with different sensor.
Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
---
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-roccat-konepure.c | 3 ++-
2 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ffe4c7a..ef18cd9 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -716,6 +716,7 @@
#define USB_DEVICE_ID_ROCCAT_KONE 0x2ced
#define USB_DEVICE_ID_ROCCAT_KONEPLUS 0x2d51
#define USB_DEVICE_ID_ROCCAT_KONEPURE 0x2dbe
+#define USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL 0x2db4
#define USB_DEVICE_ID_ROCCAT_KONEXTD 0x2e22
#define USB_DEVICE_ID_ROCCAT_KOVAPLUS 0x2d50
#define USB_DEVICE_ID_ROCCAT_LUA 0x2c2e
diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c
index c79d0b0..5850959 100644
--- a/drivers/hid/hid-roccat-konepure.c
+++ b/drivers/hid/hid-roccat-konepure.c
@@ -262,6 +262,7 @@ static int konepure_raw_event(struct hid_device *hdev,
static const struct hid_device_id konepure_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL) },
{ }
};
@@ -300,5 +301,5 @@ module_init(konepure_init);
module_exit(konepure_exit);
MODULE_AUTHOR("Stefan Achatz");
-MODULE_DESCRIPTION("USB Roccat KonePure driver");
+MODULE_DESCRIPTION("USB Roccat KonePure/Optical driver");
MODULE_LICENSE("GPL v2");
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Mika Westerberg @ 2013-08-29 10:03 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook, srinivas pandruvada
In-Reply-To: <alpine.LNX.2.00.1308282222190.22181@pobox.suse.cz>
On Wed, Aug 28, 2013 at 10:31:44PM +0200, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> A HID device could send a malicious feature report that would cause the
> sensor-hub HID driver to read past the end of heap allocation, leaking
> kernel memory contents to the caller.
>
> CVE-2013-2898
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
^ permalink raw reply
* Re: [PATCH 1/1] MAINTAINERS: Change maintainer for cyttsp driver
From: Javier Martinez Canillas @ 2013-08-29 9:48 UTC (permalink / raw)
To: Ferruh Yigit
Cc: Dmitry Torokhov, Henrik Rydberg, ttdrivers, linux-input,
linux-kernel
In-Reply-To: <51E4EEEA.4000208@cypress.com>
On Tue, Jul 16, 2013 at 8:57 AM, Ferruh Yigit <fery@cypress.com> wrote:
>
> On 07/15/2013 12:41 AM, Javier Martinez Canillas wrote:
>>
>> I haven't had time to work on this driver for a long time and
>> Ferruh has been doing a great job making it more generic,
>> adding support for new hardware and providing bug fixes.
>
> Thank you a lot for your work on cyttsp drivers,
> we would like to see your support back whenever you have time again.
> Your expertise/know-how on issue is valuable.
>
>
>>
>> So, let's make MAINTAINERS reflect reality and add him as the
>> cyttsp maintainer instead of me.
>>
>> Also, since Ferruh works for Cypress, we may change the driver
>> status from Maintained to Supported.
>>
>> Signed-off-by: Javier Martinez Canillas <javier@dowhile0.org>
>> ---
>>
>> Ferruh, please send your ack if you are willing to take over
>> maintainance of this driver.
>
>
> Acked-by: Ferruh Yigit <fery@cypress.com>
>
>
>>
>> Also, please confirm that you have been working on behalf of
>> Cypress instead of doing it on your free time. Otherwise we
>> should keep the driver status to maintained instead supported.
>
> Right, I am a Cypress employee and working on TrueTouch drivers.
>
>>
>> Thanks a lot and best regards,
>> Javier
>>
>> MAINTAINERS | 4 ++--
>> 1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 9d771d9..4ba996c 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -2458,9 +2458,9 @@ S: Maintained
>> F: drivers/media/common/cypress_firmware*
>> CYTTSP TOUCHSCREEN DRIVER
>> -M: Javier Martinez Canillas <javier@dowhile0.org>
>> +M: Ferruh Yigit <fery@cypress.com>
>> L: linux-input@vger.kernel.org
>> -S: Maintained
>> +S: Supported
>> F: drivers/input/touchscreen/cyttsp*
>> F: include/linux/input/cyttsp.h
>>
>
>
>
> --
>
> Regards,
> ferruh
>
Hi Dmitry and Henrik,
Any comments about this patch? It was sent a couple of months ago...
Thanks a lot and best regards,
Javier
^ permalink raw reply
* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Benjamin Tissoires @ 2013-08-29 9:48 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282159590.22181@pobox.suse.cz>
On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> From: Kees Cook <keescook@chromium.org>
>
> 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..b987926 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_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
I don't have access to the device anymore, but I still kept the report
descriptors (this is the interesting part):
0xa1, 0x02, // Collection (Logical) 60
0x75, 0x08, // Report Size (8) 62
0x95, 0x07, // Report Count (7) 64
0x46, 0xff, 0x00, // Physical Maximum (255) 66
0x26, 0xff, 0x00, // Logical Maximum (255) 69
0x09, 0x02, // Usage (Vendor Usage 2) 72
0x91, 0x02, // Output (Data,Var,Abs) 74
0xc0, // End Collection 76
So with the current implementation of hid_validate_report(), it works,
but if another Buzz controller show up at some point with extras
fields in this output report... we will be screwed. So please, amend
hid_validate_report(), or specifically test here that the LED output
report is 7 bytes.
Cheers,
Benjamin
> + return -ENODEV;
> +
> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
> if (!buzz) {
> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>
> --
> Jiri Kosina
> SUSE Labs
> --
> 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
* Re: [PATCH 01/14] HID: validate HID report id size
From: Benjamin Tissoires @ 2013-08-29 9:43 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308291135260.23267@pobox.suse.cz>
On Thu, Aug 29, 2013 at 11:36 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Thu, 29 Aug 2013, Benjamin Tissoires wrote:
>
>> >> The "Report ID" field of a HID report is used to build indexes of
>> >> reports. The kernel's index of these is limited to 256 entries, so any
>> >> malicious device that sets a Report ID greater than 255 will trigger
>> >> memory corruption on the host:
>> >>
>> >> [ 1347.156239] BUG: unable to handle kernel paging request at ffff88094958a878
>> >> [ 1347.156261] IP: [<ffffffff813e4da0>] hid_register_report+0x2a/0x8b
>> >>
>> >> CVE-2013-2888
>> >>
>> >> Signed-off-by: Kees Cook <keescook@chromium.org>
>> >> Cc: stable@kernel.org
>> >
>> > Applied this one to hid.git#for-3.11/CVE-2013-2888
>>
>> Well, I am a little bit lost here. I know the patch will not break any
>> existing things, but still, I really can not understand how this can
>> happen:
>>
>> - Report IDs are specifically specified to be 1-byte fields. That
>> means that the max they can have is 0xFF, which is 255 :-/
>
> Hmm, with a specially crafted device, you can end up with item->size > 1
> coming out from fetch_item() for report ID, no?
Then the problem is in the parser, and if (item->size > 1) for
HID_GLOBAL_ITEM_TAG_REPORT_ID, we should directly bail out and reject
the device instead of continuing... which is exactly Kees' patch :)
Sorry for the noise :)
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 01/14] HID: validate HID report id size
From: Jiri Kosina @ 2013-08-29 9:36 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: linux-input, Kees Cook
In-Reply-To: <CAN+gG=FuTm0Mn-gTZeOEict90SBuppjHx5Ag4MPb5h1bC+2YUQ@mail.gmail.com>
On Thu, 29 Aug 2013, Benjamin Tissoires wrote:
> >> The "Report ID" field of a HID report is used to build indexes of
> >> reports. The kernel's index of these is limited to 256 entries, so any
> >> malicious device that sets a Report ID greater than 255 will trigger
> >> memory corruption on the host:
> >>
> >> [ 1347.156239] BUG: unable to handle kernel paging request at ffff88094958a878
> >> [ 1347.156261] IP: [<ffffffff813e4da0>] hid_register_report+0x2a/0x8b
> >>
> >> CVE-2013-2888
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> Cc: stable@kernel.org
> >
> > Applied this one to hid.git#for-3.11/CVE-2013-2888
>
> Well, I am a little bit lost here. I know the patch will not break any
> existing things, but still, I really can not understand how this can
> happen:
>
> - Report IDs are specifically specified to be 1-byte fields. That
> means that the max they can have is 0xFF, which is 255 :-/
Hmm, with a specially crafted device, you can end up with item->size > 1
coming out from fetch_item() for report ID, no?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 02/14] HID: provide a helper for validating hid reports
From: Benjamin Tissoires @ 2013-08-29 9:35 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282158570.22181@pobox.suse.cz>
Hi Kees,
On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> From: Kees Cook <keescook@chromium.org>
>
> 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, its field count, and the
> value count within the fields.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
> include/linux/hid.h | 4 ++++
> 2 files changed, 54 insertions(+)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 5ea7d51..55798b2 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -759,6 +759,56 @@ 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_report - validate existing device report
> + *
> + * @device: hid device
> + * @type: which report type to examine
> + * @id: which report ID to examine (0 for first)
> + * @fields: expected number of fields
> + * @report_counts: expected number of values per field
> + *
> + * Validate the report details after parsing.
> + */
> +struct hid_report *hid_validate_report(struct hid_device *hid,
> + unsigned int type, unsigned int id,
You should consider having an u8 for id, or at least add a check
against id >= HID_MAX_IDS
> + unsigned int fields,
> + unsigned int report_counts)
> +{
> + struct hid_report *report;
> + unsigned int i;
> +
> + if (type > HID_FEATURE_REPORT) {
> + hid_err(hid, "invalid HID report %u\n", type);
> + return NULL;
> + }
> +
> + report = hid->report_enum[type].report_id_hash[id];
for code readability, and better checking, I'd prefer use
hid_get_report() here instead. Or just add an inlined accessor in
hid.h to retrieve the report from the id as many drivers are using the
report_id_hash directly.
> + if (!report) {
> + hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
> + return NULL;
> + }
> + if (report->maxfield < fields) {
> + hid_err(hid, "not enough fields in %s %u\n",
> + hid_report_names[type], id);
> + return NULL;
> + }
> + for (i = 0; i < fields; i++) {
> + if (report->field[i]->report_count < report_counts) {
This is wrong.
you can have a per field different report_count, and it is perfectly
correct. So the only validate value for report_counts would be 1.
Otherwise, if you want to provide better checking, you need to provide
an array of report_counts, which start beeing a little bit annoying
for users.
> + hid_err(hid, "not enough values in %s %u fields\n",
> + hid_report_names[type], id);
> + return NULL;
> + }
> + }
> + return report;
> +}
> +EXPORT_SYMBOL_GPL(hid_validate_report);
> +
> /**
> * hid_open_report - open a driver-specific device report
> *
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index ff545cc..76e41d8 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_report(struct hid_device *hid,
> + unsigned int type, unsigned int id,
> + unsigned int fields,
> + 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);
>
> --
> Jiri Kosina
> SUSE Labs
> --
Cheers,
Benjamin
^ permalink raw reply
* Re: [PATCH 01/14] HID: validate HID report id size
From: Benjamin Tissoires @ 2013-08-29 9:21 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308291102380.23267@pobox.suse.cz>
On Thu, Aug 29, 2013 at 11:03 AM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 28 Aug 2013, Jiri Kosina wrote:
>
>> From: Kees Cook <keescook@chromium.org>
>>
>> The "Report ID" field of a HID report is used to build indexes of
>> reports. The kernel's index of these is limited to 256 entries, so any
>> malicious device that sets a Report ID greater than 255 will trigger
>> memory corruption on the host:
>>
>> [ 1347.156239] BUG: unable to handle kernel paging request at ffff88094958a878
>> [ 1347.156261] IP: [<ffffffff813e4da0>] hid_register_report+0x2a/0x8b
>>
>> CVE-2013-2888
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@kernel.org
>
> Applied this one to hid.git#for-3.11/CVE-2013-2888
Well, I am a little bit lost here. I know the patch will not break any
existing things, but still, I really can not understand how this can
happen:
- Report IDs are specifically specified to be 1-byte fields. That
means that the max they can have is 0xFF, which is 255 :-/
- If I look at the implementation, hid_get_report() is where we fetch
the report ID from the data. But the report ID is declared as an u8...
so with a max of 255.
So the only problem might comes when specific hid drivers call
manually hid_register_report(), and in this case, we can easily
prevent this by looking at their code. No? Or we could also
specifically change the type of the argument id in
hid_register_report() to be an u8.
Basically, I'd be eager to have proper hid report descriptors and
events that can produce them, so I can add them to my HID regressions
test suite.
Cheers,
Benjamin
>
> Thanks,
>
> --
> Jiri Kosina
> SUSE Labs
> --
> 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
* Re: [PATCH 01/14] HID: validate HID report id size
From: Jiri Kosina @ 2013-08-29 9:03 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282158220.22181@pobox.suse.cz>
On Wed, 28 Aug 2013, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> The "Report ID" field of a HID report is used to build indexes of
> reports. The kernel's index of these is limited to 256 entries, so any
> malicious device that sets a Report ID greater than 255 will trigger
> memory corruption on the host:
>
> [ 1347.156239] BUG: unable to handle kernel paging request at ffff88094958a878
> [ 1347.156261] IP: [<ffffffff813e4da0>] hid_register_report+0x2a/0x8b
>
> CVE-2013-2888
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
Applied this one to hid.git#for-3.11/CVE-2013-2888
Thanks,
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Benjamin Tissoires @ 2013-08-29 8:59 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook, Henrik Rydberg
In-Reply-To: <alpine.LNX.2.00.1308282221440.22181@pobox.suse.cz>
Hi Kees,
I would be curious to have the HID report descriptors (maybe off list)
to understand how things can be that bad.
On overall, I'd prefer all those checks to be in hid-core so that we
have the guarantee that we don't have to open a new CVE each time a
specific hid driver do not check for these ranges.
More specific comments inlined:
On 28/08/13 22:31, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> When working on report indexes, always validate that they are in bounds.
> Without this, a HID device could report a malicious feature report that
> could trick the driver into a heap overflow:
>
> [ 634.885003] usb 1-1: New USB device found, idVendor=0596, idProduct=0500
> ...
> [ 676.469629] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
>
> CVE-2013-2897
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-multitouch.c | 25 ++++++++++++++++++++-----
> 1 file changed, 20 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index cb0e361..2aa275e 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -330,9 +330,18 @@ static void mt_feature_mapping(struct hid_device *hdev,
> break;
> }
> }
> + /* Ignore if value index is out of bounds. */
> + if (td->inputmode_index < 0 ||
td->inputmode_index can not be less than 0
> + td->inputmode_index >= field->report_count) {
if this is really required, we could just change the for loop above to
go from 0 to field->report_count instead.
However, I think we could just rely on usage->usage_index to get the
actual index.
I'll do more tests today and tell you later.
> + dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n");
> + td->inputmode = -1;
> + }
>
> break;
> case HID_DG_CONTACTMAX:
> + /* Ignore if value count is out of bounds. */
> + if (field->report_count < 1)
> + break;
If you can trigger this, I would say that the fix should be in hid-core,
not in every driver. A null report_count should not be allowed by the
HID protocol.
> td->maxcontact_report_id = field->report->id;
> td->maxcontacts = field->value[0];
> if (!td->maxcontacts &&
> @@ -743,15 +752,21 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
> unsigned count;
> int r, n;
>
> + if (report->maxfield == 0)
> + return;
> +
> /*
> * Includes multi-packet support where subsequent
> * packets are sent with zero contactcount.
> */
> - if (td->cc_index >= 0) {
> - struct hid_field *field = report->field[td->cc_index];
> - int value = field->value[td->cc_value_index];
> - if (value)
> - td->num_expected = value;
> + if (td->cc_index >= 0 && td->cc_index < report->maxfield) {
> + field = report->field[td->cc_index];
looks like we previously overwrote the definition of field :(
> + if (td->cc_value_index >= 0 &&
> + td->cc_value_index < field->report_count) {
> + int value = field->value[td->cc_value_index];
> + if (value)
> + td->num_expected = value;
> + }
I can not see why td->cc_index and td->cc_value_index could have bad
values. They are initially created by hid-core, and are not provided by
the device.
Anyway, if you are able to produce bad values with them, I'd rather have
those checks during the assignment of td->cc_index (in
mt_touch_input_mapping()), instead of checking them for each report.
Cheers,
Benjamin
> }
>
> for (r = 0; r < report->maxfield; r++) {
>
^ permalink raw reply
* Re: [PATCH] Input: add SYN_MAX and SYN_CNT constants
From: Peter Hutterer @ 2013-08-29 1:06 UTC (permalink / raw)
To: David Herrmann; +Cc: linux-input, Dmitry Torokhov
In-Reply-To: <1377602632-23259-1-git-send-email-dh.herrmann@gmail.com>
On Tue, Aug 27, 2013 at 01:23:52PM +0200, David Herrmann wrote:
> SYN_* events are special and not enabled via set_bit() for devices. Hence,
> they haven't been really needed, yet. However, user-space can still make
> great use of that for int->string debugging helpers or alike.
>
> Also, I haven't seen any reason not to define these, so here they are.
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> include/uapi/linux/input.h | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/include/uapi/linux/input.h b/include/uapi/linux/input.h
> index 4649ee3..2fb6fae 100644
> --- a/include/uapi/linux/input.h
> +++ b/include/uapi/linux/input.h
> @@ -194,6 +194,8 @@ struct input_keymap_entry {
> #define SYN_CONFIG 1
> #define SYN_MT_REPORT 2
> #define SYN_DROPPED 3
> +#define SYN_MAX 0xf
> +#define SYN_CNT (SYN_MAX+1)
>
> /*
> * Keys and buttons
> --
> 1.8.4
yes please!
Acked-by: Peter Hutterer <peter.hutterer@who-t.net>
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 1/3 v2] Input: wacom - Support EMR and MFT sensors of Cintiq Companion Hybrid
From: Peter Hutterer @ 2013-08-29 1:10 UTC (permalink / raw)
To: Jason Gerecke; +Cc: linuxwacom-devel, linux-input, pinglinux
In-Reply-To: <1377537210-2536-1-git-send-email-killertofu@gmail.com>
On Mon, Aug 26, 2013 at 10:13:30AM -0700, Jason Gerecke wrote:
> Adds support for the sensors integrated in to the Cintiq Companion
> Hybrid. These sensors use by-and-large the same protocol as the
> Cintiq 24HD touch.
>
> NOTE: The ExpressKeys on the Cintiq Companion Hybrid are wired to
> both the EMR controller and CPU GPIO pins. It may be necessary to
> disable their functionality in this driver if building a custom
> Android kernel for this device (lest two events be sent to userspace
> for every button press).
>
> Signed-off-by: Jason Gerecke <killertofu@gmail.com>
> ---
> Changes in v2:
> * Remove "#if 1" block surrounding conditional ExpressKey reporting
> * Change to C-style comments
[...]
> diff --git a/drivers/input/tablet/wacom_wac.h b/drivers/input/tablet/wacom_wac.h
> index dfc9e08..f47a5c1 100644
> --- a/drivers/input/tablet/wacom_wac.h
> +++ b/drivers/input/tablet/wacom_wac.h
> @@ -80,6 +80,7 @@ enum {
> WACOM_22HD,
> DTK,
> WACOM_24HD,
> + ASTERIX,
> CINTIQ,
> WACOM_BEE,
> WACOM_13HD,
> --
> 1.8.3.4
I assume ASTERIX is some internal codename - can we use the commonly used
name instead?
Cheers,
Peter
^ permalink raw reply
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Kees Cook @ 2013-08-28 21:16 UTC (permalink / raw)
To: Jiri Kosina
Cc: Srinivas Pandruvada, linux-input, Mika Westerberg,
srinivas pandruvada
In-Reply-To: <alpine.LNX.2.00.1308282241270.22181@pobox.suse.cz>
On Wed, Aug 28, 2013 at 1:42 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 28 Aug 2013, Srinivas Pandruvada wrote:
>
>> > A HID device could send a malicious feature report that would cause the
>> > sensor-hub HID driver to read past the end of heap allocation, leaking
>> > kernel memory contents to the caller.
>> >
>> > CVE-2013-2898
>> >
>> > Signed-off-by: Kees Cook <keescook@chromium.org>
>> > Cc: stable@kernel.org
>> > ---
>> > drivers/hid/hid-sensor-hub.c | 3 ++-
>> > 1 file changed, 2 insertions(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
>> > index ca749810..aa34755 100644
>> > --- a/drivers/hid/hid-sensor-hub.c
>> > +++ b/drivers/hid/hid-sensor-hub.c
>> > @@ -221,7 +221,8 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device
>> > *hsdev, u32 report_id,
>> > mutex_lock(&data->mutex);
>> > report = sensor_hub_report(report_id, hsdev->hdev,
>> > HID_FEATURE_REPORT);
>> > - if (!report || (field_index >= report->maxfield)) {
>> > + if (!report || (field_index >= report->maxfield) ||
>> > + report->field[field_index]->report_count < 1) {
>> Is it based on some HID device is sending junk report or just from a code
>> review?
>
> My understanding is that this whole Kees' patchset is about potentially
> evil devices doing bad things (on purpose).
Correct, though this particular flaw is pretty weak. It requires both
malicious device and malicious user-space. However, with the advent of
things like HTML5 USB API, it's possible these could be combined to
attack a device.
Regardless, this fix seems obviously correct and trivial to me.
-Kees
>
>> > ret = -EINVAL;
>> > goto done_proc;
>> > }
>> Thanks,
>> Srinivas
>>
>
> --
> Jiri Kosina
> SUSE Labs
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 14/14] HID: check for NULL field when setting values
From: Kees Cook @ 2013-08-28 21:14 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input
In-Reply-To: <alpine.LNX.2.00.1308282232080.22181@pobox.suse.cz>
On Wed, Aug 28, 2013 at 1:32 PM, Jiri Kosina <jkosina@suse.cz> wrote:
> On Wed, 28 Aug 2013, Jiri Kosina wrote:
>
>> From: Kees Cook <keescook@chromium.org>
>>
>> Defensively check that the field to be worked on is not NULL.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@kernel.org
>> ---
>> drivers/hid/hid-core.c | 7 ++++++-
>> 1 file changed, 6 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 55798b2..192be6b 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report);
>>
>> int hid_set_field(struct hid_field *field, unsigned offset, __s32 value)
>> {
>> - unsigned size = field->report_size;
>> + unsigned size;
>> +
>> + if (!field)
>> + return -1;
>> +
>> + size = field->report_size;
>
> Kees,
>
> do you actually see any way how field could ever be null in
> hid_set_field()?
Yes, report->field[0] may be uninitialized (NULL due to kzalloc) if
hid_add_field() bails out during the padding field check:
if (!parser->local.usage_index) /* Ignore padding fields */
return 0;
The report will be registered (earlier call to hid_register_report()
was successful), but the entire field list will be NULL.
The hid-ntrig.c fix is a fix for such a problem
(report->field[0]->value[0]) where field[0] can be NULL. I was able to
make a reproduction case for this, with the crash can be seen in the
hid-ntrig.c patch.
-Kees
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 13/14] HID: picolcd_core: validate output report details
From: Bruno Prémont @ 2013-08-28 21:00 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282222460.22181@pobox.suse.cz>
On Wed, 28 August 2013 Jiri Kosina <jkosina@suse.cz> wrote:
> From: Kees Cook <keescook@chromium.org>
>
> A HID device could send a malicious output report that would cause the
> picolcd HID driver to trigger a NULL dereference during attr file writing.
>
> CVE-2013-2899
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-picolcd_core.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
> index b48092d..72bba1e 100644
> --- a/drivers/hid/hid-picolcd_core.c
> +++ b/drivers/hid/hid-picolcd_core.c
> @@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
> buf += 10;
> cnt -= 10;
> }
> - if (!report)
> + if (!report || report->maxfield < 1)
> return -EINVAL;
>
> while (cnt > 0 && (buf[cnt-1] == '\n' || buf[cnt-1] == '\r'))
I will check tomorrow or Friday evening what the documentation I have
says for this report and test, might be a report->maxfield != 1 would
be even better suited.
Too late today for looking into it.
Bruno
^ permalink raw reply
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Jiri Kosina @ 2013-08-28 20:42 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: linux-input, Kees Cook, Mika Westerberg, srinivas pandruvada
In-Reply-To: <521E60CE.6050000@linux.intel.com>
On Wed, 28 Aug 2013, Srinivas Pandruvada wrote:
> > A HID device could send a malicious feature report that would cause the
> > sensor-hub HID driver to read past the end of heap allocation, leaking
> > kernel memory contents to the caller.
> >
> > CVE-2013-2898
> >
> > Signed-off-by: Kees Cook <keescook@chromium.org>
> > Cc: stable@kernel.org
> > ---
> > drivers/hid/hid-sensor-hub.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index ca749810..aa34755 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -221,7 +221,8 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device
> > *hsdev, u32 report_id,
> > mutex_lock(&data->mutex);
> > report = sensor_hub_report(report_id, hsdev->hdev,
> > HID_FEATURE_REPORT);
> > - if (!report || (field_index >= report->maxfield)) {
> > + if (!report || (field_index >= report->maxfield) ||
> > + report->field[field_index]->report_count < 1) {
> Is it based on some HID device is sending junk report or just from a code
> review?
My understanding is that this whole Kees' patchset is about potentially
evil devices doing bad things (on purpose).
> > ret = -EINVAL;
> > goto done_proc;
> > }
> Thanks,
> Srinivas
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Srinivas Pandruvada @ 2013-08-28 20:42 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook, Mika Westerberg, srinivas pandruvada
In-Reply-To: <alpine.LNX.2.00.1308282222190.22181@pobox.suse.cz>
On 08/28/2013 01:31 PM, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> A HID device could send a malicious feature report that would cause the
> sensor-hub HID driver to read past the end of heap allocation, leaking
> kernel memory contents to the caller.
>
> CVE-2013-2898
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-sensor-hub.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> index ca749810..aa34755 100644
> --- a/drivers/hid/hid-sensor-hub.c
> +++ b/drivers/hid/hid-sensor-hub.c
> @@ -221,7 +221,8 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
>
> mutex_lock(&data->mutex);
> report = sensor_hub_report(report_id, hsdev->hdev, HID_FEATURE_REPORT);
> - if (!report || (field_index >= report->maxfield)) {
> + if (!report || (field_index >= report->maxfield) ||
> + report->field[field_index]->report_count < 1) {
Is it based on some HID device is sending junk report or just from a
code review?
> ret = -EINVAL;
> goto done_proc;
> }
Thanks,
Srinivas
^ permalink raw reply
* Re: [PATCH 14/14] HID: check for NULL field when setting values
From: Jiri Kosina @ 2013-08-28 20:32 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282223090.22181@pobox.suse.cz>
On Wed, 28 Aug 2013, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> Defensively check that the field to be worked on is not NULL.
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-core.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index 55798b2..192be6b 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report);
>
> int hid_set_field(struct hid_field *field, unsigned offset, __s32 value)
> {
> - unsigned size = field->report_size;
> + unsigned size;
> +
> + if (!field)
> + return -1;
> +
> + size = field->report_size;
Kees,
do you actually see any way how field could ever be null in
hid_set_field()?
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH 14/14] HID: check for NULL field when setting values
From: Jiri Kosina @ 2013-08-28 20:32 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook
From: Kees Cook <keescook@chromium.org>
Defensively check that the field to be worked on is not NULL.
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-core.c | 7 ++++++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 55798b2..192be6b 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1206,7 +1206,12 @@ EXPORT_SYMBOL_GPL(hid_output_report);
int hid_set_field(struct hid_field *field, unsigned offset, __s32 value)
{
- unsigned size = field->report_size;
+ unsigned size;
+
+ if (!field)
+ return -1;
+
+ size = field->report_size;
hid_dump_input(field->report->device, field->usage + offset, value);
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 13/14] HID: picolcd_core: validate output report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Bruno Prémont
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious output report that would cause the
picolcd HID driver to trigger a NULL dereference during attr file writing.
CVE-2013-2899
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-picolcd_core.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index b48092d..72bba1e 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -290,7 +290,7 @@ static ssize_t picolcd_operation_mode_store(struct device *dev,
buf += 10;
cnt -= 10;
}
- if (!report)
+ if (!report || report->maxfield < 1)
return -EINVAL;
while (cnt > 0 && (buf[cnt-1] == '\n' || buf[cnt-1] == '\r'))
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Mika Westerberg, srinivas pandruvada
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious feature report that would cause the
sensor-hub HID driver to read past the end of heap allocation, leaking
kernel memory contents to the caller.
CVE-2013-2898
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-sensor-hub.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index ca749810..aa34755 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -221,7 +221,8 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
mutex_lock(&data->mutex);
report = sensor_hub_report(report_id, hsdev->hdev, HID_FEATURE_REPORT);
- if (!report || (field_index >= report->maxfield)) {
+ if (!report || (field_index >= report->maxfield) ||
+ report->field[field_index]->report_count < 1) {
ret = -EINVAL;
goto done_proc;
}
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 11/14] HID: multitouch: validate feature report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Henrik Rydberg, Benjamin Tissoires
From: Kees Cook <keescook@chromium.org>
When working on report indexes, always validate that they are in bounds.
Without this, a HID device could report a malicious feature report that
could trick the driver into a heap overflow:
[ 634.885003] usb 1-1: New USB device found, idVendor=0596, idProduct=0500
...
[ 676.469629] BUG kmalloc-192 (Tainted: G W ): Redzone overwritten
CVE-2013-2897
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-multitouch.c | 25 ++++++++++++++++++++-----
1 file changed, 20 insertions(+), 5 deletions(-)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index cb0e361..2aa275e 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -330,9 +330,18 @@ static void mt_feature_mapping(struct hid_device *hdev,
break;
}
}
+ /* Ignore if value index is out of bounds. */
+ if (td->inputmode_index < 0 ||
+ td->inputmode_index >= field->report_count) {
+ dev_err(&hdev->dev, "HID_DG_INPUTMODE out of range\n");
+ td->inputmode = -1;
+ }
break;
case HID_DG_CONTACTMAX:
+ /* Ignore if value count is out of bounds. */
+ if (field->report_count < 1)
+ break;
td->maxcontact_report_id = field->report->id;
td->maxcontacts = field->value[0];
if (!td->maxcontacts &&
@@ -743,15 +752,21 @@ static void mt_touch_report(struct hid_device *hid, struct hid_report *report)
unsigned count;
int r, n;
+ if (report->maxfield == 0)
+ return;
+
/*
* Includes multi-packet support where subsequent
* packets are sent with zero contactcount.
*/
- if (td->cc_index >= 0) {
- struct hid_field *field = report->field[td->cc_index];
- int value = field->value[td->cc_value_index];
- if (value)
- td->num_expected = value;
+ if (td->cc_index >= 0 && td->cc_index < report->maxfield) {
+ field = report->field[td->cc_index];
+ if (td->cc_value_index >= 0 &&
+ td->cc_value_index < field->report_count) {
+ int value = field->value[td->cc_value_index];
+ if (value)
+ td->num_expected = value;
+ }
}
for (r = 0; r < report->maxfield; r++) {
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 10/14] HID: ntrig: validate feature report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Rafi Rubin
From: Kees Cook <keescook@chromium.org>
A HID device could send a malicious feature report that would cause the
ntrig HID driver to trigger a NULL dereference during initialization:
[57383.031190] usb 3-1: New USB device found, idVendor=1b96, idProduct=0001
...
[57383.315193] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
[57383.315308] IP: [<ffffffffa08102de>] ntrig_probe+0x25e/0x420 [hid_ntrig]
CVE-2013-2896
Signed-off-by: Kees Cook <keescook@chromium.org>
Cc: stable@kernel.org
---
drivers/hid/hid-ntrig.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
index ef95102..5482156 100644
--- a/drivers/hid/hid-ntrig.c
+++ b/drivers/hid/hid-ntrig.c
@@ -115,7 +115,8 @@ static inline int ntrig_get_mode(struct hid_device *hdev)
struct hid_report *report = hdev->report_enum[HID_FEATURE_REPORT].
report_id_hash[0x0d];
- if (!report)
+ if (!report || report->maxfield < 1 ||
+ report->field[0]->report_count < 1)
return -EINVAL;
hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
* [PATCH 09/14] HID: logitech-dj: validate output report details
From: Jiri Kosina @ 2013-08-28 20:31 UTC (permalink / raw)
To: linux-input; +Cc: Kees Cook, Nestor Lopez Casado
From: Kees Cook <keescook@chromium.org>
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..7b99c2a 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_report(hdev, HID_OUTPUT_REPORT, REPORT_ID_DJ_SHORT,
+ 1, 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);
--
Jiri Kosina
SUSE Labs
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox