Re: [PATCH net-next v4 07/10] net: pcs: xpcs: add Rockchip RK3568 platform glue driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in xpcs_rk_create allows a concurrent unbind of the supplier to free the MDIO bus, leading to a Use-After-Free.
- [Medium] Incorrect LIFO teardown ordering in devres clock cleanup action violates proper device initialization and PM hierarchy rules.
--

commit 7142dbf6b28b5ee94d0edc969bcf16d1183a5f41
Author: Coia Prant <coiaprant@gmail.com>

net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This commit adds a platform glue driver for the Synopsys DesignWare xpcs
integrated in the Rockchip RK3568 SoC. It creates a virtual mdio bus to
translate mdio operations to apb3 memory-mapped register accesses.

Link: https://dl.radxa.com/rock3/docs/hw/datasheet/Rockchip%20RK3568%20TRM%20Part2%20V1.1-20210301.pdf (Page 2078)

> diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
> new file mode 100644
> index 0000000000000..68bcaa1a5cbbf
> --- /dev/null
> +++ b/drivers/net/pcs/pcs-xpcs-rk.c
[ ... ]
> +static void xpcs_rk_exit_clk(void *data)
> +{
> +	struct dw_xpcs_rk *pxpcs = data;
> +	struct device *dev = &pxpcs->pdev->dev;
> +
> +	clk_disable_unprepare(pxpcs->eee_clk);
> +
> +	pm_runtime_force_suspend(dev);

[Severity: Medium]
Does this sequence result in the clock being disabled before the device is
suspended?

In xpcs_rk_init_clk(), eee_clk is enabled first, followed by a call to
pm_runtime_enable(), allowing the device to become runtime active. During 
teardown, pm_runtime_force_suspend() invokes xpcs_rk_pm_runtime_suspend()
to disable csr_clk.

Calling clk_disable_unprepare() for eee_clk before pm_runtime_force_suspend() 
violates last-in, first-out ordering, disabling the prerequisite clock while
the device is still considered active. Should pm_runtime_force_suspend() be
called prior to disabling the eee_clk?

> +}
[ ... ]
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> +	device_lock(&pdev->dev);
> +	pxpcs = platform_get_drvdata(pdev);
> +	if (!pxpcs || !pxpcs->bus) {
> +		device_unlock(&pdev->dev);
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
> +	device_unlock(&pdev->dev);

[Severity: High]
Does dropping the device lock here introduce a time-of-check to time-of-use
race condition?

If a concurrent unbind of the rk_xpcs-dwxpcs supplier driver occurs 
immediately after device_unlock() but before device_link_add(), the unbind 
would trigger devres cleanup, freeing the mdio bus.

> +	if (IS_ERR(xpcs)) {
> +		put_device(&pdev->dev);
> +		return xpcs;
> +	}
> +
> +	if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {

Since device_link_add() permits linking to devices without drivers, it could
succeed even if the supplier was just unbound. The consumer driver would then
resume probing with the returned xpcs pointer referencing a freed mdio bus,
leading to a use-after-free when the mac driver attempts to read or write
pcs registers.

Could the device_link_add() call be moved inside the lock block protected
by device_lock() to safely establish the dependency before the lock is
released?

> +		xpcs_destroy(xpcs);
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-ENOMEM);
> +	}
> +
> +	put_device(&pdev->dev);
> +	return xpcs;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907153713.2572547-2-coiaprant@gmail.com?part=7