Re: [PATCH] phy: sunplus: fix error handling in sp_uphy_init()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Kernel panic due to dereferencing an invalid error pointer from nvmem_cell_get.
- [Medium] Memory leak of the buffer returned by nvmem_cell_read.
- [Medium] Unvalidated array access may lead to an out-of-bounds read.
--

--- Patch [1]: [PATCH] phy: sunplus: fix error handling in sp_uphy_init() ---
commit bbdc3fb921cb2657598b63724660355fd956b455
Author: Felix Gu <ustc.gu@gmail.com>

phy: sunplus: fix error handling in sp_uphy_init()

Fixes the error paths in sp_uphy_init() to correctly undo initialization
steps. It returns directly if clock enable fails, releases the clock if
reset deassert fails, and asserts reset and clock if disconnect voltage
update fails.

> diff --git a/drivers/phy/sunplus/phy-sunplus-usb2.c b/drivers/phy/sunplus/phy-sunplus-usb2.c
> index 637a5fbae6d9a..0ad4c7160d17c 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 isn't a bug introduced by this patch, but looking at update_disc_vol()
called here in drivers/phy/sunplus/phy-sunplus-usb2.c, is there a potential
dereference of an invalid pointer?

If nvmem_cell_get() fails and returns an error pointer other than
-EPROBE_DEFER (such as -ENOENT), or if it returns NULL, the code doesn't
return early. It falls through and passes this invalid pointer directly to
nvmem_cell_read():

	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);
	nvmem_cell_put(cell);

Will this cause a panic when nvmem_cell_read() attempts to dereference the
cell pointer?

[Severity: Medium]
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);
		...

The nvmem_cell_read() function returns a dynamically allocated buffer that
the caller is responsible for freeing. It appears otp_v is never passed to
kfree() before the function returns, which would result in a memory leak.

[Severity: Medium]
This is another pre-existing issue, but could the array access in
update_disc_vol() lead to an out-of-bounds read?

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

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

The size of the read buffer is provided in otp_l. If a device tree
incorrectly provides a 1-byte nvmem cell, unconditionally accessing the
second byte of the buffer at *(otp_v + 1) without checking if otp_l is at
least 2 might lead to reading out-of-bounds memory.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260803-sunplus-usb3-v1-1-5a562524c869@gmail.com?part=1

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