Re: [PATCH] usb: gadget: aspeed-vhub: cancel wake work on device removal

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Tue, 2026-09-08 at 04:17 +0000, Fan Wu wrote:
> wake_work is armed from the gadget .wakeup callback to resume suspended
> downstream ports, and it is never cancelled in ast_vhub_remove(), so a
> work item queued before or during removal can run after devm has freed
> vhub and its ports, a use-after-free.
> 
> Cancelling the work alone is not sufficient: usb_gadget_wakeup() takes
> no lock and the unbind path never clears wakeup_en, so a remote-wakeup
> request in flight on another CPU can re-arm the work after the cancel
> and before vhub is freed.
> 
> ast_vhub_del_dev() already clears d->registered under vhub->lock before
> unregistering the gadget. Refuse the wakeup in ast_vhub_udc_wakeup()
> once d->registered is clear: the check and the schedule_work() are then
> atomic against the unbind, so a wakeup that passed before the flag was
> cleared is drained by the cancel_work_sync() after the del_dev loop,
> and one that arrives later returns without arming. Also move INIT_WORK()
> to the top of ast_vhub_probe(), since the probe error path reuses
> ast_vhub_remove() and would otherwise cancel a never-initialized work
> item.
> 
> This issue was found by an in-house static analysis tool.

Reviewed-by: Benjamin Herrenschmidt <benh@kernel.crashing.org>

> Fixes: 7ecca2a4080c ("usb/gadget: Add driver for Aspeed SoC virtual hub")
> Cc: stable@vger.kernel.org
> Assisted-by: Codex:gpt-5.6
> Co-developed-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Song Li <songl@zju.edu.cn>
> Signed-off-by: Fan Wu <fanwu01@zju.edu.cn>
> ---
>  drivers/usb/gadget/udc/aspeed-vhub/core.c | 4 ++++
>  drivers/usb/gadget/udc/aspeed-vhub/dev.c  | 2 +-
>  drivers/usb/gadget/udc/aspeed-vhub/hub.c  | 5 +++--
>  drivers/usb/gadget/udc/aspeed-vhub/vhub.h | 1 +
>  4 files changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/core.c b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> index 4a8b9ff83..069673f0d 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/core.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/core.c
> @@ -267,6 +267,9 @@ static void ast_vhub_remove(struct platform_device *pdev)
>  	for (i = 0; i < vhub->max_ports; i++)
>  		ast_vhub_del_dev(&vhub->ports[i].dev);
>  
> +	/* Final drain; the worker takes vhub->lock, so stay outside of it */
> +	cancel_work_sync(&vhub->wake_work);
> +
>  	spin_lock_irqsave(&vhub->lock, flags);
>  
>  	/* Mask & ack all interrupts  */
> @@ -328,6 +331,7 @@ static int ast_vhub_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  
>  	spin_lock_init(&vhub->lock);
> +	INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
>  	vhub->pdev = pdev;
>  	vhub->port_irq_mask = GENMASK(VHUB_IRQ_DEV1_BIT + vhub->max_ports - 1,
>  				      VHUB_IRQ_DEV1_BIT);
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/dev.c b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> index 8b9449d16..4b389de0d 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/dev.c
> @@ -280,7 +280,7 @@ static int ast_vhub_udc_wakeup(struct usb_gadget* gadget)
>  	int rc = -EINVAL;
>  
>  	spin_lock_irqsave(&d->vhub->lock, flags);
> -	if (!d->wakeup_en)
> +	if (!d->wakeup_en || !d->registered)
>  		goto err;
>  
>  	DDBG(d, "Device initiated wakeup\n");
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/hub.c b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> index 02fe1a08d..d0345f310 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/hub.c
> @@ -558,7 +558,7 @@ void ast_vhub_device_connect(struct ast_vhub *vhub,
>  		ast_vhub_send_host_wakeup(vhub);
>  }
>  
> -static void ast_vhub_wake_work(struct work_struct *work)
> +void ast_vhub_wake_work(struct work_struct *work)
>  {
>  	struct ast_vhub *vhub = container_of(work,
>  					     struct ast_vhub,
> @@ -588,6 +588,8 @@ static void ast_vhub_wake_work(struct work_struct *work)
>  
>  void ast_vhub_hub_wake_all(struct ast_vhub *vhub)
>  {
> +	lockdep_assert_held(&vhub->lock);
> +
>  	/*
>  	 * A device is trying to wake the world, because this
>  	 * can recurse into the device, we break the call chain
> @@ -1076,7 +1078,6 @@ static int ast_vhub_init_desc(struct ast_vhub *vhub)
>  int ast_vhub_init_hub(struct ast_vhub *vhub)
>  {
>  	vhub->speed = USB_SPEED_UNKNOWN;
> -	INIT_WORK(&vhub->wake_work, ast_vhub_wake_work);
>  
>  	return ast_vhub_init_desc(vhub);
>  }
> diff --git a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> index aca2050e2..96f1c3709 100644
> --- a/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> +++ b/drivers/usb/gadget/udc/aspeed-vhub/vhub.h
> @@ -547,6 +547,7 @@ void ast_vhub_hub_suspend(struct ast_vhub *vhub);
>  void ast_vhub_hub_resume(struct ast_vhub *vhub);
>  void ast_vhub_hub_reset(struct ast_vhub *vhub);
>  void ast_vhub_hub_wake_all(struct ast_vhub *vhub);
> +void ast_vhub_wake_work(struct work_struct *work);
>  
>  /* dev.c */
>  int ast_vhub_init_dev(struct ast_vhub *vhub, unsigned int idx);