On Thu, Aug 29, 2013 at 9:51 PM, Kees Cook <keescook@xxxxxxxxxxxx> wrote:
> On Thu, Aug 29, 2013 at 2:35 AM, Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxx> wrote:
>> Hi Kees,
>>
>> On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina <jkosina@xxxxxxx> wrote:
>>> From: Kees Cook <keescook@xxxxxxxxxxxx>
>>>
>>> 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@xxxxxxxxxxxx>
>>> Cc: stable@xxxxxxxxxx
>>> ---
>>> 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
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
