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

[sashiko-bot@kernel.org]

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 <coiaprant@gmail.com>
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

> diff --git a/drivers/net/pcs/Kconfig b/drivers/net/pcs/Kconfig
> --- a/drivers/net/pcs/Kconfig
> +++ b/drivers/net/pcs/Kconfig
[ ... ]
> +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?

> +	help
> +	  Support for XPCS controller on Rockchip RK356x SoC.
[ ... ]
> 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
[ ... ]
> +static int xpcs_rk_init_clk(struct dw_xpcs_rk *pxpcs)
> +{
[ ... ]
> +	/*
> +	 * 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?

> +	return 0;
> +}
[ ... ]
> +struct dw_xpcs *xpcs_rk_create(struct device *dev, struct device_node *np)
> +{
[ ... ]
> +	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?

> +		device_link_remove(dev, &pdev->dev);
> +		put_device(&pdev->dev);
> +		return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	xpcs = xpcs_create_mdiodev(pxpcs->bus, (int)port);
[ ... ]

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