[PATCH] phy: sunplus: fix refcount leak in sp_uphy_init()

[PATCH] phy: sunplus: fix refcount leak in sp_uphy_init()

[Wentao Liang]

If update_disc_vol() fails after reset_control_deassert() succeeds,
the function returns without calling reset_control_assert() and
clk_disable_unprepare(). This leaks the reset control deassert count
and leaves the clock enabled.

Fix it by jumping to the err_reset label instead of returning
directly, so the cleanup path is executed.

Cc: stable@vger.kernel.org
Fixes: 99d9ccd97385 ("phy: usb: Add USB2.0 phy driver for Sunplus SP7021")
Signed-off-by: Wentao Liang <vulab@iscas.ac.cn>
---
 drivers/phy/sunplus/phy-sunplus-usb2.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
index 637a5fbae6d9..c7bc3ac89ca9 100644
--- a/drivers/phy/sunplus/phy-sunplus-usb2.c
+++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
@@ -129,7 +129,7 @@ static int sp_uphy_init(struct phy *phy)
 	/* disconnect voltage */
 	ret = update_disc_vol(usbphy);
 	if (ret < 0)
-		return ret;
+		goto err_reset;
 
 	/* board uphy 0 internal register modification for tid certification */
 	val = readl(usbphy->phy_regs + CONFIG9);
-- 
2.34.1


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

Re: [PATCH] phy: sunplus: fix refcount leak in sp_uphy_init()

[sashiko-bot]

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

Pre-existing issues:
- [High] Kernel panic due to dereferencing an ERR_PTR if the optional nvmem cell fails to load.
- [High] Memory leak of the buffer returned by `nvmem_cell_read()`.
- [High] Unconditional out-of-bounds read on the nvmem buffer.
- [High] Imbalanced cleanup in error handling causing reference count underflows.
--

commit 98fd47236db60bb82bb9a431f603e28290ff2a8b
Author: Wentao Liang <vulab@iscas.ac.cn>

phy: sunplus: fix refcount leak in sp_uphy_init()

This commit changes the error path in sp_uphy_init to correctly jump to
err_reset instead of returning directly when update_disc_vol fails,
ensuring resources are cleaned up.

Cc: stable@vger.kernel.org
Fixes: 99d9ccd97385 ("phy: usb: Add USB2.0 phy driver for Sunplus SP7021")

> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 637a5fbae6d9a..c7bc3ac89ca9a 100644
> --- a/drivers/phy/sunplus/phy-sunplus-usb2.c
> +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> @@ -129,7 +129,7 @@ static int sp_uphy_init(struct phy *phy)
>  	/* disconnect voltage */
>  	ret = update_disc_vol(usbphy);

[Severity: High]
This is a pre-existing issue, but could this call lead to a kernel panic
if the optional nvmem cell fails to load?

In drivers/phy/sunplus/phy-sunplus-usb2.c:update_disc_vol(), the return
value of nvmem_cell_get is checked, but if it is an error pointer other
than -EPROBE_DEFER, it is passed directly to nvmem_cell_read:

drivers/phy/sunplus/phy-sunplus-usb2.c:update_disc_vol() {
	cell = nvmem_cell_get(usbphy->dev, disc_name);
	if (IS_ERR_OR_NULL(cell)) {
		if (PTR_ERR(cell) == -EPROBE_DEFER)
			return -EPROBE_DEFER;
	}

	otp_v = nvmem_cell_read(cell, &otp_l);
	...
}

Does this cause an invalid memory access when nvmem_cell_read attempts to
dereference the unhandled error pointer?

[Severity: High]
This is a pre-existing issue, but does update_disc_vol() leak the buffer
returned by nvmem_cell_read()?

	otp_v = nvmem_cell_read(cell, &otp_l);
	nvmem_cell_put(cell);

	if (!IS_ERR(otp_v)) {
		set = *(otp_v + 1);
		set = (set << (sizeof(char) * 8)) | *otp_v;
	...

Is the dynamically allocated memory for otp_v leaked when the function
returns?

[Severity: High]
This is a pre-existing issue, but is there a potential out-of-bounds read
on the nvmem buffer here?

	if (!IS_ERR(otp_v)) {
		set = *(otp_v + 1);
	...

Are we unconditionally accessing the second byte of the buffer without
verifying that the returned length in otp_l is at least 2 bytes?

>  	if (ret < 0)
> -		return ret;
> +		goto err_reset;

[Severity: High]
This is a pre-existing issue, but do the error paths in sp_uphy_init()
incorrectly clean up resources that failed to initialize?

If clk_prepare_enable fails, the code jumps to err_clk and calls
clk_disable_unprepare on the same clock:

drivers/phy/sunplus/phy-sunplus-usb2.c:sp_uphy_init() {
	ret = clk_prepare_enable(usbphy->phy_clk);
	if (ret)
		goto err_clk;

	ret = reset_control_deassert(usbphy->rstc);
	if (ret)
		goto err_reset;
	...
err_reset:
	reset_control_assert(usbphy->rstc);
err_clk:
	clk_disable_unprepare(usbphy->phy_clk);

	return ret;
}

Would this decrement the clock framework's reference counts for a clock
that was never successfully enabled? Similarly, if reset_control_deassert
fails, it asserts the reset that failed to deassert.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260616142234.1690457-1-vulab@iscas.ac.cn?part=1

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