Re: [PATCH 3/3] phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Missing cleanup of the USB role switch on the probe error path leads to a memory leak and a triggerable use-after-free vulnerability.
- [Medium] Missing bounds check on `args_count` in `ma35_usb_phy_xlate` leads to an uninitialized stack memory read and potential out-of-bounds access.
- [Medium] Unchecked return values of `regmap_read()` lead to the use of uninitialized stack variables.
- [Medium] Failure to explicitly clear `MISCFCR0_UHOVRCURH` when `nuvoton,oc-active-high` is absent allows bootloader state to bleed over.
--

commit c78d99f91b7305f8a53b899cb3cb3fa45c43c783
Author: Joey Lu <a0987203069@gmail.com>

phy: nuvoton: phy-ma35d1-usb2: extend to dual-port with OTG support

The existing driver handled only PHY0 in device mode (DWC2 gadget). Extend it
to manage both PHY ports and integrate OTG support, per reviewer suggestion to
reuse the existing driver rather than add a separate one.

> diff --git a/drivers/phy/nuvoton/phy-ma35d1-usb2.c b/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> index 9a459b700ed48..336680161104f 100644
> --- a/drivers/phy/nuvoton/phy-ma35d1-usb2.c
> +++ b/drivers/phy/nuvoton/phy-ma35d1-usb2.c

[ ... ]

> +static int ma35_usb_phy_init(struct phy *phy)
>  {
> -	struct ma35_usb_phy *p_phy = phy_get_drvdata(phy);
> +	struct ma35_phy_port *port = phy_get_drvdata(phy);
> +	struct ma35_usb_phy *p = container_of(port - port->idx,
> +					      struct ma35_usb_phy, port[0]);
> +	unsigned int n = port->idx;
>  	unsigned int val;
>  	int ret;
>  
> -	ret = clk_prepare_enable(p_phy->clk);
> -	if (ret < 0) {
> -		dev_err(p_phy->dev, "Failed to enable PHY clock: %d\n", ret);
> -		return ret;
> -	}
> +	regmap_read(p->sysreg, MA35_SYS_USBPMISCR, &val);

[Severity: Medium]
Could this leave val uninitialized if regmap_read() fails?

If regmap_read() fails (e.g. due to a bus error), val remains uninitialized.
Evaluating it in the next line could lead to skipping the PHY initialization
incorrectly.

> +	if (val & USBPMISCR_PHY_SUSPEND(n))
> +		return 0;

[ ... ]

> +static enum usb_role ma35_role_sw_get(struct usb_role_switch *sw)
> +{
> +	struct ma35_usb_phy *p = usb_role_switch_get_drvdata(sw);
> +	u32 val;
> +
> +	regmap_read(p->sysreg, MA35_SYS_PWRONOTP, &val);
> +
> +	return (val & PWRONOTP_USBP0ID) ? USB_ROLE_HOST : USB_ROLE_DEVICE;

[Severity: Medium]
Does this code return an unpredictable role if regmap_read() fails?

If regmap_read() encounters an error, val will remain uninitialized, which
could lead to nondeterministic driver behavior when evaluated for the returned
role.

> +}

[ ... ]

> +static struct phy *ma35_usb_phy_xlate(struct device *dev,
> +				      const struct of_phandle_args *args)
> +{
> +	struct ma35_usb_phy *p = dev_get_drvdata(dev);
> +
> +	if (args->args[0] >= MA35_PHY_NUM)

[Severity: Medium]
Can this code access uninitialized memory if args->args_count is 0?

If the device tree incorrectly specifies #phy-cells = <0>, args_count will
be 0 and the args->args array (allocated on the caller's stack) might be
uninitialized. Could we add a bounds check for args->args_count >= 1 before
accessing args->args[0]?

> +		return ERR_PTR(-EINVAL);
> +
> +	return p->port[args->args[0]].phy;
> +}

[ ... ]

>  static int ma35_usb_phy_probe(struct platform_device *pdev)
>  {

[ ... ]

> +	if (of_property_read_bool(pdev->dev.of_node, "nuvoton,oc-active-high"))
> +		regmap_update_bits(p->sysreg, MA35_SYS_MISCFCR0,
> +				   MISCFCR0_UHOVRCURH, MISCFCR0_UHOVRCURH);

[Severity: Medium]
Does this code fail to clear MISCFCR0_UHOVRCURH when "nuvoton,oc-active-high"
is absent?

If a bootloader previously set this bit to active-high, Linux will fail to
clear it upon boot when active-low is expected, potentially breaking USB
functionality.

> +
> +	for (n = 0; n < MA35_PHY_NUM; n++) {

[ ... ]

> +	ret = ma35_role_switch_init(pdev, p);
> +	if (ret)
> +		return ret;
>  
> -	provider = devm_of_phy_provider_register(&pdev->dev, of_phy_simple_xlate);
> +	provider = devm_of_phy_provider_register(&pdev->dev, ma35_usb_phy_xlate);
>  	if (IS_ERR(provider))
>  		return dev_err_probe(&pdev->dev, PTR_ERR(provider),
> -				     "Failed to register PHY provider\n");
> +				     "failed to register PHY provider\n");

[Severity: High]
Does this error path leak the USB role switch created by
ma35_role_switch_init()?

If devm_of_phy_provider_register() fails, the function returns immediately
without unregistering the role switch.

The driver core will then free the devm-allocated memory (including the
struct ma35_usb_phy *p passed as driver data). Since the role switch remains
registered, any later interaction with it (e.g. from sysfs) will invoke
ma35_role_sw_get():

static enum usb_role ma35_role_sw_get(struct usb_role_switch *sw)
{
	struct ma35_usb_phy *p = usb_role_switch_get_drvdata(sw);
	u32 val;

	regmap_read(p->sysreg, MA35_SYS_PWRONOTP, &val);

This dereferences the freed p pointer, causing a Use-After-Free.

> +
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615054911.48821-1-a0987203069@gmail.com?part=3