Re: [PATCH 3/7] Input: gscps2 - protect buffer access in read and report helpers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware polling loop in `gscps2_writeb_output()` deadlocks if called synchronously with interrupts disabled.
- [High] Unbounded IO read loop in `gscps2_read_data()` can cause hard CPU lockup.
--

commit 9b1c3aa5a5184e24692c48f162f22f58a5172f7a
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: gscps2 - protect buffer access in read and report helpers

This commit adds a spinlock to gscps2_read_data() and gscps2_report_data() to
prevent a race condition on the ring buffer consumer index. It fixes an issue
where concurrent execution could lead to duplicate, skipped, or out-of-order
bytes.

> diff --git a/drivers/input/serio/gscps2.c b/drivers/input/serio/gscps2.c
> index 5b6e311f8a02c..fef6fffb6f865 100644
> --- a/drivers/input/serio/gscps2.c
> +++ b/drivers/input/serio/gscps2.c
> @@ -238,6 +238,8 @@ static void gscps2_read_data(struct gscps2port *ps2port)
>  {
>  	u8 status;
>  
> +	guard(spinlock_irqsave)(&ps2port->lock);
> +
>  	do {
>  		status = gscps2_readb_status(ps2port->addr);
>  		if (!(status & GSC_STAT_RBNE))

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

If the MMIO space becomes unreadable due to a hardware failure, device
disconnection, or bus error, gscps2_readb_status() typically returns 0xFF.

Since 0xFF & 0x01 (GSC_STAT_RBNE) is non-zero, this exit condition never
becomes true:

    if (!(status & GSC_STAT_RBNE))
        break;

The loop body reads the input data, but since the hardware is in a failed
state, it will also return 0xFF, causing the loop to never terminate.

[ ... ]

> @@ -255,7 +257,7 @@ static bool gscps2_report_data(struct gscps2port *ps2port)
>  	unsigned int rxflags;
>  	u8 data, status;
>  
> -	while (ps2port->act != ps2port->append) {
> +	while (true) {
>  		/*
>  		 * Did new data arrived while we read existing data ?
>  		 * If yes, exit now and let the new irq handler start
> @@ -264,17 +266,20 @@ static bool gscps2_report_data(struct gscps2port *ps2port)
>  		if (gscps2_readb_status(ps2port->addr) & GSC_STAT_CMPINTR)
>  			return true;
>  
> -		status = ps2port->buffer[ps2port->act].str;
> -		data   = ps2port->buffer[ps2port->act].data;
> +		scoped_guard(spinlock_irqsave, &ps2port->lock) {
> +			if (ps2port->act == ps2port->append)
> +				return false;
> +
> +			status = ps2port->buffer[ps2port->act].str;
> +			data   = ps2port->buffer[ps2port->act].data;
> +			ps2port->act = (ps2port->act + 1) & BUFFER_SIZE;
> +		}
>  
> -		ps2port->act = (ps2port->act + 1) & BUFFER_SIZE;
>  		rxflags = ((status & GSC_STAT_TERR) ? SERIO_TIMEOUT : 0) |
>  			  ((status & GSC_STAT_PERR) ? SERIO_PARITY  : 0);
>  
>  		serio_interrupt(ps2port->port, data, rxflags);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the
hardware polling loop in gscps2_writeb_output() to deadlock if called
synchronously from here?

When serio_interrupt() is called with interrupts disabled (since it is invoked
from the gscps2_interrupt() hardware IRQ handler), an input driver like atkbd
can call serio_write() synchronously.

This calls gscps2_writeb_output(), which has this loop:

    while (gscps2_readb_status(addr) & GSC_STAT_RBNE)
        cpu_relax();

If a new byte arrives from the hardware device, GSC_STAT_RBNE (Receive Buffer
Not Empty) becomes set.

The loop waits for RBNE to be cleared, but RBNE is only cleared when the
receive data register is read.

Since we are spinning inside this loop, we never read the data register, and
the IRQ handler cannot run to read it either.

>  	}
> -
> -	return false;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260830-gscps2-v1-0-c733d4cae7f9@gmail.com?part=3