[PATCH v2 0/2] phy: sunplus: fix two issues in update_disc_vol()

[PATCH v2 0/2] phy: sunplus: fix two issues in update_disc_vol()

[Felix Gu]

Signed-off-by: Felix Gu <ustc.gu@gmail.com>
---
Changes in v2:
- Fix Vinod's comments.
- Split the commit into two patches, one for memory leak, one for error
  pointer deference.
- Link to v1: https://patch.msgid.link/20260804-sunplus-v1-1-79f52226cb50@gmail.com

To: Vincent Shih <vincent.sunplus@gmail.com>
To: Vinod Koul <vkoul@kernel.org>
To: Neil Armstrong <neil.armstrong@linaro.org>
Cc: linux-usb@vger.kernel.org
Cc: linux-phy@lists.infradead.org
Cc: linux-kernel@vger.kernel.org

---
Felix Gu (2):
      phy: sunplus: fix memory leak in update_disc_vol()
      phy: sunplus: fix ERR_PTR deref in update_disc_vol()

 drivers/phy/sunplus/phy-sunplus-usb2.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
---
base-commit: 9a4cdc958dd79fc6c3b20b51a10debec6ca09fec
change-id: 20260804-sunplus-53a7bb1bcd72

Best regards,
--  
Felix Gu <ustc.gu@gmail.com>


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

[PATCH v2 1/2] phy: sunplus: fix memory leak in update_disc_vol()

[Felix Gu]

nvmem_cell_read() returns a kmalloc'd buffer that update_disc_vol()
never freed. Free it after extracting the disconnect-voltage value.

Fixes: 99d9ccd97385 ("phy: usb: Add USB2.0 phy driver for Sunplus SP7021")
Signed-off-by: Felix Gu <ustc.gu@gmail.com>
---
 drivers/phy/sunplus/phy-sunplus-usb2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
index 637a5fbae6d9..842087bdfba5 100644
--- a/drivers/phy/sunplus/phy-sunplus-usb2.c
+++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
@@ -81,7 +81,7 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
 	char *disc_name = "disc_vol";
 	ssize_t otp_l = 0;
 	char *otp_v;
-	u32 val, set;
+	u32 val, set = OTP_DISC_LEVEL_DEFAULT;
 
 	cell = nvmem_cell_get(usbphy->dev, disc_name);
 	if (IS_ERR_OR_NULL(cell)) {
@@ -96,11 +96,11 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
 		set = *(otp_v + 1);
 		set = (set << (sizeof(char) * 8)) | *otp_v;
 		set = (set >> usbphy->disc_vol_addr_off) & J_DISC;
+		if (set == 0)
+			set = OTP_DISC_LEVEL_DEFAULT;
+		kfree(otp_v);
 	}
 
-	if (IS_ERR(otp_v) || set == 0)
-		set = OTP_DISC_LEVEL_DEFAULT;
-
 	val = readl(usbphy->phy_regs + CONFIG7);
 	val = (val & ~J_DISC) | set;
 	writel(val, usbphy->phy_regs + CONFIG7);

-- 
2.43.0


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

[PATCH v2 2/2] phy: sunplus: fix ERR_PTR deref in update_disc_vol()

[Felix Gu]

nvmem_cell_get() returns ERR_PTR on failure. For any error other
than -EPROBE_DEFER the code fell through and passed the invalid
pointer to nvmem_cell_read(), which dereferences it.

Skip the read and write the default disconnect voltage when the
lookup fails.

Fixes: 99d9ccd97385 ("phy: usb: Add USB2.0 phy driver for Sunplus SP7021")
Signed-off-by: Felix Gu <ustc.gu@gmail.com>
---
 drivers/phy/sunplus/phy-sunplus-usb2.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
index 842087bdfba5..eb834788ea2f 100644
--- a/drivers/phy/sunplus/phy-sunplus-usb2.c
+++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
@@ -84,9 +84,10 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
 	u32 val, set = OTP_DISC_LEVEL_DEFAULT;
 
 	cell = nvmem_cell_get(usbphy->dev, disc_name);
-	if (IS_ERR_OR_NULL(cell)) {
+	if (IS_ERR(cell)) {
 		if (PTR_ERR(cell) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
+		goto skip_nvmem_read;
 	}
 
 	otp_v = nvmem_cell_read(cell, &otp_l);
@@ -101,6 +102,7 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
 		kfree(otp_v);
 	}
 
+skip_nvmem_read:
 	val = readl(usbphy->phy_regs + CONFIG7);
 	val = (val & ~J_DISC) | set;
 	writel(val, usbphy->phy_regs + CONFIG7);

-- 
2.43.0


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

Re: [PATCH v2 1/2] phy: sunplus: fix memory leak in update_disc_vol()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on the buffer returned by `nvmem_cell_read`.
- [High] Clock and reset control resources leak on the error path of `sp_uphy_init`.
--

--- Patch [1]: [PATCH v2 1/2] phy: sunplus: fix memory leak in update_disc_vol() ---
commit 3e3f886e3032055dfbdc001346d3637c0a80b19d
Author: Felix Gu <ustc.gu@gmail.com>

phy: sunplus: fix memory leak in update_disc_vol()

nvmem_cell_read() returns a kmalloc'd buffer that update_disc_vol()
never freed. Free it after extracting the disconnect-voltage value.

> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 637a5fbae6d9a..842087bdfba53 100644
> --- a/drivers/phy/sunplus/phy-sunplus-usb2.c
> +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> @@ -81,7 +81,7 @@ static int update_disc_vol(struct sp_usbphy *usbphy)

[ ... ]

> @@ -96,11 +96,11 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
>  		set = *(otp_v + 1);

[Severity: High]
This is a pre-existing issue, but does this code need a bounds check
on the buffer returned by nvmem_cell_read()?

If the NVMEM cell defined in the device tree has a size of 1 byte,
nvmem_cell_read() will allocate 1 byte and set otp_l = 1. This code
unconditionally reads *(otp_v + 1), which could result in an out-of-bounds
read.

Additionally, if the cell size is 0 bytes, nvmem_cell_read() will allocate
0 bytes, returning ZERO_SIZE_PTR. Since ZERO_SIZE_PTR passes the IS_ERR()
check, dereferencing this pointer would immediately panic the kernel.

>  		set = (set << (sizeof(char) * 8)) | *otp_v;
>  		set = (set >> usbphy->disc_vol_addr_off) & J_DISC;
> +		if (set == 0)
> +			set = OTP_DISC_LEVEL_DEFAULT;
> +		kfree(otp_v);
>  	}
>  
> -	if (IS_ERR(otp_v) || set == 0)
> -		set = OTP_DISC_LEVEL_DEFAULT;
> -

[Severity: High]
This isn't a bug introduced by this patch, but does the error path in
sp_uphy_init() leak clock and reset control resources?

In sp_uphy_init(), if update_disc_vol() returns -EPROBE_DEFER, the code
appears to return directly instead of jumping to the err_reset cleanup
label:

drivers/phy/sunplus/phy-sunplus-usb2.c:sp_uphy_init() {
    ...
	ret = update_disc_vol(usbphy);
	if (ret < 0)
		return ret;
    ...
}

Upon probe deferral retry, wouldn't the enable counts increment again
without being decreased, permanently leaking the clock and reset control?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-sunplus-v2-0-d5379c664dfc@gmail.com?part=1

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

Re: [PATCH v2 2/2] phy: sunplus: fix ERR_PTR deref in update_disc_vol()

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing validation of nvmem cell length can lead to an out-of-bounds read or ZERO_SIZE_PTR dereference.
- [High] Clock and reset control resources are leaked due to unbalanced operations in `sp_uphy_init()` error paths.
--

--- Patch [2]: [PATCH v2 2/2] phy: sunplus: fix ERR_PTR deref in update_disc_vol() ---
commit ec8ef9e1698074ff5a592bf46bc32ad0bdd396db
Author: Felix Gu <ustc.gu@gmail.com>

phy: sunplus: fix ERR_PTR deref in update_disc_vol()

nvmem_cell_get() returns ERR_PTR on failure. The code previously fell through
to nvmem_cell_read(), dereferencing the invalid pointer. This patch skips the
read and writes the default disconnect voltage when the lookup fails.

> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 842087bdfba53..eb834788ea2fc 100644
> --- a/drivers/phy/sunplus/phy-sunplus-usb2.c
> +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> @@ -84,9 +84,10 @@ static int update_disc_vol(struct sp_usbphy *usbphy)
>  	u32 val, set = OTP_DISC_LEVEL_DEFAULT;
>  
>  	cell = nvmem_cell_get(usbphy->dev, disc_name);
> -	if (IS_ERR_OR_NULL(cell)) {
> +	if (IS_ERR(cell)) {
>  		if (PTR_ERR(cell) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;

[Severity: High]
This is a pre-existing issue, but does this return path leak the clock and
reset control references?

When update_disc_vol() returns -EPROBE_DEFER, the caller sp_uphy_init()
returns the error directly without performing cleanup:

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;
        ...
        ret = update_disc_vol(usbphy);
        if (ret < 0)
                return ret;
        ...
}

This early return bypasses the err_reset and err_clk cleanup labels, leading
to unbounded incrementing of reference counts on subsequent retries if the
nvmem provider is not yet ready.

> +		goto skip_nvmem_read;
>  	}
>  
>  	otp_v = nvmem_cell_read(cell, &otp_l);

[Severity: High]
This isn't a bug introduced by this patch, but should the length of the
nvmem cell returned in otp_l be validated?

If nvmem_cell_read() returns a cell with a length of 0, the allocator returns
ZERO_SIZE_PTR which passes the !IS_ERR() check. Dereferencing this pointer
would trigger a page fault:

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

If the cell length is 1, *(otp_v + 1) would cause an out-of-bounds read.

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260807-sunplus-v2-0-d5379c664dfc@gmail.com?part=2

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