Re: [PATCH 2/2] Input: add OnePlus/OPPO Pad pogo-pin keyboard serdev driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] input: UAF in led_work due to teardown ordering
- [High] serdev: UAF in plug_work due to active port during devres teardown
- [High] workqueue: UAF due to detect_work and hb_work cyclic dependency
- [High] workqueue: UAF due to pogo_wake_irq_enable missing synchronize_irq
- [High] input: Race between plug_work and power_off leaks input devices
- [Low] leds: Success logging in probe violates guidelines
--

commit 0ce1872d7cb39e14970c678cd47393649379aea8
Author: Yibo Wang <wyibo6109@gmail.com>

Input: add OnePlus/OPPO Pad pogo-pin keyboard serdev driver

Adds a serdev driver for the Tinno keyboard MCU used by OnePlus and OPPO
Pad smart keyboards. Communication happens over a half-duplex UART using
a custom framed protocol. The driver exposes touchpad, keys, and backlight
controls to userspace.

> diff --git a/drivers/input/keyboard/oneplus-pogo-kbd.c b/drivers/input/keyboard/oneplus-pogo-kbd.c
> index 0000000000000..589f31ecef848
> --- /dev/null
> +++ b/drivers/input/keyboard/oneplus-pogo-kbd.c
> @@ -778,7 +778,7 @@
>  static void pogo_wake_irq_enable(struct pogo_kbd *kb, bool enable)
>  {
>  	if (enable == kb->wake_irq_enabled)
>  		return;

[Severity: High]
If pogo_wake_isr() fires concurrently, sets kb->wake_irq_enabled = false,
and then pogo_remove() calls pogo_wake_irq_enable(kb, false), will this
return early and bypass disable_irq()?

Since disable_irq() also acts as a synchronize_irq(), missing it might
allow the preempted ISR to resume and queue detect_work after it has
already been cancelled in pogo_remove().

[ ... ]

> @@ -849,10 +849,10 @@
>  static void pogo_plug_work(struct work_struct *work)
>  {
>  	struct pogo_kbd *kb = container_of(work, struct pogo_kbd, plug_work);
>  	int ret;
>  
>  	if (!kb->powered || !kb->connected)
>  		return;
>  
>  	if (!kb->kbd) {
>  		ret = pogo_register_inputs(kb);

[Severity: High]
Can a race between pogo_plug_work() and pogo_power_off() leak input
devices?

If pogo_plug_work() evaluates kb->connected as true, and concurrently
pogo_power_off() sets kb->connected = false (unregistering nothing since
kb->kbd is still NULL):

- pogo_plug_work() would then allocate and register the input devices.
- The state becomes desynchronized: kb->connected is false, but devices
  are registered.

When the driver unbinds, pogo_remove() calls pogo_power_off(), which will
skip unregistering the devices because kb->connected is false. Will this
leave the input devices leaked and pointing to freed memory?

[ ... ]

> @@ -1074,6 +1074,6 @@
>  	/* evaluate the current attach state; enables the irq when idle */
>  	schedule_delayed_work(&kb->detect_work,
>  			      msecs_to_jiffies(POGO_DETECT_DEBOUNCE_MS));
>  
>  	dev_info(dev, "pogo keyboard link ready (wake irq %d)\n", kb->wake_irq);

[Severity: Low]
This isn't a bug, but does this success log violate the LED subsystem
guidelines? The guidelines suggest only logging errors or warnings during
probe, to avoid cluttering the kernel log with success messages.

>  	return 0;
>  }

[ ... ]

> @@ -1084,7 +1084,7 @@
>  static void pogo_remove(struct serdev_device *serdev)
>  {
>  	struct pogo_kbd *kb = serdev_device_get_drvdata(serdev);
>  
>  	pogo_wake_irq_enable(kb, false);
>  	cancel_delayed_work_sync(&kb->detect_work);
>  	cancel_delayed_work_sync(&kb->hb_work);

[Severity: High]
Because detect_work and hb_work can queue each other, and neither checks a
shared teardown flag (like kb->going_away), could hb_work re-arm
detect_work after detect_work was cancelled?

If hb_work is running concurrently during pogo_remove():
1. cancel_delayed_work_sync() for detect_work returns.
2. hb_work calls schedule_delayed_work() for detect_work.
3. cancel_delayed_work_sync() for hb_work waits for hb_work to finish.

This leaves detect_work pending after pogo_remove() returns, which would
lead to a use-after-free when devres frees the kb structure.

>  	cancel_work_sync(&kb->plug_work);

[Severity: High]
Since the serdev port is managed by devm_serdev_device_open(), the UART
receiver remains active until devres cleanup, which executes after
pogo_remove() returns.

Could an incoming UART frame trigger pogo_handle_sync() and re-queue
plug_work after this cancellation? If the work remains queued after devres
frees the kb memory, it would cause a use-after-free.

Similarly, if pogo_probe() fails after devm_serdev_device_open(), could an
incoming frame schedule the work on the error path where it is never
cancelled? Does the UART receiver need to be closed or the callback
disabled prior to work cancellation?

>  	cancel_work_sync(&kb->led_work);
>  	pogo_power_off(kb);

[Severity: High]
Because pogo_power_off(kb) is called after cancelling led_work, the input
devices are still fully registered and active during cancellation.

Can a concurrent EV_LED event routed by the input core trigger
pogo_kbd_event(), which unconditionally calls schedule_work() for
led_work? If led_work is re-armed after it was cancelled, it will execute
on freed memory once devres cleans up. Should the input devices be
unregistered before cancelling led_work?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009122046.551915-1-wyibo6109@gmail.com?part=2