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

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 [off-list ref]

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)

[ ... ]

+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