* [PATCH 12/14] HID: sensor-hub: validate feature report details @ 2013-08-28 20:31 Jiri Kosina 2013-08-28 20:42 ` Srinivas Pandruvada 2013-08-29 10:03 ` Mika Westerberg 0 siblings, 2 replies; 10+ messages in thread 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-28 20:31 [PATCH 12/14] HID: sensor-hub: validate feature report details Jiri Kosina @ 2013-08-28 20:42 ` Srinivas Pandruvada 2013-08-28 20:42 ` Jiri Kosina 2013-08-29 10:03 ` Mika Westerberg 1 sibling, 1 reply; 10+ messages in thread From: Srinivas Pandruvada @ 2013-08-28 20:42 UTC (permalink / raw) To: Jiri Kosina; +Cc: linux-input, Kees Cook, Mika Westerberg, srinivas pandruvada 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-28 20:42 ` Srinivas Pandruvada @ 2013-08-28 20:42 ` Jiri Kosina 2013-08-28 21:16 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Jiri Kosina @ 2013-08-28 20:42 UTC (permalink / raw) To: Srinivas Pandruvada Cc: linux-input, Kees Cook, Mika Westerberg, srinivas pandruvada 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-28 20:42 ` Jiri Kosina @ 2013-08-28 21:16 ` Kees Cook 2013-08-29 18:13 ` Srinivas Pandruvada 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2013-08-28 21:16 UTC (permalink / raw) To: Jiri Kosina Cc: Srinivas Pandruvada, linux-input, Mika Westerberg, srinivas pandruvada 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-28 21:16 ` Kees Cook @ 2013-08-29 18:13 ` Srinivas Pandruvada 2013-08-29 19:47 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Srinivas Pandruvada @ 2013-08-29 18:13 UTC (permalink / raw) To: Kees Cook; +Cc: Jiri Kosina, linux-input, Mika Westerberg, srinivas pandruvada On 08/28/2013 02:16 PM, Kees Cook wrote: > 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. Agree fix is simple, but the malicious feature report can contains other junk also. Can we really address all such issues? >>>> ret = -EINVAL; >>>> goto done_proc; >>>> } >>> Thanks, >>> Srinivas >>> >> -- >> Jiri Kosina >> SUSE Labs > > Thanks, Srinivas ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-29 18:13 ` Srinivas Pandruvada @ 2013-08-29 19:47 ` Kees Cook 0 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2013-08-29 19:47 UTC (permalink / raw) To: Srinivas Pandruvada Cc: Jiri Kosina, linux-input, Mika Westerberg, srinivas pandruvada On Thu, Aug 29, 2013 at 11:13 AM, Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com> wrote: > On 08/28/2013 02:16 PM, Kees Cook wrote: >> >> 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. > > Agree fix is simple, but the malicious feature report can contains other > junk also. Can we really address all such issues? I certainly hope so! :) It should be possible to range check all usages. We just need to examine the code closely. -Kees >>>>> >>>>> ret = -EINVAL; >>>>> goto done_proc; >>>>> } >>>> >>>> Thanks, >>>> Srinivas >>>> >>> -- >>> Jiri Kosina >>> SUSE Labs >> >> >> Thanks, > > Srinivas > -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-28 20:31 [PATCH 12/14] HID: sensor-hub: validate feature report details Jiri Kosina 2013-08-28 20:42 ` Srinivas Pandruvada @ 2013-08-29 10:03 ` Mika Westerberg 2013-09-04 16:05 ` Kees Cook 1 sibling, 1 reply; 10+ messages in thread From: Mika Westerberg @ 2013-08-29 10:03 UTC (permalink / raw) To: Jiri Kosina; +Cc: linux-input, Kees Cook, srinivas pandruvada 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 [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-08-29 10:03 ` Mika Westerberg @ 2013-09-04 16:05 ` Kees Cook 2013-09-04 18:14 ` Jiri Kosina 0 siblings, 1 reply; 10+ messages in thread From: Kees Cook @ 2013-09-04 16:05 UTC (permalink / raw) To: Mika Westerberg; +Cc: Jiri Kosina, linux-input, srinivas pandruvada Jiri, Should this one have been part of the batch you applied? It doesn't use hid_validate_report(). -Kees On Thu, Aug 29, 2013 at 3:03 AM, Mika Westerberg <mika.westerberg@linux.intel.com> wrote: > 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> -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-09-04 16:05 ` Kees Cook @ 2013-09-04 18:14 ` Jiri Kosina 2013-09-04 18:26 ` Kees Cook 0 siblings, 1 reply; 10+ messages in thread From: Jiri Kosina @ 2013-09-04 18:14 UTC (permalink / raw) To: Kees Cook; +Cc: Mika Westerberg, linux-input, srinivas pandruvada On Wed, 4 Sep 2013, Kees Cook wrote: > Should this one have been part of the batch you applied? It doesn't > use hid_validate_report(). It's there [1], I just somehow forgot to send out information mail, sorry for that. [1] https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-3.12/upstream&id=9e8910257397372633e74b333ef891f20c800ee4 -- Jiri Kosina SUSE Labs ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details 2013-09-04 18:14 ` Jiri Kosina @ 2013-09-04 18:26 ` Kees Cook 0 siblings, 0 replies; 10+ messages in thread From: Kees Cook @ 2013-09-04 18:26 UTC (permalink / raw) To: Jiri Kosina; +Cc: Mika Westerberg, linux-input, srinivas pandruvada On Wed, Sep 4, 2013 at 11:14 AM, Jiri Kosina <jkosina@suse.cz> wrote: > On Wed, 4 Sep 2013, Kees Cook wrote: > >> Should this one have been part of the batch you applied? It doesn't >> use hid_validate_report(). > > It's there [1], I just somehow forgot to send out information mail, sorry > for that. > > [1] https://git.kernel.org/cgit/linux/kernel/git/jikos/hid.git/commit/?h=for-3.12/upstream&id=9e8910257397372633e74b333ef891f20c800ee4 Ah-ha! Okay, thanks. :) -Kees -- Kees Cook Chrome OS Security ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2013-09-04 18:26 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-08-28 20:31 [PATCH 12/14] HID: sensor-hub: validate feature report details Jiri Kosina 2013-08-28 20:42 ` Srinivas Pandruvada 2013-08-28 20:42 ` Jiri Kosina 2013-08-28 21:16 ` Kees Cook 2013-08-29 18:13 ` Srinivas Pandruvada 2013-08-29 19:47 ` Kees Cook 2013-08-29 10:03 ` Mika Westerberg 2013-09-04 16:05 ` Kees Cook 2013-09-04 18:14 ` Jiri Kosina 2013-09-04 18:26 ` Kees Cook
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).