* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Kees Cook @ 2013-08-30 18:27 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Benjamin Tissoires, Jiri Kosina, linux-input, Henrik Rydberg
In-Reply-To: <CAN+gG=EvuO4R2d=bj=_9aWo1e96ee_i0sqSDJNd=3h+dVf=oWQ@mail.gmail.com>
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
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Benjamin Tissoires @ 2013-08-30 15:27 UTC (permalink / raw)
To: Kees Cook; +Cc: Benjamin Tissoires, Jiri Kosina, linux-input, Henrik Rydberg
In-Reply-To: <CAGXu5jKkzWmMZeckmO6BeKnJs-AjrFuZvqWNxDa6Rjb7UzAeyw@mail.gmail.com>
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
^ permalink raw reply
* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Benjamin Tissoires @ 2013-08-30 13:39 UTC (permalink / raw)
To: Kees Cook; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAGXu5jKjEQDpSKT_jH-1kvzF3tSmjPsvgqJ+Bx6EzQTg8O-e0A@mail.gmail.com>
On Thu, Aug 29, 2013 at 9:58 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Aug 29, 2013 at 7:50 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote:
>>> On Thu, Aug 29, 2013 at 2:48 AM, Benjamin Tissoires
>>> <benjamin.tissoires@gmail.com> wrote:
>>>> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>>> From: Kees Cook <keescook@chromium.org>
>>>>>
>>>>> This driver must validate the availability of the HID output report and
>>>>> its size before it can write LED states via buzz_set_leds(). This stops
>>>>> a heap overflow that is possible if a device provides a malicious HID
>>>>> output report:
>>>>>
>>>>> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
>>>>> ...
>>>>> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>>>>>
>>>>> CVE-2013-2890
>>>>>
>>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>>> Cc: stable@kernel.org
>>>>> ---
>>>>> drivers/hid/hid-sony.c | 4 ++++
>>>>> 1 file changed, 4 insertions(+)
>>>>>
>>>>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>>>>> index 87fbe29..b987926 100644
>>>>> --- a/drivers/hid/hid-sony.c
>>>>> +++ b/drivers/hid/hid-sony.c
>>>>> @@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
>>>>> drv_data = hid_get_drvdata(hdev);
>>>>> BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
>>>>>
>>>>> + /* Validate expected report characteristics. */
>>>>> + if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
>>>>
>>>> I don't have access to the device anymore, but I still kept the report
>>>> descriptors (this is the interesting part):
>>>>
>>>> 0xa1, 0x02, // Collection (Logical) 60
>>>> 0x75, 0x08, // Report Size (8) 62
>>>> 0x95, 0x07, // Report Count (7) 64
>>>> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
>>>> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
>>>> 0x09, 0x02, // Usage (Vendor Usage 2) 72
>>>> 0x91, 0x02, // Output (Data,Var,Abs) 74
>>>> 0xc0, // End Collection 76
>>>>
>>>> So with the current implementation of hid_validate_report(), it works,
>>>> but if another Buzz controller show up at some point with extras
>>>> fields in this output report... we will be screwed. So please, amend
>>>> hid_validate_report(), or specifically test here that the LED output
>>>> report is 7 bytes.
>>>
>>> hid_validate_report() checks for "at least" 7 in this call, so it
>>> should be fine, unless I've misunderstood something.
>>>
>>
>> Sure, it' s fine with the current implementation of
>> hid_validate_report(). However, as I mentioned in patch
>> 2/14, I am complaining about the implementation of hid_validate_report().
>>
>> In this case, if a new Buzz controller pops out with an extra usage
>> (Vendor 3 for instance), mapped to another LED, and that the report
>> count is for this usage < 7, the test invalidate the report.
>>
>> for instance, let's imagine they pop up a new version:
>>
>> 0xa1, 0x02, // Collection (Logical) 60
>> 0x75, 0x08, // Report Size (8) 62
>> 0x95, 0x07, // **Report Count (7)** 64
>> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
>> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
>> 0x09, 0x02, // Usage (Vendor Usage 2) 72
>> 0x91, 0x02, // Output (Data,Var,Abs) 74
>> 0x75, 0x08, // Report Size (8) 62
>> 0x95, 0x04, // **Report Count (4)** 64
>> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
>> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
>> 0x09, 0x03, // Usage (Vendor Usage 3) 72
>> 0x91, 0x02, // Output (Data,Var,Abs) 74
>> 0xc0, // End Collection 76
>>
>> Ok, it's a lot of "if", but still this output report is valid, and the
>> test will fail. And if we call hid_validate_report(hdev,
>> HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap
>> overflow will appear again.
>>
>> Does it makes more sense?
>
> Right, yeah, I understand what you meant here, but I guess my point
> was, if there's something that uses <7, then the driver needs
> adjustment too, beyond just the hid_validate_report() call, since it
> would need to know to operate only on 4 instead of 7. My thinking was,
> if such a thing is detected, it would need to identify which device it
> was and fix both the bounds-checking, and the report-value-setting.
> For example:
>
> if (i_am_vendor_3()) {
> hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 4);
> } else {
> hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7);
> }
>
hmm... not really. In my case, I supposed the device presented both vendor collections, one after the other. So the test could be:
hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7);
if (I_contain_vendor_3())
hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 2, 4);
But this will not work if the small report is in front of the large one.
I can propose another implementation of hid_validate_report() which will be covering this case:
instead of checking a range of fields, just check the specific field index used later:
+struct hid_report *hid_validate_report(struct hid_device *hid,
+ unsigned int type, unsigned int id,
+ unsigned int field_index,
+ unsigned int report_counts)
+{
+ struct hid_report *report;
+
+ if (type > HID_FEATURE_REPORT) {
+ hid_err(hid, "invalid HID report %u\n", type);
+ return NULL;
+ }
+
+ report = hid->report_enum[type].report_id_hash[id];
+ if (!report) {
+ hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
+ return NULL;
+ }
+ if (report->maxfield <= field_index) {
+ hid_err(hid, "not enough fields in %s %u\n",
+ hid_report_names[type], id);
+ return NULL;
+ }
+
+ if (report->field[field_index]->report_count < report_counts) {
+ hid_err(hid, "not enough values in %s %u field #%d\n",
+ hid_report_names[type], id, field_index);
+ return NULL;
+ }
+ return report;
+}
+EXPORT_SYMBOL_GPL(hid_validate_report);
Only hid-zpff and hid-lenovo-tpkbd are checking for more than one report, and we can afford a for loop on the field indexes for them.
Cheers,
Benjamin
> ...
>
> value[0] = 0x00;
> value[1] = (leds & 1) ? 0xff : 0x00;
> value[2] = (leds & 2) ? 0xff : 0x00;
> value[3] = (leds & 4) ? 0xff : 0x00;
> if (!i_am_vendor_3()) {
> value[4] = (leds & 8) ? 0xff : 0x00;
> value[5] = 0x00;
> value[6] = 0x00;
> }
>
> But actually, the logic would be id or usage based, but still, it
> seems to me that the hid_validate_report() call must match the actual
> value array assignments.
>
> -Kees
>
>>
>> Cheers,
>> Benjamin
>>
>>>>> + return -ENODEV;
>>>>> +
>>>>> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
>>>>> if (!buzz) {
>>>>> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>>>>>
>>>>> --
>>>>> Jiri Kosina
>>>>> SUSE Labs
>>>>> --
>>>>> 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
>>> Chrome OS Security
>
>
>
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* Re: [PATCH 02/14] HID: provide a helper for validating hid reports
From: Benjamin Tissoires @ 2013-08-30 13:05 UTC (permalink / raw)
To: Kees Cook; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAGXu5j+G0o6DZOar8j24ZDfqJc4_M_2RBAxToW+CxR1aoejyDw@mail.gmail.com>
On Thu, Aug 29, 2013 at 9:51 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Aug 29, 2013 at 2:35 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> Hi Kees,
>>
>> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>> From: Kees Cook <keescook@chromium.org>
>>>
>>> Many drivers need to validate the characteristics of their HID report
>>> during initialization to avoid misusing the reports. This adds a common
>>> helper to perform validation of the report, its field count, and the
>>> value count within the fields.
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Cc: stable@kernel.org
>>> ---
>>> drivers/hid/hid-core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>>> include/linux/hid.h | 4 ++++
>>> 2 files changed, 54 insertions(+)
>>>
>>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>>> index 5ea7d51..55798b2 100644
>>> --- a/drivers/hid/hid-core.c
>>> +++ b/drivers/hid/hid-core.c
>>> @@ -759,6 +759,56 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
>>> }
>>> EXPORT_SYMBOL_GPL(hid_parse_report);
>>>
>>> +static const char * const hid_report_names[] = {
>>> + "HID_INPUT_REPORT",
>>> + "HID_OUTPUT_REPORT",
>>> + "HID_FEATURE_REPORT",
>>> +};
>>> +/**
>>> + * hid_validate_report - validate existing device report
>>> + *
>>> + * @device: hid device
>>> + * @type: which report type to examine
>>> + * @id: which report ID to examine (0 for first)
>>> + * @fields: expected number of fields
>>> + * @report_counts: expected number of values per field
>>> + *
>>> + * Validate the report details after parsing.
>>> + */
>>> +struct hid_report *hid_validate_report(struct hid_device *hid,
>>> + unsigned int type, unsigned int id,
>>
>> You should consider having an u8 for id, or at least add a check
>> against id >= HID_MAX_IDS
>
> Right, as you saw in later email, I added this check globally in the
> first patch.
>
Yes, but still, a driver may call this with 257 as an id, leading to a
report pointer with garbage value. And here, the function is used once
after parsing, so the test can be added without any impact.
>>
>>> + unsigned int fields,
>>> + unsigned int report_counts)
>>> +{
>>> + struct hid_report *report;
>>> + unsigned int i;
>>> +
>>> + if (type > HID_FEATURE_REPORT) {
>>> + hid_err(hid, "invalid HID report %u\n", type);
>>> + return NULL;
>>> + }
>>> +
>>> + report = hid->report_enum[type].report_id_hash[id];
>>
>> for code readability, and better checking, I'd prefer use
>> hid_get_report() here instead. Or just add an inlined accessor in
>> hid.h to retrieve the report from the id as many drivers are using the
>> report_id_hash directly.
>
> I did not do this because hid_get_report() examines ->numbered and I
> explicitly didn't want that limitation since most of these drivers are
> blinding accessing the arrays by id number. If the structure members
> were opaque and all the drivers were forced to use the access
> functions, sure, we'd be fine.
>
Well, what I have in mind is just to add the accessor, and to convert
all the drivers to use it. If a new driver try to not use it, we don't
allow him to be upstream :)
>>
>>> + if (!report) {
>>> + hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
>>> + return NULL;
>>> + }
>>> + if (report->maxfield < fields) {
>>> + hid_err(hid, "not enough fields in %s %u\n",
>>> + hid_report_names[type], id);
>>> + return NULL;
>>> + }
>>> + for (i = 0; i < fields; i++) {
>>> + if (report->field[i]->report_count < report_counts) {
>>
>> This is wrong.
>> you can have a per field different report_count, and it is perfectly
>> correct. So the only validate value for report_counts would be 1.
>> Otherwise, if you want to provide better checking, you need to provide
>> an array of report_counts, which start beeing a little bit annoying
>> for users.
>
> Well, nothing needs multiple differing report_counts in practice, so I
> opted to just do it this way since nothing calling the function needs
> more granular checking.
>
So I would recommend at least to add a comment somewhere that this is
the current implementation, and we may need to change it if we have
different devices which would need a different processing.
Cheers,
Benjamin
> -Kees
>
>>
>>> + hid_err(hid, "not enough values in %s %u fields\n",
>>> + hid_report_names[type], id);
>>> + return NULL;
>>> + }
>>> + }
>>> + return report;
>>> +}
>>> +EXPORT_SYMBOL_GPL(hid_validate_report);
>>> +
>>> /**
>>> * hid_open_report - open a driver-specific device report
>>> *
>>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>>> index ff545cc..76e41d8 100644
>>> --- a/include/linux/hid.h
>>> +++ b/include/linux/hid.h
>>> @@ -749,6 +749,10 @@ void hid_output_report(struct hid_report *report, __u8 *data);
>>> struct hid_device *hid_allocate_device(void);
>>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>>> int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
>>> +struct hid_report *hid_validate_report(struct hid_device *hid,
>>> + unsigned int type, unsigned int id,
>>> + unsigned int fields,
>>> + unsigned int report_counts);
>>> int hid_open_report(struct hid_device *device);
>>> int hid_check_keys_pressed(struct hid_device *hid);
>>> int hid_connect(struct hid_device *hid, unsigned int connect_mask);
>>>
>>> --
>>> Jiri Kosina
>>> SUSE Labs
>>> --
>>
>> Cheers,
>> Benjamin
>
>
>
> --
> Kees Cook
> Chrome OS Security
^ permalink raw reply
* [PATCH] HID: roccat: Added support for KonePureOptical v2
From: Stefan Achatz @ 2013-08-30 12:10 UTC (permalink / raw)
To: Jiri Kosina, linux-input, linux-kernel
KonePureOptical is a KonePure with different sensor.
Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
---
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 1 +
drivers/hid/hid-roccat-konepure.c | 3 ++-
3 files changed, 4 insertions(+), 1 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 36668d1..8c4e82c 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1679,6 +1679,7 @@ static const struct hid_device_id hid_have_special_driver[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_ISKU) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPLUS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KOVAPLUS) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_LUA) },
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_PYRA_WIRED) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ffe4c7a..ef18cd9 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -716,6 +716,7 @@
#define USB_DEVICE_ID_ROCCAT_KONE 0x2ced
#define USB_DEVICE_ID_ROCCAT_KONEPLUS 0x2d51
#define USB_DEVICE_ID_ROCCAT_KONEPURE 0x2dbe
+#define USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL 0x2db4
#define USB_DEVICE_ID_ROCCAT_KONEXTD 0x2e22
#define USB_DEVICE_ID_ROCCAT_KOVAPLUS 0x2d50
#define USB_DEVICE_ID_ROCCAT_LUA 0x2c2e
diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c
index c79d0b0..5850959 100644
--- a/drivers/hid/hid-roccat-konepure.c
+++ b/drivers/hid/hid-roccat-konepure.c
@@ -262,6 +262,7 @@ static int konepure_raw_event(struct hid_device *hdev,
static const struct hid_device_id konepure_devices[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL) },
{ }
};
@@ -300,5 +301,5 @@ module_init(konepure_init);
module_exit(konepure_exit);
MODULE_AUTHOR("Stefan Achatz");
-MODULE_DESCRIPTION("USB Roccat KonePure driver");
+MODULE_DESCRIPTION("USB Roccat KonePure/Optical driver");
MODULE_LICENSE("GPL v2");
--
1.7.3.4
^ permalink raw reply related
* Re: [PATCH 4/6] si4713 : HID blacklist Si4713 USB development board
From: Jiri Kosina @ 2013-08-30 11:47 UTC (permalink / raw)
To: Dinesh Ram; +Cc: linux-media, dinesh.ram, linux-input
In-Reply-To: <228d4c6c3c411f4f5aad408d5bff88bb09a82e4e.1377861337.git.dinram@cisco.com>
On Fri, 30 Aug 2013, Dinesh Ram wrote:
> The Si4713 development board contains a Si4713 FM transmitter chip
> and is handled by the radio-usb-si4713 driver.
> The board reports itself as (10c4:8244) Cygnal Integrated Products, Inc.
> and misidentifies itself as a HID device in its USB interface descriptor.
> This patch ignores this device as an HID device and hence loads the custom driver.
>
> Signed-off-by: Dinesh Ram <dinram@cisco.com>
Signed-off-by: Jiri Kosina <jkosina@suse.cz>
Please feel free to take it together with my sigoff with the rest of the
series.
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* [PATCH 4/6] si4713 : HID blacklist Si4713 USB development board
From: Dinesh Ram @ 2013-08-30 11:28 UTC (permalink / raw)
To: linux-media; +Cc: dinesh.ram, Dinesh Ram, Jiri Kosina, linux-input
In-Reply-To: <a661e3d7ccefe3baa8134888a0471ce1e5463f47.1377861337.git.dinram@cisco.com>
The Si4713 development board contains a Si4713 FM transmitter chip
and is handled by the radio-usb-si4713 driver.
The board reports itself as (10c4:8244) Cygnal Integrated Products, Inc.
and misidentifies itself as a HID device in its USB interface descriptor.
This patch ignores this device as an HID device and hence loads the custom driver.
Signed-off-by: Dinesh Ram <dinram@cisco.com>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: linux-input@vger.kernel.org
---
drivers/hid/hid-core.c | 1 +
drivers/hid/hid-ids.h | 2 ++
2 files changed, 3 insertions(+)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index 36668d1..109510f 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -1977,6 +1977,7 @@ static const struct hid_device_id hid_ignore_list[] = {
{ HID_USB_DEVICE(USB_VENDOR_ID_BERKSHIRE, USB_DEVICE_ID_BERKSHIRE_PCWD) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CIDC, 0x0103) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI470X) },
+ { HID_USB_DEVICE(USB_VENDOR_ID_CYGNAL, USB_DEVICE_ID_CYGNAL_RADIO_SI4713) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CMEDIA, USB_DEVICE_ID_CM109) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_HIDCOM) },
{ HID_USB_DEVICE(USB_VENDOR_ID_CYPRESS, USB_DEVICE_ID_CYPRESS_ULTRAMOUSE) },
diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
index ffe4c7a..2a38726 100644
--- a/drivers/hid/hid-ids.h
+++ b/drivers/hid/hid-ids.h
@@ -241,6 +241,8 @@
#define USB_VENDOR_ID_CYGNAL 0x10c4
#define USB_DEVICE_ID_CYGNAL_RADIO_SI470X 0x818a
+#define USB_DEVICE_ID_CYGNAL_RADIO_SI4713 0x8244
+
#define USB_VENDOR_ID_CYPRESS 0x04b4
#define USB_DEVICE_ID_CYPRESS_MOUSE 0x0001
#define USB_DEVICE_ID_CYPRESS_HIDCOM 0x5500
--
1.8.4.rc2
^ permalink raw reply related
* Re: [PATCH] Input: add driver for Neonode zForce based touchscreens
From: Heiko Stübner @ 2013-08-29 23:37 UTC (permalink / raw)
To: Dmitry Torokhov; +Cc: Henrik Rydberg, linux-kernel, linux-input
In-Reply-To: <20130829162903.GB1576@core.coreip.homeip.net>
Hi Dmitry,
Am Donnerstag, 29. August 2013, 18:29:04 schrieb Dmitry Torokhov:
> Hi Heiko,
>
> On Fri, Aug 16, 2013 at 01:59:39PM +0200, Heiko Stübner wrote:
> > This adds a driver for touchscreens using the zforce infrared
> > technology from Neonode connected via i2c to the host system.
> >
> > It supports multitouch with up to two fingers and tracking of the
> > contacts in hardware.
>
> Generally looks good, just a few comments...
>
> > Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> > ---
> >
> > drivers/input/touchscreen/Kconfig | 13 +
> > drivers/input/touchscreen/Makefile | 1 +
> > drivers/input/touchscreen/zforce_ts.c | 837
> > +++++++++++++++++++++++++++++++++ include/linux/input/zforce_ts.h
> > | 26 +
> > 4 files changed, 877 insertions(+)
> > create mode 100644 drivers/input/touchscreen/zforce_ts.c
> > create mode 100644 include/linux/input/zforce_ts.h
> >
> > diff --git a/drivers/input/touchscreen/Kconfig
> > b/drivers/input/touchscreen/Kconfig index 3b9758b..ade11b7 100644
> > --- a/drivers/input/touchscreen/Kconfig
> > +++ b/drivers/input/touchscreen/Kconfig
> > @@ -919,4 +919,17 @@ config TOUCHSCREEN_TPS6507X
> >
> > To compile this driver as a module, choose M here: the
> > module will be called tps6507x_ts.
> >
> > +config TOUCHSCREEN_ZFORCE
> > + tristate "Neonode zForce infrared touchscreens"
> > + depends on I2C
> > + depends on GPIOLIB
> > + help
> > + Say Y here if you have a touchscreen using the zforce
> > + infraread technology from Neonode.
> > +
> > + If unsure, say N.
> > +
> > + To compile this driver as a module, choose M here: the
> > + module will be called zforce_ts.
> > +
> >
> > endif
> >
> > diff --git a/drivers/input/touchscreen/Makefile
> > b/drivers/input/touchscreen/Makefile index f5216c1..7587883 100644
> > --- a/drivers/input/touchscreen/Makefile
> > +++ b/drivers/input/touchscreen/Makefile
> > @@ -75,3 +75,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE) +=
> > mainstone-wm97xx.o
> >
> > obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE) += zylonite-wm97xx.o
> > obj-$(CONFIG_TOUCHSCREEN_W90X900) += w90p910_ts.o
> > obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
> >
> > +obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> > diff --git a/drivers/input/touchscreen/zforce_ts.c
> > b/drivers/input/touchscreen/zforce_ts.c new file mode 100644
> > index 0000000..92af632
> > --- /dev/null
> > +++ b/drivers/input/touchscreen/zforce_ts.c
> > @@ -0,0 +1,837 @@
> > +/*
> > + * Copyright (C) 2012-2013 MundoReader S.L.
> > + * Author: Heiko Stuebner <heiko@sntech.de>
> > + *
> > + * based in parts on Nook zforce driver
> > + *
> > + * Copyright (C) 2010 Barnes & Noble, Inc.
> > + * Author: Pieter Truter<ptruter@intrinsyc.com>
> > + *
> > + * This software is licensed under the terms of the GNU General Public
> > + * License version 2, as published by the Free Software Foundation, and
> > + * may be copied, distributed, and modified under those terms.
> > + *
> > + * This program is distributed in the hope that it will be useful,
> > + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> > + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> > + * GNU General Public License for more details.
> > + */
> > +
> > +#include <linux/module.h>
> > +#include <linux/hrtimer.h>
> > +#include <linux/slab.h>
> > +#include <linux/input.h>
> > +#include <linux/interrupt.h>
> > +#include <linux/i2c.h>
> > +#include <linux/delay.h>
> > +#include <linux/gpio.h>
> > +#include <linux/device.h>
> > +#include <linux/sysfs.h>
> > +#include <linux/input/zforce_ts.h>
> > +#include <linux/input/mt.h>
> > +
> > +#define WAIT_TIMEOUT msecs_to_jiffies(1000)
> > +
> > +#define FRAME_START 0xee
> > +
> > +/* Offsets of the different parts of the payload the controller sends */
> > +#define PAYLOAD_HEADER 0
> > +#define PAYLOAD_LENGTH 1
> > +#define PAYLOAD_BODY 2
> > +
> > +/* Response offsets */
> > +#define RESPONSE_ID 0
> > +#define RESPONSE_DATA 1
> > +
> > +/* Commands */
> > +#define COMMAND_DEACTIVATE 0x00
> > +#define COMMAND_INITIALIZE 0x01
> > +#define COMMAND_RESOLUTION 0x02
> > +#define COMMAND_SETCONFIG 0x03
> > +#define COMMAND_DATAREQUEST 0x04
> > +#define COMMAND_SCANFREQ 0x08
> > +#define COMMAND_STATUS 0X1e
> > +
> > +/*
> > + * Responses the controller sends as a result of
> > + * command requests
> > + */
> > +#define RESPONSE_DEACTIVATE 0x00
> > +#define RESPONSE_INITIALIZE 0x01
> > +#define RESPONSE_RESOLUTION 0x02
> > +#define RESPONSE_SETCONFIG 0x03
> > +#define RESPONSE_SCANFREQ 0x08
> > +#define RESPONSE_STATUS 0X1e
> > +
> > +/*
> > + * Notifications are send by the touch controller without
> > + * being requested by the driver and include for example
> > + * touch indications
> > + */
> > +#define NOTIFICATION_TOUCH 0x04
> > +#define NOTIFICATION_BOOTCOMPLETE 0x07
> > +#define NOTIFICATION_OVERRUN 0x25
> > +#define NOTIFICATION_PROXIMITY 0x26
> > +#define NOTIFICATION_INVALID_COMMAND 0xfe
> > +
> > +#define ZFORCE_REPORT_POINTS 2
> > +#define ZFORCE_MAX_AREA 0xff
> > +
> > +#define STATE_DOWN 0
> > +#define STATE_MOVE 1
> > +#define STATE_UP 2
> > +
> > +#define SETCONFIG_DUALTOUCH (1 << 0)
> > +
> > +struct zforce_point {
> > + int coord_x;
> > + int coord_y;
> > + int state;
> > + int id;
> > + int area_major;
> > + int area_minor;
> > + int orientation;
> > + int pressure;
> > + int prblty;
> > +};
> > +
> > +/*
> > + * @client the i2c_client
> > + * @input the input device
> > + * @suspending in the process of going to suspend (don't emit
wakeup
> > + * events for commands executed to suspend the device)
> > + * @suspended device suspended
> > + * @access_mutex serialize i2c-access, to keep multipart reads together
> > + * @command_done completion to wait for the command result
> > + * @command_mutex serialize commands send to the ic
> > + * @command_waiting the id of the command that that is currently
waiting
> > + * for a result
> > + * @command_result returned result of the command
> > + */
> > +struct zforce_ts {
> > + struct i2c_client *client;
> > + struct input_dev *input;
> > + const struct zforce_ts_platdata *pdata;
> > + char phys[32];
> > +
> > + bool suspending;
> > + bool suspended;
> > + bool boot_complete;
> > +
> > + /* Firmware version information */
> > + u16 version_major;
> > + u16 version_minor;
> > + u16 version_build;
> > + u16 version_rev;
> > +
> > + struct mutex access_mutex;
> > +
> > + struct completion command_done;
> > + struct mutex command_mutex;
> > + int command_waiting;
> > + int command_result;
> > +};
> > +
> > +static int zforce_command(struct zforce_ts *ts, u8 cmd)
> > +{
> > + struct i2c_client *client = ts->client;
> > + char buf[3];
> > + int ret;
> > +
> > + dev_dbg(&client->dev, "%s: 0x%x\n", __func__, cmd);
> > +
> > + buf[0] = FRAME_START;
> > + buf[1] = 1; /* data size, command only */
> > + buf[2] = cmd;
> > +
> > + mutex_lock(&ts->access_mutex);
> > + ret = i2c_master_send(client, &buf[0], ARRAY_SIZE(buf));
> > + mutex_unlock(&ts->access_mutex);
>
> I am unsure why you need this lock. Doesn't i2c core already ensure
> necessary locking?
The access_lock is really only there to ensure the multi-reads stay together
and do not get split up. For individual reads the i2c core of course does all
the necessary things.
> Also, it does not look like zforec_command() will reace with your
> interrupt handler that does multi-reads...
I'm unsure :-) ... what about the following scenario:
- touch -> interrupt
- isr reads first packet
- user closes device -> stop command sent
- isr reads payload
I'll fix the rest of the issues in the next version too.
Thanks for the review
Heiko
^ permalink raw reply
* [PATCH 2/2] Input: wacom - LED is only suported through digitizer interface
From: Ping Cheng @ 2013-08-29 23:04 UTC (permalink / raw)
To: linux-input; +Cc: dmitry.torokhov, Ping Cheng
Signed-off-by: Ping Cheng <pingc@wacom.com>
---
drivers/input/tablet/wacom_sys.c | 24 ++++++++++++++----------
1 file changed, 14 insertions(+), 10 deletions(-)
diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 1c70a8d..f1469e4 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -978,14 +978,17 @@ static int wacom_initialize_leds(struct wacom *wacom)
case INTUOS5S:
case INTUOS5:
case INTUOS5L:
- wacom->led.select[0] = 0;
- wacom->led.select[1] = 0;
- wacom->led.llv = 32;
- wacom->led.hlv = 0;
- wacom->led.img_lum = 0;
-
- error = sysfs_create_group(&wacom->intf->dev.kobj,
- &intuos5_led_attr_group);
+ if (wacom->wacom_wac.features.device_type == BTN_TOOL_PEN) {
+ wacom->led.select[0] = 0;
+ wacom->led.select[1] = 0;
+ wacom->led.llv = 32;
+ wacom->led.hlv = 0;
+ wacom->led.img_lum = 0;
+
+ error = sysfs_create_group(&wacom->intf->dev.kobj,
+ &intuos5_led_attr_group);
+ } else
+ return 0;
break;
default:
@@ -1021,8 +1024,9 @@ static void wacom_destroy_leds(struct wacom *wacom)
case INTUOS5S:
case INTUOS5:
case INTUOS5L:
- sysfs_remove_group(&wacom->intf->dev.kobj,
- &intuos5_led_attr_group);
+ if (wacom->wacom_wac.features.device_type == BTN_TOOL_PEN)
+ sysfs_remove_group(&wacom->intf->dev.kobj,
+ &intuos5_led_attr_group);
break;
}
}
--
1.8.1.2
^ permalink raw reply related
* [PATCH 1/2] Input: wacom - testing result shows get_report is unnecessary.
From: Ping Cheng @ 2013-08-29 23:03 UTC (permalink / raw)
To: linux-input; +Cc: dmitry.torokhov, Ping Cheng
Signed-off-by: Ping Cheng <pingc@wacom.com>
---
drivers/input/tablet/wacom_sys.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/input/tablet/wacom_sys.c b/drivers/input/tablet/wacom_sys.c
index 79b69ea..1c70a8d 100644
--- a/drivers/input/tablet/wacom_sys.c
+++ b/drivers/input/tablet/wacom_sys.c
@@ -524,9 +524,6 @@ static int wacom_set_device_mode(struct usb_interface *intf, int report_id, int
error = wacom_set_report(intf, WAC_HID_FEATURE_REPORT,
report_id, rep_data, length, 1);
- if (error >= 0)
- error = wacom_get_report(intf, WAC_HID_FEATURE_REPORT,
- report_id, rep_data, length, 1);
} while ((error < 0 || rep_data[1] != mode) && limit++ < WAC_MSG_RETRIES);
kfree(rep_data);
--
1.8.1.2
^ permalink raw reply related
* Re: [PATCH] HID: roccat: Added support for KonePureOptical
From: Jiri Kosina @ 2013-08-29 20:41 UTC (permalink / raw)
To: Stefan Achatz; +Cc: linux-input, linux-kernel
In-Reply-To: <1377786628.2404.1.camel@neuromancer.tessier-ashpool>
On Thu, 29 Aug 2013, Stefan Achatz wrote:
> KonePureOptical is a KonePure with different sensor.
>
> Signed-off-by: Stefan Achatz <erazor_de@users.sourceforge.net>
> ---
> drivers/hid/hid-ids.h | 1 +
> drivers/hid/hid-roccat-konepure.c | 3 ++-
> 2 files changed, 3 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/hid/hid-ids.h b/drivers/hid/hid-ids.h
> index ffe4c7a..ef18cd9 100644
> --- a/drivers/hid/hid-ids.h
> +++ b/drivers/hid/hid-ids.h
> @@ -716,6 +716,7 @@
> #define USB_DEVICE_ID_ROCCAT_KONE 0x2ced
> #define USB_DEVICE_ID_ROCCAT_KONEPLUS 0x2d51
> #define USB_DEVICE_ID_ROCCAT_KONEPURE 0x2dbe
> +#define USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL 0x2db4
> #define USB_DEVICE_ID_ROCCAT_KONEXTD 0x2e22
> #define USB_DEVICE_ID_ROCCAT_KOVAPLUS 0x2d50
> #define USB_DEVICE_ID_ROCCAT_LUA 0x2c2e
> diff --git a/drivers/hid/hid-roccat-konepure.c b/drivers/hid/hid-roccat-konepure.c
> index c79d0b0..5850959 100644
> --- a/drivers/hid/hid-roccat-konepure.c
> +++ b/drivers/hid/hid-roccat-konepure.c
> @@ -262,6 +262,7 @@ static int konepure_raw_event(struct hid_device *hdev,
>
> static const struct hid_device_id konepure_devices[] = {
> { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE) },
> + { HID_USB_DEVICE(USB_VENDOR_ID_ROCCAT, USB_DEVICE_ID_ROCCAT_KONEPURE_OPTICAL) },
> { }
> };
I believe you are missing hid_have_special_driver[] entry.
>
> @@ -300,5 +301,5 @@ module_init(konepure_init);
> module_exit(konepure_exit);
>
> MODULE_AUTHOR("Stefan Achatz");
> -MODULE_DESCRIPTION("USB Roccat KonePure driver");
> +MODULE_DESCRIPTION("USB Roccat KonePure/Optical driver");
> MODULE_LICENSE("GPL v2");
> --
> 1.7.3.4
>
>
>
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Kees Cook @ 2013-08-29 19:58 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAN+gG=FeU1ht-hmeGThw=sRshn7gZXde3PePFHpWEpKA1-K8=Q@mail.gmail.com>
On Thu, Aug 29, 2013 at 7:50 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote:
>> On Thu, Aug 29, 2013 at 2:48 AM, Benjamin Tissoires
>> <benjamin.tissoires@gmail.com> wrote:
>>> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>>> From: Kees Cook <keescook@chromium.org>
>>>>
>>>> This driver must validate the availability of the HID output report and
>>>> its size before it can write LED states via buzz_set_leds(). This stops
>>>> a heap overflow that is possible if a device provides a malicious HID
>>>> output report:
>>>>
>>>> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
>>>> ...
>>>> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>>>>
>>>> CVE-2013-2890
>>>>
>>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>>> Cc: stable@kernel.org
>>>> ---
>>>> drivers/hid/hid-sony.c | 4 ++++
>>>> 1 file changed, 4 insertions(+)
>>>>
>>>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>>>> index 87fbe29..b987926 100644
>>>> --- a/drivers/hid/hid-sony.c
>>>> +++ b/drivers/hid/hid-sony.c
>>>> @@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
>>>> drv_data = hid_get_drvdata(hdev);
>>>> BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
>>>>
>>>> + /* Validate expected report characteristics. */
>>>> + if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
>>>
>>> I don't have access to the device anymore, but I still kept the report
>>> descriptors (this is the interesting part):
>>>
>>> 0xa1, 0x02, // Collection (Logical) 60
>>> 0x75, 0x08, // Report Size (8) 62
>>> 0x95, 0x07, // Report Count (7) 64
>>> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
>>> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
>>> 0x09, 0x02, // Usage (Vendor Usage 2) 72
>>> 0x91, 0x02, // Output (Data,Var,Abs) 74
>>> 0xc0, // End Collection 76
>>>
>>> So with the current implementation of hid_validate_report(), it works,
>>> but if another Buzz controller show up at some point with extras
>>> fields in this output report... we will be screwed. So please, amend
>>> hid_validate_report(), or specifically test here that the LED output
>>> report is 7 bytes.
>>
>> hid_validate_report() checks for "at least" 7 in this call, so it
>> should be fine, unless I've misunderstood something.
>>
>
> Sure, it' s fine with the current implementation of
> hid_validate_report(). However, as I mentioned in patch
> 2/14, I am complaining about the implementation of hid_validate_report().
>
> In this case, if a new Buzz controller pops out with an extra usage
> (Vendor 3 for instance), mapped to another LED, and that the report
> count is for this usage < 7, the test invalidate the report.
>
> for instance, let's imagine they pop up a new version:
>
> 0xa1, 0x02, // Collection (Logical) 60
> 0x75, 0x08, // Report Size (8) 62
> 0x95, 0x07, // **Report Count (7)** 64
> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
> 0x09, 0x02, // Usage (Vendor Usage 2) 72
> 0x91, 0x02, // Output (Data,Var,Abs) 74
> 0x75, 0x08, // Report Size (8) 62
> 0x95, 0x04, // **Report Count (4)** 64
> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
> 0x09, 0x03, // Usage (Vendor Usage 3) 72
> 0x91, 0x02, // Output (Data,Var,Abs) 74
> 0xc0, // End Collection 76
>
> Ok, it's a lot of "if", but still this output report is valid, and the
> test will fail. And if we call hid_validate_report(hdev,
> HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap
> overflow will appear again.
>
> Does it makes more sense?
Right, yeah, I understand what you meant here, but I guess my point
was, if there's something that uses <7, then the driver needs
adjustment too, beyond just the hid_validate_report() call, since it
would need to know to operate only on 4 instead of 7. My thinking was,
if such a thing is detected, it would need to identify which device it
was and fix both the bounds-checking, and the report-value-setting.
For example:
if (i_am_vendor_3()) {
hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 4);
} else {
hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7);
}
...
value[0] = 0x00;
value[1] = (leds & 1) ? 0xff : 0x00;
value[2] = (leds & 2) ? 0xff : 0x00;
value[3] = (leds & 4) ? 0xff : 0x00;
if (!i_am_vendor_3()) {
value[4] = (leds & 8) ? 0xff : 0x00;
value[5] = 0x00;
value[6] = 0x00;
}
But actually, the logic would be id or usage based, but still, it
seems to me that the hid_validate_report() call must match the actual
value array assignments.
-Kees
>
> Cheers,
> Benjamin
>
>>>> + return -ENODEV;
>>>> +
>>>> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
>>>> if (!buzz) {
>>>> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>>>>
>>>> --
>>>> Jiri Kosina
>>>> SUSE Labs
>>>> --
>>>> 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
>> Chrome OS Security
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 02/14] HID: provide a helper for validating hid reports
From: Kees Cook @ 2013-08-29 19:51 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAN+gG=H5v=XZ1OOqwiTiOK8fuieMCGRcg_gCks1A5732H4J72Q@mail.gmail.com>
On Thu, Aug 29, 2013 at 2:35 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> Hi Kees,
>
> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> From: Kees Cook <keescook@chromium.org>
>>
>> Many drivers need to validate the characteristics of their HID report
>> during initialization to avoid misusing the reports. This adds a common
>> helper to perform validation of the report, its field count, and the
>> value count within the fields.
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@kernel.org
>> ---
>> drivers/hid/hid-core.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/hid.h | 4 ++++
>> 2 files changed, 54 insertions(+)
>>
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index 5ea7d51..55798b2 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -759,6 +759,56 @@ int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size)
>> }
>> EXPORT_SYMBOL_GPL(hid_parse_report);
>>
>> +static const char * const hid_report_names[] = {
>> + "HID_INPUT_REPORT",
>> + "HID_OUTPUT_REPORT",
>> + "HID_FEATURE_REPORT",
>> +};
>> +/**
>> + * hid_validate_report - validate existing device report
>> + *
>> + * @device: hid device
>> + * @type: which report type to examine
>> + * @id: which report ID to examine (0 for first)
>> + * @fields: expected number of fields
>> + * @report_counts: expected number of values per field
>> + *
>> + * Validate the report details after parsing.
>> + */
>> +struct hid_report *hid_validate_report(struct hid_device *hid,
>> + unsigned int type, unsigned int id,
>
> You should consider having an u8 for id, or at least add a check
> against id >= HID_MAX_IDS
Right, as you saw in later email, I added this check globally in the
first patch.
>
>> + unsigned int fields,
>> + unsigned int report_counts)
>> +{
>> + struct hid_report *report;
>> + unsigned int i;
>> +
>> + if (type > HID_FEATURE_REPORT) {
>> + hid_err(hid, "invalid HID report %u\n", type);
>> + return NULL;
>> + }
>> +
>> + report = hid->report_enum[type].report_id_hash[id];
>
> for code readability, and better checking, I'd prefer use
> hid_get_report() here instead. Or just add an inlined accessor in
> hid.h to retrieve the report from the id as many drivers are using the
> report_id_hash directly.
I did not do this because hid_get_report() examines ->numbered and I
explicitly didn't want that limitation since most of these drivers are
blinding accessing the arrays by id number. If the structure members
were opaque and all the drivers were forced to use the access
functions, sure, we'd be fine.
>
>> + if (!report) {
>> + hid_err(hid, "missing %s %u\n", hid_report_names[type], id);
>> + return NULL;
>> + }
>> + if (report->maxfield < fields) {
>> + hid_err(hid, "not enough fields in %s %u\n",
>> + hid_report_names[type], id);
>> + return NULL;
>> + }
>> + for (i = 0; i < fields; i++) {
>> + if (report->field[i]->report_count < report_counts) {
>
> This is wrong.
> you can have a per field different report_count, and it is perfectly
> correct. So the only validate value for report_counts would be 1.
> Otherwise, if you want to provide better checking, you need to provide
> an array of report_counts, which start beeing a little bit annoying
> for users.
Well, nothing needs multiple differing report_counts in practice, so I
opted to just do it this way since nothing calling the function needs
more granular checking.
-Kees
>
>> + hid_err(hid, "not enough values in %s %u fields\n",
>> + hid_report_names[type], id);
>> + return NULL;
>> + }
>> + }
>> + return report;
>> +}
>> +EXPORT_SYMBOL_GPL(hid_validate_report);
>> +
>> /**
>> * hid_open_report - open a driver-specific device report
>> *
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index ff545cc..76e41d8 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -749,6 +749,10 @@ void hid_output_report(struct hid_report *report, __u8 *data);
>> struct hid_device *hid_allocate_device(void);
>> struct hid_report *hid_register_report(struct hid_device *device, unsigned type, unsigned id);
>> int hid_parse_report(struct hid_device *hid, __u8 *start, unsigned size);
>> +struct hid_report *hid_validate_report(struct hid_device *hid,
>> + unsigned int type, unsigned int id,
>> + unsigned int fields,
>> + unsigned int report_counts);
>> int hid_open_report(struct hid_device *device);
>> int hid_check_keys_pressed(struct hid_device *hid);
>> int hid_connect(struct hid_device *hid, unsigned int connect_mask);
>>
>> --
>> Jiri Kosina
>> SUSE Labs
>> --
>
> Cheers,
> Benjamin
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Kees Cook @ 2013-08-29 19:47 UTC (permalink / raw)
To: Srinivas Pandruvada
Cc: Jiri Kosina, linux-input, Mika Westerberg, srinivas pandruvada
In-Reply-To: <521F8F3A.7050002@linux.intel.com>
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
* Re: [PATCH 11/14] HID: multitouch: validate feature report details
From: Kees Cook @ 2013-08-29 19:41 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input, Henrik Rydberg
In-Reply-To: <521F0D5F.7040108@redhat.com>
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++) {
>>
>
--
Kees Cook
Chrome OS Security
^ permalink raw reply
* Re: [PATCH 12/14] HID: sensor-hub: validate feature report details
From: Srinivas Pandruvada @ 2013-08-29 18:13 UTC (permalink / raw)
To: Kees Cook; +Cc: Jiri Kosina, linux-input, Mika Westerberg, srinivas pandruvada
In-Reply-To: <CAGXu5jL2vrO5NR+Bm+HeWa0WihwN4nAr5b=Fga9QE8WW2Wjt6w@mail.gmail.com>
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
* [git pull] Input updates for 3.11-rc7
From: Dmitry Torokhov @ 2013-08-29 17:31 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-input
[-- Attachment #1: Type: text/plain, Size: 1114 bytes --]
Hi Linus,
Please pull from:
git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
or
master.kernel.org:/pub/scm/linux/kernel/git/dtor/input.git for-linus
to receive updates for the input subsystem. Just a couple of new IDs in
Wacom and xpad drivers, i8042 is now disabled on ARC, and data checks
in Elantech driver that were overly relaxed by the previous patch are
now tightened.
Changelog:
---------
Mag (1):
Input: xpad - add signature for Razer Onza Classic Edition
Matteo Delfino (1):
Input: elantech - fix packet check for v3 and v4 hardware
Mischa Jonker (1):
Input: i8042 - disable the driver on ARC platforms
Ping Cheng (1):
Input: wacom - add support for 0x300 and 0x301
Diffstat:
--------
drivers/input/joystick/xpad.c | 1 +
drivers/input/mouse/elantech.c | 44 ++++++++++++++++++++++++++++++++++++----
drivers/input/mouse/elantech.h | 1 +
drivers/input/serio/Kconfig | 3 ++-
drivers/input/tablet/wacom_wac.c | 10 ++++++++-
5 files changed, 53 insertions(+), 6 deletions(-)
--
Dmitry
[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* Re: [PATCH v3 0/3] Input: omap-keypad: Wakeup capability and w/a for i689 errata.
From: Dmitry Torokhov @ 2013-08-29 16:34 UTC (permalink / raw)
To: Illia Smyrnov; +Cc: linux-input, linux-kernel, linux-omap, Felipe Balbi
In-Reply-To: <521C7A85.6000504@ti.com>
Hi Illia,
On Tue, Aug 27, 2013 at 01:08:05PM +0300, Illia Smyrnov wrote:
> Hello Dmitry,
>
> could you take reviewed patches from this patchset?
>
> Reviewed patches:
> [PATCH v3 1/3] Input: omap-keypad: Enable wakeup capability for keypad.
> [PATCH v3 3/3] Input: omap-keypad: Setup irq type from DT
> are not depend on
> [PATCH v3 2/3] Input: omap-keypad: errata i689: Correct debounce time
>
> Thanks.
Yes, they have been applied.
Thanks.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] input/serio: disable i8042 PC keyboard ctrl for ARC
From: Dmitry Torokhov @ 2013-08-29 16:33 UTC (permalink / raw)
To: Mischa Jonker
Cc: Heiko Carstens, Greg Kroah-Hartman, linux-input, linux-kernel
In-Reply-To: <1377715616-2548-1-git-send-email-mjonker@synopsys.com>
On Wed, Aug 28, 2013 at 08:46:56PM +0200, Mischa Jonker wrote:
> It causes crashes when enabled, and we don't have such a peripheral
> anyway on ARC platforms.
>
> Signed-off-by: Mischa Jonker <mjonker@synopsys.com>
> ---
> drivers/input/serio/Kconfig | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/input/serio/Kconfig b/drivers/input/serio/Kconfig
> index 94c17c2..1e691a3 100644
> --- a/drivers/input/serio/Kconfig
> +++ b/drivers/input/serio/Kconfig
> @@ -22,7 +22,8 @@ config SERIO_I8042
> tristate "i8042 PC Keyboard controller" if EXPERT || !X86
> default y
> depends on !PARISC && (!ARM || ARCH_SHARK || FOOTBRIDGE_HOST) && \
> - (!SUPERH || SH_CAYMAN) && !M68K && !BLACKFIN && !S390
> + (!SUPERH || SH_CAYMAN) && !M68K && !BLACKFIN && !S390 && \
> + !ARC
> help
> i8042 is the chip over which the standard AT keyboard and PS/2
> mouse are connected to the computer. If you use these devices,
Applied, thank you.
--
Dmitry
^ permalink raw reply
* Re: [PATCH] Input: add driver for Neonode zForce based touchscreens
From: Dmitry Torokhov @ 2013-08-29 16:29 UTC (permalink / raw)
To: Heiko Stübner; +Cc: Henrik Rydberg, linux-kernel, linux-input
In-Reply-To: <201308161359.40134.heiko@sntech.de>
Hi Heiko,
On Fri, Aug 16, 2013 at 01:59:39PM +0200, Heiko Stübner wrote:
> This adds a driver for touchscreens using the zforce infrared
> technology from Neonode connected via i2c to the host system.
>
> It supports multitouch with up to two fingers and tracking of the
> contacts in hardware.
Generally looks good, just a few comments...
>
> Signed-off-by: Heiko Stuebner <heiko@sntech.de>
> ---
> drivers/input/touchscreen/Kconfig | 13 +
> drivers/input/touchscreen/Makefile | 1 +
> drivers/input/touchscreen/zforce_ts.c | 837 +++++++++++++++++++++++++++++++++
> include/linux/input/zforce_ts.h | 26 +
> 4 files changed, 877 insertions(+)
> create mode 100644 drivers/input/touchscreen/zforce_ts.c
> create mode 100644 include/linux/input/zforce_ts.h
>
> diff --git a/drivers/input/touchscreen/Kconfig b/drivers/input/touchscreen/Kconfig
> index 3b9758b..ade11b7 100644
> --- a/drivers/input/touchscreen/Kconfig
> +++ b/drivers/input/touchscreen/Kconfig
> @@ -919,4 +919,17 @@ config TOUCHSCREEN_TPS6507X
> To compile this driver as a module, choose M here: the
> module will be called tps6507x_ts.
>
> +config TOUCHSCREEN_ZFORCE
> + tristate "Neonode zForce infrared touchscreens"
> + depends on I2C
> + depends on GPIOLIB
> + help
> + Say Y here if you have a touchscreen using the zforce
> + infraread technology from Neonode.
> +
> + If unsure, say N.
> +
> + To compile this driver as a module, choose M here: the
> + module will be called zforce_ts.
> +
> endif
> diff --git a/drivers/input/touchscreen/Makefile b/drivers/input/touchscreen/Makefile
> index f5216c1..7587883 100644
> --- a/drivers/input/touchscreen/Makefile
> +++ b/drivers/input/touchscreen/Makefile
> @@ -75,3 +75,4 @@ obj-$(CONFIG_TOUCHSCREEN_WM97XX_MAINSTONE) += mainstone-wm97xx.o
> obj-$(CONFIG_TOUCHSCREEN_WM97XX_ZYLONITE) += zylonite-wm97xx.o
> obj-$(CONFIG_TOUCHSCREEN_W90X900) += w90p910_ts.o
> obj-$(CONFIG_TOUCHSCREEN_TPS6507X) += tps6507x-ts.o
> +obj-$(CONFIG_TOUCHSCREEN_ZFORCE) += zforce_ts.o
> diff --git a/drivers/input/touchscreen/zforce_ts.c b/drivers/input/touchscreen/zforce_ts.c
> new file mode 100644
> index 0000000..92af632
> --- /dev/null
> +++ b/drivers/input/touchscreen/zforce_ts.c
> @@ -0,0 +1,837 @@
> +/*
> + * Copyright (C) 2012-2013 MundoReader S.L.
> + * Author: Heiko Stuebner <heiko@sntech.de>
> + *
> + * based in parts on Nook zforce driver
> + *
> + * Copyright (C) 2010 Barnes & Noble, Inc.
> + * Author: Pieter Truter<ptruter@intrinsyc.com>
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#include <linux/module.h>
> +#include <linux/hrtimer.h>
> +#include <linux/slab.h>
> +#include <linux/input.h>
> +#include <linux/interrupt.h>
> +#include <linux/i2c.h>
> +#include <linux/delay.h>
> +#include <linux/gpio.h>
> +#include <linux/device.h>
> +#include <linux/sysfs.h>
> +#include <linux/input/zforce_ts.h>
> +#include <linux/input/mt.h>
> +
> +#define WAIT_TIMEOUT msecs_to_jiffies(1000)
> +
> +#define FRAME_START 0xee
> +
> +/* Offsets of the different parts of the payload the controller sends */
> +#define PAYLOAD_HEADER 0
> +#define PAYLOAD_LENGTH 1
> +#define PAYLOAD_BODY 2
> +
> +/* Response offsets */
> +#define RESPONSE_ID 0
> +#define RESPONSE_DATA 1
> +
> +/* Commands */
> +#define COMMAND_DEACTIVATE 0x00
> +#define COMMAND_INITIALIZE 0x01
> +#define COMMAND_RESOLUTION 0x02
> +#define COMMAND_SETCONFIG 0x03
> +#define COMMAND_DATAREQUEST 0x04
> +#define COMMAND_SCANFREQ 0x08
> +#define COMMAND_STATUS 0X1e
> +
> +/*
> + * Responses the controller sends as a result of
> + * command requests
> + */
> +#define RESPONSE_DEACTIVATE 0x00
> +#define RESPONSE_INITIALIZE 0x01
> +#define RESPONSE_RESOLUTION 0x02
> +#define RESPONSE_SETCONFIG 0x03
> +#define RESPONSE_SCANFREQ 0x08
> +#define RESPONSE_STATUS 0X1e
> +
> +/*
> + * Notifications are send by the touch controller without
> + * being requested by the driver and include for example
> + * touch indications
> + */
> +#define NOTIFICATION_TOUCH 0x04
> +#define NOTIFICATION_BOOTCOMPLETE 0x07
> +#define NOTIFICATION_OVERRUN 0x25
> +#define NOTIFICATION_PROXIMITY 0x26
> +#define NOTIFICATION_INVALID_COMMAND 0xfe
> +
> +#define ZFORCE_REPORT_POINTS 2
> +#define ZFORCE_MAX_AREA 0xff
> +
> +#define STATE_DOWN 0
> +#define STATE_MOVE 1
> +#define STATE_UP 2
> +
> +#define SETCONFIG_DUALTOUCH (1 << 0)
> +
> +struct zforce_point {
> + int coord_x;
> + int coord_y;
> + int state;
> + int id;
> + int area_major;
> + int area_minor;
> + int orientation;
> + int pressure;
> + int prblty;
> +};
> +
> +/*
> + * @client the i2c_client
> + * @input the input device
> + * @suspending in the process of going to suspend (don't emit wakeup
> + * events for commands executed to suspend the device)
> + * @suspended device suspended
> + * @access_mutex serialize i2c-access, to keep multipart reads together
> + * @command_done completion to wait for the command result
> + * @command_mutex serialize commands send to the ic
> + * @command_waiting the id of the command that that is currently waiting
> + * for a result
> + * @command_result returned result of the command
> + */
> +struct zforce_ts {
> + struct i2c_client *client;
> + struct input_dev *input;
> + const struct zforce_ts_platdata *pdata;
> + char phys[32];
> +
> + bool suspending;
> + bool suspended;
> + bool boot_complete;
> +
> + /* Firmware version information */
> + u16 version_major;
> + u16 version_minor;
> + u16 version_build;
> + u16 version_rev;
> +
> + struct mutex access_mutex;
> +
> + struct completion command_done;
> + struct mutex command_mutex;
> + int command_waiting;
> + int command_result;
> +};
> +
> +static int zforce_command(struct zforce_ts *ts, u8 cmd)
> +{
> + struct i2c_client *client = ts->client;
> + char buf[3];
> + int ret;
> +
> + dev_dbg(&client->dev, "%s: 0x%x\n", __func__, cmd);
> +
> + buf[0] = FRAME_START;
> + buf[1] = 1; /* data size, command only */
> + buf[2] = cmd;
> +
> + mutex_lock(&ts->access_mutex);
> + ret = i2c_master_send(client, &buf[0], ARRAY_SIZE(buf));
> + mutex_unlock(&ts->access_mutex);
I am unsure why you need this lock. Doesn't i2c core already ensure
necessary locking?
Also, it does not look like zforec_command() will reace with your
interrupt handler that does multi-reads...
> + if (ret < 0) {
> + dev_err(&client->dev, "i2c send data request error: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int zforce_send_wait(struct zforce_ts *ts, const char *buf, int len)
> +{
> + struct i2c_client *client = ts->client;
> + int ret;
> +
> + ret = mutex_trylock(&ts->command_mutex);
> + if (!ret) {
> + dev_err(&client->dev, "already waiting for a command\n");
> + return -EBUSY;
> + }
> +
> + dev_dbg(&client->dev, "sending %d bytes for command 0x%x\n",
> + buf[1], buf[2]);
> +
> + ts->command_waiting = buf[2];
> +
> + mutex_lock(&ts->access_mutex);
> + ret = i2c_master_send(client, buf, len);
> + mutex_unlock(&ts->access_mutex);
Same here.
> + if (ret < 0) {
> + dev_err(&client->dev, "i2c send data request error: %d\n", ret);
> + goto unlock;
> + }
> +
> + dev_dbg(&client->dev, "waiting for result for command 0x%x\n", buf[2]);
> +
> + if (wait_for_completion_timeout(&ts->command_done, WAIT_TIMEOUT) == 0) {
> + ret = -ETIME;
> + goto unlock;
> + }
> +
> + ret = ts->command_result;
> +
> +unlock:
> + mutex_unlock(&ts->command_mutex);
> + return ret;
> +}
> +
> +static int zforce_command_wait(struct zforce_ts *ts, u8 cmd)
> +{
> + struct i2c_client *client = ts->client;
> + char buf[3];
> + int ret;
> +
> + dev_dbg(&client->dev, "%s: 0x%x\n", __func__, cmd);
> +
> + buf[0] = FRAME_START;
> + buf[1] = 1; /* data size, command only */
> + buf[2] = cmd;
> +
> + ret = zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));
> + if (ret < 0) {
> + dev_err(&client->dev, "i2c send data request error: %d\n", ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int zforce_resolution(struct zforce_ts *ts, u16 x, u16 y)
> +{
> + struct i2c_client *client = ts->client;
> + char buf[7] = { FRAME_START, 5, COMMAND_RESOLUTION,
> + (x & 0xff), ((x >> 8) & 0xff),
> + (y & 0xff), ((y >> 8) & 0xff) };
> +
> + dev_dbg(&client->dev, "set resolution to (%d,%d)\n", x, y);
> +
> + return zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));
> +}
> +
> +static int zforce_scan_frequency(struct zforce_ts *ts, u16 idle, u16 finger,
> + u16 stylus)
> +{
> + struct i2c_client *client = ts->client;
> + char buf[9] = { FRAME_START, 7, COMMAND_SCANFREQ,
> + (idle & 0xff), ((idle >> 8) & 0xff),
> + (finger & 0xff), ((finger >> 8) & 0xff),
> + (stylus & 0xff), ((stylus >> 8) & 0xff) };
> +
> + dev_dbg(&client->dev, "set scan frequency to (idle: %d, finger: %d, stylus: %d)\n",
> + idle, finger, stylus);
> +
> + return zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));
> +}
> +
> +static int zforce_setconfig(struct zforce_ts *ts, char b1)
> +{
> + struct i2c_client *client = ts->client;
> + char buf[7] = { FRAME_START, 5, COMMAND_SETCONFIG,
> + b1, 0, 0, 0 };
> +
> + dev_dbg(&client->dev, "set config to (%d)\n", b1);
> +
> + return zforce_send_wait(ts, &buf[0], ARRAY_SIZE(buf));
> +}
> +
> +static int zforce_start(struct zforce_ts *ts)
> +{
> + struct i2c_client *client = ts->client;
> + const struct zforce_ts_platdata *pdata = client->dev.platform_data;
> + int ret;
> +
> + dev_dbg(&client->dev, "starting device\n");
> +
> + ret = zforce_command_wait(ts, COMMAND_INITIALIZE);
> + if (ret) {
> + dev_err(&client->dev, "Unable to initialize, %d\n", ret);
> + return ret;
> + }
> +
> + ret = zforce_resolution(ts, pdata->x_max, pdata->y_max);
> + if (ret) {
> + dev_err(&client->dev, "Unable to set resolution, %d\n", ret);
> + goto error;
> + }
> +
> + ret = zforce_scan_frequency(ts, 10, 50, 50);
> + if (ret) {
> + dev_err(&client->dev, "Unable to set scan frequency, %d\n",
> + ret);
> + goto error;
> + }
> +
> + if (zforce_setconfig(ts, SETCONFIG_DUALTOUCH)) {
> + dev_err(&client->dev, "Unable to set config\n");
> + goto error;
> + }
> +
> + /* start sending touch events */
> + ret = zforce_command(ts, COMMAND_DATAREQUEST);
> + if (ret) {
> + dev_err(&client->dev, "Unable to request data\n");
> + goto error;
> + }
> +
> + /* Per NN, initial cal. take max. of 200msec.
> + * Allow time to complete this calibration
> + */
> + msleep(200);
> +
> + return 0;
> +
> +error:
> + zforce_command_wait(ts, COMMAND_DEACTIVATE);
> + return ret;
> +}
> +
> +static int zforce_stop(struct zforce_ts *ts)
> +{
> + struct i2c_client *client = ts->client;
> + int ret;
> +
> + dev_dbg(&client->dev, "stopping device\n");
> +
> + /* deactivates touch sensing and puts the device into sleep */
> + ret = zforce_command_wait(ts, COMMAND_DEACTIVATE);
> + if (ret != 0) {
> + dev_err(&client->dev, "could not deactivate device, %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static int zforce_touch_event(struct zforce_ts *ts, u8 *payload)
> +{
> + struct i2c_client *client = ts->client;
> + struct zforce_point point[ZFORCE_REPORT_POINTS];
> + const struct zforce_ts_platdata *pdata = client->dev.platform_data;
dev_get_plantdata()
> + int count, i;
> +
> + count = payload[0];
> + if (count > ZFORCE_REPORT_POINTS) {
> + dev_warn(&client->dev, "to many coordinates %d, expected max %d\n",
> + count, ZFORCE_REPORT_POINTS);
> + count = ZFORCE_REPORT_POINTS;
> + }
> +
> + for (i = 0; i < count; i++) {
> + point[i].coord_x =
> + payload[9 * i + 2] << 8 | payload[9 * i + 1];
> + point[i].coord_y =
> + payload[9 * i + 4] << 8 | payload[9 * i + 3];
> +
> + if (point[i].coord_x > pdata->x_max ||
> + point[i].coord_y > pdata->y_max) {
> + dev_warn(&client->dev, "coordinates (%d,%d) invalid\n",
> + point[i].coord_x, point[i].coord_y);
> + point[i].coord_x = point[i].coord_y = 0;
> + }
> +
> + point[i].state = payload[9 * i + 5] & 0x03;
> + point[i].id = (payload[9 * i + 5] & 0xfc) >> 2;
> +
> + /* determine touch major, minor and orientation */
> + point[i].area_major = max(payload[9 * i + 6],
> + payload[9 * i + 7]);
> + point[i].area_minor = min(payload[9 * i + 6],
> + payload[9 * i + 7]);
> + point[i].orientation = payload[9 * i + 6] > payload[9 * i + 7];
> +
> + point[i].pressure = payload[9 * i + 8];
> + point[i].prblty = payload[9 * i + 9];
> + }
> +
> + for (i = 0; i < count; i++) {
> + dev_dbg(&client->dev, "point %d/%d: state %d, id %d, pressure %d, prblty %d, x %d, y %d, amajor %d, aminor %d, ori %d\n",
> + i, count, point[i].state, point[i].id,
> + point[i].pressure, point[i].prblty,
> + point[i].coord_x, point[i].coord_y,
> + point[i].area_major, point[i].area_minor,
> + point[i].orientation);
> +
> + /* the zforce id starts with "1", so needs to be decreased */
> + input_mt_slot(ts->input, point[i].id - 1);
> +
> + input_mt_report_slot_state(ts->input, MT_TOOL_FINGER,
> + point[i].state != STATE_UP);
> +
> + if (point[i].state != STATE_UP) {
> + input_report_abs(ts->input, ABS_MT_POSITION_X,
> + point[i].coord_x);
> + input_report_abs(ts->input, ABS_MT_POSITION_Y,
> + point[i].coord_y);
> + input_report_abs(ts->input, ABS_MT_TOUCH_MAJOR,
> + point[i].area_major);
> + input_report_abs(ts->input, ABS_MT_TOUCH_MINOR,
> + point[i].area_minor);
> + input_report_abs(ts->input, ABS_MT_ORIENTATION,
> + point[i].orientation);
> + }
> + }
> +
> + if (point[0].state != STATE_UP) {
> + input_report_abs(ts->input, ABS_X, point[0].coord_x);
> + input_report_abs(ts->input, ABS_Y, point[0].coord_y);
> + }
> +
> + input_report_key(ts->input, BTN_TOUCH, point[0].state != STATE_UP);
> +
> + input_sync(ts->input);
> +
> + return 0;
> +}
> +
> +static int zforce_read_packet(struct zforce_ts *ts, u8 *buf)
> +{
> + struct i2c_client *client = ts->client;
> + int ret;
> +
> + mutex_lock(&ts->access_mutex);
> +
> + /* read 2 byte message header */
> + ret = i2c_master_recv(client, buf, 2);
> + if (ret < 0) {
> + dev_err(&client->dev, "error reading header: %d\n", ret);
> + goto unlock;
> + }
> +
> + if (buf[PAYLOAD_HEADER] != FRAME_START) {
> + dev_err(&client->dev, "invalid frame start: %d\n", buf[0]);
> + ret = -EIO;
> + goto unlock;
> + }
> +
> + if (buf[PAYLOAD_LENGTH] <= 0 || buf[PAYLOAD_LENGTH] > 255) {
> + dev_err(&client->dev, "invalid payload length: %d\n",
> + buf[PAYLOAD_LENGTH]);
> + ret = -EIO;
> + goto unlock;
> + }
> +
> + /* read the message */
> + ret = i2c_master_recv(client, &buf[PAYLOAD_BODY], buf[PAYLOAD_LENGTH]);
> + if (ret < 0) {
> + dev_err(&client->dev, "error reading payload: %d\n", ret);
> + goto unlock;
> + }
> +
> + dev_dbg(&client->dev, "read %d bytes for response command 0x%x\n",
> + buf[PAYLOAD_LENGTH], buf[PAYLOAD_BODY]);
> +
> +unlock:
> + mutex_unlock(&ts->access_mutex);
> + return ret;
> +}
> +
> +static void zforce_complete(struct zforce_ts *ts, int cmd, int result)
> +{
> + struct i2c_client *client = ts->client;
> +
> + if (ts->command_waiting == cmd) {
> + dev_dbg(&client->dev, "completing command 0x%x\n", cmd);
> + ts->command_result = result;
> + complete(&ts->command_done);
> + } else {
> + dev_dbg(&client->dev, "command %d not for us\n", cmd);
> + }
> +}
> +
> +static irqreturn_t zforce_interrupt(int irq, void *dev_id)
> +{
> + struct zforce_ts *ts = dev_id;
> + struct i2c_client *client = ts->client;
> + const struct zforce_ts_platdata *pdata = client->dev.platform_data;
> + int ret;
> + u8 payload_buffer[512];
> + u8 *payload;
> +
> + /*
> + * When suspended, emit a wakeup signal if necessary and return.
> + * Due to the level-interrupt we will get re-triggered later.
> + */
> + if (ts->suspended) {
> + if (device_may_wakeup(&client->dev))
> + pm_wakeup_event(&client->dev, 500);
> + msleep(20);
> + return IRQ_HANDLED;
> + }
> +
> + dev_dbg(&client->dev, "handling interrupt\n");
> +
> + /* Don't emit wakeup events from commands run by zforce_suspend */
> + if (!ts->suspending && device_may_wakeup(&client->dev))
> + pm_stay_awake(&client->dev);
> +
> + while (!gpio_get_value(pdata->gpio_int)) {
> + ret = zforce_read_packet(ts, payload_buffer);
> + if (ret < 0) {
> + dev_err(&client->dev, "could not read packet, ret: %d\n",
> + ret);
> + break;
> + }
> +
> + payload = &payload_buffer[PAYLOAD_BODY];
> +
> + switch (payload[RESPONSE_ID]) {
> + case NOTIFICATION_TOUCH:
> + /* Always report touch-events received while
> + * suspending, when being a wakeup source
> + */
> + if (ts->suspending && device_may_wakeup(&client->dev))
> + pm_wakeup_event(&client->dev, 500);
> + zforce_touch_event(ts, &payload[RESPONSE_DATA]);
> + break;
> + case NOTIFICATION_BOOTCOMPLETE:
> + ts->boot_complete = payload[RESPONSE_DATA];
> + zforce_complete(ts, payload[RESPONSE_ID], 0);
> + break;
> + case RESPONSE_INITIALIZE:
> + case RESPONSE_DEACTIVATE:
> + case RESPONSE_SETCONFIG:
> + case RESPONSE_RESOLUTION:
> + case RESPONSE_SCANFREQ:
> + zforce_complete(ts, payload[RESPONSE_ID],
> + payload[RESPONSE_DATA]);
> + break;
> + case RESPONSE_STATUS:
> + /* Version Payload Results
> + * [2:major] [2:minor] [2:build] [2:rev]
> + */
> + ts->version_major = (payload[RESPONSE_DATA + 1] << 8) |
> + payload[RESPONSE_DATA];
> + ts->version_minor = (payload[RESPONSE_DATA + 3] << 8) |
> + payload[RESPONSE_DATA + 2];
> + ts->version_build = (payload[RESPONSE_DATA + 5] << 8) |
> + payload[RESPONSE_DATA + 4];
> + ts->version_rev = (payload[RESPONSE_DATA + 7] << 8) |
> + payload[RESPONSE_DATA + 6];
> + dev_dbg(&ts->client->dev, "Firmware Version %04x:%04x %04x:%04x\n",
> + ts->version_major, ts->version_minor,
> + ts->version_build, ts->version_rev);
> +
> + zforce_complete(ts, payload[RESPONSE_ID], 0);
> + break;
> + case NOTIFICATION_INVALID_COMMAND:
> + dev_err(&ts->client->dev, "invalid command: 0x%x\n",
> + payload[RESPONSE_DATA]);
> + break;
> + default:
> + dev_err(&ts->client->dev, "unrecognized response id: 0x%x\n",
> + payload[RESPONSE_ID]);
> + break;
> + }
> + }
> +
> + if (!ts->suspending && device_may_wakeup(&client->dev))
> + pm_relax(&client->dev);
> +
> + dev_dbg(&client->dev, "finished interrupt\n");
> +
> + return IRQ_HANDLED;
> +}
> +
> +static int zforce_input_open(struct input_dev *dev)
> +{
> + struct zforce_ts *ts = input_get_drvdata(dev);
> + int ret;
> +
> + ret = zforce_start(ts);
> + if (ret)
> + return ret;
> +
> + return 0;
> +}
> +
> +static void zforce_input_close(struct input_dev *dev)
> +{
> + struct zforce_ts *ts = input_get_drvdata(dev);
> + struct i2c_client *client = ts->client;
> + int ret;
> +
> + ret = zforce_stop(ts);
> + if (ret)
> + dev_warn(&client->dev, "stopping zforce failed\n");
> +
> + return;
> +}
> +
> +#ifdef CONFIG_PM_SLEEP
> +static int zforce_suspend(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct zforce_ts *ts = i2c_get_clientdata(client);
> + struct input_dev *input = ts->input;
> + int ret = 0;
> +
> + mutex_lock(&input->mutex);
> + ts->suspending = true;
> +
> + /* when configured as wakeup source, device should always wake system
> + * therefore start device if necessary
> + */
> + if (device_may_wakeup(&client->dev)) {
> + dev_dbg(&client->dev, "suspend while being a wakeup source\n");
> +
> + /* need to start device if not open, to be wakeup source */
> + if (!input->users) {
> + ret = zforce_start(ts);
> + if (ret)
> + goto unlock;
> + }
> +
> + enable_irq_wake(client->irq);
> + } else if (input->users) {
> + dev_dbg(&client->dev, "suspend without being a wakeup source\n");
> +
> + ret = zforce_stop(ts);
> + if (ret)
> + goto unlock;
> +
> + disable_irq(client->irq);
> + }
> +
> + ts->suspended = true;
> +
> +unlock:
> + ts->suspending = false;
> + mutex_unlock(&input->mutex);
> +
> + return ret;
> +}
> +
> +static int zforce_resume(struct device *dev)
> +{
> + struct i2c_client *client = to_i2c_client(dev);
> + struct zforce_ts *ts = i2c_get_clientdata(client);
> + struct input_dev *input = ts->input;
> + int ret = 0;
> +
> + mutex_lock(&input->mutex);
> +
> + ts->suspended = false;
> +
> + if (device_may_wakeup(&client->dev)) {
> + dev_dbg(&client->dev, "resume from being a wakeup source\n");
> +
> + disable_irq_wake(client->irq);
> +
> + /* need to stop device if it was not open on suspend */
> + if (!input->users) {
> + ret = zforce_stop(ts);
> + if (ret)
> + goto unlock;
> + }
> + } else if (input->users) {
> + dev_dbg(&client->dev, "resume without being a wakeup source\n");
> +
> + enable_irq(client->irq);
> +
> + ret = zforce_start(ts);
> + if (ret < 0)
> + goto unlock;
> + }
> +
> +unlock:
> + mutex_unlock(&input->mutex);
> +
> + return ret;
> +}
> +#endif
> +
> +static SIMPLE_DEV_PM_OPS(zforce_pm_ops, zforce_suspend, zforce_resume);
> +
> +static void zforce_reset(void *data)
> +{
> + struct zforce_ts *ts = data;
> +
> + gpio_set_value(ts->pdata->gpio_rst, 0);
> +}
> +
> +static int zforce_probe(struct i2c_client *client,
> + const struct i2c_device_id *id)
> +{
> + const struct zforce_ts_platdata *pdata = client->dev.platform_data;
dev_get_platdata()
> + struct zforce_ts *ts;
> + struct input_dev *input_dev;
> + int ret;
> +
> + if (!pdata)
> + return -EINVAL;
> +
> + ts = devm_kzalloc(&client->dev, sizeof(struct zforce_ts), GFP_KERNEL);
> + if (!ts)
> + return -ENOMEM;
> +
> + ret = devm_gpio_request_one(&client->dev, pdata->gpio_int, GPIOF_IN,
> + "zforce_ts_int");
> + if (ret) {
> + dev_err(&client->dev, "request of gpio %d failed, %d\n",
> + pdata->gpio_int, ret);
> + return ret;
> + }
> +
> + ret = devm_gpio_request_one(&client->dev, pdata->gpio_rst,
> + GPIOF_OUT_INIT_LOW, "zforce_ts_rst");
> + if (ret) {
> + dev_err(&client->dev, "request of gpio %d failed, %d\n",
> + pdata->gpio_rst, ret);
> + return ret;
> + }
> +
> + ret = devm_add_action(&client->dev, zforce_reset, ts);
> + if (ret) {
> + dev_err(&client->dev, "failed to register reset action, %d\n",
> + ret);
> + return ret;
> + }
> +
> + msleep(20);
Why are we sleeping here? We have not called zforce_reset() - it will be
called on unwinding only.
> +
> + snprintf(ts->phys, sizeof(ts->phys),
> + "%s/input0", dev_name(&client->dev));
> +
> + input_dev = devm_input_allocate_device(&client->dev);
> + if (!input_dev) {
> + dev_err(&client->dev, "could not allocate input device\n");
> + return -ENOMEM;
> + }
> +
> + mutex_init(&ts->access_mutex);
> + mutex_init(&ts->command_mutex);
> +
> + ts->pdata = pdata;
> + ts->client = client;
> + ts->input = input_dev;
> +
> + input_dev->name = "Neonode zForce touchscreen";
> + input_dev->phys = ts->phys;
> + input_dev->id.bustype = BUS_I2C;
> + input_dev->dev.parent = &client->dev;
> +
> + input_dev->open = zforce_input_open;
> + input_dev->close = zforce_input_close;
> +
> + set_bit(EV_KEY, input_dev->evbit);
> + set_bit(EV_SYN, input_dev->evbit);
> + set_bit(EV_ABS, input_dev->evbit);
> + set_bit(BTN_TOUCH, input_dev->keybit);
> +
> + /* For single touch */
> + input_set_abs_params(input_dev, ABS_X, 0, pdata->x_max, 0, 0);
> + input_set_abs_params(input_dev, ABS_Y, 0, pdata->y_max, 0, 0);
> +
> + /* For multi touch */
> + input_mt_init_slots(input_dev, ZFORCE_REPORT_POINTS, 0);
> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
> + pdata->x_max, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
> + pdata->y_max, 0, 0);
> +
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
> + ZFORCE_MAX_AREA, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MINOR, 0,
> + ZFORCE_MAX_AREA, 0, 0);
> + input_set_abs_params(input_dev, ABS_MT_ORIENTATION, 0, 1, 0, 0);
> +
> + input_set_drvdata(ts->input, ts);
> +
> + init_completion(&ts->command_done);
> +
> + /* The zforce pulls the interrupt low when it has data ready.
> + * After it is triggered the isr thread runs until all the available
> + * packets have been read and the interrupt is high again.
> + * Therefore we can trigger the interrupt anytime it is low and do
> + * not need to limit it to the interrupt edge.
> + */
> + ret = devm_request_threaded_irq(&client->dev, client->irq, NULL,
> + zforce_interrupt,
> + IRQF_TRIGGER_LOW | IRQF_ONESHOT,
> + input_dev->name, ts);
> + if (ret) {
> + dev_err(&client->dev, "irq %d request failed\n", client->irq);
> + return ret;
> + }
> +
> + i2c_set_clientdata(client, ts);
> +
> + /* let the controller boot */
> + gpio_set_value(pdata->gpio_rst, 1);
> +
> + ts->command_waiting = NOTIFICATION_BOOTCOMPLETE;
> + if (wait_for_completion_timeout(&ts->command_done, WAIT_TIMEOUT) == 0)
> + dev_warn(&client->dev, "bootcomplete timed out\n");
> +
> + /* need to start device to get version information */
> + ret = zforce_command_wait(ts, COMMAND_INITIALIZE);
> + if (ret) {
> + dev_err(&client->dev, "unable to initialize, %d\n", ret);
> + return ret;
> + }
> +
> + /* this gets the firmware version among other informations */
> + ret = zforce_command_wait(ts, COMMAND_STATUS);
> + if (ret < 0) {
> + dev_err(&client->dev, "couldn't get status, %d\n", ret);
> + zforce_stop(ts);
> + return ret;
> + }
> +
> + /* stop device and put it into sleep until it is opened */
> + ret = zforce_stop(ts);
> + if (ret < 0)
> + return ret;
> +
> + device_set_wakeup_capable(&client->dev, true);
> +
> + ret = input_register_device(input_dev);
> + if (ret) {
> + dev_err(&client->dev, "could not register input device, %d\n",
> + ret);
> + return ret;
> + }
> +
> + return 0;
> +}
> +
> +static struct i2c_device_id zforce_idtable[] = {
> + { "zforce-ts", 0 },
> + { }
> +};
> +MODULE_DEVICE_TABLE(i2c, zforce_idtable);
> +
> +static struct i2c_driver zforce_driver = {
> + .driver = {
> + .owner = THIS_MODULE,
> + .name = "zforce-ts",
> + .pm = &zforce_pm_ops,
> + },
> + .probe = zforce_probe,
> + .id_table = zforce_idtable,
> +};
> +
> +module_i2c_driver(zforce_driver);
> +
> +MODULE_AUTHOR("Heiko Stuebner <heiko@sntech.de>");
> +MODULE_DESCRIPTION("zForce TouchScreen Driver");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/input/zforce_ts.h b/include/linux/input/zforce_ts.h
> new file mode 100644
> index 0000000..0472ab2
> --- /dev/null
> +++ b/include/linux/input/zforce_ts.h
I think we now have a separate directory for platform data...
> @@ -0,0 +1,26 @@
> +/* drivers/input/touchscreen/zforce.c
> + *
> + * Copyright (C) 2012-2013 MundoReader S.L.
> + *
> + * This software is licensed under the terms of the GNU General Public
> + * License version 2, as published by the Free Software Foundation, and
> + * may be copied, distributed, and modified under those terms.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + */
> +
> +#ifndef _LINUX_INPUT_ZFORCE_TS_H
> +#define _LINUX_INPUT_ZFORCE_TS_H
> +
> +struct zforce_ts_platdata {
> + int gpio_int;
> + int gpio_rst;
> +
> + unsigned int x_max;
> + unsigned int y_max;
> +};
> +
> +#endif /* _LINUX_INPUT_ZFORCE_TS_H */
> --
> 1.7.10.4
>
Thanks.
--
Dmitry
--
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
^ permalink raw reply
* Re: [PATCH 1/1] MAINTAINERS: Change maintainer for cyttsp driver
From: Dmitry Torokhov @ 2013-08-29 15:44 UTC (permalink / raw)
To: Javier Martinez Canillas
Cc: Ferruh Yigit, Henrik Rydberg, ttdrivers, linux-input,
linux-kernel
In-Reply-To: <CABxcv=mOasXUvC301Dn3fGQqo2ipk_oYEaU8ht1VCJiz2bC8Sw@mail.gmail.com>
On Thu, Aug 29, 2013 at 11:48:57AM +0200, Javier Martinez Canillas wrote:
> On Tue, Jul 16, 2013 at 8:57 AM, Ferruh Yigit <fery@cypress.com> wrote:
> >
> > On 07/15/2013 12:41 AM, Javier Martinez Canillas wrote:
> >>
> >> I haven't had time to work on this driver for a long time and
> >> Ferruh has been doing a great job making it more generic,
> >> adding support for new hardware and providing bug fixes.
> >
> > Thank you a lot for your work on cyttsp drivers,
> > we would like to see your support back whenever you have time again.
> > Your expertise/know-how on issue is valuable.
> >
> >
> >>
> >> So, let's make MAINTAINERS reflect reality and add him as the
> >> cyttsp maintainer instead of me.
> >>
> >> Also, since Ferruh works for Cypress, we may change the driver
> >> status from Maintained to Supported.
> >>
> >> Signed-off-by: Javier Martinez Canillas <javier@dowhile0.org>
> >> ---
> >>
> >> Ferruh, please send your ack if you are willing to take over
> >> maintainance of this driver.
> >
> >
> > Acked-by: Ferruh Yigit <fery@cypress.com>
> >
> >
> >>
> >> Also, please confirm that you have been working on behalf of
> >> Cypress instead of doing it on your free time. Otherwise we
> >> should keep the driver status to maintained instead supported.
> >
> > Right, I am a Cypress employee and working on TrueTouch drivers.
> >
> >>
> >> Thanks a lot and best regards,
> >> Javier
> >>
> >> MAINTAINERS | 4 ++--
> >> 1 files changed, 2 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/MAINTAINERS b/MAINTAINERS
> >> index 9d771d9..4ba996c 100644
> >> --- a/MAINTAINERS
> >> +++ b/MAINTAINERS
> >> @@ -2458,9 +2458,9 @@ S: Maintained
> >> F: drivers/media/common/cypress_firmware*
> >> CYTTSP TOUCHSCREEN DRIVER
> >> -M: Javier Martinez Canillas <javier@dowhile0.org>
> >> +M: Ferruh Yigit <fery@cypress.com>
> >> L: linux-input@vger.kernel.org
> >> -S: Maintained
> >> +S: Supported
> >> F: drivers/input/touchscreen/cyttsp*
> >> F: include/linux/input/cyttsp.h
> >>
> >
> >
> >
> > --
> >
> > Regards,
> > ferruh
> >
>
> Hi Dmitry and Henrik,
>
> Any comments about this patch? It was sent a couple of months ago...
Sorry, missed it. Applied now.
--
Dmitry
^ permalink raw reply
* Re: [PATCH 10/14] HID: ntrig: validate feature report details
From: Rafi Rubin @ 2013-08-29 15:12 UTC (permalink / raw)
To: Jiri Kosina; +Cc: linux-input, Kees Cook
In-Reply-To: <alpine.LNX.2.00.1308282221210.22181@pobox.suse.cz>
Signed-off-by: Rafi Rubin <rafi@seas.upenn.edu>
Thanks Kees,
Rafi
On 08/28/13 16:31, Jiri Kosina wrote:
> From: Kees Cook <keescook@chromium.org>
>
> A HID device could send a malicious feature report that would cause the
> ntrig HID driver to trigger a NULL dereference during initialization:
>
> [57383.031190] usb 3-1: New USB device found, idVendor=1b96, idProduct=0001
> ...
> [57383.315193] BUG: unable to handle kernel NULL pointer dereference at 0000000000000030
> [57383.315308] IP: [<ffffffffa08102de>] ntrig_probe+0x25e/0x420 [hid_ntrig]
>
> CVE-2013-2896
>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> Cc: stable@kernel.org
> ---
> drivers/hid/hid-ntrig.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/hid/hid-ntrig.c b/drivers/hid/hid-ntrig.c
> index ef95102..5482156 100644
> --- a/drivers/hid/hid-ntrig.c
> +++ b/drivers/hid/hid-ntrig.c
> @@ -115,7 +115,8 @@ static inline int ntrig_get_mode(struct hid_device *hdev)
> struct hid_report *report = hdev->report_enum[HID_FEATURE_REPORT].
> report_id_hash[0x0d];
>
> - if (!report)
> + if (!report || report->maxfield < 1 ||
> + report->field[0]->report_count < 1)
> return -EINVAL;
>
> hid_hw_request(hdev, report, HID_REQ_GET_REPORT);
>
^ permalink raw reply
* Re: [PATCH 2/4] hid-sensor-hub: fix indentation accross the code
From: Andy Shevchenko @ 2013-08-29 14:53 UTC (permalink / raw)
To: Srinivas Pandruvada; +Cc: Jiri Kosina, linux-input, Srinivas Pandruvada
In-Reply-To: <52123982.5020704@linux.intel.com>
On Mon, 2013-08-19 at 08:28 -0700, Srinivas Pandruvada wrote:
> On 08/14/2013 01:07 AM, Andy Shevchenko wrote:
> > Patch just rearranges lines to be more compact and/or readable. Additionally it
> > converts double space to one in several places.
> >
> > There is no functional change.
Jiri, anything about this patch and the rest of the series?
It seems you applied only patch 1/4. Do you have comments?
> >
> > Signed-off-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> > ---
> > drivers/hid/hid-sensor-hub.c | 31 +++++++++++++++----------------
> > 1 file changed, 15 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
> > index ffc80cf..d0687d0 100644
> > --- a/drivers/hid/hid-sensor-hub.c
> > +++ b/drivers/hid/hid-sensor-hub.c
> > @@ -103,8 +103,7 @@ static int sensor_hub_get_physical_device_count(
> >
> > list_for_each_entry(report, &report_enum->report_list, list) {
> > field = report->field[0];
> > - if (report->maxfield && field &&
> > - field->physical)
> > + if (report->maxfield && field && field->physical)
> > cnt++;
> > }
> >
> > @@ -192,12 +191,12 @@ int sensor_hub_set_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> > u32 field_index, s32 value)
> > {
> > struct hid_report *report;
> > - struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > int ret = 0;
> >
> > 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)) {
> > ret = -EINVAL;
> > goto done_proc;
> > }
> > @@ -216,12 +215,12 @@ int sensor_hub_get_feature(struct hid_sensor_hub_device *hsdev, u32 report_id,
> > u32 field_index, s32 *value)
> > {
> > struct hid_report *report;
> > - struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > int ret = 0;
> >
> > 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)) {
> > ret = -EINVAL;
> > goto done_proc;
> > }
> > @@ -241,7 +240,7 @@ int sensor_hub_input_attr_get_raw_value(struct hid_sensor_hub_device *hsdev,
> > u32 usage_id,
> > u32 attr_usage_id, u32 report_id)
> > {
> > - struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > + struct sensor_hub_data *data = hid_get_drvdata(hsdev->hdev);
> > unsigned long flags;
> > struct hid_report *report;
> > int ret_val = 0;
> > @@ -302,7 +301,7 @@ int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
> >
> > /* Initialize with defaults */
> > info->usage_id = usage_id;
> > - info->attrib_id = attr_usage_id;
> > + info->attrib_id = attr_usage_id;
> > info->report_id = -1;
> > info->index = -1;
> > info->units = -1;
> > @@ -333,7 +332,7 @@ int sensor_hub_input_get_attribute_info(struct hid_sensor_hub_device *hsdev,
> > if (field->usage[j].hid ==
> > attr_usage_id &&
> > field->usage[j].collection_index ==
> > - collection_index) {
> > + collection_index) {
> > sensor_hub_fill_attr_info(info,
> > i, report->id,
> > field->unit,
> > @@ -357,7 +356,7 @@ EXPORT_SYMBOL_GPL(sensor_hub_input_get_attribute_info);
> > #ifdef CONFIG_PM
> > static int sensor_hub_suspend(struct hid_device *hdev, pm_message_t message)
> > {
> > - struct sensor_hub_data *pdata = hid_get_drvdata(hdev);
> > + struct sensor_hub_data *pdata = hid_get_drvdata(hdev);
> > struct hid_sensor_hub_callbacks_list *callback;
> >
> > hid_dbg(hdev, " sensor_hub_suspend\n");
> > @@ -374,7 +373,7 @@ static int sensor_hub_suspend(struct hid_device *hdev, pm_message_t message)
> >
> > static int sensor_hub_resume(struct hid_device *hdev)
> > {
> > - struct sensor_hub_data *pdata = hid_get_drvdata(hdev);
> > + struct sensor_hub_data *pdata = hid_get_drvdata(hdev);
> > struct hid_sensor_hub_callbacks_list *callback;
> >
> > hid_dbg(hdev, " sensor_hub_resume\n");
> > @@ -394,6 +393,7 @@ static int sensor_hub_reset_resume(struct hid_device *hdev)
> > return 0;
> > }
> > #endif
> > +
> > /*
> > * Handle raw report as sent by device
> > */
> > @@ -421,7 +421,6 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> > spin_lock_irqsave(&pdata->lock, flags);
> >
> > for (i = 0; i < report->maxfield; ++i) {
> > -
> > hid_dbg(hdev, "%d collection_index:%x hid:%x sz:%x\n",
> > i, report->field[i]->usage->collection_index,
> > report->field[i]->usage->hid,
> > @@ -434,7 +433,7 @@ static int sensor_hub_raw_event(struct hid_device *hdev,
> > pdata->pending.raw_data = kmalloc(sz, GFP_ATOMIC);
> > if (pdata->pending.raw_data) {
> > memcpy(pdata->pending.raw_data, ptr, sz);
> > - pdata->pending.raw_size = sz;
> > + pdata->pending.raw_size = sz;
> > } else
> > pdata->pending.raw_size = 0;
> > complete(&pdata->pending.ready);
> > @@ -539,7 +538,7 @@ static int sensor_hub_probe(struct hid_device *hdev,
> > field->physical) {
> > name = kasprintf(GFP_KERNEL, "HID-SENSOR-%x",
> > field->physical);
> > - if (name == NULL) {
> > + if (name == NULL) {
> > hid_err(hdev, "Failed MFD device name\n");
> > ret = -ENOMEM;
> > goto err_free_names;
> > @@ -617,8 +616,8 @@ static struct hid_driver sensor_hub_driver = {
> > .raw_event = sensor_hub_raw_event,
> > #ifdef CONFIG_PM
> > .suspend = sensor_hub_suspend,
> > - .resume = sensor_hub_resume,
> > - .reset_resume = sensor_hub_reset_resume,
> > + .resume = sensor_hub_resume,
> > + .reset_resume = sensor_hub_reset_resume,
> > #endif
> > };
> > module_hid_driver(sensor_hub_driver);
> Agreed.
>
> Thanks,
> Srinivas
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Benjamin Tissoires @ 2013-08-29 14:50 UTC (permalink / raw)
To: Kees Cook; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAGXu5jLAieQo8FkvEAU6D4c2i388Y3AnH3RNkdsyz3yg7K6sag@mail.gmail.com>
On Thu, Aug 29, 2013 at 4:40 PM, Kees Cook <keescook@chromium.org> wrote:
> On Thu, Aug 29, 2013 at 2:48 AM, Benjamin Tissoires
> <benjamin.tissoires@gmail.com> wrote:
>> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>>> From: Kees Cook <keescook@chromium.org>
>>>
>>> This driver must validate the availability of the HID output report and
>>> its size before it can write LED states via buzz_set_leds(). This stops
>>> a heap overflow that is possible if a device provides a malicious HID
>>> output report:
>>>
>>> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
>>> ...
>>> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>>>
>>> CVE-2013-2890
>>>
>>> Signed-off-by: Kees Cook <keescook@chromium.org>
>>> Cc: stable@kernel.org
>>> ---
>>> drivers/hid/hid-sony.c | 4 ++++
>>> 1 file changed, 4 insertions(+)
>>>
>>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>>> index 87fbe29..b987926 100644
>>> --- a/drivers/hid/hid-sony.c
>>> +++ b/drivers/hid/hid-sony.c
>>> @@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
>>> drv_data = hid_get_drvdata(hdev);
>>> BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
>>>
>>> + /* Validate expected report characteristics. */
>>> + if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
>>
>> I don't have access to the device anymore, but I still kept the report
>> descriptors (this is the interesting part):
>>
>> 0xa1, 0x02, // Collection (Logical) 60
>> 0x75, 0x08, // Report Size (8) 62
>> 0x95, 0x07, // Report Count (7) 64
>> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
>> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
>> 0x09, 0x02, // Usage (Vendor Usage 2) 72
>> 0x91, 0x02, // Output (Data,Var,Abs) 74
>> 0xc0, // End Collection 76
>>
>> So with the current implementation of hid_validate_report(), it works,
>> but if another Buzz controller show up at some point with extras
>> fields in this output report... we will be screwed. So please, amend
>> hid_validate_report(), or specifically test here that the LED output
>> report is 7 bytes.
>
> hid_validate_report() checks for "at least" 7 in this call, so it
> should be fine, unless I've misunderstood something.
>
Sure, it' s fine with the current implementation of
hid_validate_report(). However, as I mentioned in patch
2/14, I am complaining about the implementation of hid_validate_report().
In this case, if a new Buzz controller pops out with an extra usage
(Vendor 3 for instance), mapped to another LED, and that the report
count is for this usage < 7, the test invalidate the report.
for instance, let's imagine they pop up a new version:
0xa1, 0x02, // Collection (Logical) 60
0x75, 0x08, // Report Size (8) 62
0x95, 0x07, // **Report Count (7)** 64
0x46, 0xff, 0x00, // Physical Maximum (255) 66
0x26, 0xff, 0x00, // Logical Maximum (255) 69
0x09, 0x02, // Usage (Vendor Usage 2) 72
0x91, 0x02, // Output (Data,Var,Abs) 74
0x75, 0x08, // Report Size (8) 62
0x95, 0x04, // **Report Count (4)** 64
0x46, 0xff, 0x00, // Physical Maximum (255) 66
0x26, 0xff, 0x00, // Logical Maximum (255) 69
0x09, 0x03, // Usage (Vendor Usage 3) 72
0x91, 0x02, // Output (Data,Var,Abs) 74
0xc0, // End Collection 76
Ok, it's a lot of "if", but still this output report is valid, and the
test will fail. And if we call hid_validate_report(hdev,
HID_OUTPUT_REPORT, 0, 1, 4), the validation will fail, but the heap
overflow will appear again.
Does it makes more sense?
Cheers,
Benjamin
>>> + return -ENODEV;
>>> +
>>> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
>>> if (!buzz) {
>>> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>>>
>>> --
>>> Jiri Kosina
>>> SUSE Labs
>>> --
>>> 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
> Chrome OS Security
^ permalink raw reply
* Re: [PATCH 04/14] HID: sony: validate HID output report details
From: Kees Cook @ 2013-08-29 14:40 UTC (permalink / raw)
To: Benjamin Tissoires; +Cc: Jiri Kosina, linux-input
In-Reply-To: <CAN+gG=F8Y2aoEMr-Zy=JLgT3VT_QGTBi60Hem299p8Sdc-icVg@mail.gmail.com>
On Thu, Aug 29, 2013 at 2:48 AM, Benjamin Tissoires
<benjamin.tissoires@gmail.com> wrote:
> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@suse.cz> wrote:
>> From: Kees Cook <keescook@chromium.org>
>>
>> This driver must validate the availability of the HID output report and
>> its size before it can write LED states via buzz_set_leds(). This stops
>> a heap overflow that is possible if a device provides a malicious HID
>> output report:
>>
>> [ 108.171280] usb 1-1: New USB device found, idVendor=054c, idProduct=0002
>> ...
>> [ 117.507877] BUG kmalloc-192 (Not tainted): Redzone overwritten
>>
>> CVE-2013-2890
>>
>> Signed-off-by: Kees Cook <keescook@chromium.org>
>> Cc: stable@kernel.org
>> ---
>> drivers/hid/hid-sony.c | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/drivers/hid/hid-sony.c b/drivers/hid/hid-sony.c
>> index 87fbe29..b987926 100644
>> --- a/drivers/hid/hid-sony.c
>> +++ b/drivers/hid/hid-sony.c
>> @@ -537,6 +537,10 @@ static int buzz_init(struct hid_device *hdev)
>> drv_data = hid_get_drvdata(hdev);
>> BUG_ON(!(drv_data->quirks & BUZZ_CONTROLLER));
>>
>> + /* Validate expected report characteristics. */
>> + if (!hid_validate_report(hdev, HID_OUTPUT_REPORT, 0, 1, 7))
>
> I don't have access to the device anymore, but I still kept the report
> descriptors (this is the interesting part):
>
> 0xa1, 0x02, // Collection (Logical) 60
> 0x75, 0x08, // Report Size (8) 62
> 0x95, 0x07, // Report Count (7) 64
> 0x46, 0xff, 0x00, // Physical Maximum (255) 66
> 0x26, 0xff, 0x00, // Logical Maximum (255) 69
> 0x09, 0x02, // Usage (Vendor Usage 2) 72
> 0x91, 0x02, // Output (Data,Var,Abs) 74
> 0xc0, // End Collection 76
>
> So with the current implementation of hid_validate_report(), it works,
> but if another Buzz controller show up at some point with extras
> fields in this output report... we will be screwed. So please, amend
> hid_validate_report(), or specifically test here that the LED output
> report is 7 bytes.
hid_validate_report() checks for "at least" 7 in this call, so it
should be fine, unless I've misunderstood something.
-Kees
>
> Cheers,
> Benjamin
>
>> + return -ENODEV;
>> +
>> buzz = kzalloc(sizeof(*buzz), GFP_KERNEL);
>> if (!buzz) {
>> hid_err(hdev, "Insufficient memory, cannot allocate driver data\n");
>>
>> --
>> Jiri Kosina
>> SUSE Labs
>> --
>> 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
Chrome OS Security
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox