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

[Shihuang Liu]

On Mon, Oct 19, 2026 at 09:48:26AM +0200, Alejandro Colomar wrote:
> Hi Shihuang,
> 
> > Date: 2026-10-17 01:04:08+0800
> > From: Shihuang Liu <shlomojune6@xxxxxxxxx>
> >
> > 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@xxxxxxxxx>
> > ---
> >
> >  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.
> 

After sending this patch, I noticed the following comment from an
earlier patch:

> We don't want to do this as merely an open-coded strlen/sprintf; it doesn't gain us anything.

See:
<https://lore.kernel.org/all/202609151055.AFC6267@keescook/>

Replacing strlcat with an open-coded strlen + snprintf, or with alternatives 
such as strnlen, strlen, or strscat, would not provide any meaningful benefit.

According to Dmitry's reply to this series, my draft proposal is to replace 
the usb_make_path() + suffix pattern with a helper defined in the core header, as follows:

    static inline __printf(4, 5)
    int usb_make_subpath(struct usb_device *dev, char *buf, size_t size,
                 const char *fmt, ...)
    {
        struct va_format vaf;
        va_list args;
        int actual;

        va_start(args, fmt);
        vaf.fmt = fmt;
        vaf.va = &args;
        actual = snprintf(buf, size, "usb-%s-%s/%pV", dev->bus->bus_name,
                  dev->devpath, &vaf);
        va_end(args);

        return actual >= size ? -1 : actual;
    }


> 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.

Great. Improving its efficiency is exactly what I would like to do as well

Thanks for reviewing this and for your suggestions.

Shihuang