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

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

New issues:
- [Medium] Missing keycode mapping for PA-RISC keyboard scancode 0x92 leads to spurious dmesg warnings.
- [Medium] The managed software node allocated for the keyboard serio device leaks if port registration is canceled before execution.

Pre-existing issues:
- [High] Lockless modifications to `ps2port_list` race with lockless traversal in the shared hardirq handler, leading to potential list corruption and panics.
- [High] `gscps2_remove()` frees the `serio` port before unregistering the interrupt handler, creating a use-after-free vulnerability.
- [High] The interrupt handler unconditionally returns `IRQ_HANDLED` for a shared IRQ line, defeating spurious interrupt detection.
--

--- Patch [1]: [PATCH] Input: gscps2 - supply PA-RISC keyboard keymap via device property ---
commit 66edd58351136502cfbfeb7361cae9d5adeebf48
Author: Dmitry Torokhov <[email protected]>

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

This patch refactors the PA-RISC keyboard driver to supply the keymap via a
software node device property instead of hardcoding it in the generic atkbd
driver. It moves hpps2atkbd.h to the serio subsystem and attaches the
property during port registration.

diff --git a/drivers/input/serio/gscps2.c b/drivers/input/serio/gscps2.c
--- a/drivers/input/serio/gscps2.c
+++ b/drivers/input/serio/gscps2.c
[ ... ]
> @@ -398,6 +397,17 @@ static int __init gscps2_probe(struct parisc_device *dev)
>  		goto fail;
>  #endif
>  
> +	if (ps2port->id == GSC_ID_KEYBOARD) {
> +		ret = device_create_managed_software_node(&serio->dev,
> +							  gscps2_props, NULL);

[Severity: Medium]
Does this leak the software node if driver unbinding races with the
asynchronous port registration?

serio_register_port() queues port registration asynchronously on a
workqueue. If the driver is unbound and calls serio_unregister_port()
before the workqueue executes device_add(), device_is_registered()
evaluates to false in serio_destroy_port(), bypassing device_del().
Because device_del() is skipped, the software node is never removed.

> +		if (ret) {
> +			dev_err(&dev->dev,
> +				"failed to add software node for keyboard: %d\n",
> +				ret);
> +			goto fail;
> +		}
> +	}
> +
>  	pr_info("serio: %s port at 0x%08lx irq %d @ %s\n",
>  		ps2port->port->name,
>  		hpa,
>  		ps2port->padev->irq,
>  		ps2port->port->phys);
>  
>  	serio_register_port(ps2port->port);

[Severity: High]
This is a pre-existing issue, but does gscps2_remove() create a
use-after-free window?

In the driver removal path, serio_unregister_port() is called before
free_irq() and before the port is removed from ps2port_list. If a shared
interrupt fires in this window, gscps2_interrupt() will execute, traverse
the list, and dereference the freed ps2port->port pointer.

>  	list_add_tail(&ps2port->node, &ps2port_list);

[Severity: High]
This is a pre-existing issue, but can this race with the interrupt handler
and corrupt the list?

The global ps2port_list is modified here locklessly. Concurrently, the
shared interrupt handler gscps2_interrupt() iterates over this list using
list_for_each_entry() without any locks. Since the interrupt is registered
as IRQF_SHARED earlier in this function, an interrupt can trigger on another
CPU while the list is being modified.

[Severity: High]
This is also a pre-existing issue, but does gscps2_interrupt() defeat
spurious interrupt detection?

It appears the interrupt handler unconditionally returns IRQ_HANDLED
regardless of whether it actually serviced any pending data. Because this
IRQ is requested with IRQF_SHARED, this prevents the generic IRQ subsystem
from detecting unhandled states and disabling the IRQ line if another
device sharing this line asserts an interrupt but its driver fails to
clear it.

diff --git a/drivers/input/serio/hpps2atkbd.h b/drivers/input/serio/hpps2atkbd.h
new file mode 100644
--- /dev/null
+++ b/drivers/input/serio/hpps2atkbd.h
[ ... ]
> +	KEYMAP_ENTRY(0x79, KEY_KPPLUS),		KEYMAP_ENTRY(0x7a, KEY_KP3),
> +	KEYMAP_ENTRY(0x7b, KEY_KPMINUS),	KEYMAP_ENTRY(0x7c, KEY_KPASTERISK),
> +	KEYMAP_ENTRY(0x7d, KEY_KP9),		KEYMAP_ENTRY(0x7e, KEY_SCROLLLOCK),
> +	KEYMAP_ENTRY(0x7f, KEY_102ND),		KEYMAP_ENTRY(0x91, KEY_RIGHTALT),
> +	KEYMAP_ENTRY(0x94, KEY_RIGHTCTRL),	KEYMAP_ENTRY(0x9d, KEY_CAPSLOCK),
> +	KEYMAP_ENTRY(0x9f, KEY_LEFTMETA),	KEYMAP_ENTRY(0xa7, KEY_RIGHTMETA),

[Severity: Medium]
Is the mapping for scancode 0x92 intentionally omitted here?

The previous PA-RISC keycode table explicitly mapped scancode 0x92 to 255
(ATKBD_KEY_NULL), which atkbd ignores silently. With the new gscps2_keymap
property array, scancode 0x92 defaults to 0 (ATKBD_KEY_UNKNOWN).

If the physical keyboard emits this scancode, atkbd will process it and flood
the kernel logs with "Unknown key %s ..." warnings.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1