Re: [PATCH v9 1/6] phy: realtek: usb2: introduce phy_reg_desc struct to the driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] usb: phy: unaligned IO pointer arithmetic on void __iomem *
- [High] phy: realtek: missing NULL check and iounmap() for of_iomap()
- [High] phy: realtek: off-by-one array bounds check allows out-of-bounds access
--

commit a5449cff40ee54bc6f98436955a56c20a0155c71
Author: Rustam Adilov <adilov@disroot.org>

phy: realtek: usb2: introduce phy_reg_desc struct to the driver

In RTL9607C SoC, there are a few differences in usb2 phy compared
to existing RTD SoCs, like vstatus register is at certain offset
from the base, busy bit and new_reg_req bit are at different bits
of the register.

To avoid duplicating these variables across phy cfg and driver data,
create a per-SoC struct that would hold them and with that add the
default rtd_phy_reg_desc to which all RTD SoCs driver data will
point towards.

> diff --git a/drivers/phy/realtek/phy-rtk-usb2.c b/drivers/phy/realtek/phy-rtk-usb2.c
> index 248550ef98cab..e8d0f0119ec0a 100644
> --- a/drivers/phy/realtek/phy-rtk-usb2.c
> +++ b/drivers/phy/realtek/phy-rtk-usb2.c
> @@ -156,27 +164,28 @@ static inline int utmi_wait_register(void __iomem *reg, u32 mask, u32 result)
>  static char rtk_phy_read(struct phy_reg *phy_reg, char addr)
>  {
>  	void __iomem *reg_gusb2phyacc0 = phy_reg->reg_gusb2phyacc0;
> +	const struct phy_reg_desc *reg_desc = phy_reg->desc;
>  	unsigned int val;
>  	int ret = 0;
>  
>  	addr -= OFFEST_PHY_READ;
>  
>  	/* polling until VBusy == 0 */
> -	ret = utmi_wait_register(reg_gusb2phyacc0, PHY_VSTS_BUSY, 0);
> +	ret = utmi_wait_register(reg_gusb2phyacc0, reg_desc->vstatus_busy, 0);

[Severity: High]
Can this dereference an out-of-bounds array element?

In rtk_phy_toggle(), the bounds check uses > instead of >=:

    if (index > rtk_phy->num_phy) {

This incorrectly allows index to equal the array size num_phy. When
do_rtk_phy_toggle() uses this out-of-bounds index to access the dynamically
allocated phy_parameter array, an invalid phy_reg element is passed to
rtk_phy_read().

Dereferencing the explicitly added desc pointer from this invalid phy_reg
element will immediately cause a NULL pointer dereference or memory corruption.

[ ... ]

> @@ -957,6 +968,7 @@ static int get_phy_data_by_efuse(struct rtk_phy *rtk_phy,
>  
>  static int parse_phy_data(struct rtk_phy *rtk_phy)
>  {
> +	struct phy_cfg *phy_cfg = rtk_phy->phy_cfg;
>  	struct device *dev = rtk_phy->dev;
>  	struct device_node *np = dev->of_node;
>  	struct phy_parameter *phy_parameter;
> @@ -974,6 +986,8 @@ static int parse_phy_data(struct rtk_phy *rtk_phy)
>  		phy_parameter->phy_reg.reg_wrap_vstatus = of_iomap(np, 0);
>  		phy_parameter->phy_reg.reg_gusb2phyacc0 = of_iomap(np, 1) + index;

[Severity: High]
Does this code handle of_iomap() failures correctly?

of_iomap() can return NULL. The driver fails to check this return value and
proceeds, which can cause a NULL pointer dereference later when
reg_wrap_vstatus is offset and dereferenced in rtk_phy_write().

Additionally, this maps the exact same indices (0 and 1) redundantly in a
loop up to num_phy times, which may leak I/O memory if mapped multiple times.

[Severity: High]
Does this pointer arithmetic result in an unaligned pointer?

Adding index to the void __iomem * returned by of_iomap(np, 1) increments the
address by exactly index bytes (e.g. 1 byte for the second PHY in a dual-port
configuration). This creates an unaligned I/O pointer.

Executing a 32-bit I/O access on this unaligned pointer in rtk_phy_write()
can cause a hardware Alignment Fault.

>  		phy_parameter->phy_reg.vstatus_index = index;
> +		phy_parameter->phy_reg.desc = phy_cfg->reg_desc;
> +
>  
>  		if (of_property_read_bool(np, "realtek,inverse-hstx-sync-clock"))
>  			phy_parameter->inverse_hstx_sync_clock = true;

[ ... ]

> @@ -1060,6 +1074,12 @@ static void rtk_usb2phy_remove(struct platform_device *pdev)
>  	remove_debug_files(rtk_phy);
>  }

[Severity: High]
Does this missing cleanup cause an I/O memory leak?

The teardown function completes without calling iounmap() for the regions
mapped by of_iomap() in parse_phy_data(), resulting in a permanent I/O memory
leak on driver removal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016180314.31760-1-adilov@disroot.org?part=1