Re: [PATCH] Input: gscps2 - advance receive buffer write index

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] Lockless access to the ring buffer consumer index causes a data race and buffer corruption.
- [High] Lockless modifications and traversals of the global `ps2port_list` lead to list corruption and use-after-free.
- [High] Registering the shared IRQ before adding the device to the port list creates a window for an unhandled interrupt storm.
- [High] Unbounded hardware polling loops can hang the CPU if the device enters a faulty state.
- [High] The shared interrupt handler unconditionally returns `IRQ_HANDLED`, defeating the kernel's spurious interrupt detector.
--

commit b6fd4271cba05f59b05437ee042428ed8db2fb92
Author: Xu Rao <raoxu@uniontech.com>

Input: gscps2 - advance receive buffer write index

Commit 44f920069911 moved the receive loop into gscps2_read_data() and
gscps2_report_data(), but omitted the producer index update. This patch
restores the omitted index update so data reaches the serio core.

> diff --git a/drivers/input/serio/gscps2.c b/drivers/input/serio/gscps2.c
> index 22b2f57fd91f5..bf9b993f57330 100644
> --- a/drivers/input/serio/gscps2.c
> +++ b/drivers/input/serio/gscps2.c
> @@ -219,6 +219,7 @@ static void gscps2_read_data(struct gscps2port *ps2port)
>  		ps2port->buffer[ps2port->append].str = status;
>  		ps2port->buffer[ps2port->append].data =
>  				gscps2_readb_input(ps2port->addr);
> +		ps2port->append = (ps2port->append + 1) & BUFFER_SIZE;
>  	} while (true);
>  }

[Severity: High]
This is a pre-existing issue, but can this unbounded loop hang the CPU?

If the hardware enters a faulty state or is hot-unplugged, reading the status
register might continually return 0xFF. The loop condition earlier in this
function:

	if (!(status & GSC_STAT_RBNE))
		break;

would remain true forever (0xFF & 0x01). Similar unbounded polling exists in
gscps2_flush() and gscps2_writeb_output(). Should there be a timeout or retry
limit here?

[Severity: High]
This isn't a bug introduced by this patch, but does the lockless access to
ps2port->act cause a data race?

In gscps2_report_data(), ps2port->act is evaluated and advanced without
holding ps2port->lock:

gscps2_report_data() {
    ...
    while (ps2port->act != ps2port->append) {
        ...
        ps2port->act = (ps2port->act + 1) & BUFFER_SIZE;
    }
}

Since gscps2_interrupt() can be called synchronously from process context
(like in gscps2_writeb_output() with gscps2_interrupt(0, NULL)) while local
interrupts are enabled, a hardware IRQ could fire concurrently on another CPU.

Could both CPUs execute gscps2_report_data() simultaneously and corrupt the
buffer?

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to iterate
ps2port_list without locks?

gscps2_probe() and gscps2_remove() modify the global ps2port_list locklessly:

gscps2_probe() {
    ...
    list_add_tail(&ps2port->node, &ps2port_list);
}

Meanwhile, gscps2_interrupt() iterates this list without any locks:

gscps2_interrupt() {
    ...
    list_for_each_entry(ps2port, &ps2port_list, node) {
}

Since gscps2_interrupt() can be invoked at any time by a shared hardware IRQ
or manually via gscps2_writeb_output(), could this traversal race with a
concurrent probe or remove operation on a different CPU and lead to a
use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but does registering the shared
IRQ before adding the device to the port list create an interrupt storm
window?

In gscps2_probe(), the IRQ is requested before the device is added to the
list:

gscps2_probe() {
    ...
    if (request_irq(dev->irq, gscps2_interrupt, IRQF_SHARED, ...))
    ...
    list_add_tail(&ps2port->node, &ps2port_list);
}

If an interrupt fires immediately after registration, gscps2_interrupt() will
iterate the list, find nothing, and return IRQ_HANDLED without clearing the
hardware interrupt condition. For a level-triggered IRQ, could this
immediately re-fire and hang the CPU?

[Severity: High]
This isn't a bug introduced by this patch, but does the interrupt handler
always return IRQ_HANDLED?

gscps2_interrupt() is registered with IRQF_SHARED, but exits unconditionally
claiming it handled the interrupt:

gscps2_interrupt() {
    ...
    return IRQ_HANDLED;
}

If a different device sharing the same IRQ line continuously asserts the
interrupt due to a fault, the kernel relies on handlers returning IRQ_NONE
to detect the spurious storm and safely disable the IRQ line.

By always returning IRQ_HANDLED, does this prevent the kernel from detecting
spurious interrupts from neighboring devices?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/460B5655BA580C60+20260624094739.850306-1-raoxu@uniontech.com?part=1