HID: picolcd: clamp eeprom debugfs read to bytes actually received
Subsystems (3)
PATCH 1/1 HID: picolcd: clamp eeprom debugfs read to bytes actually received
picolcd_debug_eeprom_read() trusts resp->raw_data[2] -- a length byte supplied by the device in its REPORT_EE_DATA reply -- clamped only to the caller's read() count:
ret = resp->raw_data[2]; if (ret > s) ret = s; if (copy_to_user(u, resp->raw_data+3, ret))
It never checks resp->raw_size, the number of bytes picolcd_raw_event() actually copied into the 64-byte raw_data[] of the kmalloc'd struct picolcd_pending. A device (or a spoofed picoLCD) returning a length byte of 0xff, read with a count >= 255, makes copy_to_user() read past raw_data[] into adjacent slab memory and return it to userspace through the debugfs "eeprom" file:
BUG: KASAN: slab-out-of-bounds in _copy_to_user Read of size 255 ... picolcd_debug_eeprom_read+0x214/0x2f0 [hid_picolcd]
The debug-dump path in the same file already validates the device length byte against the received size before trusting it; this read does not. The file is created S_IRUSR (root-only) and a crafted device is needed, so it is neither unprivileged- nor remotely-triggerable.
Clamp the copy length to resp->raw_size - 3 (the payload actually received, minus the 3-byte header), floored at 0 for short replies.
--- drivers/hid/hid-picolcd_debugfs.c | 9 +++++++++ 1 file changed, 9 insertions(+)
drivers/hid/hid-picolcd_debugfs.c +9 -0
@@ -99,6 +99,15 @@ static ssize_t picolcd_debug_eeprom_read(struct file *f, char __user *u, ret = resp->raw_data[2 ];if (ret > s) ret = s; +/* +* raw_data[2] is a device-supplied length; also clamp it to +* what picolcd_raw_event() actually stored (raw_size), or a +* hostile device overruns the raw_data[] buffer. +*/ +if (ret > resp->raw_size -3 ) + ret = resp->raw_size -3 ; +if (ret <0 ) + ret =0 ;if (copy_to_user (u, resp->raw_data+3 , ret)) ret = -EFAULT ;else
Signature · 1 line
2.50.1 (Apple Git-155)
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues:
- [High] Use-After-Free on data->rc_dev and data->input_keys due to race in probe error path. --
--- Patch [1]: [PATCH] HID: picolcd: clamp eeprom debugfs read to bytes actually received --- commit 16232239549016339287f47b37b440446a47d534
Author: Ibrahim Hashimov <[email protected]>
HID: picolcd: clamp eeprom debugfs read to bytes actually received
This commit fixes an out-of-bounds slab read in picolcd_debug_eeprom_read() by correctly clamping the copy length to the received payload size.
drivers/hid/hid-picolcd_debugfs.c
Quoted 4 lines
drivers/hid/hid-picolcd_debugfs.c +0
-0
[ ... ]
[Severity: High]
This is a pre-existing issue, but I noticed a potential use-after-free in the probe error path.
If a USB device continuously sends interrupt IN packets to trigger picolcd_raw_event() while intentionally failing device initialization, could this cause a race condition?
In drivers/hid/hid-picolcd_core.c:picolcd_probe_lcd(), the probe error handler destroys the cir and keys objects while the HID device is still open:
err:
picolcd_exit_leds(data); picolcd_exit_framebuffer(data); picolcd_exit_backlight(data); picolcd_exit_lcd(data); picolcd_exit_cir(data); picolcd_exit_keys(data); return error;
Because hid_hw_close() isn't called until later, picolcd_raw_event() can concurrently dereference the freed data->rc_dev pointer outside of the spinlock in drivers/hid/hid-picolcd_cir.c:picolcd_raw_cir():
spin_lock_irqsave(&data->lock, flags);
if (!data->rc_dev || (data->status & PICOLCD_CIR_SHUN)) {
spin_unlock_irqrestore(&data->lock, flags);
return 1;
}
spin_unlock_irqrestore(&data->lock, flags);
// ... [IR data processing] ...
ir_raw_event_store(data->rc_dev, &rawir);The same timing issue appears to apply to data->input_keys, which is accessed locklessly in drivers/hid/hid-picolcd_core.c:picolcd_raw_keypad():
input_event(data->input_keys, EV_MSC, MSC_SCAN, raw_data[i]); if (raw_data[i] < PICOLCD_KEYS) key_code = data->keycode[raw_data[i]];
Could this allow concurrent events to use these pointers after they are freed by picolcd_exit_cir() and picolcd_exit_keys()?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 24 lines · Ibrahim Hashimov
picolcd_debug_eeprom_read()trusts resp->raw_data[2] -- a length byte supplied by the device in itsREPORT_EE_DATAreply -- clamped only to the caller'sread()count:ret = resp->raw_data[2]; if (ret > s) ret = s; if (copy_to_user(u, resp->raw_data+3, ret))It never checks
resp->raw_size, the number of bytespicolcd_raw_event()actually copied into the 64-byte raw_data[] of the kmalloc'd structpicolcd_pending. A device (or a spoofed picoLCD) returning a length byte of0xff, read with a count >= 255, makescopy_to_user()read past raw_data[] into adjacent slab memory and return it to userspace through the debugfs "eeprom" file:BUG: KASAN: slab-out-of-bounds in _copy_to_user Read of size 255 ... picolcd_debug_eeprom_read+0x214/0x2f0 [hid_picolcd]The debug-dump path in the same file already validates the device length byte against the received size before trusting it; this read does not. The file is created
S_IRUSR(root-only) and a crafted device is needed, so it is neither unprivileged- nor remotely-triggerable.
Quoted 7 lines
Clamp the copy length to
resp->raw_size- 3 (the payload actually received, minus the 3-byte header), floored at 0 for short replies.Fixes 9bbf2b98ba11 ("HID: add experimental access to PicoLCD device's EEPROM and FLASH")Signed-off-by Ibrahim Hashimov <[email protected]>Assisted-by AuditCode-AI:2026.07
Applied, thanks.
Signature · 2 lines
Jiri Kosina SUSE Labs