* [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
@ 2012-09-19 2:36 Kevin Daughtridge
2012-09-19 8:05 ` Jiri Slaby
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Kevin Daughtridge @ 2012-09-19 2:36 UTC (permalink / raw)
To: Jiri Kosina, linux-input
Cc: linux-usb, linux-kernel, Henrik Rydberg, 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");
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
2012-09-19 2:36 [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons Kevin Daughtridge
@ 2012-09-19 8:05 ` Jiri Slaby
2012-09-19 16:53 ` Kevin Daughtridge
2012-09-19 11:47 ` Sergei Shtylyov
2012-09-19 11:55 ` Jiri Kosina
2 siblings, 1 reply; 6+ messages in thread
From: Jiri Slaby @ 2012-09-19 8:05 UTC (permalink / raw)
To: Kevin Daughtridge
Cc: Jiri Kosina, linux-input, linux-usb, linux-kernel, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
2012-09-19 2:36 [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons Kevin Daughtridge
2012-09-19 8:05 ` Jiri Slaby
@ 2012-09-19 11:47 ` Sergei Shtylyov
2012-09-19 11:55 ` Jiri Kosina
2 siblings, 0 replies; 6+ messages in thread
From: Sergei Shtylyov @ 2012-09-19 11:47 UTC (permalink / raw)
To: Kevin Daughtridge
Cc: Jiri Kosina, linux-input, linux-usb, linux-kernel, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
2012-09-19 2:36 [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons Kevin Daughtridge
2012-09-19 8:05 ` Jiri Slaby
2012-09-19 11:47 ` Sergei Shtylyov
@ 2012-09-19 11:55 ` Jiri Kosina
2 siblings, 0 replies; 6+ messages in thread
From: Jiri Kosina @ 2012-09-19 11:55 UTC (permalink / raw)
To: Kevin Daughtridge; +Cc: linux-input, linux-usb, linux-kernel, Henrik Rydberg
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
2012-09-19 8:05 ` Jiri Slaby
@ 2012-09-19 16:53 ` Kevin Daughtridge
2012-09-19 18:32 ` Henrik Rydberg
0 siblings, 1 reply; 6+ messages in thread
From: Kevin Daughtridge @ 2012-09-19 16:53 UTC (permalink / raw)
To: Jiri Slaby, Jiri Kosina
Cc: Henrik Rydberg, Kevin Daughtridge, linux-input, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons
2012-09-19 16:53 ` Kevin Daughtridge
@ 2012-09-19 18:32 ` Henrik Rydberg
0 siblings, 0 replies; 6+ messages in thread
From: Henrik Rydberg @ 2012-09-19 18:32 UTC (permalink / raw)
To: Kevin Daughtridge
Cc: Jiri Slaby, Jiri Kosina, linux-input, linux-usb, linux-kernel
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2012-09-19 18:32 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-09-19 2:36 [PATCH v2] HID: leave dev_rdesc unmodified and use it for comparisons Kevin Daughtridge
2012-09-19 8:05 ` Jiri Slaby
2012-09-19 16:53 ` Kevin Daughtridge
2012-09-19 18:32 ` Henrik Rydberg
2012-09-19 11:47 ` Sergei Shtylyov
2012-09-19 11:55 ` Jiri Kosina
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).