[PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Kevin Daughtridge]

The dev_rdesc member of the hid_device structure is meant to store the original
report descriptor received from the device, but it is currently passed to any
report_fixup method before it is copied to the rdesc member. This patch moves
the kmemdup to before, not after, the report_fixup call, keeping dev_rdesc
unchanged.

usbhid's hid_post_reset checks the report descriptor currently returned by the
device against a descriptor that may have been modified by a driver's
report_fixup method. That leaves some devices nonfunctional after a resume, with
a "reset_resume error 1" reported. This patch checks the new descriptor against
the unmodified dev_rdesc instead.

BugLink:http://bugs.launchpad.net/bugs/1049623
Signed-off-by: Kevin Daughtridge<kevin@kdau.com>
---
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -775,12 +775,14 @@ int hid_open_report(struct hid_device *d
  		return -ENODEV;
  	size = device->dev_rsize;
  
+	start = kmemdup(start, size, GFP_KERNEL);
+	if (start == NULL)
+		return -ENOMEM;
+
  	if (device->driver->report_fixup)
  		start = device->driver->report_fixup(device, start, &size);
  
-	device->rdesc = kmemdup(start, size, GFP_KERNEL);
-	if (device->rdesc == NULL)
-		return -ENOMEM;
+	device->rdesc = start;
  	device->rsize = size;
  
  	parser = vzalloc(sizeof(struct hid_parser));
--- a/drivers/hid/usbhid/hid-core.c
+++ b/drivers/hid/usbhid/hid-core.c
@@ -1436,7 +1436,7 @@ static int hid_post_reset(struct usb_int
  		kfree(rdesc);
  		return 1;
  	}
-	status = memcmp(rdesc, hid->rdesc, hid->rsize);
+	status = memcmp(rdesc, hid->dev_rdesc, hid->rsize);
  	kfree(rdesc);
  	if (status != 0) {
  		dbg_hid("report descriptor changed\n");

Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Jiri Slaby]

On 09/19/2012 04:36 AM, Kevin Daughtridge wrote:
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -775,12 +775,14 @@ int hid_open_report(struct hid_device *d
>          return -ENODEV;
>      size = device->dev_rsize;
>  
> +    start = kmemdup(start, size, GFP_KERNEL);
> +    if (start == NULL)
> +        return -ENOMEM;
> +
>      if (device->driver->report_fixup)
>          start = device->driver->report_fixup(device, start, &size);
>  
> -    device->rdesc = kmemdup(start, size, GFP_KERNEL);
> -    if (device->rdesc == NULL)
> -        return -ENOMEM;
> +    device->rdesc = start;
>      device->rsize = size;

AFAICS this is incorrect. Some drivers return pointers to their own
static structure from their .report_fixup. Hence there are two problems:
* leak, because kmemdup'ped start is never freed
* invalid free -- kfree(device->rdesc) will try to free a static structure

regards,
-- 
js
suse labs

Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Kevin Daughtridge]

On 09/19/12 01:05 N.U., Jiri Slaby wrote:
> AFAICS this is incorrect. Some drivers return pointers to their own 
> static structure from their .report_fixup. Hence there are two problems:
> * leak, because kmemdup'ped start is never freed
> * invalid free -- kfree(device->rdesc) will try to free a static 
> structure regards, 

On 09/19/12 04:55 N.U., Jiri Kosina wrote:
> How do you avoid memory leak on 'start' here? 

Hmm. I hadn't noticed that the other drivers are returning a static 
structure. In that case, it seems that report_fixup itself is broken 
from a memory perspective, in that it returns pointers to inconsistent 
storage types depending on the driver. (As evidenced by how my first 
patch version would have also caused invalid frees that weren't evident 
from the declaration in hid.h.) I see two options:

1. Ugly workaround: make a temporary copy of the dev_rdesc, give it to 
report_fixup, make a copy of the return, store that copy in rdesc, free 
the temporary copy. Though ugly, this would at least involve the 
smallest diff.

2. Standardize the behavior of the drivers' report_fixup 
implementations. Given that some of them need to change the size of the 
descriptor, modifying the passed structure is not an option. Probably 
all of them should return a newly allocated structure, either a modified 
copy of the input or a copy of their static, that can then be stored 
directly in rdesc. Especially since report_fixup is only ever called 
right before a copy is going to be taken anyway. (Adding constness to a 
parameter isn't considered a severe ABI break, is it?)

Thoughts?

-Kevin

Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Henrik Rydberg]

Hi Kevin,

Thanks for looking to this.

> Hmm. I hadn't noticed that the other drivers are returning a static
> structure. In that case, it seems that report_fixup itself is broken
> from a memory perspective, in that it returns pointers to
> inconsistent storage types depending on the driver.

The driver can either modify the existing buffer, of return a pointer
to a buffer managed by the driver. The former requires a kmemdup
before, the latter a kmemdup after.

> 1. Ugly workaround: make a temporary copy of the dev_rdesc, give it
> to report_fixup, make a copy of the return, store that copy in
> rdesc, free the temporary copy. Though ugly, this would at least
> involve the smallest diff.

Yes, it is correct and ugly, in no particular order.

> 2. Standardize the behavior of the drivers' report_fixup
> implementations. Given that some of them need to change the size of
> the descriptor, modifying the passed structure is not an option.
> Probably all of them should return a newly allocated structure,
> either a modified copy of the input or a copy of their static, that
> can then be stored directly in rdesc. Especially since report_fixup
> is only ever called right before a copy is going to be taken anyway.

Relying on the returned pointer to be properly alloc'd is not a good
idea, in particular since it changes semantics rather drastically.

Since the current function performs two different things, perhaps
there should be two different functions instead.

> (Adding constness to a parameter isn't considered a severe ABI
> break, is it?)

Inside the kernel there is no ABI. Go wild.

Thanks,
Henrik

Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Sergei Shtylyov]

Hello.

On 19-09-2012 6:36, Kevin Daughtridge wrote:

> The dev_rdesc member of the hid_device structure is meant to store the original
> report descriptor received from the device, but it is currently passed to any
> report_fixup method before it is copied to the rdesc member. This patch moves
> the kmemdup to before, not after, the report_fixup call, keeping dev_rdesc
> unchanged.

> usbhid's hid_post_reset checks the report descriptor currently returned by the
> device against a descriptor that may have been modified by a driver's
> report_fixup method. That leaves some devices nonfunctional after a resume, with
> a "reset_resume error 1" reported. This patch checks the new descriptor against
> the unmodified dev_rdesc instead.

> BugLink:http://bugs.launchpad.net/bugs/1049623
> Signed-off-by: Kevin Daughtridge<kevin@kdau.com>

    Need space before the email. I've checked your original mail, it's not my 
Thunderbird this time (which does this kind of damage while quoting).

> ---
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -775,12 +775,14 @@ int hid_open_report(struct hid_device *d
>           return -ENODEV;
>       size = device->dev_rsize;
>
> +    start = kmemdup(start, size, GFP_KERNEL);
> +    if (start == NULL)
> +        return -ENOMEM;
> +
>       if (device->driver->report_fixup)
>           start = device->driver->report_fixup(device, start, &size);
>
> -    device->rdesc = kmemdup(start, size, GFP_KERNEL);
> -    if (device->rdesc == NULL)
> -        return -ENOMEM;
> +    device->rdesc = start;
>       device->rsize = size;
>
>       parser = vzalloc(sizeof(struct hid_parser));

    It still seems like the patch is whitespace damaged, but in another way: 
to every line satrting with space, another spece was added.

WBR, Sergei

Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons

[Jiri Kosina]

On Tue, 18 Sep 2012, Kevin Daughtridge wrote:

> The dev_rdesc member of the hid_device structure is meant to store the
> original
> report descriptor received from the device, but it is currently passed to any
> report_fixup method before it is copied to the rdesc member. This patch moves
> the kmemdup to before, not after, the report_fixup call, keeping dev_rdesc
> unchanged.
> 
> usbhid's hid_post_reset checks the report descriptor currently returned by the
> device against a descriptor that may have been modified by a driver's
> report_fixup method. That leaves some devices nonfunctional after a resume,
> with
> a "reset_resume error 1" reported. This patch checks the new descriptor
> against
> the unmodified dev_rdesc instead.
> 
> BugLink:http://bugs.launchpad.net/bugs/1049623
> Signed-off-by: Kevin Daughtridge<kevin@kdau.com>

Your patch is whitespace damaged again, please fix your workload Kevin.

> ---
> --- a/drivers/hid/hid-core.c
> +++ b/drivers/hid/hid-core.c
> @@ -775,12 +775,14 @@ int hid_open_report(struct hid_device *d
>  		return -ENODEV;
>  	size = device->dev_rsize;
>  +	start = kmemdup(start, size, GFP_KERNEL);
> +	if (start == NULL)
> +		return -ENOMEM;
> +

How do you avoid memory leak on 'start' here?

-- 
Jiri Kosina
SUSE Labs