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