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.
>
>> + 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
--
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
