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

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 <[email protected]>

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/[email protected]?part=1

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy