[1/2] HID: core: fix collection parent pointer handling

Commit Message

Philipp Zabel Jan. 11, 2019, 7:13 p.m. UTC

Storing HID collection parents as direct pointers while parsing HID
report descriptors only works as long as the collection array is not
reallocated, which happens for descriptors with a large number of
collections. Then all stored pointers are invalidated in the middle
of parsing, which results in invalid memory accesses.

Instead of storing pointers, just store array indices, which stay
valid across array reallocation. Store UINT_MAX for "no parent".

Fixes: c53431eb696f ("HID: core: store the collections as a basic tree")
Fixes: 5a4abb36f312 ("HID: core: process the Resolution Multiplier")
Signed-off-by: Philipp Zabel <[e-mail]>
---
 drivers/hid/hid-core.c | 24 ++++++++++++++++--------
 include/linux/hid.h    |  4 ++--
 2 files changed, 18 insertions(+), 10 deletions(-)

Comments

Jiri Kosina Jan. 11, 2019, 8:58 p.m. UTC | #1

On Fri, 11 Jan 2019, Philipp Zabel wrote:

> Storing HID collection parents as direct pointers while parsing HID
> report descriptors only works as long as the collection array is not
> reallocated, which happens for descriptors with a large number of
> collections. Then all stored pointers are invalidated in the middle
> of parsing, which results in invalid memory accesses.

Philip,

have you seen ee46967fc6e ("HID: core: replace the collection tree 
pointers with indices") in hid.git#for-5.0/upstream-fixes from Peter? (not 
in Linus' tree yet, will be sending it out shortly).

I believe it's fixing the same thing you're fixing here.

Philipp Zabel Jan. 12, 2019, 11:21 a.m. UTC | #2

Hi Jiri,

On Fri, Jan 11, 2019 at 9:58 PM Jiri Kosina <[e-mail]> wrote:
>
> On Fri, 11 Jan 2019, Philipp Zabel wrote:
>
> > Storing HID collection parents as direct pointers while parsing HID
> > report descriptors only works as long as the collection array is not
> > reallocated, which happens for descriptors with a large number of
> > collections. Then all stored pointers are invalidated in the middle
> > of parsing, which results in invalid memory accesses.
>
> Philip,
>
> have you seen ee46967fc6e ("HID: core: replace the collection tree
> pointers with indices") in hid.git#for-5.0/upstream-fixes from Peter? (not
> in Linus' tree yet, will be sending it out shortly).
>
> I believe it's fixing the same thing you're fixing here.

Thank you, yes, I wasn't aware this was already fixed.

regards
Philipp

Patch

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f41d5fe51abe..5424f7f59680 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -168,12 +168,12 @@  static int open_collection(struct hid_parser *parser, unsigned type)
 		parser->device->maxcollection;
 
 	collection = parser->device->collection +
-		parser->device->maxcollection++;
+		parser->device->maxcollection;
 	collection->type = type;
 	collection->usage = usage;
 	collection->level = parser->collection_stack_ptr - 1;
 	collection->parent = parser->active_collection;
-	parser->active_collection = collection;
+	parser->active_collection = parser->device->maxcollection++;
 
 	if (type == HID_COLLECTION_APPLICATION)
 		parser->device->maxapplication++;
@@ -192,8 +192,9 @@  static int close_collection(struct hid_parser *parser)
 		return -EINVAL;
 	}
 	parser->collection_stack_ptr--;
-	if (parser->active_collection)
-		parser->active_collection = parser->active_collection->parent;
+	if (parser->active_collection != UINT_MAX)
+		parser->active_collection =
+			parser->device->collection[parser->active_collection].parent;
 	return 0;
 }
 
@@ -819,6 +820,7 @@  static int hid_scan_report(struct hid_device *hid)
 		return -ENOMEM;
 
 	parser->device = hid;
+	parser->active_collection = UINT_MAX;
 	hid->group = HID_GROUP_GENERIC;
 
 	/*
@@ -1006,8 +1008,10 @@  static void hid_apply_multiplier_to_field(struct hid_device *hid,
 		usage = &field->usage[i];
 
 		collection = &hid->collection[usage->collection_index];
-		while (collection && collection != multiplier_collection)
-			collection = collection->parent;
+		while (collection && collection != multiplier_collection) {
+			collection = (collection->parent == UINT_MAX) ? NULL :
+				     &hid->collection[collection->parent];
+		}
 
 		if (collection || multiplier_collection == NULL)
 			usage->resolution_multiplier = effective_multiplier;
@@ -1045,8 +1049,11 @@  static void hid_apply_multiplier(struct hid_device *hid,
 	 */
 	multiplier_collection = &hid->collection[multiplier->usage->collection_index];
 	while (multiplier_collection &&
-	       multiplier_collection->type != HID_COLLECTION_LOGICAL)
-		multiplier_collection = multiplier_collection->parent;
+	       multiplier_collection->type != HID_COLLECTION_LOGICAL) {
+		multiplier_collection =
+			(multiplier_collection->parent == UINT_MAX) ? NULL :
+			&hid->collection[multiplier_collection->parent];
+	}
 
 	effective_multiplier = hid_calculate_multiplier(hid, multiplier);
 
@@ -1170,6 +1177,7 @@  int hid_open_report(struct hid_device *device)
 	}
 
 	parser->device = device;
+	parser->active_collection = UINT_MAX;
 
 	end = start + size;
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index d99287327ef2..e7b1b388a990 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -430,7 +430,7 @@  struct hid_local {
  */
 
 struct hid_collection {
-	struct hid_collection *parent;
+	unsigned int parent;
 	unsigned type;
 	unsigned usage;
 	unsigned level;
@@ -658,7 +658,7 @@  struct hid_parser {
 	unsigned int         *collection_stack;
 	unsigned int          collection_stack_ptr;
 	unsigned int          collection_stack_size;
-	struct hid_collection *active_collection;
+	unsigned int          active_collection;
 	struct hid_device    *device;
 	unsigned int          scan_flags;
 };