Re: [PATCH v6 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 4 potential issue(s) to consider:
- [High] input: use-after-free in work queues due to lockless removal races
- [Medium] input: silent truncation of maximum-length responses in kb->resp
- [Medium] input: oneplus-pogo: data race on kb->resp buffer in pogo_xfer
- [Medium] input: oneplus-pogo-kbd: Data race on kb->connected state variables
--

commit 55459b7da977a6dfb82c08f24907cc60a6e32cd0
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/oneplus-pogo-kbd.c b/drivers/input/keyboard/oneplus-pogo-kbd.c
> new file mode 100644
> index 0000000000000..afe87d9616174
> --- /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 (!resp) {
> +			ret = 0;
> +			break;
> +		}
> +		if (wait_for_completion_timeout(&kb->resp_done,
> +				msecs_to_jiffies(POGO_RESP_TIMEOUT_MS))) {
> +			unsigned int n = min(kb->resp_len, resp_size);
> +
> +			/* stop the RX path before copying the reply out */
> +			WRITE_ONCE(kb->tx_pending, false);
> +			memcpy(resp, kb->resp, n);

[Severity: Medium]
Can this lead to a data race on the kb->resp buffer?

If a delayed reply from a previous attempt completes kb->resp_done early,
pogo_xfer() wakes up, sets tx_pending to false, and starts reading
kb->resp. Concurrently, the RX thread in pogo_handle_frame() may receive
the reply for the current attempt, evaluate tx_pending as true, and
overwrite kb->resp via memcpy() while pogo_xfer() is reading it.

This torn read could cause spurious -EPROTO initialization failures.

[ ... ]
> +static void pogo_handle_sync(struct pogo_kbd *kb, const u8 *p, unsigned int len)
> +{
> +	bool replug = false;
[ ... ]
> +	if (!kb->connected || replug) {
> +		kb->connected = true;
> +		kb->hb_seen = false;
> +		kb->retry_ms = POGO_RETRY_MIN_MS;
> +		if (!READ_ONCE(kb->quitting))
> +			schedule_work(&kb->plug_work);
> +	}

[Severity: Medium]
Is it safe to modify kb->connected here without holding kb->state_lock?

In pogo_power_off(), kb->connected and kb->powered are cleared under
kb->state_lock. If pogo_power_off() races with this RX path, this lockless
modification can leave the driver permanently desynchronized.

If pogo_power_off() sets powered = false and connected = false, and a delayed
UART sync frame is processed here concurrently, this path will locklessly set
connected = true. The subsequent schedule_work(&kb->plug_work) will exit
early because powered is false, leaving connected = true while powered is false.

[ ... ]
> +static void pogo_handle_frame(struct pogo_kbd *kb, const u8 *f, unsigned int n)
> +{
[ ... ]
> +		if (READ_ONCE(kb->tx_pending) && cmd == kb->tx_cmd + 1) {
> +			/* 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);

[Severity: Medium]
Does this truncate maximum-length responses?

The kb->resp buffer is sized to OWB_MAX_PAYLOAD (128 bytes). By adding 2 to
len (for cmd and len), a maximum payload of 128 bytes yields len + 2 = 130.
The min_t() will cap this at sizeof(kb->resp) (128), which silently truncates
the last 2 bytes of the payload.

[ ... ]
> +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]
Can this teardown sequence lead to a use-after-free of the work queues?

If pogo_wake_isr() is currently executing and has already called
disable_irq_nosync(), kb->wake_irq_enabled will be false.

When pogo_remove() calls pogo_wake_irq_enable(kb, false), it will hit
the early return:

    if (enable == kb->wake_irq_enabled)
        return;

This bypasses the disable_irq() synchronization barrier. Because
pogo_wake_isr() checks kb->quitting without a lock, the ISR could observe
quitting == false before pogo_remove() sets it, but queue the delayed
work after pogo_remove() has already completed the cancellations, resulting
in a use-after-free.

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