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

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

[Markus Elfring]

From: Markus Elfring <elfring@users.sourceforge.net>
Date: Thu, 25 Jun 2026 11:11:26 +0200

A single space character should occasionally be put into a sequence buffer.
Thus use the function “seq_buf_putc” in these implementations.

The source code was transformed by using the Coccinelle software.

Signed-off-by: Markus Elfring <elfring@users.sourceforge.net>
---
 drivers/hid/usbhid/hid-core.c | 2 +-
 drivers/hid/usbhid/usbkbd.c   | 2 +-
 drivers/hid/usbhid/usbmouse.c | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/hid/usbhid/hid-core.c b/drivers/hid/usbhid/hid-core.c
index 96b0181cf819..a755102b8cfe 100644
--- 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, ' ');
 		seq_buf_puts(&hid_name, dev->product);
 	}
 
diff --git a/drivers/hid/usbhid/usbkbd.c b/drivers/hid/usbhid/usbkbd.c
index 83d4df0d7a45..02abd54eca49 100644
--- 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, ' ');
 		seq_buf_puts(&kbd_name, dev->product);
 	}
 
diff --git a/drivers/hid/usbhid/usbmouse.c b/drivers/hid/usbhid/usbmouse.c
index b3b2abeee614..9fce51d65cd6 100644
--- 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, ' ');
 		seq_buf_puts(&mouse_name, dev->product);
 	}
 
-- 
2.54.0

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

[sashiko-bot]

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

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

[Markus Elfring]

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

These functions were used without error detection so far.


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

Can descriptions be extended for possible failure conditions?
https://elixir.bootlin.com/linux/v7.1.1/source/lib/seq_buf.c#L203-L221


> 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);

How likely is it that such product information will be appended as expected
at mentioned source code places?

Regards,
Markus

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

[Markus Elfring]

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

I see a need to reconsider such information in more detail.



> Looking at seq_buf_putc(), it only checks if there is room for one byte.
> If the sequence buffer is exactly 128 bytes long

This is the case as such a technical detail can be determined from the member “name”
of the data structure “hid_device”.
https://elixir.bootlin.com/linux/v7.1.1/source/include/linux/hid.h#L721


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

Other contributors can add further experiences for corresponding data length characteristics.


> This leaves hid->name completely unterminated.

I would like to point the comment “Don't count the trailing null byte against the capacity”
once more from the implementation of the function “seq_buf_puts”.
https://elixir.bootlin.com/linux/v7.1.1/source/lib/seq_buf.c#L193-L194


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

If such a data reuse would be attempted, API requirements for sequence buffers
should probably be taken better into account at a concrete place.

Regards,
Markus