Re: [PATCH] USB: usbsevseg: cleanup sysfs show handlers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] usb: usbsevseg: off-by-one in textmode_show() emits trailing NUL byte
--

commit a74c1b2c71cf0a70caf0b902cff95d63b0e182d0
Author: Dmitry Antipov <dmantipov@yandex.ru>

USB: usbsevseg: cleanup sysfs show handlers

This commit replaces sprintf() and strcat() calls in usbsevseg sysfs show
handlers with sysfs_emit() and sysfs_emit_at().

> diff --git a/drivers/usb/misc/usbsevseg.c b/drivers/usb/misc/usbsevseg.c
> index 89d25fcef6424..f456a7161a260 100644
> --- a/drivers/usb/misc/usbsevseg.c
> +++ b/drivers/usb/misc/usbsevseg.c

[ ... ]

> @@ -246,25 +246,19 @@ static ssize_t textmode_show(struct device *dev,
>  {
>  	struct usb_interface *intf = to_usb_interface(dev);
>  	struct usb_sevsegdev *mydev = usb_get_intfdata(intf);
> -	int i;
> -
> -	buf[0] = 0;
> +	int i, ret = 0;
>  
>  	for (i = 0; i < ARRAY_SIZE(display_textmodes); i++) {
> -		if (mydev->textmode == i) {
> -			strcat(buf, " [");
> -			strcat(buf, display_textmodes[i]);
> -			strcat(buf, "] ");
> -		} else {
> -			strcat(buf, " ");
> -			strcat(buf, display_textmodes[i]);
> -			strcat(buf, " ");
> -		}
> +		if (mydev->textmode == i)
> +			ret += sysfs_emit_at(buf, ret, " [%s] ",
> +					     display_textmodes[i]);
> +		else
> +			ret += sysfs_emit_at(buf, ret, " %s ",
> +					     display_textmodes[i]);
>  	}
> -	strcat(buf, "\n");
> -
>  
> -	return strlen(buf);
> +	ret += sysfs_emit_at(buf, ret - 1, "\n");
> +	return ret;

[Severity: Medium]
Does passing ret - 1 to sysfs_emit_at() while still adding its return value
to ret cause textmode_show() in drivers/usb/misc/usbsevseg.c to include a
trailing NUL byte in the output returned to userspace when reading the
textmode sysfs attribute?

At the end of the loop, buf[0 .. ret - 1] holds the formatted string with a
trailing space at buf[ret - 1] and a NUL terminator at buf[ret]. Calling
sysfs_emit_at(buf, ret - 1, "\n") overwrites the space at buf[ret - 1] with
a newline, writes the NUL terminator at buf[ret], and returns 1.

Adding that return value to ret increments ret to strlen(buf) + 1. When
sysfs_kf_seq_show() in fs/sysfs/file.c calls seq_commit(sf, count) with that
returned count, the NUL byte at buf[ret - 1] is committed into the seq_file
buffer and exposed to userspace.

Should this either pass ret instead of ret - 1 to preserve the original
trailing space before the newline, or avoid incrementing ret if overwriting
the trailing space was intentional?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261029124716.611540-1-dmantipov@yandex.ru?part=1