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: Use-After-Free in workqueues due to incorrect teardown sequence
- [High] input: Resource leak and UAF due to race between plug_work and power_off
- [Medium] input: Data race and corruption on kb->resp buffer in UART RX path
- [Medium] input: Data race on wake_irq_enabled state causing permanent IRQ disablement
- [Medium] input: Missing Kconfig dependencies for INPUT_MT and LEDS_CLASS
- [Low] input: Success logging in probe path violates subsystem guidelines
--

commit 3f827ef8e9c7e47ecd0b1c17c9418abab787c03c
Author: Yibo Wang <wyibo6109@gmail.com>

Input: add OnePlus/OPPO Pad pogo-pin keyboard serdev driver
    
Clean-room serdev driver for the Tinno keyboard MCU used by the OnePlus
Pad 3 Pro / Pad 4 and OPPO Pad 5 Pro smart keyboards, replacing the
vendor platform driver that depends on a downstream msm_geni_serial
callback and a user-space held tty fd.

> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -768,4 +768,15 @@ config KEYBOARD_CYPRESS_SF
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cypress-sf.
>  
> +config KEYBOARD_ONEPLUS_POGO
> +	tristate "OnePlus/OPPO Pad pogo-pin keyboard"
> +	depends on SERIAL_DEV_BUS && GPIOLIB

[Severity: Medium]
Should this Kconfig entry select CONFIG_INPUT_MT and depend on
CONFIG_LEDS_CLASS?

The driver explicitly calls input_mt_init_slots() and
devm_led_classdev_register_ext(). If CONFIG_INPUT_MT is disabled, the kernel
will fail to link. If CONFIG_LEDS_CLASS is configured as a module and this
driver is built-in, it will cause a linker error or silently drop
functionality. Standard kernel practices require this Kconfig to select
CONFIG_INPUT_MT and depend on CONFIG_LEDS_CLASS.

> diff --git a/drivers/input/keyboard/oneplus-pogo-kbd.c b/drivers/input/keyboard/oneplus-pogo-kbd.c
> --- /dev/null
> +++ b/drivers/input/keyboard/oneplus-pogo-kbd.c
[ ... ]
> +static int pogo_xfer(struct pogo_kbd *kb, u8 cmd, const u8 *payload, u8 len,
> +		     u8 *resp, unsigned int resp_size)
> +{
[ ... ]
> +		if (wait_for_completion_timeout(&kb->resp_done,
> +				msecs_to_jiffies(POGO_RESP_TIMEOUT_MS))) {
> +			unsigned int n = min(kb->resp_len, resp_size);
> +
> +			memcpy(resp, kb->resp, n);
> +			ret = n;
> +			break;
> +		}
> +		dev_dbg(kb->dev, "cmd 0x%02x: no reply\n", cmd);
> +		ret = -ETIMEDOUT;
> +	}
> +	WRITE_ONCE(kb->tx_pending, false);
> +	mutex_unlock(&kb->io_lock);
> +
> +	return ret;
> +}

[Severity: Medium]
Is there a data race on kb->resp when timeouts occur?

If wait_for_completion_timeout() times out, it proceeds to the next loop
iteration and calls reinit_completion(&kb->resp_done) without setting
kb->tx_pending = false.

If a delayed reply from the previous attempt arrives concurrently, the
serdev RX callback pogo_handle_frame() sees kb->tx_pending == true and
writes the payload into kb->resp locklessly:

    if (READ_ONCE(kb->tx_pending) && cmd == kb->tx_cmd + 1) {
        kb->resp_len = min_t(unsigned int, len + 2, sizeof(kb->resp));
        memcpy(kb->resp, &f[3], kb->resp_len);

This can overwrite kb->resp while pogo_xfer() is concurrently reading from
it, or it can spuriously complete the new iteration before the packet is
even sent.

[ ... ]
> +static void pogo_wake_irq_enable(struct pogo_kbd *kb, bool enable)
> +{
> +	if (enable == kb->wake_irq_enabled)
> +		return;
> +	if (enable)
> +		enable_irq(kb->wake_irq);
> +	else
> +		disable_irq(kb->wake_irq);
> +	kb->wake_irq_enabled = enable;
> +}

[Severity: Medium]
Does updating the state after enabling the IRQ cause a race condition?

If the wake line is already asserted, the IRQ can fire immediately on another
CPU as soon as enable_irq() executes. The ISR pogo_wake_isr() would then run,
call disable_irq_nosync(), and assign kb->wake_irq_enabled = false.

When the original CPU resumes, it unconditionally assigns
kb->wake_irq_enabled = true, overwriting the ISR's state update. The
software state now claims the IRQ is enabled, while the hardware IRQ is
actually disabled.

Subsequent calls to pogo_wake_irq_enable() will return early without enabling
the IRQ, leaving the keyboard permanently asleep.

[ ... ]
> +static void pogo_power_off(struct pogo_kbd *kb)
> +{
> +	dev_dbg(kb->dev, "power off\n");
> +	kb->powered = false;
> +	gpiod_set_value_cansleep(kb->power_gpio, 0);
> +	if (kb->connected) {
> +		kb->connected = false;
> +		pogo_unregister_inputs(kb);
> +		dev_info(kb->dev, "keyboard detached\n");
> +	}
> +}

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

In pogo_plug_work(), device registration proceeds outside the lock:

    if (!kb->kbd) {
        ret = pogo_register_inputs(kb);

Concurrently, pogo_hb_work() can call pogo_power_off(), which sets
kb->connected = false and calls pogo_unregister_inputs(). Since plug_work
has not yet assigned kb->kbd under the lock, pogo_unregister_inputs() does
nothing.

After plug_work completes registration and assigns kb->kbd, if the driver
is unbound, pogo_remove() calls pogo_power_off(). Because kb->connected is
already false, it skips unregistering the devices.

This leaks the input devices, and their private data pointers retain the
freed kb struct, causing a use-after-free when accessed by userspace.

[ ... ]
> +static int pogo_probe(struct serdev_device *serdev)
> +{
[ ... ]
> +	/* 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);
> +	return 0;
> +}

[Severity: Low]
Should this success message be removed to match subsystem guidelines?

The driver prints an explicit success log message upon normal completion of
the probe sequence. The LED and Input maintainer guidelines request that
drivers only log errors or warnings.

[ ... ]
> +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);
> +	cancel_work_sync(&kb->plug_work);
> +	cancel_work_sync(&kb->led_work);
> +	pogo_power_off(kb);
> +}

[Severity: High]
Could this teardown sequence leave a window for use-after-free bugs?

First, cancel_work_sync(&kb->led_work) is called before pogo_power_off(kb)
unregisters the input devices. Userspace can trigger pogo_kbd_event() via
sysfs/evdev and queue led_work after the cancellation.

Second, the serdev port is managed via devm_serdev_device_open(), so it
remains open until devres cleanup runs after pogo_remove() returns. An
incoming UART frame could trigger pogo_receive_buf() and queue plug_work
on the freed memory.

Third, detect_work and hb_work can schedule each other. Sequentially
cancelling them without a state flag (e.g., kb->quitting = true) allows
hb_work to queue detect_work immediately after detect_work is cancelled.

Also, if pogo_probe() fails after opening the serdev, plug_work is never
cancelled before devres cleanup.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009121143.550498-1-2024312277@stu.hit.edu.cn?part=2