Re: [PATCH 2/5] Input: misc - replace strlcat() with strscpy()

[Alejandro Colomar <alx+linux-hardening@kernel.org>]

[-- Attachment #1: Type: text/plain, Size: 9387 bytes --]

Hi Shihuang,

> Date: 2026-10-17 01:04:08+0800
> From: Shihuang Liu <shlomojune6@gmail.com>
>
> The ati_remote2, cm109, ims-pcu, keyspan_remote, powermate and
> yealink drivers build the physical path by appending a constant
> "/input0" suffix with strlcat() after usb_make_path().
> 
> In preparation for removing the deprecated strlcat() API[1],
> replace it with strnlen() followed by strscpy() into the remainder
> of the buffer, as commit 34b228b739f5 ("HID: usbhid: replace
> strlcat with better alternatives") did for the usbhid drivers.
> 
> Link: https://github.com/KSPP/linux/issues/370 [1]
> Signed-off-by: Shihuang Liu <shlomojune6@gmail.com>
> ---
>
>  drivers/input/misc/ati_remote2.c    | 4 +++-
>  drivers/input/misc/cm109.c          | 4 +++-
>  drivers/input/misc/ims-pcu.c        | 8 ++++++--
>  drivers/input/misc/keyspan_remote.c | 4 +++-
>  drivers/input/misc/powermate.c      | 4 +++-
>  drivers/input/misc/yealink.c        | 4 +++-
>  6 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/input/misc/ati_remote2.c b/drivers/input/misc/ati_remote2.c
> index 8db2dca84975c..929163f52177e 100644
> --- a/drivers/input/misc/ati_remote2.c
> +++ b/drivers/input/misc/ati_remote2.c
> @@ -767,6 +767,7 @@ static int ati_remote2_probe(struct usb_interface *interface, const struct usb_d
>  	struct usb_device *udev = interface_to_usbdev(interface);
>  	struct usb_host_interface *alt = interface->cur_altsetting;
>  	struct ati_remote2 *ar2;
> +	size_t len;
>  	int r;
>  
>  	if (alt->desc.bInterfaceNumber)
> @@ -823,7 +824,8 @@ static int ati_remote2_probe(struct usb_interface *interface, const struct usb_d
>  		goto fail3;
>  
>  	usb_make_path(udev, ar2->phys, sizeof(ar2->phys));
> -	strlcat(ar2->phys, "/input0", sizeof(ar2->phys));
> +	len = strnlen(ar2->phys, sizeof(ar2->phys));

Why strnlen(3)?  ar2->phys should be a string, right?  If it is not,
then we'd already have a buffer overread in strlcat(3), and then the
commit should be reporting a bug fix.  This should use strlen(3)
instead.

> +	strscpy(ar2->phys + len, "/input0", sizeof(ar2->phys) - len);

NAKing this.  This is precisely what strlcat(3)'s documentation in
OpenBSD says you shouldn't do:

	Since it is known how many characters were copied the first
	time, things can be sped up a bit by using a copy instead of an
	append:

		char *dir, *file, pname[PATH_MAX];
		size_t n;

		...

		n = strlcpy(pname, dir, sizeof(pname));
		if (n >= sizeof(pname))
			goto toolong;
		if (strlcpy(pname + n, file, sizeof(pname) - n) >= sizeof(pname) - n)
			goto toolong;

	However, one may question the validity of such optimizations, as
	they defeat the whole purpose of strlcpy() and strlcat().  As a
	matter of fact, the first version of this manual page got it
	wrong.

<https://man.openbsd.org/strlcpy>

The right approach is to have a function that does the right thing.
There are two viable alternatives:

-  Write strscat().  It would take the strlen() internally, and so it
   would be unnecessarily inefficient.  Prefer the next one.

-  Write stpecpy().  It returns a pointer, which allows chaining easily.

Here's a viable implementation of stpecpy():

	char *
	stpecpy(char *dst, const char end[0], const char *restrict src)
	{
		ssize_t  dlen;

		if (dst == NULL)
			return NULL;

		dlen = strscpy(dst, src, end - dst);
		if (dlen < 0)
			return NULL;

		return dst + dlen;
	}

This would need to be accompanied by a seprintf() too, which would be
used by usb_make_path() to produce a pointer suitable for chaining.
I've been working on adding those.

If you want to keep it easy, I'd do strscat().  We can modify it later
to be more efficient.  After all, we already have the inefficiency of
calling strlcat(3) now.


Have a lovely day!
Alex

>  	strlcat(ar2->name, "ATI Remote Wonder II", sizeof(ar2->name));
>  
> diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
> index 353d3c1d347d5..d3aee3c498702 100644
> --- a/drivers/input/misc/cm109.c
> +++ b/drivers/input/misc/cm109.c
> @@ -685,6 +685,7 @@ static int cm109_usb_probe(struct usb_interface *intf,
>  	struct input_dev *input_dev = NULL;
>  	int ret, pipe, i;
>  	int error = -ENOMEM;
> +	size_t len;
>  
>  	interface = intf->cur_altsetting;
>  
> @@ -766,7 +767,8 @@ static int cm109_usb_probe(struct usb_interface *intf,
>  
>  	/* find out the physical bus location */
>  	usb_make_path(udev, dev->phys, sizeof(dev->phys));
> -	strlcat(dev->phys, "/input0", sizeof(dev->phys));
> +	len = strnlen(dev->phys, sizeof(dev->phys));
> +	strscpy(dev->phys + len, "/input0", sizeof(dev->phys) - len);
>  
>  	/* register settings for the input device */
>  	input_dev->name = nfo->name;
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index b1a0edcc49b4e..697813fb11da8 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -205,6 +205,7 @@ static int ims_pcu_setup_buttons(struct ims_pcu *pcu,
>  	struct input_dev *input;
>  	int i;
>  	int error;
> +	size_t len;
>  
>  	input = input_allocate_device();
>  	if (!input) {
> @@ -216,7 +217,8 @@ static int ims_pcu_setup_buttons(struct ims_pcu *pcu,
>  		 "IMS PCU#%d Button Interface", pcu->device_no);
>  
>  	usb_make_path(pcu->udev, buttons->phys, sizeof(buttons->phys));
> -	strlcat(buttons->phys, "/input0", sizeof(buttons->phys));
> +	len = strnlen(buttons->phys, sizeof(buttons->phys));
> +	strscpy(buttons->phys + len, "/input0", sizeof(buttons->phys) - len);
>  
>  	memcpy(buttons->keymap, keymap, sizeof(*keymap) * keymap_len);
>  
> @@ -286,6 +288,7 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>  	struct ims_pcu_gamepad *gamepad;
>  	struct input_dev *input;
>  	int error;
> +	size_t len;
>  
>  	gamepad = kzalloc_obj(*gamepad);
>  	input = input_allocate_device();
> @@ -302,7 +305,8 @@ static int ims_pcu_setup_gamepad(struct ims_pcu *pcu)
>  		 "IMS PCU#%d Gamepad Interface", pcu->device_no);
>  
>  	usb_make_path(pcu->udev, gamepad->phys, sizeof(gamepad->phys));
> -	strlcat(gamepad->phys, "/input1", sizeof(gamepad->phys));
> +	len = strnlen(gamepad->phys, sizeof(gamepad->phys));
> +	strscpy(gamepad->phys + len, "/input1", sizeof(gamepad->phys) - len);
>  
>  	input->name = gamepad->name;
>  	input->phys = gamepad->phys;
> diff --git a/drivers/input/misc/keyspan_remote.c b/drivers/input/misc/keyspan_remote.c
> index 70cd6586459ef..f0f31f2823d57 100644
> --- a/drivers/input/misc/keyspan_remote.c
> +++ b/drivers/input/misc/keyspan_remote.c
> @@ -430,6 +430,7 @@ static int keyspan_probe(struct usb_interface *interface, const struct usb_devic
>  	struct usb_keyspan *remote;
>  	struct input_dev *input_dev;
>  	int i, error;
> +	size_t len;
>  
>  	error = usb_find_int_in_endpoint(interface->cur_altsetting, &endpoint);
>  	if (error)
> @@ -482,7 +483,8 @@ static int keyspan_probe(struct usb_interface *interface, const struct usb_devic
>  			 le16_to_cpu(udev->descriptor.idProduct));
>  
>  	usb_make_path(udev, remote->phys, sizeof(remote->phys));
> -	strlcat(remote->phys, "/input0", sizeof(remote->phys));
> +	len = strnlen(remote->phys, sizeof(remote->phys));
> +	strscpy(remote->phys + len, "/input0", sizeof(remote->phys) - len);
>  	memcpy(remote->keymap, keyspan_key_table, sizeof(remote->keymap));
>  
>  	input_dev->name = remote->name;
> diff --git a/drivers/input/misc/powermate.c b/drivers/input/misc/powermate.c
> index 754379d2625cf..dbf9f06f3059e 100644
> --- a/drivers/input/misc/powermate.c
> +++ b/drivers/input/misc/powermate.c
> @@ -299,6 +299,7 @@ static int powermate_probe(struct usb_interface *intf, const struct usb_device_i
>  	struct input_dev *input_dev;
>  	int pipe, maxp;
>  	int error = -ENOMEM;
> +	size_t len;
>  
>  	interface = intf->cur_altsetting;
>  	if (interface->desc.bNumEndpoints < 1)
> @@ -334,7 +335,8 @@ static int powermate_probe(struct usb_interface *intf, const struct usb_device_i
>  	pm->input = input_dev;
>  
>  	usb_make_path(udev, pm->phys, sizeof(pm->phys));
> -	strlcat(pm->phys, "/input0", sizeof(pm->phys));
> +	len = strnlen(pm->phys, sizeof(pm->phys));
> +	strscpy(pm->phys + len, "/input0", sizeof(pm->phys) - len);
>  
>  	spin_lock_init(&pm->lock);
>  
> diff --git a/drivers/input/misc/yealink.c b/drivers/input/misc/yealink.c
> index 560f895a00cda..1a3bebc8c3513 100644
> --- a/drivers/input/misc/yealink.c
> +++ b/drivers/input/misc/yealink.c
> @@ -812,6 +812,7 @@ static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  	struct yealink_dev *yld;
>  	struct input_dev *input_dev;
>  	int ret, pipe, i;
> +	size_t len;
>  
>  	interface = intf->cur_altsetting;
>  
> @@ -891,7 +892,8 @@ static int usb_probe(struct usb_interface *intf, const struct usb_device_id *id)
>  
>  	/* find out the physical bus location */
>  	usb_make_path(udev, yld->phys, sizeof(yld->phys));
> -	strlcat(yld->phys,  "/input0", sizeof(yld->phys));
> +	len = strnlen(yld->phys, sizeof(yld->phys));
> +	strscpy(yld->phys + len, "/input0", sizeof(yld->phys) - len);
>  
>  	/* register settings for the input device */
>  	input_dev->name = nfo->name;
> -- 
> 2.43.0
> 
> 

-- 
<https://www.alejandro-colomar.es>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]