INPUT: fix hidinput_connect ignoring retval from input_register_device

PATCH In review Dirk Hohndel 4 participants 14 messages Oct 29, 2007 HID

lore

Share & tools

lore

Dirk Hohndel

author

2007-10-29 06:51 UTC #0 lore raw

PATCH 1/1 INPUT: fix hidinput_connect ignoring retval from input_register_device

[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(-)

drivers/hid/hid-input.c +8 -2

index dd332f2..c8640e7 100644
@@ -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;
 }

Signature · 1 line

gitgui.0.8.4.g8d863

Jeff Garzik

2007-10-29 07:53 UTC #1 lore raw

Quoted 21 lines · Dirk Hohndel

[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(-)

drivers/hid/hid-input.c +4 -1

index dd332f2..c8640e7 100644
@@ -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;
+				}

drivers/hid/hid-input.c int hidinput_connect(struct hid_device *hid) · line 1201

Quoted 10 lines

drivers/hid/hid-input.c +4 -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

Dirk Hohndel

author

2007-10-29 08:49 UTC #2 lore raw

Quoted 2 lines · Jeff Garzik

You would also want to kfree(hidinput) on failure too.

Oops, of course. Thanks for catching that. Here's the updated patch

[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(-)

drivers/hid/hid-input.c +10 -2

index dd332f2..880161b 100644
@@ -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;
 }

Signature · 1 line

gitgui.0.8.4.g8d863

Jiri Kosina

2007-10-29 10:14 UTC #4 lore raw

In reply to Dirk Hohndel

Quoted 3 lines · Dirk Hohndel

[INPUT] hidinput_connect incorrectly ignored return value from input_register_device
Signed-off-by Dirk Hohndel <[email protected]>

Will apply, thanks a lot.

Signature · 1 line

Jiri Kosina

Dmitry Torokhov

2007-10-29 12:54 UTC #5 lore raw

Quoted 7 lines · Jiri Kosina

Quoted 3 lines · Dirk Hohndel
[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.

Signature · 1 line

Dmitry

Jiri Kosina

2007-10-29 12:57 UTC #6 lore raw

Quoted 5 lines · Dmitry Torokhov

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.

Signature · 1 line

Jiri Kosina

Jeff Garzik

2007-10-29 14:49 UTC #7 lore raw

In reply to Dmitry Torokhov

Quoted 8 lines · Dmitry Torokhov

Quoted 6 lines · Jiri Kosina
Quoted 3 lines · Dirk Hohndel
[INPUT] hidinput_connect incorrectly ignored return value from input_register_device
Signed-off-by Dirk Hohndel <[email protected]>
Will apply

Quoted 8 lines

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

Dirk Hohndel

author

2007-10-29 15:20 UTC #8 lore raw

Quoted 16 lines · Jeff Garzik

Quoted 6 lines · Dmitry Torokhov
Quoted 5 lines · Jiri Kosina
Quoted 3 lines · Dirk Hohndel
[INPUT] hidinput_connect incorrectly ignored return value from input_register_device
Signed-off-by Dirk Hohndel <[email protected]>
Quoted 8 lines
Quoted 1 line
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.

Quoted 3 lines

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(-)

drivers/hid/hid-input.c +18 -6

index dd332f2..5bff5cc 100644
@@ -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);

Signature · 1 line

gitgui.0.8.4.g8d863

Dmitry Torokhov

2007-10-29 15:33 UTC #9 lore raw

Quoted 4 lines · Dirk Hohndel

Actually, the more I look at the code that bails when it runs out of memory, the more I wonder about that.

Quoted 9 lines

       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.

Signature · 1 line

Dmitry

Hohndel, Dirk

author

2007-10-29 15:38 UTC #10 lore raw

Quoted 11 lines · Dmitry Torokhov

Quoted 10 lines · Dirk Hohndel
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);

Quoted 6 lines

Quoted 3 lines
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...

Dmitry Torokhov

2007-10-29 17:11 UTC #11 lore raw

Quoted 20 lines · Dirk

Quoted 17 lines · Dmitry Torokhov
Quoted 15 lines · Dirk Hohndel
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.

Quoted 6 lines

Quoted 2 lines
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.

Signature · 1 line

Dmitry

Jiri Kosina

2007-10-29 17:28 UTC #12 lore raw

In reply to Dirk Hohndel

Quoted 7 lines · Dirk Hohndel

	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.

Quoted 1 line

Would something like this work?

Yes, I think that this patch is in principle fine, modulo the redundant NULL-ptr checks.

Thanks,

Signature · 1 line

Jiri Kosina

Dirk Hohndel

author

2007-10-29 22:50 UTC #13 lore raw

[sorry - mail config screwup caused this to bounce for the more restrictive of the recipient addresses, including the mailing lists, so resending again]

Quoted 5 lines · Jiri Kosina

Quoted 2 lines
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 :-)

[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(-)

drivers/hid/hid-input.c +14 -4

index dd332f2..aa9b52d 100644
@@ -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);

Signature · 1 line

gitgui.0.8.4.g8d863