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

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

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

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