Re: [PATCH v3 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 5 potential issue(s) to consider:
- [High] input: incorrect work cancellation order and UART teardown sequence cause UAF
- [High] input: early return in pogo_wake_irq_enable bypasses teardown synchronization causing UAF
- [Medium] input: data race on link state variables kb->connected and kb->hb_seen
- [Medium] input: spontaneous battery status frames break protocol by masquerading as command replies
- [Low] input: lockless tx_pending synchronization allows late echoes to falsely complete retries
--

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

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

This commit adds a serdev driver for the Tinno keyboard MCU used by
OnePlus and OPPO smart keyboards. The MCU communicates over a half-duplex
UART using a framed protocol. It supports touchpad, function keys, and
backlight control.

--- a/drivers/input/keyboard/oneplus-pogo-kbd.c
+++ 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)
> +{
[ ... ]
> +		/*
> +		 * Drop any late echo/reply of the previous attempt: with
> +		 * tx_pending cleared the RX path ignores frames until the
> +		 * transfer below re-arms, so a delayed reply cannot complete
> +		 * the fresh iteration or race the resp buffer.
> +		 */
> +		WRITE_ONCE(kb->tx_pending, false);
> +		reinit_completion(&kb->echo_done);
> +		reinit_completion(&kb->resp_done);
> +		kb->tx_cmd = cmd;
> +		WRITE_ONCE(kb->tx_pending, true);

[Severity: Low]
Does this lockless synchronization for tx_pending allow late echoes to falsely
complete retries?

If the UART RX path (pogo_handle_frame) observes tx_pending == true and
matches the command, but then stalls just before calling
complete(&kb->echo_done), pogo_xfer() could time out and start a new retry.
When the RX thread resumes and calls complete(), it would satisfy the new
retry's completion with the old echo, potentially masking a real transmission
failure.

[ ... ]
> +static void pogo_handle_sync(struct pogo_kbd *kb, const u8 *p, unsigned int len)
> +{
[ ... ]
> +	if (!kb->connected || replug) {
> +		kb->connected = true;
> +		kb->hb_seen = false;

[Severity: Medium]
Could this modify the link state variables concurrently with pogo_power_off()?

Since pogo_power_off() modifies kb->connected and kb->powered while holding
kb->state_lock, but pogo_handle_sync() updates kb->connected and kb->hb_seen
here locklessly, the RX thread might clobber the teardown state. This could
leave kb->connected = true after the hardware is unpowered, bypassing checks
in sysfs interfaces and potentially backfeeding power through the UART pins.

> +		kb->retry_ms = POGO_RETRY_MIN_MS;
> +		if (!READ_ONCE(kb->quitting))
> +			schedule_work(&kb->plug_work);
> +	}
> +}
> +
> +static void pogo_handle_frame(struct pogo_kbd *kb, const u8 *f, unsigned int n)
> +{
[ ... ]
> +	default:
> +		if (cmd == OWB_CMD_GENERAL_ACK && len >= 4 &&
> +		    payload[0] == OWB_GEN_BATTERY_STATUS)
> +			kb->battery_level = payload[2];
> +
> +		if (READ_ONCE(kb->tx_pending) && cmd == kb->tx_cmd + 1) {

[Severity: Medium]
Should there be an 'else' block for the battery status check?

The MCU can spontaneously transmit battery status frames (cmd 0x3B,
payload[0] 0x0E). If the host is currently waiting for a general command
reply (kb->tx_cmd == 0x3A), the condition cmd == kb->tx_cmd + 1 will match.
This causes the battery frame to be falsely accepted as the command reply,
which could cause pogo_set_general() to fail with -EPROTO and drop the real
reply that arrives later.

> +			/* reply layout for callers: cmd, len, payload */
> +			kb->resp_len = min_t(unsigned int, len + 2, sizeof(kb->resp));
> +			memcpy(kb->resp, &f[3], kb->resp_len);
> +			complete(&kb->resp_done);
> +		} else {
[ ... ]
> +static void pogo_wake_irq_enable(struct pogo_kbd *kb, bool enable)
> +{
> +	if (enable == kb->wake_irq_enabled)
> +		return;

[Severity: High]
Does this early return bypass the synchronization barrier during teardown?

In pogo_wake_isr(), the hardware interrupt calls disable_irq_nosync() and sets
kb->wake_irq_enabled = false. If pogo_remove() calls pogo_wake_irq_enable(kb, false)
while the ISR is executing after the flag has been updated, this early return
will skip the disable_irq() call entirely.

This allows pogo_remove() to proceed with teardown and free the memory while the
ISR is still running and about to queue detect_work, leading to a use-after-free.

> +	if (enable) {
[ ... ]
> +static void pogo_remove(struct serdev_device *serdev)
> +{
> +	struct pogo_kbd *kb = serdev_device_get_drvdata(serdev);
> +
> +	/*
> +	 * Stop the world first: with quitting set, the RX path and the
> +	 * works stop queueing each other, so the cancels below cannot be
> +	 * undone again.  The wake IRQ is disabled (and any in-flight ISR
> +	 * finished) before detect_work is cancelled.
> +	 */
> +	WRITE_ONCE(kb->quitting, true);
> +	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);

[Severity: High]
Could the cancellation order and serdev teardown sequence cause a use-after-free?

Because detect_work is cancelled before hb_work, if hb_work is running
concurrently, it can re-queue detect_work after it has been cancelled. The
lockless check !READ_ONCE(kb->quitting) allows a thread to read false, stall,
and queue the work after these cancellations are finished.

Additionally, since the serdev port remains open during pogo_remove() (it closes
later via devres), the UART RX callback can still execute, evaluate the lockless
quitting flag as false, and queue plug_work after cancel_work_sync(&kb->plug_work)
has already returned. 

This sequence can leave works queued while devres frees the kb memory.

> +
> +	/* unregisters the input devices, the last possible led_work queuer */
> +	pogo_power_off(kb);
> +
> +	/* flush a led_work queued before the inputs went away */
> +	cancel_work_sync(&kb->led_work);
> +}

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