[PATCH] Input: wacom_w8001 - replace strlcat() with a strscpy() helper

[PATCH] Input: wacom_w8001 - replace strlcat() with a strscpy() helper

[Ian Bridges]

In preparation for removing the strlcat() API[1], replace its five
uses with a small append helper built on strnlen() and strscpy().

The five calls append device name fragments to a basename buffer
that grows in place across the setup functions. The helper takes
the same arguments as strlcat() and writes the same bytes, including
when a fragment is truncated.

Link: https://github.com/KSPP/linux/issues/370 [1]
Signed-off-by: Ian Bridges <icb@fastmail.org>
---
The produced device names are unchanged.

A seq_buf conversion was considered instead of the helper and set
aside for two reasons. seq_buf_puts() drops a whole fragment when it
does not fit, while strlcat() writes the partial fragment, so the
produced bytes would change at the truncation boundary. The setup
functions would also change signature to take a struct seq_buf
pointer. A v2 can adopt seq_buf if that tradeoff is preferred.

The patch was tested as follows.

- W=1 build of drivers/input/touchscreen/, zero warnings.
- A userspace differential harness compiled the old and the new
  functions side by side. The name buffers were byte identical over
  their full length, and the return values and id assignments matched
  in every case.

 drivers/input/touchscreen/wacom_w8001.c | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
index 45930d731873..d8d1cdc3f09e 100644
--- a/drivers/input/touchscreen/wacom_w8001.c
+++ b/drivers/input/touchscreen/wacom_w8001.c
@@ -417,6 +417,13 @@ 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)
 {
@@ -453,7 +460,7 @@ static int w8001_setup_pen(struct w8001 *w8001, char *basename,
 	}
 
 	w8001->id = 0x90;
-	strlcat(basename, " Penabled", basename_sz);
+	w8001_append_suffix(basename, " Penabled", basename_sz);
 
 	return 0;
 }
@@ -503,14 +510,14 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 	case 2:
 		w8001->pktlen = W8001_PKTLEN_TOUCH93;
 		w8001->id = 0x93;
-		strlcat(basename, " 1FG", basename_sz);
+		w8001_append_suffix(basename, " 1FG", basename_sz);
 		break;
 
 	case 1:
 	case 3:
 	case 4:
 		w8001->pktlen = W8001_PKTLEN_TOUCH9A;
-		strlcat(basename, " 1FG", basename_sz);
+		w8001_append_suffix(basename, " 1FG", basename_sz);
 		w8001->id = 0x9a;
 		break;
 
@@ -534,7 +541,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);
 
-		strlcat(basename, " 2FG", basename_sz);
+		w8001_append_suffix(basename, " 2FG", basename_sz);
 		if (w8001->max_pen_x && w8001->max_pen_y)
 			w8001->id = 0xE3;
 		else
@@ -542,7 +549,7 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
 		break;
 	}
 
-	strlcat(basename, " Touchscreen", basename_sz);
+	w8001_append_suffix(basename, " Touchscreen", basename_sz);
 
 	return 0;
 }
-- 
2.47.3

Re: [PATCH] Input: wacom_w8001 - replace strlcat() with a strscpy() helper

[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 after `input_unregister_device()` in the error path leads to a Use-After-Free / Double Free vulnerability.
--

--- Patch [1]: [PATCH] Input: wacom_w8001 - replace strlcat() with a strscpy() helper ---
commit e4f21712f81562322908d563a501b6a8bead0360
Author: Ian Bridges <icb@fastmail.org>

Input: wacom_w8001 - replace strlcat() with a strscpy() helper

This commit replaces five uses of strlcat() with a custom strscpy()-based
helper, w8001_append_suffix(). This prepares for the eventual removal of
the strlcat() API.

Link: https://github.com/KSPP/linux/issues/370 [1]

> diff --git a/drivers/input/touchscreen/wacom_w8001.c b/drivers/input/touchscreen/wacom_w8001.c
> index 45930d7318733..d8d1cdc3f09e3 100644
> --- a/drivers/input/touchscreen/wacom_w8001.c
> +++ b/drivers/input/touchscreen/wacom_w8001.c

[ ... ]

> @@ -534,7 +541,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);
>  
> -		strlcat(basename, " 2FG", basename_sz);
> +		w8001_append_suffix(basename, " 2FG", basename_sz);
>  		if (w8001->max_pen_x && w8001->max_pen_y)
>  			w8001->id = 0xE3;
>  		else
> @@ -542,7 +549,7 @@ static int w8001_setup_touch(struct w8001 *w8001, char *basename,
>  		break;
>  	}
>  
> -	strlcat(basename, " Touchscreen", basename_sz);
> +	w8001_append_suffix(basename, " Touchscreen", basename_sz);
>  
>  	return 0;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but calling input_free_device()
on a successfully registered device after input_unregister_device() in the
error path of w8001_connect() leads to a double free.

If w8001_connect() successfully registers the pen device but subsequently
fails to register the touch device, execution jumps to the fail4 label:

drivers/input/touchscreen/wacom_w8001.c:w8001_connect() {
...
	if (!err_touch) {
		...
		err = input_register_device(w8001->touch_dev);
		if (err)
			goto fail4;
...
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);
...
}

The fail4 block calls input_unregister_device(w8001->pen_dev), which drops
the device reference. Execution then falls through to the fail1 label, where
input_free_device(input_dev_pen) is unconditionally called. Because
input_dev_pen and w8001->pen_dev point to the same device, this results in a
second free.

Could this error path be updated to avoid the double free?

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