| Message ID | alpine.LNX.2.00.1308282158220.22181@pobox.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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,
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 -- 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
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?
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 -- 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
So I have now queued subset of the patches that don't deal with hid_validate_report(); waiting for v2 based on Benjamin's comments with the rest. The patches are public for distributions to apply them if they feel urgent need, but for upstream I'd rather do things properly, and Benjamin had quite some valuable feedback. Thanks again,
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c index 36668d1..5ea7d51 100644 --- a/drivers/hid/hid-core.c +++ b/drivers/hid/hid-core.c @@ -63,6 +63,8 @@ struct hid_report *hid_register_report(struct hid_device *device, unsigned type, struct hid_report_enum *report_enum = device->report_enum + type; struct hid_report *report; + if (id >= HID_MAX_IDS) + return NULL; if (report_enum->report_id_hash[id]) return report_enum->report_id_hash[id]; @@ -404,8 +406,10 @@ static int hid_parser_global(struct hid_parser *parser, struct hid_item *item) case HID_GLOBAL_ITEM_TAG_REPORT_ID: parser->global.report_id = item_udata(item); - if (parser->global.report_id == 0) { - hid_err(parser->device, "report_id 0 is invalid\n"); + if (parser->global.report_id == 0 || + parser->global.report_id >= HID_MAX_IDS) { + hid_err(parser->device, "report_id %u is invalid\n", + parser->global.report_id); return -1; } return 0; @@ -575,7 +579,7 @@ static void hid_close_report(struct hid_device *device) for (i = 0; i < HID_REPORT_TYPES; i++) { struct hid_report_enum *report_enum = device->report_enum + i; - for (j = 0; j < 256; j++) { + for (j = 0; j < HID_MAX_IDS; j++) { struct hid_report *report = report_enum->report_id_hash[j]; if (report) hid_free_report(report); diff --git a/include/linux/hid.h b/include/linux/hid.h index 0c48991..ff545cc 100644 --- a/include/linux/hid.h +++ b/include/linux/hid.h @@ -393,10 +393,12 @@ struct hid_report { struct hid_device *device; /* associated device */ }; +#define HID_MAX_IDS 256 + struct hid_report_enum { unsigned numbered; struct list_head report_list; - struct hid_report *report_id_hash[256]; + struct hid_report *report_id_hash[HID_MAX_IDS]; }; #define HID_REPORT_TYPES 3