Re: [PATCH v3 7/9] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] `regulator_is_enabled()` returns a negative error code on failure, which incorrectly evaluates to true and causes an unbalanced disable WARN_ON.

Pre-existing issues:
- [High] If the regulator was initially enabled in hardware AND explicitly enabled by the driver, only one disable action is registered, leading to a resource leak and WARN_ON.
- [High] If `regulator_enable()` fails, the function returns early without registering a disable action, leaking the implicit `enable_count` reference if the regulator was already enabled.
- [High] Missing cancellation of the pending workqueue task (`channel->work`) during device removal leads to a Use-After-Free.
--

--- Patch [7]: [PATCH v3 7/9] phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator ---
commit 30054990ee806fb8d79768ae79587b9acfe0f6a6
Author: Biju Das <biju.das.jz@bp.renesas.com>

phy: renesas: phy-rcar-gen3-usb2: Fix devm action registration for disabled VBUS regulator

This commit updates the VBUS regulator registration to always add a devm
disable action based on the actual enabled state, rather than only when
an explicit enable was requested. This ensures the regulator's enable_count
is correctly decremented during device removal even if it was left enabled
by the bootloader.

> diff --git a/drivers/phy/renesas/phy-rcar-gen3-usb2.c b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> index d06fb52ed5f1b..ef38c3b365d42 100644
> --- a/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> +++ b/drivers/phy/renesas/phy-rcar-gen3-usb2.c
> @@ -905,15 +905,17 @@ static int rcar_gen3_phy_usb2_vbus_regulator_get_exclusive_enable(struct rcar_ge
>  	if (IS_ERR(channel->vbus))
>  		return PTR_ERR(channel->vbus);
>  
> -	if (!enable)
> -		return 0;
> +	if (enable) {
> +		ret = regulator_enable(channel->vbus);
> +		if (ret)
> +			return ret;

[Severity: High]
This is a pre-existing issue, but does this early return leak the implicit
enable_count reference if the regulator was already enabled?

If devm_regulator_get_exclusive() initializes enable_count to 1 (because the
hardware was already enabled), and this regulator_enable() call fails, we
return early and bypass registering the devm disable action. When the probe
fails and devres cleans up, regulator_put() will be called with
enable_count == 1, which triggers a WARN_ON(regulator->enable_count) and
leaks the underlying rdev->use_count.

> +	}
>  
> -	ret = regulator_enable(channel->vbus);
> -	if (ret)
> -		return ret;
> +	if (regulator_is_enabled(channel->vbus))
> +		return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> +						channel->vbus);

[Severity: Medium]
Does regulator_is_enabled() return a negative error code on failure, and
will that be incorrectly evaluated as true here?

If a hardware or I2C error occurs, regulator_is_enabled() can return a
negative value. This condition would evaluate as true, queueing the
rcar_gen3_phy_usb2_vbus_disable_action even if the regulator is actually
disabled or its enable_count is 0. 

On device removal, calling regulator_disable() with an enable_count of 0
triggers a WARN in the regulator core.

[Severity: High]
This is a pre-existing issue, but does this logic lead to a resource leak if
the regulator was initially enabled in hardware and explicitly enabled by the
driver?

If the bootloader leaves the regulator enabled,
devm_regulator_get_exclusive() initializes enable_count to 1. If the driver
also explicitly requests enable = true, regulator_enable() increments
enable_count to 2. 

However, this logic only registers a single disable action. During unbind,
the action decrements enable_count to 1, and devm_regulator_release() calls
regulator_put() which triggers a WARN_ON and destroys the consumer struct
without fully decrementing the underlying rdev->use_count.

> -	return devm_add_action_or_reset(dev, rcar_gen3_phy_usb2_vbus_disable_action,
> -					channel->vbus);
> +	return 0;
>  }

[Severity: High]
This is a pre-existing issue, but does this device removal sequence miss
canceling the pending workqueue task?

drivers/phy/renesas/phy-rcar-gen3-usb2.c:rcar_gen3_phy_usb2_remove() {
	struct rcar_gen3_chan *channel = platform_get_drvdata(pdev);

	if (channel->is_otg_channel)
		device_remove_file(&pdev->dev, &dev_attr_role);
}

The driver schedules channel->work from its IRQ handler. If the driver is
unbound while an IRQ is being processed, rcar_gen3_phy_usb2_remove()
executes but does not call cancel_work_sync(&channel->work).

As devres cleans up, it frees the channel structure. If the workqueue
executes after channel is freed, it will dereference freed memory leading
to a use-after-free.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260707143646.245177-1-biju.das.jz@bp.renesas.com?part=7

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy