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@xxxxxxxxxxxxxxxxxxxxx>
> 
> 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