Re: [PATCH] xhci: dbgtty: convert rx push tasklet to BH workqueue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] usb: xhci-dbgtty: UAF race between teardown and event_work queueing BH
- [High] USB: xhci: DbC TTY double free due to unprotected port->registered check
--

commit c7660e30ebb6c7dc3548c7784608b00e579fc030
Author: Kun Luo <luokun485@gmail.com>

xhci: dbgtty: convert rx push tasklet to BH workqueue
    
The kernel is converting remaining tasklet users to BH workqueues so
softirq handling can go through the workqueue path. The dbc TTY rx
push path is a straightforward bottom-half: schedule from IRQ /
unthrottle, run the same work, kill on port teardown.

> diff --git a/drivers/usb/host/xhci-dbgtty.c b/drivers/usb/host/xhci-dbgtty.c
> index 3d51e8d82659d..c18215751d921 100644
> --- a/drivers/usb/host/xhci-dbgtty.c
> +++ b/drivers/usb/host/xhci-dbgtty.c

[ ... ]

> @@ -189,9 +190,9 @@ dbc_read_complete(struct xhci_dbc *dbc, struct dbc_request *req)
>  		}
>  	}
>  
> -	/* defer moving data from req to tty buffer to a tasklet */
> +	/* defer moving data from req to tty buffer to BH workqueue */
>  	list_add_tail(&req->list_pool, &port->read_queue);
> -	tasklet_schedule(&port->push);
> +	queue_work(system_bh_wq, &port->push);

[Severity: High]
Does this code introduce a use-after-free race during teardown?

If a USB hot-unplug or unbind event occurs while the device is in the
DS_CONFIGURED state, and a bulk transfer event is actively being processed
by dbc->event_work on another CPU:

CPU0 (teardown via xhci_dbc_stop):
Calls dbc->driver->disconnect() which calls cancel_work_sync(&port->push).
After disconnect completes, it calls cancel_delayed_work_sync(&dbc->event_work).

CPU1 (concurrent event processing):
event_work calls xhci_dbc_giveback() which drops dbc->lock.
req->complete() calls dbc_read_complete().
queue_work(system_bh_wq, &port->push) is executed.

Because cancel_work_sync() on CPU0 has already finished, the BH work is queued
on a device that is about to be freed. Once CPU0's teardown completes,
xhci_dbc_tty_remove() frees port and dbc. The scheduled BH work subsequently
executes dbc_rx_push(), dereferencing the freed port pointer.

>  out:
>  	spin_unlock_irqrestore(&port->port_lock, flags);
>  }

[ ... ]

> @@ -479,7 +480,7 @@ xhci_dbc_tty_init_port(struct xhci_dbc *dbc, struct dbc_port *port)
>  static void
>  xhci_dbc_tty_exit_port(struct dbc_port *port)
>  {
> -	tasklet_kill(&port->push);
> +	cancel_work_sync(&port->push);

[Severity: High]
Can this destruction path race with concurrent disconnect events, leading to a
double free of TTY and FIFO resources?

If a concurrent sysfs disable write invokes xhci_dbc_stop(), and a physical
cable unplug event is processed by dbc->event_work calling
xhci_dbc_handle_events():

xhci_dbc_stop() reads dbc->state == DS_CONFIGURED under dbc->lock, but drops
the lock without changing the state. This allows event_work to also observe
DS_CONFIGURED, handle the unplug, and return EVT_DISC.

Both threads then concurrently invoke dbc->driver->disconnect().

Since xhci_dbc_tty_unregister_device() checks and clears port->registered
locklessly before calling xhci_dbc_tty_exit_port(), both threads bypass the
check and proceed with destruction. This causes a double execution of
tty_port_destroy(), idr_remove(), and kfifo_free().

>  	tty_port_destroy(&port->port);
>  }

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