* Re: [PATCH] Input: usbtouchscreen - validate Nexio reply length
2026-06-30 6:52 [PATCH] Input: usbtouchscreen - validate Nexio reply length Pengpeng Hou
@ 2026-06-30 7:04 ` sashiko-bot
0 siblings, 0 replies; 2+ messages in thread
From: sashiko-bot @ 2026-06-30 7:04 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: dmitry.torokhov, linux-input
Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
Pre-existing issues:
- [Critical] Missing `usb_kill_urb()` in `usbtouch_disconnect` for devices with `irq_always` leads to a Use-After-Free.
- [High] Missing URB restart in `usbtouch_reset_resume` for `irq_always` devices.
- [High] Heap out-of-bounds read in `usbtouch_process_multi()` when handling fragmented packets from a malicious device.
- [Medium] The driver unconditionally submits a shared ACK URB in the interrupt handler without checking if it is already active, leading to a WARN log-spam Denial of Service.
--
commit 5d3372d0fad0625b140375a0207438f22cdad247
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
Input: usbtouchscreen - validate Nexio reply length
This commit updates nexio_init() to require at least two bytes of reply
data before checking the embedded reply length at buf[1]. This prevents
reading uninitialized or out-of-bounds data when actual_len is 1.
> diff --git a/drivers/input/touchscreen/usbtouchscreen.c b/drivers/input/touchscreen/usbtouchscreen.c
> index 0bbacb517c286..0e07bcce5f96b 100644
> --- a/drivers/input/touchscreen/usbtouchscreen.c
> +++ b/drivers/input/touchscreen/usbtouchscreen.c
> @@ -1011,7 +1011,7 @@ static int nexio_init(struct usbtouch_usb *usbtouch)
> ret = usb_bulk_msg(dev, usb_rcvbulkpipe(dev, input_ep),
> buf, NEXIO_BUFSIZE, &actual_len,
> NEXIO_TIMEOUT);
> - if (ret < 0 || actual_len < 1 || buf[1] != actual_len)
> + if (ret < 0 || actual_len < 2 || buf[1] != actual_len)
> continue;
> switch (buf[0]) {
> case 0x83: /* firmware version */
[Severity: High]
This isn't a bug introduced by this patch, but is it possible that the
polling URB will never be restarted for irq_always devices after a reset?
In usbtouch_reset_resume():
if (usbtouch->is_open)
return usb_submit_urb(usbtouch->irq, GFP_NOIO);
Unlike usbtouch_resume(), this misses the || usbtouch->type->irq_always
condition. If an irq_always device goes through a reset-resume cycle while
not actively opened by userspace, does this permanently break the hardware
polling contract and freeze the device until it is explicitly reopened?
[Severity: Critical]
This is a pre-existing issue, but does usbtouch_disconnect() have a
use-after-free risk for irq_always devices?
usbtouch_disconnect() calls:
usb_free_urb(usbtouch->irq);
if (usbtouch->type->exit)
usbtouch->type->exit(usbtouch);
usbtouch_free_buffers(interface_to_usbdev(intf), usbtouch);
kfree(usbtouch);
Since usbtouch_close() omits calling usb_kill_urb() for irq_always devices,
and usb_free_urb() only decrements the reference count without cancelling
the in-flight URB, the URB remains active.
When usbtouch_disconnect() returns, usb_disable_interface() will forcibly
cancel active URBs, triggering the completion handler usbtouch_irq():
struct usbtouch_usb *usbtouch = urb->context;
struct device *dev = &usbtouch->interface->dev;
Won't this completion handler access the already-freed usbtouch context
upon normal device unbind or unplug?
[Severity: High]
This isn't a bug introduced by this patch, but does usbtouch_process_multi()
properly validate bounds against the actual packet length?
tmp = pkt_len - usbtouch->buf_len;
if (usbtouch->buf_len + tmp >= usbtouch->type->rept_size)
goto out_flush_buf;
memcpy(usbtouch->buffer + usbtouch->buf_len, pkt, tmp);
If a device advertises a small wMaxPacketSize, the usbtouch->data buffer
is allocated small. When calculating tmp to read from the current URB
payload pkt, it doesn't verify that the received data length is at least
tmp.
If tmp is greater than the received length, will this memcpy() read past
the end of the newly received data and out of the bounds of the small heap
allocation?
[Severity: Medium]
This is a pre-existing issue, but can the unconditional submission of
the ACK URB lead to a log-spam denial of service?
In nexio_read_data():
/* send ACK */
ret = usb_submit_urb(priv->ack, GFP_ATOMIC);
if (ret)
dev_warn(dev, "Failed to submit ACK URB: %d\n", ret);
If a device sends rapid touch packets via the bulk IN endpoint faster
than they can be acknowledged, the driver will attempt to submit the
already-active ACK URB.
Since the USB core rejects resubmitting an active URB and emits a kernel
warning, could a malicious or buggy device spam IN packets to trigger a
continuous stream of kernel WARN stack traces?
--
Sashiko AI review · https://sashiko.dev/#/patchset/2026063007023179.2-ccfa108-0009-Input-usbtouchscreen---vali-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 2+ messages in thread