[PATCH] Input: cm109 - fix URB double-submit due to missing open check

[PATCH] Input: cm109 - fix URB double-submit due to missing open check

[Xue Lei]

Add a dev->open check at the beginning of cm109_input_ev() to reject
events when the device is not open. There is no point in handling
input events for a device that has not been fully initialized.

The cm109 driver's event callback cm109_input_ev() can be invoked via
kd_mksound() -> input_inject_event() as soon as the input handle is
registered in kbd_connect(), which happens during
input_register_device() before cm109_input_open() has completed.
Without checking dev->open, the callback calls
cm109_toggle_buzzer_async() which submits urb_ctl while
cm109_input_open() is also about to submit the same URB, causing a
'list_add double add' BUG in usb_hcd_link_urb_to_ep().

Two crash scenarios have been observed:

Scenario 1: Same-CPU preemption (CONFIG_PREEMPT=y)

  During input_register_device(), kbd_connect() calls
  input_open_device() which invokes cm109_input_open(). Between
  setting ctl_urb_pending=1 and submitting the URB, the kworker
  thread is preempted by a process writing to a tty (triggering
  kd_mksound). The preempting process calls input_inject_event()
  -> cm109_input_ev() -> cm109_toggle_buzzer_async(), which sees
  ctl_urb_pending=0 (not yet set) and submits urb_ctl. When the
  kworker resumes, it also submits the same urb_ctl.

  CPU 0 (kworker)              CPU 0 (preempting)
  ---------------              ------------------
  input_open():
    // about to set pending=1
    <preempted>
                               input_ev():
                                 toggle_buzzer_async():
                                   ctl_urb_pending == 0
                                   submit_urb(urb_ctl) [1st]
    <resumes>
    ctl_urb_pending = 1
    submit_urb(urb_ctl) [2nd] --> CRASH

Scenario 2: Cross-CPU concurrency

  kbd_connect() has made the device handle visible to kd_mksound
  on CPU 1, but cm109_input_open() on CPU 0 has not yet finished.
  CPU 1 calls cm109_input_ev() first, sees dev->open=0 and
  ctl_urb_pending=0 (device was just kzalloc'd), and submits
  urb_ctl. Shortly after, CPU 0's cm109_input_open() also submits
  the same urb_ctl that is already on the endpoint list.

  CPU 0 (kworker)              CPU 1 (repro)
  ---------------              ---------------
  input_register_device()
    kbd_connect()
      input_register_handle()
                               kd_mksound() sees handle
      input_open_device()        input_ev():
        cm109_input_open():        toggle_buzzer_async():
          // not started yet         ctl_urb_pending == 0
                                     open == 0
                                     submit_urb [1st]
          ctl_urb_pending = 1
          submit_urb [2nd] --> CRASH

In both cases the root cause is that cm109_input_ev() does not
verify the device is open before triggering URB submission. Adding
the open check at function entry prevents the event callback from
racing with cm109_input_open() on URB submission.

Reported-by: syzbot+e69c25cf38a53d0cf64c@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=e69c25cf38a53d0cf64c
Fixes: c04148f915e5 ("Input: add driver for USB VoIP phones with CM109 chipset")
Signed-off-by: Xue Lei <Xue.Lei@windriver.com>
---
 drivers/input/misc/cm109.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
index 353d3c1d347d..558cb73d5c6d 100644
--- a/drivers/input/misc/cm109.c
+++ b/drivers/input/misc/cm109.c
@@ -601,6 +601,9 @@ static int cm109_input_ev(struct input_dev *idev, unsigned int type,
 {
 	struct cm109_dev *dev = input_get_drvdata(idev);
 
+	if (!dev->open)
+		return -ENODEV;
+
 	dev_dbg(&dev->intf->dev,
 		"input_ev: type=%u code=%u value=%d\n", type, code, value);
 
-- 
2.43.0

Re: [PATCH] Input: cm109 - fix URB double-submit due to missing open check

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] TOCTOU Race leading to Use-After-Free during device disconnect
- [High] State Machine Stall due to unhandled URB submission failure
- [High] DMA API Violation from passing coherent buffer to streaming DMA mapping
--

--- Patch [1]: [PATCH] Input: cm109 - fix URB double-submit due to missing open check ---
commit 8242b0347566c678c3b0f2e9b93934d136b010bb
Author: Xue Lei <Xue.Lei@windriver.com>

Input: cm109 - fix URB double-submit due to missing open check

This commit adds a dev->open check in cm109_input_ev() to reject input
events when the device is not fully initialized, preventing a double-submit
of urb_ctl.

Closes: https://syzkaller.appspot.com/bug?extid=e69c25cf38a53d0cf64c

> diff --git a/drivers/input/misc/cm109.c b/drivers/input/misc/cm109.c
> index 353d3c1d347d5..558cb73d5c6df 100644
> --- a/drivers/input/misc/cm109.c
> +++ b/drivers/input/misc/cm109.c
> @@ -601,6 +601,9 @@ static int cm109_input_ev(struct input_dev *idev, unsigned int type,
>  {
>  	struct cm109_dev *dev = input_get_drvdata(idev);
>  
> +	if (!dev->open)
> +		return -ENODEV;
> +

[Severity: High]
This is a pre-existing issue, but does this lockless check introduce a TOCTOU
race during device disconnect?

Looking at cm109_input_close(), dev->open is cleared after traffic is
stopped:

cm109_input_close()
    cm109_stop_traffic(dev);
    dev->open = 0;

If a concurrent event injection happens simultaneously, couldn't
cm109_input_ev() see dev->open == 1 just before cm109_input_close() sets
it to 0?

Since cm109_usb_cleanup() frees the dev structure without explicitly calling
usb_kill_urb(), could this allow an asynchronously submitted URB to complete
and access the freed dev structure?

>  	dev_dbg(&dev->intf->dev,
>  		"input_ev: type=%u code=%u value=%d\n", type, code, value);
>  

[Severity: High]
This is a pre-existing issue, but can the URB submission loop stall
permanently on failure?

Looking at cm109_submit_buzz_toggle(), if usb_submit_urb() fails due to
memory pressure or bus errors, it logs the error but does not clear
dev->ctl_urb_pending:

cm109_submit_buzz_toggle()
    error = usb_submit_urb(dev->urb_ctl, GFP_ATOMIC);
    if (error)
        dev_err(&dev->intf->dev, ...);

If ctl_urb_pending remains 1, wouldn't subsequent calls to
cm109_toggle_buzzer_async() always take this path:

cm109_toggle_buzzer_async()
    if (dev->ctl_urb_pending) {
        dev->buzzer_pending = 1;
        return;
    }

Since the URB was never actually submitted, its completion callback won't
run to submit urb_irq, meaning the device would permanently stop receiving
input events and buzzer updates until re-plugged?

[Severity: High]
This is a pre-existing issue, but does the synchronous buzzer toggle violate
DMA mapping rules?

In cm109_usb_probe(), dev->ctl_data is allocated using usb_alloc_coherent().
Later, cm109_toggle_buzzer_sync() passes this coherent buffer directly to
usb_control_msg():

cm109_toggle_buzzer_sync()
    error = usb_control_msg(dev->udev,
                            usb_sndctrlpipe(dev->udev, 0),
                            ...
                            dev->ctl_data,
                            USB_PKT_LEN, USB_CTRL_SET_TIMEOUT);

Since usb_control_msg() uses streaming DMA mapping via dma_map_single()
without the URB_NO_TRANSFER_DMA_MAP flag, could passing a buffer allocated
with a coherent API trigger mapping errors on architectures with
CONFIG_DMA_API_DEBUG enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709073133.373657-1-Xue.Lei@windriver.com?part=1