Re: [PATCH] usb: chipidea: stop asynchronous work before destroying roles

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: chipidea: stale OTG event flags and teardown race in
  ci_hdrc_otg_destroy()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] usb: chipidea: uncanceled otg_fsm_hrtimer in
  ci_hdrc_otg_destroy() [1]
--

commit b8def951ac8ca82d212a5f915b8e9c3017f3be77
Author: Xu Yang <xu.yang_2@nxp.com>

usb: chipidea: stop asynchronous work before destroying roles

This commit reorders role and OTG teardown so that asynchronous work items,
interrupts, and the OTG FSM are stopped before the gadget and host roles and
workqueue are destroyed.

> diff --git a/drivers/usb/chipidea/otg.c b/drivers/usb/chipidea/otg.c
> index fecc7d7e2f0d7..ec11056dddfaf 100644
> --- a/drivers/usb/chipidea/otg.c
> +++ b/drivers/usb/chipidea/otg.c
> @@ -264,12 +264,21 @@ int ci_hdrc_otg_init(struct ci_hdrc *ci)
>   */
>  void ci_hdrc_otg_destroy(struct ci_hdrc *ci)
>  {
> -	if (ci->wq)
> -		destroy_workqueue(ci->wq);
> +	disable_irq(ci->irq);
> +	if (disable_work_sync(&ci->work)) {
> +		ci->id_event = false;
> +		ci->b_sess_valid_event = false;
> +		/* Balance disable_irq_nosync() performed in ci_otg_queue_work() */
> +		enable_irq(ci->irq);
> +	}
> +
> +	if (ci_otg_is_fsm_mode(ci))
> +		ci_hdrc_otg_fsm_remove(ci);

[Severity: Critical]
This problem wasn't introduced by this patch, but can ci->otg_fsm_hrtimer
fire after teardown [1] because neither ci_hdrc_otg_fsm_remove() nor
ci_hdrc_otg_destroy() cancels it?

In OTG FSM mode, ci_otg_add_timer() arms ci->otg_fsm_hrtimer with
ci_otg_hrtimer_func(), while ci_hdrc_otg_fsm_remove() only removes the sysfs
group.  If an armed timer expires during or after teardown,
ci_otg_hrtimer_func() dereferences the freed struct ci_hdrc and queues work on
the destroyed ci->wq workqueue, causing a use-after-free.

>  	/* Disable all OTG irq and clear status */
>  	hw_write_otgsc(ci, OTGSC_INT_EN_BITS | OTGSC_INT_STATUS_BITS,
>  						OTGSC_INT_STATUS_BITS);
> -	if (ci_otg_is_fsm_mode(ci))
> -		ci_hdrc_otg_fsm_remove(ci);
> +	if (ci->wq)
> +		destroy_workqueue(ci->wq);
> +	enable_irq(ci->irq);
>  }

[Severity: Medium]
Can leaving ci->id_event or ci->b_sess_valid_event set while ci->work is
disabled cause an unhandled interrupt storm after enable_irq(ci->irq)?

ci_hdrc_otg_destroy() only clears ci->id_event and ci->b_sess_valid_event when
disable_work_sync(&ci->work) cancels a pending work item, and it disables
ci->work before hw_write_otgsc() clears cable->enabled.  An extcon event
arriving via ci_cable_notifier() -> ci_irq() before hw_write_otgsc() runs
(which disable_irq(ci->irq) does not block), or an earlier OTG FSM transition
past OTG_STATE_A_IDLE in ci_otg_fsm_work(), leaves ci->id_event or
ci->b_sess_valid_event set with ci->work disabled:

drivers/usb/chipidea/core.c:ci_irq_handler() {
	if (ci->is_otg) {
		...
		if (ci->id_event || ci->b_sess_valid_event) {
			ci_otg_queue_work(ci);
			return IRQ_HANDLED;
		}
	}

	/* Handle device/host interrupt */
	if (ci->role != CI_ROLE_END)
		ret = ci_role(ci)->irq(ci);
}

Once enable_irq(ci->irq) runs at the end of ci_hdrc_otg_destroy(), any host or
gadget interrupt during role teardown takes that early return in
ci_irq_handler().  Because ci->work is disabled, queue_work() in
ci_otg_queue_work() returns false and immediately calls enable_irq(ci->irq) in
hardirq context without ever calling ci_role(ci)->irq(ci) to clear the
hardware interrupt status.

Disabling OTG interrupts with hw_write_otgsc() before disable_work_sync() and
unconditionally clearing ci->id_event and ci->b_sess_valid_event after
disable_work_sync() would avoid this.

[1] https://sashiko.dev/#/bug/linux-8569f327-701f-4376-8f59-b4c911b172bb

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112092556.2747348-1-xu.yang_2@oss.nxp.com?part=1