Re: [PATCH v5 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: missing irq synchronization in teardown causes UAF
- [High] input: TOCTOU race in work scheduling during remove causes UAF
- [Medium] input: data race on rx response buffer during concurrent replies
- [Medium] input: missing select TOUCHSCREEN_PROPERTIES in Kconfig
- [Medium] input: data race on link state variables between rx and workqueue
--

commit cfd6c4eb0ba7d1e7dd71a926b7629e2bc8db3a2e
Author: Yibo Wang <wyibo6109@gmail.com>
Subject: Input: add OnePlus/OPPO Pad pogo-pin keyboard serdev driver

This patch introduces a clean-room serdev driver for the Tinno keyboard
MCU used by OnePlus and OPPO smart keyboards. The driver communicates via a
framed one-wire bus protocol over a half-duplex UART and exposes standard
input and LED interfaces.

> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index d8c64f333462c..f3931b7fbe748 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -768,4 +768,17 @@ 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
> +	depends on LEDS_CLASS
> +	select INPUT_MT

[Severity: Medium]
Is a select TOUCHSCREEN_PROPERTIES dependency missing here?

The driver relies on touchscreen_parse_properties() to parse standard
Device Tree touchscreen overrides. Without this select statement, if the
config is not enabled elsewhere in the kernel build, the stub version from
the header is compiled instead. This silently ignores the DT properties
intended for the touchpad.

[ ... ]

> 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
> @@ -382,10 +382,10 @@ static int pogo_xfer(struct pogo_kbd *kb, u8 cmd, const u8 *payload, u8 len,
>  		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);
>  			ret = n;
>  			break;
>  		}

[Severity: Medium]
Could this cause a data race on the kb->resp buffer during concurrent
replies?

Because kb->tx_pending is cleared after wait_for_completion_timeout()
returns, if the MCU sends a duplicate reply frame or if the TX thread wakes
up due to a timeout just as a delayed frame is received, the serdev RX thread
will enter pogo_handle_frame().

The RX thread would evaluate kb->tx_pending as true, since the TX thread
hasn't cleared it yet, and start copying data into kb->resp. If the TX
thread executes memcpy(resp, kb->resp, n) concurrently, it will read
corrupted data.

Should kb->tx_pending be cleared atomically inside the RX handler when the
first valid reply is consumed instead?

[ ... ]

> @@ -714,10 +714,10 @@ static void pogo_handle_sync(struct pogo_kbd *kb, const u8 *p, unsigned int len)
>  		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]
Does this mutate the link state variables without holding kb->state_lock?

pogo_power_off() and pogo_power_on() safely modify kb->connected and
kb->powered while holding the mutex. However, the RX path here modifies
these variables directly. If this runs simultaneously with pogo_power_off(),
a race condition occurs. This can leave the driver state machine inconsistent
(for example, kb->connected is true while kb->powered is false).

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

[Severity: High]
Can this lockless check lead to a use-after-free during device removal?

The serdev port remains open until devres cleanup, which happens after
pogo_remove() returns. An active RX thread could evaluate
!READ_ONCE(kb->quitting) as true, and then stall or get preempted before
actually calling schedule_work().

Concurrently, pogo_remove() would set kb->quitting to true and run
cancel_work_sync(&kb->plug_work). Because the work isn't queued yet, the
cancellation returns immediately. When the preempted RX thread resumes, it
queues the work. Devres then frees the kb struct, leaving the queued work
item to execute on freed memory.

[ ... ]

> @@ -861,7 +861,7 @@ static void pogo_wake_irq_enable(struct pogo_kbd *kb, bool enable)
>  {
>  	if (enable == kb->wake_irq_enabled)
>  		return;

[Severity: High]
Does this optimization incorrectly bypass the disable_irq() synchronization
barrier during teardown?

If the hardware IRQ fires concurrently, pogo_wake_isr() immediately calls
disable_irq_nosync() and sets kb->wake_irq_enabled to false. When
pogo_remove() subsequently calls pogo_wake_irq_enable(kb, false), it
sees the flag is already false and returns early.

This completely bypasses the standard disable_irq() call that would
otherwise wait for the executing ISR to finish. pogo_remove() will
proceed to cancel the delayed works, but the still-running ISR can
afterward queue detect_work using its own bypassed lockless checks,
leading to a use-after-free once devres cleans up the kb struct.

[ ... ]

> @@ -1181,9 +1181,9 @@ static void pogo_remove(struct serdev_device *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);

(The teardown code above is the other side of the high-severity races
identified in pogo_wake_irq_enable() and pogo_handle_sync() where background
threads queue work on freed memory).

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