Re: [PATCH] Input: gscps2 - supply PA-RISC keyboard keymap via device property

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free in shared interrupt handler during device removal.
- [High] Data race and hardware state corruption due to missing lock in `gscps2_flush()`.
--

--- Patch [1]: [PATCH] Input: gscps2 - supply PA-RISC keyboard keymap via device property ---
commit a01a8cad369fa3c00ef5c659a70dd84ac7f1e275
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: gscps2 - supply PA-RISC keyboard keymap via device property

This commit modifies the gscps2 PS/2 port driver to attach a software node
with a linux,keymap property to the serio device. This replaces compile-time
architecture-specific keyboard hacks in the generic atkbd driver. It also
moves the keymap definitions to the serio driver and resolves conflicting
keys via a compile-time config.

> diff --git a/drivers/input/serio/gscps2.c b/drivers/input/serio/gscps2.c
> index bf9b993f57330..cd1bca57fda33 100644
> --- a/drivers/input/serio/gscps2.c
> +++ b/drivers/input/serio/gscps2.c
[ ... ]
> @@ -434,6 +449,9 @@ static void __exit gscps2_remove(struct parisc_device *dev)
>  {
>  	struct gscps2port *ps2port = dev_get_drvdata(&dev->dev);
>  
> +	if (ps2port->id == GSC_ID_KEYBOARD)
> +		device_remove_software_node(&ps2port->port->dev);
> +
>  	serio_unregister_port(ps2port->port);
>  	free_irq(dev->irq, ps2port);

[Severity: High]
This is a pre-existing issue, but could this sequence in gscps2_remove() lead
to a use-after-free if a shared interrupt fires?

serio_unregister_port() synchronously disconnects the driver and drops the
reference to the port, freeing the serio structure. However, the device
remains in ps2port_list and the shared IRQ is still active since free_irq()
hasn't been called yet.

If another device on the same shared IRQ line triggers an interrupt during
this window, gscps2_interrupt() will iterate over ps2port_list, find this
port, and call serio_interrupt(ps2port->port, ...).

Since ps2port->port was just freed by serio_unregister_port(), will this
dereference freed memory?

Should the device be removed from ps2port_list and its IRQ disabled via
free_irq() before unregistering the serio port?

>  	gscps2_flush(ps2port);

[Severity: High]
This is also a pre-existing issue, but is there a missing lock in
gscps2_flush() that causes a data race?

Looking at gscps2_flush():

static void gscps2_flush(struct gscps2port *ps2port)
{
        while (gscps2_readb_status(ps2port->addr) & GSC_STAT_RBNE)
                gscps2_readb_input(ps2port->addr);
        ps2port->act = ps2port->append = 0;
}

This modifies the shared act and append indices and reads hardware registers
without acquiring ps2port->lock.

When gscps2_open() executes, it calls gscps2_reset() which in turn calls
gscps2_flush(). Since the shared IRQ is requested prior to registration, a
concurrent interrupt triggered by another shared device could cause
gscps2_interrupt() to run.

Because gscps2_interrupt() acquires the lock, reads the same registers, and
modifies append via gscps2_read_data(), will this cause non-serialized
hardware access and buffer state corruption?

>  	list_del(&ps2port->node);
>  	iounmap(ps2port->addr);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/am_9BvmZu9g4RlUM@google.com?part=1