Re: [PATCH] HID: usbhid: Use seq_buf_putc() in three functions

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Replacing `seq_buf_puts()` with `seq_buf_putc()` introduces a NUL-termination bug when the manufacturer string is exactly 127 bytes long, leading to an out-of-bounds read.
--

commit c41669030bdd8635988fc175a1caf4183a28344a
Author: Markus Elfring <elfring@users.sourceforge.net>

HID: usbhid: Use seq_buf_putc() in three functions

This patch updates three probe functions to use seq_buf_putc() instead
of seq_buf_puts() when inserting a single space character into sequence
buffers.

> diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
> --- a/drivers/hid/usbhid/hid-core.c
> +++ b/drivers/hid/usbhid/hid-core.c
> @@ -1412,7 +1412,7 @@ static int usbhid_probe(struct usb_interface *intf, const struct usb_device_id *
>  
>  	if (dev->product) {
>  		if (dev->manufacturer)
> -			seq_buf_puts(&hid_name, " ");
> +			seq_buf_putc(&hid_name, ' ');

[Severity: Medium]
Does this change introduce a missing null termination bug in usbhid_probe()
if the manufacturer string is exactly 127 bytes long?

The previous code using seq_buf_puts() requires two bytes of available
capacity (one for the space, one for the trailing null byte) and safely fails
if there isn't enough room.

Looking at seq_buf_putc(), it only checks if there is room for one byte.
If the sequence buffer is exactly 128 bytes long and already contains 127
characters from dev->manufacturer plus the null terminator, seq_buf_putc()
will overwrite the null terminator at index 127 with a space character.

This leaves hid->name completely unterminated. When exposed to userspace via
sysfs or input ioctls, this could lead to an out-of-bounds read into the
adjacent hid->phys memory field.

>  		seq_buf_puts(&hid_name, dev->product);
>  	}
>  
> diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
> --- a/drivers/hid/usbhid/usbkbd.c
> +++ b/drivers/hid/usbhid/usbkbd.c

[ ... ]

> @@ -302,7 +302,7 @@ static int usb_kbd_probe(struct usb_interface *iface,
>  
>  	if (dev->product) {
>  		if (dev->manufacturer)
> -			seq_buf_puts(&kbd_name, " ");
> +			seq_buf_putc(&kbd_name, ' ');

[Severity: Medium]
Does this have the same unterminated string issue for kbd->name in
usb_kbd_probe()?

If an untrusted USB device presents a manufacturer string of exactly 127
bytes, this overwrite could allow an out-of-bounds read into the adjacent
kbd->phys array.

>  		seq_buf_puts(&kbd_name, dev->product);
>  	}
>  
> diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c
> --- a/drivers/hid/usbhid/usbmouse.c
> +++ b/drivers/hid/usbhid/usbmouse.c

[ ... ]

> @@ -150,7 +150,7 @@ static int usb_mouse_probe(struct usb_interface *intf, const struct usb_device_i
>  
>  	if (dev->product) {
>  		if (dev->manufacturer)
> -			seq_buf_puts(&mouse_name, " ");
> +			seq_buf_putc(&mouse_name, ' ');

[Severity: Medium]
Is mouse->name vulnerable to the same null terminator overwrite here in
usb_mouse_probe()?

Similarly, this could leak the contents of mouse->phys to userspace.

>  		seq_buf_puts(&mouse_name, dev->product);
>  	}
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/89e9c672-efee-4ed2-9ef5-637ec9707f0b@web.de?part=1