Re: [PATCH 02/14] HID: provide a helper for validating hid reports

On Thu, Aug 29, 2013 at 9:51 PM, Kees Cook [off-list ref] wrote:

On Thu, Aug 29, 2013 at 2:35 AM, Benjamin Tissoires
[off-list ref] wrote:

quoted

Hi Kees,

On Wed, Aug 28, 2013 at 10:30 PM, Jiri Kosina [off-list ref] wrote:

quoted

From: Kees Cook <redacted>

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

quoted

quoted

+                                      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 :)

quoted

quoted

+       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

quoted

quoted

+                       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