[PATCH] phy: sunplus: fix crash when the disc_vol nvmem cell is missing

[PATCH] phy: sunplus: fix crash when the disc_vol nvmem cell is missing

[Felix Gu]

nvmem_cell_get() can fail with errors other than -EPROBE_DEFER, but
update_disc_vol() only handled -EPROBE_DEFER and then called
nvmem_cell_read() with the error pointer, crashing the kernel.

Fall back to the default disconnect voltage on a failed cell lookup,
and free the buffer returned by nvmem_cell_read().

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 | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
index 637a5fbae6d9..94146118e9c2 100644
--- a/drivers/phy/sunplus/phy-sunplus-usb2.c
+++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
@@ -81,12 +81,13 @@ 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)) {
+	if (IS_ERR(cell)) {
 		if (PTR_ERR(cell) == -EPROBE_DEFER)
 			return -EPROBE_DEFER;
+		goto out;
 	}
 
 	otp_v = nvmem_cell_read(cell, &otp_l);
@@ -96,11 +97,12 @@ 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;
-
+out:
 	val = readl(usbphy->phy_regs + CONFIG7);
 	val = (val & ~J_DISC) | set;
 	writel(val, usbphy->phy_regs + CONFIG7);

---
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

Re: [PATCH] phy: sunplus: fix crash when the disc_vol nvmem cell is missing

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing length check on the dynamically sized buffer returned by nvmem_cell_read() leads to a heap out-of-bounds read and potential information leak to MMIO.
- [High] Clock and reset resource leak in sp_uphy_init() error path when update_disc_vol() fails.
--

--- Patch [1]: [PATCH] phy: sunplus: fix crash when the disc_vol nvmem cell is missing ---
commit 3ff3eec8ebe3bd1dab2325248c9d68c39dbf60d4
Author: Felix Gu <ustc.gu@gmail.com>

phy: sunplus: fix crash when the disc_vol nvmem cell is missing

nvmem_cell_get() can fail with errors other than -EPROBE_DEFER, but
update_disc_vol() only handled -EPROBE_DEFER and then called
nvmem_cell_read() with the error pointer, crashing the kernel.

Fall back to the default disconnect voltage on a failed cell lookup,
and free the buffer returned by nvmem_cell_read().

> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 637a5fbae6d9a..94146118e9c2e 100644
> --- a/drivers/phy/sunplus/phy-sunplus-usb2.c
> +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> @@ -81,12 +81,13 @@ 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)) {
> +	if (IS_ERR(cell)) {
>  		if (PTR_ERR(cell) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;

[Severity: High]
This isn't a bug introduced by this patch, but if update_disc_vol()
returns -EPROBE_DEFER here, do we leak the clock and reset resources in
sp_uphy_init()?

In 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;

It looks like this returns directly instead of routing to the err_reset and
err_clk cleanup labels.

> +		goto out;
>  	}
>  
>  	otp_v = nvmem_cell_read(cell, &otp_l);
> @@ -96,11 +97,12 @@ 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;

[Severity: High]
This is a pre-existing issue, but is it safe to unconditionally access two
bytes from the nvmem buffer without checking the returned length (otp_l)?

The nvmem_cell_read() function dynamically allocates a buffer sized exactly
to the cell's byte count. If the NVMEM cell is defined as 1 byte in the
device tree, wouldn't *(otp_v + 1) result in a slab out-of-bounds read?

> +		if (set == 0)
> +			set = OTP_DISC_LEVEL_DEFAULT;
> +		kfree(otp_v);
>  	}
>  
> -	if (IS_ERR(otp_v) || set == 0)
> -		set = OTP_DISC_LEVEL_DEFAULT;
> -
> +out:
>  	val = readl(usbphy->phy_regs + CONFIG7);
>  	val = (val & ~J_DISC) | set;
>  	writel(val, usbphy->phy_regs + CONFIG7);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260804-sunplus-v1-1-79f52226cb50@gmail.com?part=1

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

Re: [PATCH] phy: sunplus: fix crash when the disc_vol nvmem cell is missing

[Vinod Koul]

On 04-08-26, 21:58, Felix Gu wrote:

Where is the crash report?
Patch title should document the change not the reason, please read the
documentation on how to create patches

> nvmem_cell_get() can fail with errors other than -EPROBE_DEFER, but
> update_disc_vol() only handled -EPROBE_DEFER and then called
> nvmem_cell_read() with the error pointer, crashing the kernel.
> 
> Fall back to the default disconnect voltage on a failed cell lookup,
> and free the buffer returned by nvmem_cell_read().
> 
> 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 | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 637a5fbae6d9..94146118e9c2 100644
> --- a/drivers/phy/sunplus/phy-sunplus-usb2.c
> +++ b/drivers/phy/sunplus/phy-sunplus-usb2.c
> @@ -81,12 +81,13 @@ 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)) {
> +	if (IS_ERR(cell)) {
>  		if (PTR_ERR(cell) == -EPROBE_DEFER)
>  			return -EPROBE_DEFER;
> +		goto out;
>  	}
>  
>  	otp_v = nvmem_cell_read(cell, &otp_l);
> @@ -96,11 +97,12 @@ 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;
> -
> +out:
>  	val = readl(usbphy->phy_regs + CONFIG7);
>  	val = (val & ~J_DISC) | set;
>  	writel(val, usbphy->phy_regs + CONFIG7);
> 
> ---
> base-commit: 9a4cdc958dd79fc6c3b20b51a10debec6ca09fec
> change-id: 20260804-sunplus-53a7bb1bcd72
> 
> Best regards,
> --  
> Felix Gu <ustc.gu@gmail.com>

-- 
~Vinod

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