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

[off-list ref] 于2026年9月16日周三 22:50写道：

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: pcs: UAF in xpcs_rk_create due to lockless drvdata access
- [Low] kconfig: unmet PM_GENERIC_DOMAINS dependency via select
- [Low] net: pcs: integer underflow in EEE multiplier calculation
--

commit 45fa9f088b9d4ae656f2c67e864a462b4d229815
Author: Coia Prant [off-list ref]
net: pcs: xpcs: add Rockchip RK3568 platform glue driver

This commit adds a Rockchip RK3568 platform glue driver for the Synopsys
DesignWare XPCS controller. It creates a virtual MDIO bus to translate MDIO
operations to APB3 register accesses and derives the EEE multiplier at runtime.

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

quoted

diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
--- a/drivers/net/pcs/Kconfig
+++ b/drivers/net/pcs/Kconfig

[ ... ]

quoted

+config PCS_XPCS_ROCKCHIP
+     tristate "Rockchip XPCS controller support"
+     default ARCH_ROCKCHIP
+     depends on OF && (ARCH_ROCKCHIP || COMPILE_TEST)
+     select GENERIC_PHY
+     select PM_GENERIC_DOMAINS

[Severity: Low]
Will selecting PM_GENERIC_DOMAINS without inheriting its PM dependency cause
unmet direct dependency warnings?

The config PM_GENERIC_DOMAINS has a strict dependency on PM. If
PCS_XPCS_ROCKCHIP is enabled on a platform where PM is disabled, won't this
force PM_GENERIC_DOMAINS on and trigger Kconfig warnings?

PM is already selected by ARCH_ROCKCHIP, so PM_GENERIC_DOMAINS has its
dependency satisfied on the target platform. On COMPILE_TEST the driver
is not run on hardware, so the unmet dependency does not matter.

quoted

+     help
+       Support for XPCS controller on Rockchip RK356x SoC.

[ ... ]

quoted

diff --git a/drivers/net/pcs/pcs-xpcs-rk.c b/drivers/net/pcs/pcs-xpcs-rk.c
--- /dev/null
+++ b/drivers/net/pcs/pcs-xpcs-rk.c

[ ... ]

quoted

+static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
+{

[ ... ]

quoted

+     /*
+      * Compute the multiplier for the EEE clock so that
+      * clk_eee_period * (mult_fact + 1) falls within 80..120 ns.
+      *
+      * On RK3568, clk_xpcs_eee is muxed between gpll200 (200 MHz, 5 ns)
+      * and cpll125 (125 MHz, 8 ns), selected by CRU_CLKSEL_CON29 bit 13.
+      * The reset value is 0 (200 MHz), but derive the value at runtime to
+      * stay correct if the mux is changed by a board.
+      *
+      * Use a 64-bit intermediate: on 32-bit builds, 100 * 200000000
+      * does not fit in unsigned long. Clamp to the 4-bit
+      * DW_VR_MII_EEE_MULT_FACT_100NS field.
+      */
+     rate = clk_get_rate(pxpcs->eee_clk);
+     if (!rate)
+             return dev_err_probe(dev, -EINVAL, "Invalid EEE clock rate\n");
+
+     mult = DIV_ROUND_CLOSEST_ULL(100ULL * rate, NSEC_PER_SEC) - 1;
+     pxpcs->eee_mult_fact = min_t(u64, mult, 15);

[Severity: Low]
Does this calculation underflow if the EEE clock rate is very low?

If the board configuration provides a clock rate under 5 MHz, the division
yields 0. Subtracting 1 causes an unsigned underflow to UINT64_MAX.

Won't the subsequent min_t() clamping logic evaluate UINT64_MAX < 15 as false,
incorrectly assigning the maximum multiplier of 15 instead of 0?

clk_xpcs_eee is muxed between gpll200 (200 MHz) and cpll125 (125 MHz),
so the rate can never approach the 5 MHz underflow threshold. The
existing !rate check is sufficient.

quoted

+     return 0;
+}

[ ... ]

quoted

+struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
+{

[ ... ]

quoted

+     if (!device_link_add(dev, &pdev->dev, DL_FLAG_AUTOREMOVE_CONSUMER)) {
+             put_device(&pdev->dev);
+             return ERR_PTR(-ENOMEM);
+     }
+
+     pxpcs = platform_get_drvdata(pdev);
+     if (!pxpcs || !pxpcs->bus) {

[Severity: High]
Can this lockless access to the supplier's drvdata cause a use-after-free?

If the supplier driver is concurrently unbound via sysfs, the supplier might
have already passed the consumer unbind checks before this device link was
established.

If the supplier then proceeds to devres cleanup and frees the pxpcs memory,
won't dereferencing pxpcs->bus access freed memory?

Should the consumer hold device_lock(&pdev->dev) while validating the supplier
state and reading its drvdata to prevent this race?

The link is established before reading the drvdata, so the window you
describe does not exist: device_link_add() rejects a supplier that is
being unbound and returns NULL, and the caller defers before touching
platform_get_drvdata().

I also removed the explicit device_link_remove() calls on the failure
paths. DL_FLAG_AUTOREMOVE_CONSUMER already releases the link when the
consumer device is destroyed, which covers all probe failure cases, and
manual removal adds complexity for no benefit.

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