Re: [PATCH v14 12/38] phy: rockchip: usbdp: Limit DP lane count to muxed lanes

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Incomplete fix allows USB3 corruption when phy_configure is called without set_lanes.
- [High] Missing mutex in rk_udphy_dp_phy_configure causes a TOCTOU data race with Type-C events.
--

--- Patch [12]: [PATCH v14 12/38] phy: rockchip: usbdp: Limit DP lane count to muxed lanes ---
commit 8cc28e81cd368682a27bd180cccc3fae6faaec95
Author: Sebastian Reichel <[email protected]>

phy: rockchip: usbdp: Limit DP lane count to muxed lanes

This commit adds a bounds check to prevent the DisplayPort controller from
requesting more lanes than the PHY currently has multiplexed for DP. By
rejecting invalid requests, it aims to protect USB3 functionality from being
corrupted by incorrect voltage configurations.

Link: https://lore.kernel.org/linux-phy/[email protected]/

> diff --git a/drivers/phy/rockchip/phy-rockchip-usbdp.c b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> index c4d62b234ea80..b172ce6a722ee 100644
> --- a/drivers/phy/rockchip/phy-rockchip-usbdp.c
> +++ b/drivers/phy/rockchip/phy-rockchip-usbdp.c
> @@ -1099,6 +1099,9 @@ static int rk_udphy_dp_phy_verify_link_rate(struct rk_udphy *udphy,
>  static int rk_udphy_dp_phy_verify_lanes(struct rk_udphy *udphy,
>  					struct phy_configure_opts_dp *dp)
>  {
> +	if (dp->lanes > udphy->dp_lanes)

[Severity: High]
This is a pre-existing issue, but does this unlocked read of
udphy->dp_lanes race with concurrent Type-C events?

Looking at rk_udphy_dp_phy_configure(), it performs validation and state
updates without acquiring the driver's udphy->mutex:

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
        struct rk_udphy *udphy = phy_get_drvdata(phy);
        ...
}

Meanwhile, asynchronous Type-C callbacks like rk_udphy_typec_mux_set()
mutate these same fields concurrently while correctly holding udphy->mutex.

If a Type-C event changes udphy->dp_lanes immediately after this unlocked
check passes, could rk_udphy_dp_phy_configure() proceed using outdated
values and write DP settings to USB3 lanes?

[Severity: High]
This is also a pre-existing issue, but does this check fully protect
the voltage configuration loops if phy_configure() is called with
set_voltages=1 but set_lanes=0?

If a Type-C event reduces the available DP lanes, rk_udphy_typec_mux_set()
updates udphy->dp_lanes but not the cached udphy->lanes. If the DP
controller later adjusts voltages during link training (without setting
lanes), this check in rk_udphy_dp_phy_verify_lanes() is skipped.

The code then loops over the stale udphy->lanes value in both
rk_udphy_dp_phy_verify_voltages() and rk_udphy_dp_phy_configure():

drivers/phy/rockchip/phy-rockchip-usbdp.c:rk_udphy_dp_phy_configure() {
        ...
        for (i = 0; i < udphy->lanes; i++) {
                lane = udphy->dp_lane_sel[i];
        ...
}

Will this result in applying DP voltage configurations to lanes that have
already been reassigned to USB3, bypassing the newly added protection?

> +		return -EINVAL;
> +
>  	switch (dp->lanes) {
>  	case 1:
>  	case 2:

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12

-- 
linux-phy mailing list
[email protected]
https://lists.infradead.org/mailman/listinfo/linux-phy