* [PATCH 04/14] HID: sony: validate HID output report details @ 2013-08-28 20:30 Jiri Kosina 2013-08-29 9:48 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Jiri Kosina @ 2013-08-28 20:30 UTC (permalink / raw) To: linux-input; +Cc: Kees Cook 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)) + return -ENODEV; + buzz = kzalloc(sizeof(*buzz), GFP_KERNEL); if (!buzz) { hid_err(hdev, "Insufficient memory, cannot allocate driver data\n"); -- Jiri Kosina SUSE Labs ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 04/14] HID: sony: validate HID output report details 2013-08-28 20:30 [PATCH 04/14] HID: sony: validate HID output report details Jiri Kosina @ 2013-08-29 9:48 ` Benjamin Tissoires 2013-08-29 14:40 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Tissoires @ 2013-08-29 9:48 UTC (permalink / raw) To: Jiri Kosina; +Cc: linux-input, Kees Cook 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 [flat|nested] 6+ messages in thread
* Re: [PATCH 04/14] HID: sony: validate HID output report details 2013-08-29 9:48 ` Benjamin Tissoires @ 2013-08-29 14:40 ` Kees Cook 2013-08-29 14:50 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2013-08-29 14:40 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input 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 [flat|nested] 6+ messages in thread
* Re: [PATCH 04/14] HID: sony: validate HID output report details 2013-08-29 14:40 ` Kees Cook @ 2013-08-29 14:50 ` Benjamin Tissoires 2013-08-29 19:58 ` Kees Cook 0 siblings, 1 reply; 6+ messages in thread From: Benjamin Tissoires @ 2013-08-29 14:50 UTC (permalink / raw) To: Kees Cook; +Cc: Jiri Kosina, linux-input On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote: > 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. > Sure, it' s fine with the current implementation of hid_validate_report(). However, as I mentioned in patch 2/14, I am complaining about the implementation of hid_validate_report(). In this case, if a new Buzz controller pops out with an extra usage (Vendor 3 for instance), mapped to another LED, and that the report count is for this usage < 7, the test invalidate the report. for instance, let's imagine they pop up a new version: 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 0x75, 0x08, // Report Size (8) 62 0x95, 0x04, // **Report Count (4)** 64 0x46, 0xff, 0x00, // Physical Maximum (255) 66 0x26, 0xff, 0x00, // Logical Maximum (255) 69 0x09, 0x03, // Usage (Vendor Usage 3) 72 0x91, 0x02, // Output (Data,Var,Abs) 74 0xc0, // End Collection 76 Ok, it's a lot of "if", but still this output report is valid, and the test will fail. And if we call hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap overflow will appear again. Does it makes more sense? 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 [flat|nested] 6+ messages in thread
* Re: [PATCH 04/14] HID: sony: validate HID output report details 2013-08-29 14:50 ` Benjamin Tissoires @ 2013-08-29 19:58 ` Kees Cook 2013-08-30 13:39 ` Benjamin Tissoires 0 siblings, 1 reply; 6+ messages in thread From: Kees Cook @ 2013-08-29 19:58 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input On Thu, Aug 29, 2013 at 7:50 AM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote: >> 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. >> > > Sure, it' s fine with the current implementation of > hid_validate_report(). However, as I mentioned in patch > 2/14, I am complaining about the implementation of hid_validate_report(). > > In this case, if a new Buzz controller pops out with an extra usage > (Vendor 3 for instance), mapped to another LED, and that the report > count is for this usage < 7, the test invalidate the report. > > for instance, let's imagine they pop up a new version: > > 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 > 0x75, 0x08, // Report Size (8) 62 > 0x95, 0x04, // **Report Count (4)** 64 > 0x46, 0xff, 0x00, // Physical Maximum (255) 66 > 0x26, 0xff, 0x00, // Logical Maximum (255) 69 > 0x09, 0x03, // Usage (Vendor Usage 3) 72 > 0x91, 0x02, // Output (Data,Var,Abs) 74 > 0xc0, // End Collection 76 > > Ok, it's a lot of "if", but still this output report is valid, and the > test will fail. And if we call hid_validate_report(hdev, > HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap > overflow will appear again. > > Does it makes more sense? Right, yeah, I understand what you meant here, but I guess my point was, if there's something that uses <7, then the driver needs adjustment too, beyond just the hid_validate_report() call, since it would need to know to operate only on 4 instead of 7. My thinking was, if such a thing is detected, it would need to identify which device it was and fix both the bounds-checking, and the report-value-setting. For example: if (i_am_vendor_3()) { hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 4); } else { hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7); } ... value[0] = 0x00; value[1] = (leds & 1) ? 0xff : 0x00; value[2] = (leds & 2) ? 0xff : 0x00; value[3] = (leds & 4) ? 0xff : 0x00; if (!i_am_vendor_3()) { value[4] = (leds & 8) ? 0xff : 0x00; value[5] = 0x00; value[6] = 0x00; } But actually, the logic would be id or usage based, but still, it seems to me that the hid_validate_report() call must match the actual value array assignments. -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 -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 04/14] HID: sony: validate HID output report details 2013-08-29 19:58 ` Kees Cook @ 2013-08-30 13:39 ` Benjamin Tissoires 0 siblings, 0 replies; 6+ messages in thread From: Benjamin Tissoires @ 2013-08-30 13:39 UTC (permalink / raw) To: Kees Cook; +Cc: Jiri Kosina, linux-input On Thu, Aug 29, 2013 at 9:58 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Aug 29, 2013 at 7:50 AM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote: >>> 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. >>> >> >> Sure, it' s fine with the current implementation of >> hid_validate_report(). However, as I mentioned in patch >> 2/14, I am complaining about the implementation of hid_validate_report(). >> >> In this case, if a new Buzz controller pops out with an extra usage >> (Vendor 3 for instance), mapped to another LED, and that the report >> count is for this usage < 7, the test invalidate the report. >> >> for instance, let's imagine they pop up a new version: >> >> 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 >> 0x75, 0x08, // Report Size (8) 62 >> 0x95, 0x04, // **Report Count (4)** 64 >> 0x46, 0xff, 0x00, // Physical Maximum (255) 66 >> 0x26, 0xff, 0x00, // Logical Maximum (255) 69 >> 0x09, 0x03, // Usage (Vendor Usage 3) 72 >> 0x91, 0x02, // Output (Data,Var,Abs) 74 >> 0xc0, // End Collection 76 >> >> Ok, it's a lot of "if", but still this output report is valid, and the >> test will fail. And if we call hid_validate_report(hdev, >> HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap >> overflow will appear again. >> >> Does it makes more sense? > > Right, yeah, I understand what you meant here, but I guess my point > was, if there's something that uses <7, then the driver needs > adjustment too, beyond just the hid_validate_report() call, since it > would need to know to operate only on 4 instead of 7. My thinking was, > if such a thing is detected, it would need to identify which device it > was and fix both the bounds-checking, and the report-value-setting. > For example: > > if (i_am_vendor_3()) { > hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 4); > } else { > hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7); > } > hmm... not really. In my case, I supposed the device presented both vendor collections, one after the other. So the test could be: hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7); if (I_contain_vendor_3()) hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 2, 4); But this will not work if the small report is in front of the large one. I can propose another implementation of hid_validate_report() which will be covering this case: instead of checking a range of fields, just check the specific field index used later: +struct hid_report *hid_validate_report(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 %u\n", type); + return NULL; + } + + 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 #%d\n", + hid_report_names[type], id, field_index); + return NULL; + } + return report; +} +EXPORT_SYMBOL_GPL(hid_validate_report); Only hid-zpff and hid-lenovo-tpkbd are checking for more than one report, and we can afford a for loop on the field indexes for them. Cheers, Benjamin > ... > > value[0] = 0x00; > value[1] = (leds & 1) ? 0xff : 0x00; > value[2] = (leds & 2) ? 0xff : 0x00; > value[3] = (leds & 4) ? 0xff : 0x00; > if (!i_am_vendor_3()) { > value[4] = (leds & 8) ? 0xff : 0x00; > value[5] = 0x00; > value[6] = 0x00; > } > > But actually, the logic would be id or usage based, but still, it > seems to me that the hid_validate_report() call must match the actual > value array assignments. > > -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 > > > > -- > Kees Cook > Chrome OS Security ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-08-30 13:39 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-28 20:30 [PATCH 04/14] HID: sony: validate HID output report details Jiri Kosina 2013-08-29 9:48 ` Benjamin Tissoires 2013-08-29 14:40 ` Kees Cook 2013-08-29 14:50 ` Benjamin Tissoires 2013-08-29 19:58 ` Kees Cook 2013-08-30 13:39 ` Benjamin Tissoires
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).