[v2] HID: uclogic: Add NULL check in uclogic_input_configured

[PATCH v2] HID: uclogic: Add NULL check in uclogic_input_configured

Posted by Henry Martin 8 months, 3 weeks ago

devm_kasprintf() returns NULL when memory allocation fails. Currently,
uclogic_input_configured() does not check for this case, which results in a
NULL pointer dereference.

Add NULL check after devm_kasprintf() to prevent this issue.

Fixes: dd613a4e45f8 ("HID: uclogic: Correct devm device reference for hidinput input_dev name")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
V1 -> V2: 
Simplify the handing of the condition "suffix" with if/else suggested by
Markus. 
The current implementation (directly returning -ENOMEM) is reasonable because:
1. pen_input is just a cached pointer - no resources are allocated
2. evbit modification is a software-only configuration (hardware-independent)
3. No critical state needs rollback on memory allocation failure

 drivers/hid/hid-uclogic-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index d8008933c052..83625ec6a55a 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -118,35 +118,29 @@ static int uclogic_input_configured(struct hid_device *hdev,
 		}
 	}
 
-	if (!suffix) {
+	if (!suffix && hi->report->maxfield > 0) {
 		field = hi->report->field[0];
 
-		switch (field->application) {
-		case HID_GD_KEYBOARD:
+		if (field->application == HID_GD_KEYBOARD)
 			suffix = "Keyboard";
-			break;
-		case HID_GD_MOUSE:
+		else if (field->application == HID_GD_MOUSE)
 			suffix = "Mouse";
-			break;
-		case HID_GD_KEYPAD:
+		else if (field->application == HID_GD_KEYPAD)
 			suffix = "Pad";
-			break;
-		case HID_DG_PEN:
-		case HID_DG_DIGITIZER:
+		else if (field->application == HID_DG_PEN || field->application == HID_DG_DIGITIZER)
 			suffix = "Pen";
-			break;
-		case HID_CP_CONSUMER_CONTROL:
+		else if (field->application == HID_CP_CONSUMER_CONTROL)
 			suffix = "Consumer Control";
-			break;
-		case HID_GD_SYSTEM_CONTROL:
+		else if (field->application == HID_GD_SYSTEM_CONTROL)
 			suffix = "System Control";
-			break;
-		}
 	}
 
-	if (suffix)
+	if (suffix) {
 		hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
 						 "%s %s", hdev->name, suffix);
+		if (!hi->input->name)
+			return -ENOMEM;
+	}
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v2] HID: uclogic: Add NULL check in uclogic_input_configured

Posted by Markus Elfring 8 months, 3 weeks ago

> devm_kasprintf() return NULL if memory allocation fails. Currently,
…
                call?                               failed?


> Add NULL check after devm_kasprintf() to prevent this issue.

Do you propose to improve this function implementation a bit more?


…
> ---
> V1 -> V2:
> Simplify the handing of the condition "suffix" with if/else suggested by …

* I imagine that there is a need to offer such adjustments
  in separate update steps.
  https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n81

* Can another tag become more helpful?
  https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n598


> The current implementation (directly returning -ENOMEM) is reasonable because:
…

How much will such information influence the refinement of the change description?


How do you think about to append parentheses to a function name
also in the summary phrase?

Regards,
Markus

[PATCH v3] HID: uclogic: Add NULL check in uclogic_input_configured()

Posted by Henry Martin 8 months, 3 weeks ago

devm_kasprintf() returns NULL when memory allocation fails. Currently,
uclogic_input_configured() does not check for this case, which results
in a NULL pointer dereference.

Add NULL check after devm_kasprintf() to prevent this issue.

Fixes: dd613a4e45f8 ("HID: uclogic: Correct devm device reference for hidinput input_dev name")
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
V2 -> V3: Kept the original switch structure, only optimized the outer
conditional checks while maintaining safety requirements.
V1 -> V2: Simplify the handing of the condition "suffix" with if/else.

 drivers/hid/hid-uclogic-core.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/hid-uclogic-core.c b/drivers/hid/hid-uclogic-core.c
index d8008933c052..321c43fb06ae 100644
--- a/drivers/hid/hid-uclogic-core.c
+++ b/drivers/hid/hid-uclogic-core.c
@@ -142,11 +142,12 @@ static int uclogic_input_configured(struct hid_device *hdev,
 			suffix = "System Control";
 			break;
 		}
-	}
-
-	if (suffix)
+	} else {
 		hi->input->name = devm_kasprintf(&hdev->dev, GFP_KERNEL,
 						 "%s %s", hdev->name, suffix);
+		if (!hi->input->name)
+			return -ENOMEM;
+	}
 
 	return 0;
 }
-- 
2.34.1

Re: [PATCH v3] HID: uclogic: Add NULL check in uclogic_input_configured()

Posted by Jiri Kosina 7 months, 4 weeks ago

On Tue, 1 Apr 2025, Henry Martin wrote:

> devm_kasprintf() returns NULL when memory allocation fails. Currently,
> uclogic_input_configured() does not check for this case, which results
> in a NULL pointer dereference.
> 
> Add NULL check after devm_kasprintf() to prevent this issue.
> 
> Fixes: dd613a4e45f8 ("HID: uclogic: Correct devm device reference for hidinput input_dev name")
> Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>

Applied, thanks.

-- 
Jiri Kosina
SUSE Labs

Re: [PATCH v3] HID: uclogic: Add NULL check in uclogic_input_configured()

Posted by Markus Elfring 8 months, 3 weeks ago

> devm_kasprintf() return NULL if memory allocation fails. Currently,
…
                call?                               failed?


> Add NULL check after devm_kasprintf() to prevent this issue.

Do you propose to improve this function implementation a bit more?


…
> ---
> V2 -> V3: Kept the original switch structure, only optimized the outer
> conditional checks while maintaining safety requirements.
> V1 -> V2: Simplify the handing of the condition "suffix" with if/else.

* Under which circumstances will you begin to care more to offer such adjustments
  in separate update steps?
  https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n81

* Can another tag become more helpful?
  https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst?h=v6.14#n598


Can an other control flow variant be a bit nicer finally?

	if (suffix) {
…
	} else {
…
	}


Regards,
Markus