| Message ID | alpine.LNX.2.00.1308282221440.22181@pobox.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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++) { > -- 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, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > Hi Kees, > > I would be curious to have the HID report descriptors (maybe off list) > to understand how things can be that bad. Certainly! I'll send them your way. I did have to get pretty creative to tickle these conditions. > 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. I pondered doing this, but it seemed like something that needed wider discussion, so I thought I'd start with just the dump of fixes. It seems like the entire HID report interface should use access functions to enforce range checking -- perhaps further enforced by making the structure opaque to the drivers. > 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 Well, it certainly _shouldn't_ be less than zero. :) However, it is defined as s8, and gets set from an int: for (i=0; i < field->maxusage; i++) { if (field->usage[i].hid == usage->hid) { td->inputmode_index = i; break; } } Both "i" and "maxusage" are int, and I can generate a large maxusage and usage array where the first matching hid equality happens when i is >127, causing inputmode_index to wrap. >> + 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. That's certainly true, but since usage count and report count are not directly associated, I don't know if there are devices that will freak out with this restriction. > 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. Again, I have no problem with that idea, but there seem to be lots of general cases where this may not be possible (i.e. a HID with 0 OUTPUT or FEATURE reports). I didn't see a sensible way to approach this in core without declaring a "I require $foo many OUTPUT reports, $bar many INPUT, and $baz many FEATURE" for each driver. I instead opted for the report validation function instead. >> 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. Well, the problem comes again from there not being a hard link between the usage array and the value array: td->cc_value_index = usage->usage_index; ... int value = field->value[td->cc_value_index]; And all of this would be irrelevant if there was an access function for field->value[]. Thanks for the review! -Kees > > Cheers, > Benjamin > >> } >> >> for (r = 0; r < report->maxfield; r++) { >> >
On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote: > On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires > <benjamin.tissoires@redhat.com> wrote: >> Hi Kees, >> >> I would be curious to have the HID report descriptors (maybe off list) >> to understand how things can be that bad. > > Certainly! I'll send them your way. I did have to get pretty creative > to tickle these conditions. > >> 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. > > I pondered doing this, but it seemed like something that needed wider > discussion, so I thought I'd start with just the dump of fixes. It > seems like the entire HID report interface should use access functions > to enforce range checking -- perhaps further enforced by making the > structure opaque to the drivers. The problem with access functions with range checking is when they are used at each input report. This will overload the kernel processing for static checks. > >> 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 > > Well, it certainly _shouldn't_ be less than zero. :) However, it is > defined as s8, and gets set from an int: > > for (i=0; i < field->maxusage; i++) { > if (field->usage[i].hid == usage->hid) { > td->inputmode_index = i; > break; > } > } > > Both "i" and "maxusage" are int, and I can generate a large maxusage > and usage array where the first matching hid equality happens when i > is >127, causing inputmode_index to wrap. ouch. Sorry. This piece of code (the current code) is junk. We should switch to usage->usage_index and change from __s8 to __s16 the declaration ASAP. > >>> + 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. > > That's certainly true, but since usage count and report count are not > directly associated, I don't know if there are devices that will freak > out with this restriction. Actually, I just again another long time understanding all the mess of having usage count and report count different in hid-core. I think it is a bug at some point (but it looks like I tend to be wrong on a lot of things recently :-P ), because when you look at the actual code of hid_input_field() in hid-core.c, we never go further than field->report_count when dealing with input report. So my guess is that we are declaring extra usages that are never used to parse incoming data. > >> 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. > > Again, I have no problem with that idea, but there seem to be lots of > general cases where this may not be possible (i.e. a HID with 0 OUTPUT > or FEATURE reports). I didn't see a sensible way to approach this in > core without declaring a "I require $foo many OUTPUT reports, $bar > many INPUT, and $baz many FEATURE" for each driver. I instead opted > for the report validation function instead. > I think we can just add this check in both hidinput_configure_usage() and report_features() (both in hid-input.c) so that we don't call the *_mapping() callbacks with non sense fields. This way, the specific hid drivers will know only the correct fields, and don't need to check this. So patches 9, 11 and 13 can be amended. It looks to me that you mismatched field->report_count and the actual report_count in your answer: field->report_count is the number of consecutive fields of field->report_size that are present for the parsing of the report. It has nothing to do with the total report count. >>> 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. > > Well, the problem comes again from there not being a hard link between > the usage array and the value array: > > td->cc_value_index = usage->usage_index; > ... > int value = field->value[td->cc_value_index]; > > And all of this would be irrelevant if there was an access function > for field->value[]. True, with the current implementation, td->cc_value_index can be greater than field->report_count. Still, moving this check in mt_touch_input_mapping() will allow us to make it only once. > > Thanks for the review! you are welcome :) Cheers, Benjamin >>> } >>> >>> for (r = 0; r < report->maxfield; r++) { >>> >> > > > > -- > Kees Cook > Chrome OS Security > -- > 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 Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires <benjamin.tissoires@gmail.com> wrote: > On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote: >> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires >> <benjamin.tissoires@redhat.com> wrote: >>> Hi Kees, >>> >>> I would be curious to have the HID report descriptors (maybe off list) >>> to understand how things can be that bad. >> >> Certainly! I'll send them your way. I did have to get pretty creative >> to tickle these conditions. >> >>> 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. >> >> I pondered doing this, but it seemed like something that needed wider >> discussion, so I thought I'd start with just the dump of fixes. It >> seems like the entire HID report interface should use access functions >> to enforce range checking -- perhaps further enforced by making the >> structure opaque to the drivers. > > The problem with access functions with range checking is when they are > used at each input report. This will overload the kernel processing > for static checks. > >> >>> 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 >> >> Well, it certainly _shouldn't_ be less than zero. :) However, it is >> defined as s8, and gets set from an int: >> >> for (i=0; i < field->maxusage; i++) { >> if (field->usage[i].hid == usage->hid) { >> td->inputmode_index = i; >> break; >> } >> } >> >> Both "i" and "maxusage" are int, and I can generate a large maxusage >> and usage array where the first matching hid equality happens when i >> is >127, causing inputmode_index to wrap. > > ouch. Sorry. This piece of code (the current code) is junk. We should > switch to usage->usage_index and change from __s8 to __s16 the > declaration ASAP. > >> >>>> + 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. >> >> That's certainly true, but since usage count and report count are not >> directly associated, I don't know if there are devices that will freak >> out with this restriction. > > Actually, I just again another long time understanding all the mess of > having usage count and report count different in hid-core. > > I think it is a bug at some point (but it looks like I tend to be > wrong on a lot of things recently :-P ), because when you look at the > actual code of hid_input_field() in hid-core.c, we never go further > than field->report_count when dealing with input report. > So my guess is that we are declaring extra usages that are never used > to parse incoming data. > >> >>> 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. >> >> Again, I have no problem with that idea, but there seem to be lots of >> general cases where this may not be possible (i.e. a HID with 0 OUTPUT >> or FEATURE reports). I didn't see a sensible way to approach this in >> core without declaring a "I require $foo many OUTPUT reports, $bar >> many INPUT, and $baz many FEATURE" for each driver. I instead opted >> for the report validation function instead. >> > > I think we can just add this check in both hidinput_configure_usage() > and report_features() (both in hid-input.c) so that we don't call the > *_mapping() callbacks with non sense fields. > This way, the specific hid drivers will know only the correct fields, > and don't need to check this. So patches 9, 11 and 13 can be amended. > > It looks to me that you mismatched field->report_count and the actual > report_count in your answer: field->report_count is the number of > consecutive fields of field->report_size that are present for the > parsing of the report. It has nothing to do with the total report > count. > >>>> 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. >> >> Well, the problem comes again from there not being a hard link between >> the usage array and the value array: >> >> td->cc_value_index = usage->usage_index; >> ... >> int value = field->value[td->cc_value_index]; >> >> And all of this would be irrelevant if there was an access function >> for field->value[]. > > True, with the current implementation, td->cc_value_index can be > greater than field->report_count. Still, moving this check in > mt_touch_input_mapping() will allow us to make it only once. > >> >> Thanks for the review! > > you are welcome :) Okay, so, where does the whole patch series stand? It seems like the checks are all worth adding; and my fixes are, I think, "minimal" so having them go into the tree (so that they land in stable) seems like a good idea. The work beyond that could be done on top of the existing patches? There seems to be a lot of redesign ideas, but those probably aren't so great for stable, and I'm probably not the best person to write them, since everything I know about HID code I learned two weeks ago. :) Given the 12 flaws, what do you see as the best way forward? -Kees
On Fri, Aug 30, 2013 at 8:27 PM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote: >>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires >>> <benjamin.tissoires@redhat.com> wrote: >>>> Hi Kees, >>>> >>>> I would be curious to have the HID report descriptors (maybe off list) >>>> to understand how things can be that bad. >>> >>> Certainly! I'll send them your way. I did have to get pretty creative >>> to tickle these conditions. >>> >>>> 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. >>> >>> I pondered doing this, but it seemed like something that needed wider >>> discussion, so I thought I'd start with just the dump of fixes. It >>> seems like the entire HID report interface should use access functions >>> to enforce range checking -- perhaps further enforced by making the >>> structure opaque to the drivers. >> >> The problem with access functions with range checking is when they are >> used at each input report. This will overload the kernel processing >> for static checks. >> >>> >>>> 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 >>> >>> Well, it certainly _shouldn't_ be less than zero. :) However, it is >>> defined as s8, and gets set from an int: >>> >>> for (i=0; i < field->maxusage; i++) { >>> if (field->usage[i].hid == usage->hid) { >>> td->inputmode_index = i; >>> break; >>> } >>> } >>> >>> Both "i" and "maxusage" are int, and I can generate a large maxusage >>> and usage array where the first matching hid equality happens when i >>> is >127, causing inputmode_index to wrap. >> >> ouch. Sorry. This piece of code (the current code) is junk. We should >> switch to usage->usage_index and change from __s8 to __s16 the >> declaration ASAP. >> >>> >>>>> + 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. >>> >>> That's certainly true, but since usage count and report count are not >>> directly associated, I don't know if there are devices that will freak >>> out with this restriction. >> >> Actually, I just again another long time understanding all the mess of >> having usage count and report count different in hid-core. >> >> I think it is a bug at some point (but it looks like I tend to be >> wrong on a lot of things recently :-P ), because when you look at the >> actual code of hid_input_field() in hid-core.c, we never go further >> than field->report_count when dealing with input report. >> So my guess is that we are declaring extra usages that are never used >> to parse incoming data. >> >>> >>>> 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. >>> >>> Again, I have no problem with that idea, but there seem to be lots of >>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT >>> or FEATURE reports). I didn't see a sensible way to approach this in >>> core without declaring a "I require $foo many OUTPUT reports, $bar >>> many INPUT, and $baz many FEATURE" for each driver. I instead opted >>> for the report validation function instead. >>> >> >> I think we can just add this check in both hidinput_configure_usage() >> and report_features() (both in hid-input.c) so that we don't call the >> *_mapping() callbacks with non sense fields. >> This way, the specific hid drivers will know only the correct fields, >> and don't need to check this. So patches 9, 11 and 13 can be amended. >> >> It looks to me that you mismatched field->report_count and the actual >> report_count in your answer: field->report_count is the number of >> consecutive fields of field->report_size that are present for the >> parsing of the report. It has nothing to do with the total report >> count. >> >>>>> 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. >>> >>> Well, the problem comes again from there not being a hard link between >>> the usage array and the value array: >>> >>> td->cc_value_index = usage->usage_index; >>> ... >>> int value = field->value[td->cc_value_index]; >>> >>> And all of this would be irrelevant if there was an access function >>> for field->value[]. >> >> True, with the current implementation, td->cc_value_index can be >> greater than field->report_count. Still, moving this check in >> mt_touch_input_mapping() will allow us to make it only once. >> >>> >>> Thanks for the review! >> >> you are welcome :) > > Okay, so, where does the whole patch series stand? It seems like the > checks are all worth adding; and my fixes are, I think, "minimal" so > having them go into the tree (so that they land in stable) seems like Hi Kees, well, sorry for that, but I don't want to see this specific patch in stable (11/14). The blocking problem I see with this one is that it is doing the checks in mt_touch_report() at each incoming report, which will overload the processing without a real need as this check can be done while setting the device up. And having it in stable means that we will have to fix them right after, so I'm not particularly kind of it. For Fedora, for instance, we already have included the whole patch series to fix the CVE, and I'm sure other distributions will do the same. So I would say that the CVE is not particularly a problem right now for main users. For upstream, I would say it is a different matter. Anyway, the final decision will be taken by Jiri, I am just expressing my point of view. > a good idea. The work beyond that could be done on top of the existing > patches? There seems to be a lot of redesign ideas, but those probably > aren't so great for stable, and I'm probably not the best person to > write them, since everything I know about HID code I learned two weeks > ago. :) There are not so many changes I asked, but it's clear that I would prefer doing some regressions tests before having them in stable. The problem I have now is that I am taking this week 4 days off with little to no internet connection. So I will not be able to do the proper testing before Friday. > > Given the 12 flaws, what do you see as the best way forward? If Jiri thinks we can wait one more week for some of these patches (I know that the most critical one, 1/14 is already on its way to Linus), then I can handle the v2, but not before Friday. Again, I think the current patch series is perfectly fine for distributions as they can add/remove things more easily than we can do in master. Cheers, Bnejamin -- 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 Mon, 2 Sep 2013, Benjamin Tissoires wrote: > > Given the 12 flaws, what do you see as the best way forward? > > If Jiri thinks we can wait one more week for some of these patches (I > know that the most critical one, 1/14 is already on its way to Linus), > then I can handle the v2, but not before Friday. Again, I think the > current patch series is perfectly fine for distributions as they can > add/remove things more easily than we can do in master. I agree. 1/14 will go to Linus shortly. The rest is public, and distributions are taking it depending on their requirements, but I'd like to have the patchset polished before pushing it to Linus. Therefore putting the rest on hold now, and waiting for v2. Thanks a lot to both of you,
Jiri Kosina <jkosina <at> suse.cz> writes: > > On Mon, 2 Sep 2013, Benjamin Tissoires wrote: > > > > Given the 12 flaws, what do you see as the best way forward? > > > > If Jiri thinks we can wait one more week for some of these patches (I > > know that the most critical one, 1/14 is already on its way to Linus), > > then I can handle the v2, but not before Friday. Again, I think the > > current patch series is perfectly fine for distributions as they can > > add/remove things more easily than we can do in master. > > I agree. > > 1/14 will go to Linus shortly. The rest is public, and distributions are > taking it depending on their requirements, but I'd like to have the > patchset polished before pushing it to Linus. So we actually did pick up the entire 14 patch series in Fedora on top of 3.10.10 and 3.11 to fix the CVEs. It's broken things for at least two users. One person's touchpad is now detected as a mouse with the error: Sep 04 16:25:14 HP6930p kernel: psmouse serio4: synaptics: Unable to query device. The other person reported https://bugzilla.redhat.com/show_bug.cgi?id=1003998 wherein they say these patches cause an ODEBUG backtrace and that the trackpoint on a USB keyboard doesn't work. > Therefore putting the rest on hold now, and waiting for v2. Ideas on exactly what might cause these issues would be good to know. josh -- 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
Josh Boyer <jwboyer <at> fedoraproject.org> writes: > So we actually did pick up the entire 14 patch series in Fedora on top of > 3.10.10 and 3.11 to fix the CVEs. It's broken things for at least two > users. One person's touchpad is now detected as a mouse with the error: > > Sep 04 16:25:14 HP6930p kernel: psmouse serio4: synaptics: > Unable to query device. Ignore this part. The synaptics issue was found to be unrelated. > The other person reported > https://bugzilla.redhat.com/show_bug.cgi?id=1003998 wherein they say these > patches cause an ODEBUG backtrace and that the trackpoint on a USB keyboard > doesn't work. This is still an issue as far as we can tell. josh -- 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 Fri, Aug 30, 2013 at 11:27 AM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires > <benjamin.tissoires@gmail.com> wrote: >> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote: >>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires >>> <benjamin.tissoires@redhat.com> wrote: >>>> Hi Kees, >>>> >>>> I would be curious to have the HID report descriptors (maybe off list) >>>> to understand how things can be that bad. >>> >>> Certainly! I'll send them your way. I did have to get pretty creative >>> to tickle these conditions. >>> >>>> 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. >>> >>> I pondered doing this, but it seemed like something that needed wider >>> discussion, so I thought I'd start with just the dump of fixes. It >>> seems like the entire HID report interface should use access functions >>> to enforce range checking -- perhaps further enforced by making the >>> structure opaque to the drivers. >> >> The problem with access functions with range checking is when they are >> used at each input report. This will overload the kernel processing >> for static checks. >> >>> >>>> 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 >>> >>> Well, it certainly _shouldn't_ be less than zero. :) However, it is >>> defined as s8, and gets set from an int: >>> >>> for (i=0; i < field->maxusage; i++) { >>> if (field->usage[i].hid == usage->hid) { >>> td->inputmode_index = i; >>> break; >>> } >>> } >>> >>> Both "i" and "maxusage" are int, and I can generate a large maxusage >>> and usage array where the first matching hid equality happens when i >>> is >127, causing inputmode_index to wrap. >> >> ouch. Sorry. This piece of code (the current code) is junk. We should >> switch to usage->usage_index and change from __s8 to __s16 the >> declaration ASAP. >> >>> >>>>> + 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. >>> >>> That's certainly true, but since usage count and report count are not >>> directly associated, I don't know if there are devices that will freak >>> out with this restriction. >> >> Actually, I just again another long time understanding all the mess of >> having usage count and report count different in hid-core. >> >> I think it is a bug at some point (but it looks like I tend to be >> wrong on a lot of things recently :-P ), because when you look at the >> actual code of hid_input_field() in hid-core.c, we never go further >> than field->report_count when dealing with input report. >> So my guess is that we are declaring extra usages that are never used >> to parse incoming data. >> >>> >>>> 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. >>> >>> Again, I have no problem with that idea, but there seem to be lots of >>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT >>> or FEATURE reports). I didn't see a sensible way to approach this in >>> core without declaring a "I require $foo many OUTPUT reports, $bar >>> many INPUT, and $baz many FEATURE" for each driver. I instead opted >>> for the report validation function instead. >>> >> >> I think we can just add this check in both hidinput_configure_usage() >> and report_features() (both in hid-input.c) so that we don't call the >> *_mapping() callbacks with non sense fields. Can you suggest a patch for this? (And please include reference to CVE-2013-2897 in the commit.) I'll send a v2 of the rest of the series. Thanks! -Kees >> This way, the specific hid drivers will know only the correct fields, >> and don't need to check this. So patches 9, 11 and 13 can be amended. >> >> It looks to me that you mismatched field->report_count and the actual >> report_count in your answer: field->report_count is the number of >> consecutive fields of field->report_size that are present for the >> parsing of the report. It has nothing to do with the total report >> count. >> >>>>> 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. >>> >>> Well, the problem comes again from there not being a hard link between >>> the usage array and the value array: >>> >>> td->cc_value_index = usage->usage_index; >>> ... >>> int value = field->value[td->cc_value_index]; >>> >>> And all of this would be irrelevant if there was an access function >>> for field->value[]. >> >> True, with the current implementation, td->cc_value_index can be >> greater than field->report_count. Still, moving this check in >> mt_touch_input_mapping() will allow us to make it only once. >> >>> >>> Thanks for the review! >> >> you are welcome :) > > Okay, so, where does the whole patch series stand? It seems like the > checks are all worth adding; and my fixes are, I think, "minimal" so > having them go into the tree (so that they land in stable) seems like > a good idea. The work beyond that could be done on top of the existing > patches? There seems to be a lot of redesign ideas, but those probably > aren't so great for stable, and I'm probably not the best person to > write them, since everything I know about HID code I learned two weeks > ago. :) > > Given the 12 flaws, what do you see as the best way forward?
On Wed, Sep 4, 2013 at 6:31 PM, Kees Cook <keescook@chromium.org> wrote: > On Fri, Aug 30, 2013 at 11:27 AM, Kees Cook <keescook@chromium.org> wrote: >> On Fri, Aug 30, 2013 at 8:27 AM, Benjamin Tissoires >> <benjamin.tissoires@gmail.com> wrote: >>> On Thu, Aug 29, 2013 at 9:41 PM, Kees Cook <keescook@chromium.org> wrote: >>>> On Thu, Aug 29, 2013 at 1:59 AM, Benjamin Tissoires >>>> <benjamin.tissoires@redhat.com> wrote: >>>>> Hi Kees, >>>>> >>>>> I would be curious to have the HID report descriptors (maybe off list) >>>>> to understand how things can be that bad. >>>> >>>> Certainly! I'll send them your way. I did have to get pretty creative >>>> to tickle these conditions. >>>> >>>>> 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. >>>> >>>> I pondered doing this, but it seemed like something that needed wider >>>> discussion, so I thought I'd start with just the dump of fixes. It >>>> seems like the entire HID report interface should use access functions >>>> to enforce range checking -- perhaps further enforced by making the >>>> structure opaque to the drivers. >>> >>> The problem with access functions with range checking is when they are >>> used at each input report. This will overload the kernel processing >>> for static checks. >>> >>>> >>>>> 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 >>>> >>>> Well, it certainly _shouldn't_ be less than zero. :) However, it is >>>> defined as s8, and gets set from an int: >>>> >>>> for (i=0; i < field->maxusage; i++) { >>>> if (field->usage[i].hid == usage->hid) { >>>> td->inputmode_index = i; >>>> break; >>>> } >>>> } >>>> >>>> Both "i" and "maxusage" are int, and I can generate a large maxusage >>>> and usage array where the first matching hid equality happens when i >>>> is >127, causing inputmode_index to wrap. >>> >>> ouch. Sorry. This piece of code (the current code) is junk. We should >>> switch to usage->usage_index and change from __s8 to __s16 the >>> declaration ASAP. >>> >>>> >>>>>> + 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. >>>> >>>> That's certainly true, but since usage count and report count are not >>>> directly associated, I don't know if there are devices that will freak >>>> out with this restriction. >>> >>> Actually, I just again another long time understanding all the mess of >>> having usage count and report count different in hid-core. >>> >>> I think it is a bug at some point (but it looks like I tend to be >>> wrong on a lot of things recently :-P ), because when you look at the >>> actual code of hid_input_field() in hid-core.c, we never go further >>> than field->report_count when dealing with input report. >>> So my guess is that we are declaring extra usages that are never used >>> to parse incoming data. >>> >>>> >>>>> 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. >>>> >>>> Again, I have no problem with that idea, but there seem to be lots of >>>> general cases where this may not be possible (i.e. a HID with 0 OUTPUT >>>> or FEATURE reports). I didn't see a sensible way to approach this in >>>> core without declaring a "I require $foo many OUTPUT reports, $bar >>>> many INPUT, and $baz many FEATURE" for each driver. I instead opted >>>> for the report validation function instead. >>>> >>> >>> I think we can just add this check in both hidinput_configure_usage() >>> and report_features() (both in hid-input.c) so that we don't call the >>> *_mapping() callbacks with non sense fields. > > Can you suggest a patch for this? (And please include reference to > CVE-2013-2897 in the commit.) Sure, I'll do that. Cheers, Benjamin > > I'll send a v2 of the rest of the series. > > Thanks! > > -Kees > >>> This way, the specific hid drivers will know only the correct fields, >>> and don't need to check this. So patches 9, 11 and 13 can be amended. >>> >>> It looks to me that you mismatched field->report_count and the actual >>> report_count in your answer: field->report_count is the number of >>> consecutive fields of field->report_size that are present for the >>> parsing of the report. It has nothing to do with the total report >>> count. >>> >>>>>> 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. >>>> >>>> Well, the problem comes again from there not being a hard link between >>>> the usage array and the value array: >>>> >>>> td->cc_value_index = usage->usage_index; >>>> ... >>>> int value = field->value[td->cc_value_index]; >>>> >>>> And all of this would be irrelevant if there was an access function >>>> for field->value[]. >>> >>> True, with the current implementation, td->cc_value_index can be >>> greater than field->report_count. Still, moving this check in >>> mt_touch_input_mapping() will allow us to make it only once. >>> >>>> >>>> Thanks for the review! >>> >>> you are welcome :) >> >> Okay, so, where does the whole patch series stand? It seems like the >> checks are all worth adding; and my fixes are, I think, "minimal" so >> having them go into the tree (so that they land in stable) seems like >> a good idea. The work beyond that could be done on top of the existing >> patches? There seems to be a lot of redesign ideas, but those probably >> aren't so great for stable, and I'm probably not the best person to >> write them, since everything I know about HID code I learned two weeks >> ago. :) >> >> Given the 12 flaws, what do you see as the best way forward? > > -- > Kees Cook > Chrome OS Security -- 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 --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++) {