Re: [PATCH v2 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 3 potential issue(s) to consider:
- [High] input: UAF due to lockless quitting checks and incorrect work cancellation order
- [Medium] input: Data race on link state variables due to missing state_lock in RX path
- [Medium] input: Race condition and missing memory barriers in TX/RX completion logic
--

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

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

This adds a serdev driver for the Tinno keyboard MCU used by OnePlus/OPPO Pad
smart keyboards. It implements a framed protocol over a half-duplex UART for
input events, heartbeat, and backlight control.

> diff --git a/drivers/input/keyboard/oneplus-pogo-kbd.c b/drivers/input/keyboard/oneplus-pogo-kbd.c
> index 0000000000000..298f0eff52116
> --- /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)
> +{
[ ... ]
> +	mutex_lock(&kb->io_lock);
> +	for (attempt = 0; attempt < POGO_XFER_RETRIES; attempt++) {
> +		bool echoed;
> +
> +		/*
> +		 * 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: Medium]
Since WRITE_ONCE does not emit a hardware memory barrier, could the CPU reorder
these stores on weakly-ordered architectures?

This might allow the RX thread to see tx_pending == true while still reading
the old tx_cmd, opening a race condition.

[ ... ]
> +static void pogo_handle_sync(struct pogo_kbd *kb, const u8 *p, unsigned int len)
> +{
> +	bool replug = false;
> +
> +	if (len < 2)
> +		return;
> +
> +	if (p[0] == OWB_SYNC_PLUG_IN && p[1] == 0x02 && len >= 9) {
> +		kb->brand = p[2];
> +		memcpy(kb->mac, &p[3], POGO_MAC_LEN);
> +		/*
> +		 * The MCU repeats this frame until the host has talked to it;
> +		 * only a plug-in frame after regular heartbeats is a re-plug.
> +		 */
> +		replug = kb->connected && kb->hb_seen;
> +	} else if (p[0] == OWB_SYNC_HEARTBEAT && p[1] == 0x02 && len >= 10) {
> +		kb->brand = p[3];
> +		memcpy(kb->mac, &p[4], POGO_MAC_LEN);
> +		kb->hb_seen = true;
> +	}
> +
> +	if (!kb->connected || replug) {
> +		kb->connected = true;
> +		kb->hb_seen = false;
> +		kb->retry_ms = POGO_RETRY_MIN_MS;

[Severity: Medium]
Are these link state variables modified without holding kb->state_lock here?

Because pogo_handle_sync() runs in the RX worker context, it can execute
concurrently with workqueues running pogo_power_off() or pogo_power_on().
This data race could corrupt the driver's state machine logic.

> +		if (!READ_ONCE(kb->quitting))
> +			schedule_work(&kb->plug_work);

[Severity: High]
Does this lockless quitting check allow a use-after-free?

If the RX thread evaluates !READ_ONCE(kb->quitting) as true but is preempted
before calling schedule_work(), pogo_remove() might finish cancelling plug_work
and return.

The RX thread would then resume and queue plug_work after it has been
cancelled, leading to a use-after-free when devres frees kb.

> +	}
> +}
> +
> +static void pogo_handle_frame(struct pogo_kbd *kb, const u8 *f, unsigned int n)
> +{
[ ... ]
> +	switch (cmd) {
> +	case OWB_CMD_KEYS:
[ ... ]
> +	case OWB_CMD_SYNC_UPLOAD:
> +		pogo_handle_sync(kb, payload, len);
> +		break;
> +	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]
Is there a missing acquire barrier (smp_load_acquire) when reading tx_pending
and tx_cmd here?

Furthermore, since the RX thread's check is not protected by a lock, it could
be preempted between evaluating tx_pending and reading tx_cmd, which might
result in erroneously completing a new transfer using an old response.

> +			/* 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 {
> +			dev_dbg(kb->dev, "unhandled frame cmd 0x%02x len %u\n",
> +				cmd, len);
> +		}
> +		break;
> +	}
> +}
[ ... ]
> +static void pogo_hb_work(struct work_struct *work)
> +{
[ ... ]
> +	/* re-arm: retry while the wake line still reports a keyboard */
> +	if (!READ_ONCE(kb->quitting))
> +		schedule_delayed_work(&kb->detect_work,
> +				      msecs_to_jiffies(kb->retry_ms));

[Severity: High]
Could hb_work queue detect_work after it has been cancelled?

Since pogo_remove() cancels detect_work before hb_work, if hb_work executes
concurrently with teardown, this lockless check allows schedule_delayed_work()
to run after cancel_delayed_work_sync(&kb->detect_work) has completed.

This leaves detect_work permanently queued, leading to another use-after-free
when kb is freed.

> +	kb->retry_ms = min(kb->retry_ms * 2, POGO_RETRY_MAX_MS);
> +}
[ ... ]
> +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]
Does this cancellation order correctly prevent works from being queued?

As noted above, cancelling detect_work before hb_work permits hb_work to
re-queue detect_work during teardown.

Additionally, the RX path's lockless check against kb->quitting means plug_work
can be queued after cancel_work_sync() completes, due to delayed devres serdev
closure.

> +
> +	/* 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/20261010143836.173883-1-wyibo6109@gmail.com?part=2