Re: [PATCH 1/3] usb: typec: tcpm: Don't cut a pending state delay short on a re-queue

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] usb: typec: tcpm: use-after-free in tcpm_unregister_port()
  [1]
--

commit fc426c7d09bd548d8d6ca702db9699249b1a96bd
Author: Arun Iyengar <aiyengar1990@gmail.com>

usb: typec: tcpm: Don't cut a pending state delay short on a re-queue

This commit checks whether a pending state transition delay has expired in
tcpm_state_machine_work() and re-arms port->state_machine_timer for the
remaining time if the work item was re-queued early.

> diff --git a/drivers/usb/typec/tcpm/tcpm.c b/drivers/usb/typec/tcpm/tcpm.c
> index d721baef52104..0a19b7665555b 100644
> --- a/drivers/usb/typec/tcpm/tcpm.c
> +++ b/drivers/usb/typec/tcpm/tcpm.c
> @@ -6569,6 +6569,18 @@ static void tcpm_state_machine_work(struct kthread_work *work)
>  
>  	/* If we were queued due to a delayed state change, update it now */
>  	if (port->delayed_state) {
> +		/*
> +		 * The work may have been queued again for another reason
> +		 * while it was running, for instance by tcpm_init() at port
> +		 * registration, which sets two states in a row with the
> +		 * port lock held. Do not cut a pending delay short; re-arm
> +		 * the timer for the time that is left.
> +		 */
> +		if (ktime_after(port->delayed_runtime, ktime_get())) {
> +			hrtimer_start(&port->state_machine_timer,
> +				      port->delayed_runtime, HRTIMER_MODE_ABS);
> +			goto done;
> +		}

[Severity: Critical]
This problem wasn't introduced by this patch, but can
state_machine_timer_handler() trigger a use-after-free on port->wq during
tcpm_unregister_port() [1]?

drivers/usb/typec/tcpm/tcpm.c:state_machine_timer_handler() {
	if (port->registered)
		kthread_queue_work(port->wq, &port->state_machine);
	return HRTIMER_NORESTART;
}

drivers/usb/typec/tcpm/tcpm.c:tcpm_unregister_port() {
	port->registered = false;
	kthread_destroy_worker(port->wq);
	port->wq = NULL;

	hrtimer_cancel(&port->vdm_discovery_timer);
	hrtimer_cancel(&port->enable_frs_timer);
	hrtimer_cancel(&port->vdm_state_machine_timer);
	hrtimer_cancel(&port->state_machine_timer);
    ...
}

Because tcpm_unregister_port() destroys port->wq before cancelling the
hrtimers, an expiring timer on another CPU can pass the port->registered
check and queue work onto the freed worker:

CPU0                              CPU1
tcpm_unregister_port()            state_machine_timer_handler()
                                    if (port->registered)
  port->registered = false;
  kthread_destroy_worker(port->wq);
                                    kthread_queue_work(port->wq, ...);
                                      <-- use-after-free
  hrtimer_cancel(&port->state_machine_timer);

Cancelling the hrtimers after setting port->registered = false and before
calling kthread_destroy_worker(port->wq) would avoid this race.

[1] https://sashiko.dev/#/bug/linux-3223298d-cf35-4f30-825a-9241513c9ba5

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110-tcpm-attach-reboot-v1-0-7657cc1e2dca@gmail.com?part=1