Re: [PATCH v7 7/8] usb: misc: qcom_eud: fix virtual attach/detach event handling

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but registration of the USB role switch occurs before hardware resources are mapped, exposing uninitialized state during probe and creating a use-after-unmap vulnerability during driver removal.
- [High] This problem wasn't introduced by this patch, but the primary interrupt handler sleeps in atomic context and fails to handle simultaneous interrupts, leading to IRQ storms and kernel panics.
--

commit 67f0b0cd525e7084b985eee06acbfa51320d857a
Author: Elson Serrao <elson.serrao@oss.qualcomm.com>

usb: misc: qcom_eud: fix virtual attach/detach event handling

EUD provides virtual USB attach/detach events to simulate cable
plug/unplug while maintaining the physical debug connection. However,
the current implementation incorrectly sets the USB role to HOST on
virtual detach, which doesn't represent the disconnected state.

> diff --git a/drivers/usb/misc/qcom_eud.c b/drivers/usb/misc/qcom_eud.c
> index 549c0d852bbba..605f675ec7dd0 100644
> --- a/drivers/usb/misc/qcom_eud.c
> +++ b/drivers/usb/misc/qcom_eud.c
> @@ -306,10 +306,26 @@ static irqreturn_t handle_eud_irq_thread(int irq, void *data)
>  
>  	path = chip->paths[chip->port_idx];
>  
> +	/*
> +	 * EUD virtual attach/detach event handling for low power debugging:
> +	 *
> +	 * When EUD is enabled in debug mode, the device remains physically
> +	 * connected to the PC throughout the debug session, keeping the USB
> +	 * controller active. This prevents testing of low power scenarios that
> +	 * require USB disconnection.
> +	 *
> +	 * EUD solves this by providing virtual USB attach/detach events while
> +	 * maintaining the physical connection. These events are triggered from
> +	 * the Host PC via the enumerated EUD control interface and delivered
> +	 * to the EUD driver as interrupts.
> +	 *
> +	 * These notifications are forwarded to the USB controller through role
> +	 * switch framework.
> +	 */
>  	if (chip->usb_attached)
>  		ret = usb_role_switch_set_role(path->controller_sw, USB_ROLE_DEVICE);
>  	else
> -		ret = usb_role_switch_set_role(path->controller_sw, USB_ROLE_HOST);
> +		ret = usb_role_switch_set_role(path->controller_sw, USB_ROLE_NONE);
>  	if (ret)
>  		dev_err(chip->dev, "failed to set role switch\n");
>  

[Severity: High]
This isn't a bug introduced by this patch, but looking at eud_probe()
further down in the same file, the USB role switch is registered before
hardware resources are mapped:

drivers/usb/misc/qcom_eud.c:eud_probe() {
    ...
	for_each_child_of_node_scoped(np, child) {
		ret = eud_init_path(chip, child);
		if (ret)
			return ret;
	}

	/* Primary path is mandatory. Secondary is optional */
	if (!chip->paths[0])
		return dev_err_probe(chip->dev, -ENODEV, "primary path not found\n");

	chip->base = devm_platform_ioremap_resource(pdev, 0);
    ...
}

Does this expose uninitialized state during probe and create a use-after-unmap
vulnerability during driver removal?

If devm_platform_ioremap_resource() fails during probe, or during driver
removal, chip->base is unmapped before the USB role switch is unregistered
because devm actions execute in reverse order.

If the role switch receives an event (e.g., from userspace via sysfs) in this
window, eud_role_switch_set() calls enable_eud() or disable_eud(), which
access the unmapped chip->base memory.


[Severity: High]
This is a pre-existing issue, but the primary interrupt handler
handle_eud_irq() sleeps in atomic context and fails to handle
simultaneous interrupts:

drivers/usb/misc/qcom_eud.c:handle_eud_irq() {
    ...
	switch (reg & EUD_INT_ALL) {
	case EUD_INT_VBUS:
		usb_attach_detach(chip);
		return IRQ_WAKE_THREAD;
	case EUD_INT_SAFE_MODE:
		pet_eud(chip);
		return IRQ_HANDLED;
	default:
		return IRQ_NONE;
	}
    ...
}

If both EUD_INT_VBUS and EUD_INT_SAFE_MODE bits are asserted, does the bitwise
AND operation evaluate to 0x14, bypassing the handlers and dropping both
interrupts?

Additionally, when EUD_INT_SAFE_MODE fires, handle_eud_irq() calls pet_eud():

drivers/usb/misc/qcom_eud.c:pet_eud() {
    ...
		/* Delay to make sure detach pet is done before attach pet */
		ret = readl_poll_timeout(chip->base + EUD_REG_SW_ATTACH_DET,
					reg, (reg == 0), 1, 100);
    ...
}

Does this readl_poll_timeout() macro call usleep_range() inside the hard IRQ
context? Should it use readl_poll_timeout_atomic() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260831030147.1500616-1-elson.serrao@oss.qualcomm.com?part=7