HID: core: replace the collection tree pointers with indices
diff mbox series

Message ID 20190109035018.GA12679@jelly
State Mainlined
Commit ee46967fc6e74d412fe1ec15f77fdb8624bde2b0
Delegated to: Jiri Kosina
Headers show
Series
  • HID: core: replace the collection tree pointers with indices
Related show

Commit Message

Peter Hutterer Jan. 9, 2019, 3:50 a.m. UTC
Introduced in c53431eb696f3c64c12c00afb81048af54b61532
"HID: core: store the collections as a basic tree".

Previously, the pointer to the parent collection was stored. If a device
exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to store
the collections is reallocated, the pointer to the parent collection becomes
invalid.

Replace the pointers with an index-based lookup into the collections array.

Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com>
Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
---
I have a test locally for hid-tools now but it relies on the kernel
crashing, so it's going to generate false positives. I verified the test
with printks though.

 drivers/hid/hid-core.c | 32 +++++++++++++++++++++-----------
 include/linux/hid.h    |  4 ++--
 2 files changed, 23 insertions(+), 13 deletions(-)

Comments

Kyle Pelton Jan. 9, 2019, 10:38 p.m. UTC | #1
On 1/9/19 2:48 PM, Pandruvada, Srinivas wrote:
> On Wed, 2019-01-09 at 13:50 +1000, Peter Hutterer wrote:
>> Introduced in c53431eb696f3c64c12c00afb81048af54b61532
>> "HID: core: store the collections as a basic tree".
>> 
>> Previously, the pointer to the parent collection was stored. If a
>> device
>> exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to
>> store
>> the collections is reallocated, the pointer to the parent collection
>> becomes
>> invalid.
>> 
>> Replace the pointers with an index-based lookup into the collections
>> array.
>> 
>> Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com>
>> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>Tested-by: Kyle Pelton <kyle.d.pelton@linux.intel.com>
>> ---
>> I have a test locally for hid-tools now but it relies on the kernel
>> crashing, so it's going to generate false positives. I verified the
>> test
>> with printks though.
>> 
>>  drivers/hid/hid-core.c | 32 +++++++++++++++++++++-----------
>>  include/linux/hid.h    |  4 ++--
>>  2 files changed, 23 insertions(+), 13 deletions(-)
>> 
>> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
>> index f41d5fe51abe..f9093dedf647 100644
>> --- a/drivers/hid/hid-core.c
>> +++ b/drivers/hid/hid-core.c
>> @@ -125,6 +125,7 @@ static int open_collection(struct hid_parser
>> *parser, unsigned type)
>>  {
>>  	struct hid_collection *collection;
>>  	unsigned usage;
>> +	int collection_index;
>>  
>>  	usage = parser->local.usage[0];
>>  
>> @@ -167,13 +168,13 @@ static int open_collection(struct hid_parser
>> *parser, unsigned type)
>>  	parser->collection_stack[parser->collection_stack_ptr++] =
>>  		parser->device->maxcollection;
>>  
>> -	collection = parser->device->collection +
>> -		parser->device->maxcollection++;
>> +	collection_index = parser->device->maxcollection++;
>> +	collection = parser->device->collection + collection_index;
>>  	collection->type = type;
>>  	collection->usage = usage;
>>  	collection->level = parser->collection_stack_ptr - 1;
>> -	collection->parent = parser->active_collection;
>> -	parser->active_collection = collection;
>> +	collection->parent_idx = parser->active_collection_idx;
>> +	parser->active_collection_idx = collection_index;
>>  
>>  	if (type == HID_COLLECTION_APPLICATION)
>>  		parser->device->maxapplication++;
>> @@ -192,8 +193,13 @@ 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_idx != -1) {
>> +		struct hid_device *device = parser->device;
>> +		struct hid_collection *c;
>> +
>> +		c = &device->collection[parser->active_collection_idx];
>> +		parser->active_collection_idx = c->parent_idx;
>> +	}
>>  	return 0;
>>  }
>>  
>> @@ -819,6 +825,7 @@ static int hid_scan_report(struct hid_device
>> *hid)
>>  		return -ENOMEM;
>>  
>>  	parser->device = hid;
>> +	parser->active_collection_idx = -1;
>>  	hid->group = HID_GROUP_GENERIC;
>>  
>>  	/*
>> @@ -1006,10 +1013,12 @@ 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->parent_idx != -1 &&
>> +		       collection != multiplier_collection)
>> +			collection = &hid->collection[collection-
>> >parent_idx];
>>  
>> -		if (collection || multiplier_collection == NULL)
>> +		if (collection->parent_idx != -1 ||
>> +		    multiplier_collection == NULL)
>>  			usage->resolution_multiplier =
>> effective_multiplier;
>>  
>>  	}
>> @@ -1044,9 +1053,9 @@ static void hid_apply_multiplier(struct
>> hid_device *hid,
>>  	 * applicable fields later.
>>  	 */
>>  	multiplier_collection = &hid->collection[multiplier->usage-
>> >collection_index];
>> -	while (multiplier_collection &&
>> +	while (multiplier_collection->parent_idx != -1 &&
>>  	       multiplier_collection->type != HID_COLLECTION_LOGICAL)
>> -		multiplier_collection = multiplier_collection->parent;
>> +		multiplier_collection = &hid-
>> >collection[multiplier_collection->parent_idx];
>>  
>>  	effective_multiplier = hid_calculate_multiplier(hid,
>> multiplier);
>>  
>> @@ -1170,6 +1179,7 @@ int hid_open_report(struct hid_device *device)
>>  	}
>>  
>>  	parser->device = device;
>> +	parser->active_collection_idx = -1;
>>  
>>  	end = start + size;
>>  
>> diff --git a/include/linux/hid.h b/include/linux/hid.h
>> index 93db548f8761..554e82d812da 100644
>> --- a/include/linux/hid.h
>> +++ b/include/linux/hid.h
>> @@ -429,7 +429,7 @@ struct hid_local {
>>   */
>>  
>>  struct hid_collection {
>> -	struct hid_collection *parent;
>> +	int parent_idx; /* device->collection */
>>  	unsigned type;
>>  	unsigned usage;
>>  	unsigned level;
>> @@ -657,7 +657,7 @@ struct hid_parser {
>>  	unsigned int         *collection_stack;
>>  	unsigned int          collection_stack_ptr;
>>  	unsigned int          collection_stack_size;
>> -	struct hid_collection *active_collection;
>> +	int                   active_collection_idx; /* device-
>> >collection */
>>  	struct hid_device    *device;
>>  	unsigned int          scan_flags;
>>  };
>
Pandruvada, Srinivas Jan. 9, 2019, 10:48 p.m. UTC | #2
On Wed, 2019-01-09 at 13:50 +1000, Peter Hutterer wrote:
> Introduced in c53431eb696f3c64c12c00afb81048af54b61532
> "HID: core: store the collections as a basic tree".
> 
> Previously, the pointer to the parent collection was stored. If a
> device
> exceeds 16 collections (HID_DEFAULT_NUM_COLLECTIONS), the array to
> store
> the collections is reallocated, the pointer to the parent collection
> becomes
> invalid.
> 
> Replace the pointers with an index-based lookup into the collections
> array.
> 
> Reported-by: Pandruvada, Srinivas <srinivas.pandruvada@intel.com>
> Signed-off-by: Peter Hutterer <peter.hutterer@who-t.net>
Added Kyle to this email, who is testing this change.

Thanks,
Srinivas
> ---
> I have a test locally for hid-tools now but it relies on the kernel
> crashing, so it's going to generate false positives. I verified the
> test
> with printks though.
> 
>  drivers/hid/hid-core.c | 32 +++++++++++++++++++++-----------
>  include/linux/hid.h    |  4 ++--
>  2 files changed, 23 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
> index f41d5fe51abe..f9093dedf647 100644
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -125,6 +125,7 @@ static int open_collection(struct hid_parser
> *parser, unsigned type)
>  {
>  	struct hid_collection *collection;
>  	unsigned usage;
> +	int collection_index;
>  
>  	usage = parser->local.usage[0];
>  
> @@ -167,13 +168,13 @@ static int open_collection(struct hid_parser
> *parser, unsigned type)
>  	parser->collection_stack[parser->collection_stack_ptr++] =
>  		parser->device->maxcollection;
>  
> -	collection = parser->device->collection +
> -		parser->device->maxcollection++;
> +	collection_index = parser->device->maxcollection++;
> +	collection = parser->device->collection + collection_index;
>  	collection->type = type;
>  	collection->usage = usage;
>  	collection->level = parser->collection_stack_ptr - 1;
> -	collection->parent = parser->active_collection;
> -	parser->active_collection = collection;
> +	collection->parent_idx = parser->active_collection_idx;
> +	parser->active_collection_idx = collection_index;
>  
>  	if (type == HID_COLLECTION_APPLICATION)
>  		parser->device->maxapplication++;
> @@ -192,8 +193,13 @@ 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_idx != -1) {
> +		struct hid_device *device = parser->device;
> +		struct hid_collection *c;
> +
> +		c = &device->collection[parser->active_collection_idx];
> +		parser->active_collection_idx = c->parent_idx;
> +	}
>  	return 0;
>  }
>  
> @@ -819,6 +825,7 @@ static int hid_scan_report(struct hid_device
> *hid)
>  		return -ENOMEM;
>  
>  	parser->device = hid;
> +	parser->active_collection_idx = -1;
>  	hid->group = HID_GROUP_GENERIC;
>  
>  	/*
> @@ -1006,10 +1013,12 @@ 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->parent_idx != -1 &&
> +		       collection != multiplier_collection)
> +			collection = &hid->collection[collection-
> >parent_idx];
>  
> -		if (collection || multiplier_collection == NULL)
> +		if (collection->parent_idx != -1 ||
> +		    multiplier_collection == NULL)
>  			usage->resolution_multiplier =
> effective_multiplier;
>  
>  	}
> @@ -1044,9 +1053,9 @@ static void hid_apply_multiplier(struct
> hid_device *hid,
>  	 * applicable fields later.
>  	 */
>  	multiplier_collection = &hid->collection[multiplier->usage-
> >collection_index];
> -	while (multiplier_collection &&
> +	while (multiplier_collection->parent_idx != -1 &&
>  	       multiplier_collection->type != HID_COLLECTION_LOGICAL)
> -		multiplier_collection = multiplier_collection->parent;
> +		multiplier_collection = &hid-
> >collection[multiplier_collection->parent_idx];
>  
>  	effective_multiplier = hid_calculate_multiplier(hid,
> multiplier);
>  
> @@ -1170,6 +1179,7 @@ int hid_open_report(struct hid_device *device)
>  	}
>  
>  	parser->device = device;
> +	parser->active_collection_idx = -1;
>  
>  	end = start + size;
>  
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 93db548f8761..554e82d812da 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -429,7 +429,7 @@ struct hid_local {
>   */
>  
>  struct hid_collection {
> -	struct hid_collection *parent;
> +	int parent_idx; /* device->collection */
>  	unsigned type;
>  	unsigned usage;
>  	unsigned level;
> @@ -657,7 +657,7 @@ struct hid_parser {
>  	unsigned int         *collection_stack;
>  	unsigned int          collection_stack_ptr;
>  	unsigned int          collection_stack_size;
> -	struct hid_collection *active_collection;
> +	int                   active_collection_idx; /* device-
> >collection */
>  	struct hid_device    *device;
>  	unsigned int          scan_flags;
>  };
Jiri Kosina Jan. 10, 2019, 6:11 a.m. UTC | #3
On Wed, 9 Jan 2019, Peter Hutterer wrote:

> Introduced in c53431eb696f3c64c12c00afb81048af54b61532
> "HID: core: store the collections as a basic tree".

I've removed this from the changelog and added appropriate Fixes: tag, and 
applied to for-5.0/upstream-fixes. Thanks,

Patch
diff mbox series

diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index f41d5fe51abe..f9093dedf647 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -125,6 +125,7 @@  static int open_collection(struct hid_parser *parser, unsigned type)
 {
 	struct hid_collection *collection;
 	unsigned usage;
+	int collection_index;
 
 	usage = parser->local.usage[0];
 
@@ -167,13 +168,13 @@  static int open_collection(struct hid_parser *parser, unsigned type)
 	parser->collection_stack[parser->collection_stack_ptr++] =
 		parser->device->maxcollection;
 
-	collection = parser->device->collection +
-		parser->device->maxcollection++;
+	collection_index = parser->device->maxcollection++;
+	collection = parser->device->collection + collection_index;
 	collection->type = type;
 	collection->usage = usage;
 	collection->level = parser->collection_stack_ptr - 1;
-	collection->parent = parser->active_collection;
-	parser->active_collection = collection;
+	collection->parent_idx = parser->active_collection_idx;
+	parser->active_collection_idx = collection_index;
 
 	if (type == HID_COLLECTION_APPLICATION)
 		parser->device->maxapplication++;
@@ -192,8 +193,13 @@  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_idx != -1) {
+		struct hid_device *device = parser->device;
+		struct hid_collection *c;
+
+		c = &device->collection[parser->active_collection_idx];
+		parser->active_collection_idx = c->parent_idx;
+	}
 	return 0;
 }
 
@@ -819,6 +825,7 @@  static int hid_scan_report(struct hid_device *hid)
 		return -ENOMEM;
 
 	parser->device = hid;
+	parser->active_collection_idx = -1;
 	hid->group = HID_GROUP_GENERIC;
 
 	/*
@@ -1006,10 +1013,12 @@  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->parent_idx != -1 &&
+		       collection != multiplier_collection)
+			collection = &hid->collection[collection->parent_idx];
 
-		if (collection || multiplier_collection == NULL)
+		if (collection->parent_idx != -1 ||
+		    multiplier_collection == NULL)
 			usage->resolution_multiplier = effective_multiplier;
 
 	}
@@ -1044,9 +1053,9 @@  static void hid_apply_multiplier(struct hid_device *hid,
 	 * applicable fields later.
 	 */
 	multiplier_collection = &hid->collection[multiplier->usage->collection_index];
-	while (multiplier_collection &&
+	while (multiplier_collection->parent_idx != -1 &&
 	       multiplier_collection->type != HID_COLLECTION_LOGICAL)
-		multiplier_collection = multiplier_collection->parent;
+		multiplier_collection = &hid->collection[multiplier_collection->parent_idx];
 
 	effective_multiplier = hid_calculate_multiplier(hid, multiplier);
 
@@ -1170,6 +1179,7 @@  int hid_open_report(struct hid_device *device)
 	}
 
 	parser->device = device;
+	parser->active_collection_idx = -1;
 
 	end = start + size;
 
diff --git a/include/linux/hid.h b/include/linux/hid.h
index 93db548f8761..554e82d812da 100644
--- a/include/linux/hid.h
+++ b/include/linux/hid.h
@@ -429,7 +429,7 @@  struct hid_local {
  */
 
 struct hid_collection {
-	struct hid_collection *parent;
+	int parent_idx; /* device->collection */
 	unsigned type;
 	unsigned usage;
 	unsigned level;
@@ -657,7 +657,7 @@  struct hid_parser {
 	unsigned int         *collection_stack;
 	unsigned int          collection_stack_ptr;
 	unsigned int          collection_stack_size;
-	struct hid_collection *active_collection;
+	int                   active_collection_idx; /* device->collection */
 	struct hid_device    *device;
 	unsigned int          scan_flags;
 };