* [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: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: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 ` 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 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-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-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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.