From: Kevin Daughtridge <hidden> Date: 2012-09-19 02:36:40
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<redacted>
---
@@ -775,12 +775,14 @@ int hid_open_report(struct hid_device *dreturn-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
From: Sergei Shtylyov <hidden> Date: 2012-09-19 11:49:12
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.
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<redacted>
Your patch is whitespace damaged again, please fix your workload Kevin.
From: Kevin Daughtridge <hidden> Date: 2012-09-19 16:53:15
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
From: Henrik Rydberg <hidden> Date: 2012-09-19 18:32:34
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