diff mbox

[12/14] HID: sensor-hub: validate feature report details

Message ID alpine.LNX.2.00.1308282222190.22181@pobox.suse.cz (mailing list archive)
State New, archived
Delegated to: Jiri Kosina
Headers show

Commit Message

Jiri Kosina Aug. 28, 2013, 8:31 p.m. UTC
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(-)

Comments

Jiri Kosina Aug. 28, 2013, 8:42 p.m. UTC | #1
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
>
srinivas pandruvada Aug. 28, 2013, 8:42 p.m. UTC | #2
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
Kees Cook Aug. 28, 2013, 9:16 p.m. UTC | #3
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
Mika Westerberg Aug. 29, 2013, 10:03 a.m. UTC | #4
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
srinivas pandruvada Aug. 29, 2013, 6:13 p.m. UTC | #5
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
Kees Cook Aug. 29, 2013, 7:47 p.m. UTC | #6
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 Sept. 4, 2013, 4:05 p.m. UTC | #7
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>
Jiri Kosina Sept. 4, 2013, 6:14 p.m. UTC | #8
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
Kees Cook Sept. 4, 2013, 6:26 p.m. UTC | #9
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 mbox

Patch

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;
 	}