Re: [PATCH] phy: renesas: rcar-gen3-usb2: Fix use-after-free in rcar_gen3_phy_usb2_remove due to race condition

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

New issues:
- [Critical] Accessing hardware registers in `remove()` while the device may be runtime suspended causes a synchronous external abort (kernel panic).
- [High] Unlocked hardware register access in `remove()` races with the `role_store()` sysfs operation, allowing interrupts to be re-enabled and completely bypassing the fix.

Pre-existing issues:
- [High] The patch fails to resolve the use-after-free race condition because it does not synchronize with an already-executing IRQ handler before calling `cancel_work_sync()`.
- [High] Missing initialization of `work_struct` when an IRQ is absent causes a kernel panic on sysfs writes.
- [High] Missing `cancel_work_sync()` on probe error paths leads to use-after-free if probe fails.
--

--- Patch [1]: [PATCH] phy: renesas: rcar-gen3-usb2: Fix use-after-free in rcar_gen3_phy_usb2_remove due to race condition ---
commit c1e0683754e769afd34c8d1183cbf96e6d4a4b81
Author: Pei Xiao <[email protected]>

phy: renesas: rcar-gen3-usb2: Fix use-after-free in rcar_gen3_phy_usb2_remove due to race condition

This commit attempts to fix a use-after-free in the driver remove path by
disabling OTG interrupts and canceling the work queue before the devm cleanup
happens.

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index 9a45d840efeb2..fa0e680a4b915 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -1067,8 +1067,16 @@ static void rcar_gen3_phy_usb2_remove(struct platform_device *pdev)
>  {
>  	struct rcar_gen3_chan *channel = platform_get_drvdata(pdev);
>  
> -	if (channel->is_otg_channel)
> +	if (channel->is_otg_channel) {
> +		/* Disable OTG interrupts so the IRQ handler cannot
> +		 * schedule new work.
> +		 */
> +		rcar_gen3_control_otg_irq(channel, 0);

[Severity: Critical]
Will this trigger a synchronous external abort?

If there are no active PHY consumers when rcar_gen3_phy_usb2_remove() is
called, the device will be runtime suspended and its clocks gated.

Calling rcar_gen3_control_otg_irq() here reads and writes to USB2_OBINTEN
without first waking the device via pm_runtime_resume_and_get().

[Severity: High]
Does this unlocked register access race with the sysfs role_store() function?

If CPU1 concurrently writes to the sysfs role attribute, role_store() takes
channel->lock and eventually executes rcar_gen3_control_otg_irq(channel, 1).

Because the interrupt is disabled here without channel->lock, CPU1's write
can re-enable the interrupt right before device_remove_file() is called.

This would leave the interrupt fully enabled, bypassing the intended fix.

> +
>  		device_remove_file(&pdev->dev, &dev_attr_role);
> +
> +		cancel_work_sync(&channel->work);

[Severity: High]
This is a pre-existing issue, but does this actually prevent the
use-after-free?

Since the IRQ handler is shared and managed by devres, it isn't freed until
after rcar_gen3_phy_usb2_remove() returns.

If CPU1 is already executing rcar_gen3_phy_usb2_irq(), it might have already
read the status register. CPU0 can disable the IRQ and call cancel_work_sync(),
but CPU1 can subsequently call schedule_work().

When the remove function returns and devres frees the channel, the queued work
will execute and cause a use-after-free. To fix this, the IRQ must be
explicitly freed or synchronized before cancel_work_sync().

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for sysfs writes
to trigger a NULL pointer dereference on the work queue?

In rcar_gen3_phy_usb2_probe():

    if (irq < 0 && irq != -ENXIO) {
            return irq;
    } else if (irq > 0) {
            INIT_WORK(&channel->work, rcar_gen3_phy_usb2_work);
    }

If irq <= 0, INIT_WORK is skipped. However, if dr_mode is configured,
the role sysfs attribute is still created. Writing to it will call
schedule_work(&channel->work) on an uninitialized work structure.

[Severity: High]
This is a pre-existing issue, but does the probe error path leak pending work?

If devm_request_irq() succeeds in rcar_gen3_phy_usb2_probe(), the shared IRQ
becomes live.

If a subsequent probe step fails (like devm_of_phy_provider_register), the
probe function returns an error and devres frees the IRQ and channel memory.

If the IRQ fired and scheduled channel->work, there is no explicit
cancel_work_sync() on the error path to stop it before the memory is freed,
potentially leading to a use-after-free.

> +	}
>  }
>  
>  static int rcar_gen3_phy_usb2_suspend(struct device *dev)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/d265021fb7fe432e32eedbe9e075fb041c15cbe6.1785830417.git.xiaopei01@kylinos.cn?part=1

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy