Re: [PATCH v1 4/6] phy: rockchip: Add support for Naneng eDP phy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] phy: rockchip: Missing set_lanes configuration and UB on unverified dp->lanes
- [Medium] Uninitialized variable val used if regmap_read fails
--

commit be568cbd60b1a73a69ea7b9233f6bd9cf9a8b067
Author: Damon Ding <damon.ding@rock-chips.com>

    phy: rockchip: Add support for Naneng eDP phy
    
    Add a driver for the Rockchip eDP transmitter PHY with NaNeng IP
    block, as found on the RK3568 SoC.

> diff --git a/drivers/phy/rockchip/phy-rockchip-naneng-edp.c b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c
> new file mode 100644
> index 0000000000000..a407714f639a7
> --- /dev/null
> +++ b/drivers/phy/rockchip/phy-rockchip-naneng-edp.c
[ ... ]
> +static int rockchip_edp_phy_set_rate(struct rockchip_edp_phy *edpphy,
> +				     struct phy_configure_opts_dp *dp)
> +{
[ ... ]
> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0, FIELD_PREP_WM16(EDP_PHY_PD_PLL_MASK, 0));
> +	regmap_write(edpphy->grf, EDP_PHY_GRF_CON0,
> +		     FIELD_PREP_WM16(EDP_PHY_TX_PD_MASK, ~GENMASK(dp->lanes - 1, 0)));

[Severity: High]
If a caller passes a struct where dp->lanes is 0, will this trigger undefined
behavior?

If dp->lanes is 0, ~GENMASK(dp->lanes - 1, 0) evaluates to
GENMASK(UINT_MAX, 0), triggering a negative right-shift count in C. This can
occur if a caller invokes phy_configure() with opts->dp.set_rate = 1 without
having first configured the lane count.

[ ... ]
> +static int rockchip_edp_phy_verify_config(struct rockchip_edp_phy *edpphy,
> +					  struct phy_configure_opts_dp *dp)
> +{
[ ... ]
> +	if (dp->set_lanes) {
> +		/* Verify lane count. */
> +		switch (dp->lanes) {
> +		case 1:
> +		case 2:
> +		case 4:
> +			/* valid lane count. */
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +	}
> +
> +	/*
> +	 * If changing voltages is required, check swing and pre-emphasis
> +	 * levels, per-lane.
> +	 */
> +	if (dp->set_voltages) {
> +		/* Lane count verified previously. */
> +		for (i = 0; i < dp->lanes; i++) {
> +			if (dp->voltage[i] > 3 || dp->pre[i] > 3)
> +				return -EINVAL;

[Severity: High]
Is it possible for dp->lanes to be used unvalidated here?

The comment says "Lane count verified previously", but the validation of
dp->lanes is bypassed if dp->set_lanes is false. If a driver calls
phy_configure() with opts->dp.set_voltages = 1 and opts->dp.set_lanes = 0,
an uninitialized or invalid dp->lanes value could lead to out-of-bounds array
reads.

[ ... ]
> +static int rockchip_edp_phy_configure(struct phy *phy, union phy_configure_opts *opts)
> +{
> +	struct rockchip_edp_phy *edpphy = phy_get_drvdata(phy);
> +	int ret;
> +
> +	ret = rockchip_edp_phy_verify_config(edpphy, &opts->dp);
> +	if (ret) {
> +		dev_err(edpphy->dev, "Invalid params for phy configure\n");
> +		return ret;
> +	}
> +
> +	if (opts->dp.set_rate) {
> +		ret = rockchip_edp_phy_set_rate(edpphy, &opts->dp);
> +		if (ret) {
> +			dev_err(edpphy->dev, "Failed to set rate\n");
> +			return ret;
> +		}
> +	}
> +
> +	if (opts->dp.set_voltages) {
> +		ret = rockchip_edp_phy_set_voltages(edpphy, &opts->dp);
> +		if (ret) {
> +			dev_err(edpphy->dev, "Failed to set voltages\n");
> +			return ret;
> +		}
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Is opts->dp.set_lanes intentionally ignored when configuring the PHY?

The opts->dp.set_lanes flag isn't handled here, meaning the hardware lane
count is never configured. This missing state also causes the uninitialized
dp->lanes issues in rockchip_edp_phy_set_rate() and
rockchip_edp_phy_set_voltages().

[ ... ]
> +static bool rockchip_edp_phy_enabled(struct rockchip_edp_phy *edpphy)
> +{
> +	u32 val;
> +
> +	regmap_read(edpphy->grf, EDP_PHY_GRF_STATUS0, &val);
> +
> +	return FIELD_GET(PLL_RDY, val);

[Severity: Medium]
If regmap_read() fails, does this return stack garbage?

A failure in regmap_read() leaves val uninitialized. Using the uninitialized
stack variable could cause the driver to incorrectly assume the PHY is already
enabled.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013124946.1807886-1-damon.ding@rock-chips.com?part=4