| Message ID | 50592FB1.7000800@kdau.com (mailing list archive) |
|---|---|
| State | New, archived |
| Delegated to: | Jiri Kosina |
| Headers | show |
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,
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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?
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
--- 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");
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> --- -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html