Re: [PATCH v4 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: pogo: Use-After-Free in async paths due to lockless TOCTOU and broken synchronization
- [Medium] serdev: data race and buffer overwrite in pogo_xfer() due to lockless completion handling
- [Medium] serdev: unprotected link state modifications in RX thread cause software/hardware state desync
- [Low] sysfs: custom battery_level attribute bypasses power_supply subsystem UAPI
--

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

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

This commit adds a 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.
It introduces a custom protocol implementation over a half-duplex UART
to support touch, keys, backlight, and power management.

> diff --git a/drivers/input/keyboard/oneplus-pogo-kbd.c b/drivers/input/keyboard/oneplus-pogo-kbd.c
> --- a/drivers/input/keyboard/oneplus-pogo-kbd.c
> +++ b/drivers/input/keyboard/oneplus-pogo-kbd.c
[ ... ]
>  		/*
>  		 * 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]
Is it possible for a delayed response from the previous timeout to trigger a
spurious completion here?

When the retry happens, the same cmd is set. If the RX thread was preempted
just before copying the payload, could it wake up and call
complete(&kb->resp_done) using the stale payload for this new iteration?
Explicit lock synchronization between the timeout path and the RX thread might
be needed.

[ ... ]
>  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;
>  		kb->retry_ms = POGO_RETRY_MIN_MS;

[Severity: Medium]
Does this locklessly update the link state variables while they are also
managed by pogo_power_off() under state_lock?

If a delayed heartbeat frame is processed exactly as pogo_hb_work() determines
a timeout and calls pogo_power_off(), could pogo_power_off() clear the state
just before this unsynchronized block unconditionally sets kb->connected = true
again?

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

[Severity: High]
Can this lockless check of kb->quitting lead to a use-after-free during driver
removal?

The serdev RX path is devres-managed and can run concurrently with
pogo_remove(). If the RX thread evaluates quitting as false just before
pogo_remove() sets it to true, it could queue plug_work after
cancel_work_sync(&kb->plug_work) has already finished, causing the work to run
on freed memory.

[ ... ]
>  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);
>  			complete(&kb->resp_done);
>  		}

[Severity: Medium]
Since the check for tx_pending and tx_cmd is lockless, could the RX thread be
preempted right after this condition evaluates to true?

If pogo_xfer() times out, resets tx_pending, and starts a retry with the same
command, the stalled RX thread could resume, overwrite kb->resp with the old
payload, and signal completion for the new transfer.

[ ... ]
>  static void pogo_wake_irq_enable(struct pogo_kbd *kb, bool enable)
>  {
>  	if (enable == kb->wake_irq_enabled)
>  		return;

[Severity: High]
Could this optimization skip synchronizing with a running ISR?

If pogo_wake_isr() sets kb->wake_irq_enabled = false, this check will return
immediately without calling disable_irq(). During pogo_remove(), this allows
the teardown to proceed without waiting for the ISR, which might then queue
detect_work after it has been cancelled.

[ ... ]
>  static void pogo_power_off(struct pogo_kbd *kb)
>  {
>  	bool was_connected;
>  
>  	mutex_lock(&kb->state_lock);
>  	was_connected = kb->connected;
>  	kb->connected = false;
>  	kb->powered = false;
>  	mutex_unlock(&kb->state_lock);

[Severity: Medium]
As mentioned above, if this safe transition runs concurrently with a delayed
frame processed by pogo_handle_sync(), could the hardware state become
desynchronized?

The software might incorrectly reflect a connected device while the hardware
is unpowered, causing subsequent sysfs accesses (like setting the backlight)
to time out waiting for the MCU.

[ ... ]
>  static ssize_t battery_level_show(struct device *dev,
>  				  struct device_attribute *attr, char *buf)
>  {
>  	struct pogo_kbd *kb = dev_get_drvdata(dev);
>  
>  	if (!kb->connected)
>  		return -ENODEV;
>  	/* only keyboards with a battery push 0x0E status frames */
>  	if (kb->battery_level < 0)
>  		return -ENODATA;
>  	return sysfs_emit(buf, "%d\n", kb->battery_level);
>  }
>  static DEVICE_ATTR_RO(battery_level);

[Severity: Low]
Does exposing battery_level as a custom sysfs attribute bypass the standard
power_supply subsystem?

Standard Linux userspace battery monitors (like UPower) only scan for devices
registered under the standard power_supply class. Would it be better to
register a POWER_SUPPLY_TYPE_BATTERY device using the power supply API so
desktop environments can seamlessly read the keyboard's battery status?

[ ... ]
>  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]
Are these sequential cancellations safe against background works queuing each
other?

Since detect_work is cancelled before hb_work, if hb_work evaluates !quitting
right before it is set to true here, it could queue detect_work again after
cancel_delayed_work_sync(&kb->detect_work) has already completed. This would
leave a timer active on the device structure after it is freed.

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