[PATCH] Input: wacom_w8001 - compose the device basename in one place

[PATCH] Input: wacom_w8001 - compose the device basename in one place

[Ian Bridges]

The device basename is assembled by appends spread across
w8001_setup_pen() and w8001_setup_touch(). Each append restates the
buffer and its size.

Record the touch name suffix in struct w8001 beside the other probe
results, and compose the whole basename in w8001_connect() with a
single scnprintf(). The setup functions lose their buffer and size
arguments, and the append helper is no longer needed. The produced
names are unchanged.

The single compose point makes the possible length of basename
visible to the compiler, and the pen and touch name writes then trip
the format truncation warning. Write those two names with
scnprintf() as well.

Suggested-by: David Laight <david.laight.linux@gmail.com>
Link: https://lore.kernel.org/r/20260716103236.28c1b5bb@pumpkin
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
This is an optional cleanup on top of the already applied strlcat()
removal. That patch resolved the unsafe calls. This one only
simplifies the result, following David Laight's suggestion in that
thread. If the churn is not wanted, no action is needed.

The suggestion was to return the name from the setup functions. The
functions already return error codes, so the touch suffix is
recorded in struct w8001 beside the other probe results instead,
and the pen suffix follows from the error code that
w8001_connect() already has.

The patch was tested as follows.

- W=1 build of drivers/input/touchscreen/, zero warnings.
- A differential harness compiled the applied code and this patch
  side by side and ran both connect flows against identical
  fabricated query replies, exhaustively over the device state
  space. The composed names, return values and id assignments were
  identical in every case.

 drivers/input/touchscreen/wacom_w8001.c | 42 +++++++++++--------------
 1 file changed, 18 insertions(+), 24 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index d8d1cdc3f09e..320ef8ddb331 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -94,6 +94,7 @@ struct w8001 {
 	char phys[W8001_MAX_PHYS];
 	int type;
 	unsigned int pktlen;
+	const char *touch_sfx;
 	u16 max_touch_x;
 	u16 max_touch_y;
 	u16 max_pen_x;
@@ -417,15 +418,7 @@ static int w8001_detect(struct w8001 *w8001)
 	return 0;
 }
 
-static void w8001_append_suffix(char *dest, const char *suffix, size_t dest_sz)
-{
-	size_t used = strnlen(dest, dest_sz);
-
-	strscpy(dest + used, suffix, dest_sz - used);
-}
-
-static int w8001_setup_pen(struct w8001 *w8001, char *basename,
-			   size_t basename_sz)
+static int w8001_setup_pen(struct w8001 *w8001)
 {
 	struct input_dev *dev = w8001->pen_dev;
 	struct w8001_coord coord;
@@ -460,13 +453,11 @@ static int w8001_setup_pen(struct w8001 *w8001, char *basename,
 	}
 
 	w8001->id = 0x90;
-	w8001_append_suffix(basename, " Penabled", basename_sz);
 
 	return 0;
 }
 
-static int w8001_setup_touch(struct w8001 *w8001, char *basename,
-			     size_t basename_sz)
+static int w8001_setup_touch(struct w8001 *w8001)
 {
 	struct input_dev *dev = w8001->touch_dev;
 	struct w8001_touch_query touch;
@@ -505,19 +496,20 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 	input_abs_set_res(dev, ABS_X, touch.panel_res);
 	input_abs_set_res(dev, ABS_Y, touch.panel_res);
 
+	w8001->touch_sfx = " Touchscreen";
 	switch (touch.sensor_id) {
 	case 0:
 	case 2:
 		w8001->pktlen = W8001_PKTLEN_TOUCH93;
 		w8001->id = 0x93;
-		w8001_append_suffix(basename, " 1FG", basename_sz);
+		w8001->touch_sfx = " 1FG Touchscreen";
 		break;
 
 	case 1:
 	case 3:
 	case 4:
 		w8001->pktlen = W8001_PKTLEN_TOUCH9A;
-		w8001_append_suffix(basename, " 1FG", basename_sz);
+		w8001->touch_sfx = " 1FG Touchscreen";
 		w8001->id = 0x9a;
 		break;
 
@@ -541,7 +533,7 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 		input_abs_set_res(dev, ABS_MT_POSITION_X, touch.panel_res);
 		input_abs_set_res(dev, ABS_MT_POSITION_Y, touch.panel_res);
 
-		w8001_append_suffix(basename, " 2FG", basename_sz);
+		w8001->touch_sfx = " 2FG Touchscreen";
 		if (w8001->max_pen_x && w8001->max_pen_y)
 			w8001->id = 0xE3;
 		else
@@ -549,8 +541,6 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 		break;
 	}
 
-	w8001_append_suffix(basename, " Touchscreen", basename_sz);
-
 	return 0;
 }
 
@@ -600,7 +590,7 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	struct w8001 *w8001;
 	struct input_dev *input_dev_pen;
 	struct input_dev *input_dev_touch;
-	char basename[64] = "Wacom Serial";
+	char basename[64];
 	int err, err_pen, err_touch;
 
 	w8001 = kzalloc_obj(*w8001);
@@ -630,16 +620,20 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	/* For backwards-compatibility we compose the basename based on
 	 * capabilities and then just append the tool type
 	 */
-	err_pen = w8001_setup_pen(w8001, basename, sizeof(basename));
-	err_touch = w8001_setup_touch(w8001, basename, sizeof(basename));
+	err_pen = w8001_setup_pen(w8001);
+	err_touch = w8001_setup_touch(w8001);
 	if (err_pen && err_touch) {
 		err = -ENXIO;
 		goto fail3;
 	}
 
+	scnprintf(basename, sizeof(basename), "Wacom Serial%s%s",
+		  err_pen ? "" : " Penabled",
+		  err_touch ? "" : w8001->touch_sfx);
+
 	if (!err_pen) {
-		snprintf(w8001->pen_name, sizeof(w8001->pen_name),
-			 "%s Pen", basename);
+		scnprintf(w8001->pen_name, sizeof(w8001->pen_name),
+			  "%s Pen", basename);
 		input_dev_pen->name = w8001->pen_name;
 
 		w8001_set_devdata(input_dev_pen, w8001, serio);
@@ -654,8 +648,8 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
 	}
 
 	if (!err_touch) {
-		snprintf(w8001->touch_name, sizeof(w8001->touch_name),
-			 "%s Finger", basename);
+		scnprintf(w8001->touch_name, sizeof(w8001->touch_name),
+			  "%s Finger", basename);
 		input_dev_touch->name = w8001->touch_name;
 
 		w8001_set_devdata(input_dev_touch, w8001, serio);
-- 
2.47.3

Re: [PATCH] Input: wacom_w8001 - compose the device basename in one place

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Calling `input_free_device()` on a successfully registered device pointer during the error path, leading to a double-free.
--

--- Patch [1]: [PATCH] Input: wacom_w8001 - compose the device basename in one place ---
commit c5b975c9cd0e7f7aaa34347ef2a6ab2d8f4d90b0
Author: Ian Bridges <icb@fastmail.org>

Input: wacom_w8001 - compose the device basename in one place
    
This commit simplifies the device basename composition in the wacom_w8001
driver. It removes the append helper and instead records the touch name
suffix in the device structure, allowing the basename to be assembled in a
single step using scnprintf(). The patch resolves format truncation
warnings while keeping the produced names identical.

Link: https://lore.kernel.org/r/20260716103236.28c1b5bb@pumpkin

> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c

[ ... ]

> @@ -654,8 +648,8 @@ static int w8001_connect(struct serio *serio, struct serio_driver *drv)
>  	}
>  
>  	if (!err_touch) {
> -		snprintf(w8001->touch_name, sizeof(w8001->touch_name),
> -			 "%s Finger", basename);
> +		scnprintf(w8001->touch_name, sizeof(w8001->touch_name),
> +			  "%s Finger", basename);
>  		input_dev_touch->name = w8001->touch_name;
>  
>  		w8001_set_devdata(input_dev_touch, w8001, serio);

[Severity: High]
This is a pre-existing issue, but I noticed a potential double-free in the
error handling path of w8001_connect() that occurs just after this code block.

If input_register_device(w8001->touch_dev) fails, the code jumps to the fail4
label:

fail4:
        if (w8001->pen_dev)
                input_unregister_device(w8001->pen_dev);
fail3:
        serio_close(serio);
fail2:
        serio_set_drvdata(serio, NULL);
fail1:
        input_free_device(input_dev_pen);
        input_free_device(input_dev_touch);

Since input_unregister_device() manages dropping the reference count for
successfully registered devices, falling through to fail1 calls
input_free_device(input_dev_pen) on the same non-NULL pointer, which will
drop the reference count a second time.

Could this cause a double-free of the input_dev structure? Would it be safer
to set input_dev_pen = NULL in the fail4 block after unregistering?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/amqGsb0A6_dr9JLA@dev?part=1