| Message ID | alpine.LNX.2.00.1308282222190.22181@pobox.suse.cz (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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 >
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 -- 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 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
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> -- 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 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 -- 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 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 >
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>
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
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
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; }