[INPUT] hidinput_connect incorrectly ignored return value from input_register_device
Signed-off-by: Dirk Hohndel <[email protected]>
---
drivers/hid/hid-input.c | 10 ++++++++--
1 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..c8640e7 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1186,13 +1186,19 @@ int hidinput_connect(struct hid_device *hid)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input)) {
+ input_free_device(hidinput->input);
+ return -1;
+ }
hidinput = NULL;
}
}
if (hidinput)
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input)) {
+ input_free_device(hidinput->input);
+ return -1;
+ }
return 0;
}
--
gitgui.0.8.4.g8d863
Dirk Hohndel wrote:
> [INPUT] hidinput_connect incorrectly ignored return value from input_register_device
>
> Signed-off-by: Dirk Hohndel <[email protected]>
>
> ---
> drivers/hid/hid-input.c | 10 ++++++++--
> 1 files changed, 8 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index dd332f2..c8640e7 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1186,13 +1186,19 @@ int hidinput_connect(struct hid_device *hid)
> * UGCI) cram a lot of unrelated inputs into the
> * same interface. */
> hidinput->report = report;
> - input_register_device(hidinput->input);
> + if (input_register_device(hidinput->input)) {
> + input_free_device(hidinput->input);
> + return -1;
> + }
> hidinput = NULL;
> }
> }
>
> if (hidinput)
> - input_register_device(hidinput->input);
> + if (input_register_device(hidinput->input)) {
> + input_free_device(hidinput->input);
> + return -1;
> + }
You would also want to kfree(hidinput) on failure too.
Thanks for attacking this newly added warning!
Jeff
On Mon, Oct 29, 2007 at 03:53:14AM -0400, Jeff Garzik wrote:
>
> You would also want to kfree(hidinput) on failure too.
Oops, of course. Thanks for catching that. Here's the updated patch
/D
[INPUT] hidinput_connect incorrectly ignored return value from input_register_device
Signed-off-by: Dirk Hohndel <[email protected]>
---
drivers/hid/hid-input.c | 12 ++++++++++--
1 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..880161b 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1186,13 +1186,21 @@ int hidinput_connect(struct hid_device *hid)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input)) {
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+ return -1;
+ }
hidinput = NULL;
}
}
if (hidinput)
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input)) {
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+ return -1;
+ }
return 0;
}
--
gitgui.0.8.4.g8d863
Dirk Hohndel wrote:
> [INPUT] hidinput_connect incorrectly ignored return value from input_register_device
>
> Signed-off-by: Dirk Hohndel <[email protected]>
>
> ---
> drivers/hid/hid-input.c | 12 ++++++++++--
> 1 files changed, 10 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index dd332f2..880161b 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1186,13 +1186,21 @@ int hidinput_connect(struct hid_device *hid)
> * UGCI) cram a lot of unrelated inputs into the
> * same interface. */
> hidinput->report = report;
> - input_register_device(hidinput->input);
> + if (input_register_device(hidinput->input)) {
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + return -1;
> + }
> hidinput = NULL;
> }
> }
>
> if (hidinput)
> - input_register_device(hidinput->input);
> + if (input_register_device(hidinput->input)) {
> + input_free_device(hidinput->input);
> + kfree(hidinput);
> + return -1;
ACK, thanks for revising
On Mon, 29 Oct 2007, Dirk Hohndel wrote:
> [INPUT] hidinput_connect incorrectly ignored return value from
> input_register_device
> Signed-off-by: Dirk Hohndel <[email protected]>
Will apply, thanks a lot.
--
Jiri Kosina
On 10/29/07, Jiri Kosina <[email protected]> wrote:
> On Mon, 29 Oct 2007, Dirk Hohndel wrote:
>
> > [INPUT] hidinput_connect incorrectly ignored return value from
> > input_register_device
> > Signed-off-by: Dirk Hohndel <[email protected]>
>
> Will apply
Please don't - the fix is completely broken for multi-input devices -
if 2nd device fails to register we bail out of hidinput_connect and
thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
never call hidinput_disconnect and who knows what will happen after
that.
hidinput_connect() should properly unwind already registered devices
after failure.
--
Dmitry
On Mon, 29 Oct 2007, Dmitry Torokhov wrote:
> Please don't - the fix is completely broken for multi-input devices -
> if 2nd device fails to register we bail out of hidinput_connect and
> thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
> never call hidinput_disconnect and who knows what will happen after
> that.
You are of course right, stupid me. I won't commit anything before first
morning coffee any more.
Thanks a lot.
--
Jiri Kosina
Dmitry Torokhov wrote:
> On 10/29/07, Jiri Kosina <[email protected]> wrote:
>> On Mon, 29 Oct 2007, Dirk Hohndel wrote:
>>
>>> [INPUT] hidinput_connect incorrectly ignored return value from
>>> input_register_device
>>> Signed-off-by: Dirk Hohndel <[email protected]>
>> Will apply
>
> Please don't - the fix is completely broken for multi-input devices -
> if 2nd device fails to register we bail out of hidinput_connect and
> thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
> never call hidinput_disconnect and who knows what will happen after
> that.
>
> hidinput_connect() should properly unwind already registered devices
> after failure.
Then the existing code to handle hidinput and input_dev allocation
failure probably also wants fixing... Dirk's patch was largely
following the same logic.
Jeff
On Mon, Oct 29, 2007 at 10:49:03AM -0400, Jeff Garzik wrote:
> Dmitry Torokhov wrote:
> > On 10/29/07, Jiri Kosina <[email protected]> wrote:
> >> On Mon, 29 Oct 2007, Dirk Hohndel wrote:
> >>
> >>> [INPUT] hidinput_connect incorrectly ignored return value from
> >>> input_register_device
> >>> Signed-off-by: Dirk Hohndel <[email protected]>
> >> Will apply
> > Please don't - the fix is completely broken for multi-input devices -
> > if 2nd device fails to register we bail out of hidinput_connect and
> > thus never set HID_CLAIMED_INPUT bit. So when we disconnect device we
> > never call hidinput_disconnect and who knows what will happen after
> > that.
> > hidinput_connect() should properly unwind already registered devices
> > after failure.
>
> Then the existing code to handle hidinput and input_dev allocation failure
> probably also wants fixing... Dirk's patch was largely following the same
> logic.
I was wondering about that. If I didn't get lost in the structures again, I
think it isn't too hard to simply call out directly to hidinput_disconnect to
do the cleanup / unwind; the &hid->inputs should contain those devices that
have successfully been registered before we failed.
Actually, the more I look at the code that bails when it runs out of memory,
the more I wonder about that.
hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
kfree(hidinput);
input_free_device(input_dev);
This either passes a NULL pointer to kfree or to input_free_device. That's
not nice.
Would something like this work?
[PATCH] hidinput_connect ignores retval from input_register_device
Signed-off-by: Dirk Hohndel <[email protected]>
---
drivers/hid/hid-input.c | 24 ++++++++++++++++++------
1 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..5bff5cc 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1149,10 +1149,12 @@ int hidinput_connect(struct hid_device *hid)
hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
input_dev = input_allocate_device();
if (!hidinput || !input_dev) {
- kfree(hidinput);
- input_free_device(input_dev);
+ if (hidinput)
+ kfree(hidinput);
+ if (input_dev)
+ input_free_device(input_dev);
err_hid("Out of memory during hid input probe");
- return -1;
+ goto out_unwind;
}
input_set_drvdata(input_dev, hid);
@@ -1186,15 +1188,25 @@ int hidinput_connect(struct hid_device *hid)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input))
+ goto out_cleanup;
hidinput = NULL;
}
}
- if (hidinput)
- input_register_device(hidinput->input);
+ if (hidinput && input_register_device(hidinput->input))
+ goto out_cleanup;
return 0;
+
+out_cleanup:
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+out_unwind:
+ /* unwind the ones we already registered */
+ hidinput_disconnect(hid);
+
+ return -1;
}
EXPORT_SYMBOL_GPL(hidinput_connect);
--
gitgui.0.8.4.g8d863
On 10/29/07, Dirk Hohndel <[email protected]> wrote:
>
> Actually, the more I look at the code that bails when it runs out of memory,
> the more I wonder about that.
>
> hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!hidinput || !input_dev) {
> kfree(hidinput);
> input_free_device(input_dev);
>
> This either passes a NULL pointer to kfree or to input_free_device. That's
> not nice.
>
No, that's allright. input_free_device() takes after kfree() so that
it safe to call it with NULL pointer.
--
Dmitry
On Oct 29, 2007, at 8:33 AM, Dmitry Torokhov wrote:
> On 10/29/07, Dirk Hohndel <[email protected]> wrote:
>>
>> Actually, the more I look at the code that bails when it runs out
>> of memory,
>> the more I wonder about that.
>>
>> hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
>> input_dev = input_allocate_device();
>> if (!hidinput || !input_dev) {
>> kfree(hidinput);
>> input_free_device(input_dev);
>>
>> This either passes a NULL pointer to kfree or to
>> input_free_device. That's
>> not nice.
>>
>
> No, that's allright. input_free_device() takes after kfree() so that
> it safe to call it with NULL pointer.
Ahh - should have checked that.
Will the rest of the patch work with the call to hidinput_disconnect?
If yes then
I'll send a cleaned up version that removes the unnecessary checks in
the case here...
/D
On 10/29/07, Hohndel, Dirk <[email protected]> wrote:
>
> On Oct 29, 2007, at 8:33 AM, Dmitry Torokhov wrote:
>
> > On 10/29/07, Dirk Hohndel <[email protected]> wrote:
> >>
> >> Actually, the more I look at the code that bails when it runs out
> >> of memory,
> >> the more I wonder about that.
> >>
> >> hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> >> input_dev = input_allocate_device();
> >> if (!hidinput || !input_dev) {
> >> kfree(hidinput);
> >> input_free_device(input_dev);
> >>
> >> This either passes a NULL pointer to kfree or to
> >> input_free_device. That's
> >> not nice.
> >>
> >
> > No, that's allright. input_free_device() takes after kfree() so that
> > it safe to call it with NULL pointer.
>
> Ahh - should have checked that.
>
> Will the rest of the patch work with the call to hidinput_disconnect?
Yes, I think it will.
--
Dmitry
On Mon, 29 Oct 2007, Dirk Hohndel wrote:
> hidinput = kzalloc(sizeof(*hidinput), GFP_KERNEL);
> input_dev = input_allocate_device();
> if (!hidinput || !input_dev) {
> kfree(hidinput);
> input_free_device(input_dev);
> This either passes a NULL pointer to kfree or to input_free_device. That's
> not nice.
Actually both of the cases are fine -- it is valid to pass NULL pointer to
kfree() and to input_free_device() too.
> Would something like this work?
Yes, I think that this patch is in principle fine, modulo the redundant
NULL-ptr checks.
Thanks,
--
Jiri Kosina
[sorry - mail config screwup caused this to bounce for the more restrictive
of the recipient addresses, including the mailing lists, so resending again]
On Mon, Oct 29, 2007 at 06:28:36PM +0100, Jiri Kosina wrote:
>
> > Would something like this work?
>
> Yes, I think that this patch is in principle fine, modulo the redundant
> NULL-ptr checks.
Thanks everyone with helping me to get this right! Which I hope this one is :-)
/D
[PATCH] hidinput_connect ignores retval from input_register_device
signed-off-by: Dirk Hohndel <[email protected]>
---
drivers/hid/hid-input.c | 18 ++++++++++++++----
1 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
index dd332f2..aa9b52d 100644
--- a/drivers/hid/hid-input.c
+++ b/drivers/hid/hid-input.c
@@ -1152,7 +1152,7 @@ int hidinput_connect(struct hid_device *hid)
kfree(hidinput);
input_free_device(input_dev);
err_hid("Out of memory during hid input probe");
- return -1;
+ goto out_unwind;
}
input_set_drvdata(input_dev, hid);
@@ -1186,15 +1186,25 @@ int hidinput_connect(struct hid_device *hid)
* UGCI) cram a lot of unrelated inputs into the
* same interface. */
hidinput->report = report;
- input_register_device(hidinput->input);
+ if (input_register_device(hidinput->input))
+ goto out_cleanup;
hidinput = NULL;
}
}
- if (hidinput)
- input_register_device(hidinput->input);
+ if (hidinput && input_register_device(hidinput->input))
+ goto out_cleanup;
return 0;
+
+out_cleanup:
+ input_free_device(hidinput->input);
+ kfree(hidinput);
+out_unwind:
+ /* unwind the ones we already registered */
+ hidinput_disconnect(hid);
+
+ return -1;
}
EXPORT_SYMBOL_GPL(hidinput_connect);
--
gitgui.0.8.4.g8d863