Re: [PATCH v15 4/4] media: qcom: camss: Add support for PHY API devices

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

New issues:
- [High] Unchecked return value from `phy_mipi_dphy_get_default_config_for_hsclk` leads to uninitialized PHY configuration and potential divide-by-zero kernel panic.
- [Medium] The Kconfig symbol `PHY_QCOM_MIPI_CSI2` selected by `VIDEO_QCOM_CAMSS` does not exist.
- [Medium] Missing `phy_exit()` on probe error paths leads to a PHY state resource leak.

Pre-existing issues:
- [Critical] Missing bounds check on the device tree port number leads to a heap out-of-bounds write.
--

commit 26d58015dee86032083946c737ad4b615540ce3c
Author: Bryan O'Donoghue [off-list ref]

media: qcom: camss: Add support for PHY API devices
    
Add the ability to use a PHY pointer which interacts with the standard PHY
API.
    
In the first instance the code will try to use the new PHY interface. If no
PHYs are present in the DT then the legacy method will be attempted.

[Severity: Medium]
Does the Kconfig symbol PHY_QCOM_MIPI_CSI2 exist? Selecting a non-existent
Kconfig symbol has no effect or causes unmet dependency warnings, which can
omit required features from the build.

[ ... ]

[ ... ]

+static int csiphy_stream_on(struct csiphy_device *csiphy)
+{
+	u8 bpp = csiphy_get_bpp(csiphy->res->formats->formats, csiphy->res->formats->nformats,
+				csiphy->fmt[MSM_CSIPHY_PAD_SINK].code);
+	struct csiphy_lanes_cfg *lncfg = &csiphy->cfg.csi2->lane_cfg;
+	struct phy_configure_opts_mipi_dphy *dphy_cfg;
+	union phy_configure_opts dphy_opts = { 0 };
+	struct device *dev = csiphy->camss->dev;
+	u8 num_lanes = lncfg->num_data;
+	s64 link_freq;
+	int ret;
+
+	dphy_cfg = &dphy_opts.mipi_dphy;
+
+	link_freq = camss_get_link_freq(&csiphy->subdev.entity, bpp, num_lanes);
+
+	if (link_freq < 0) {
+		dev_err(dev,
+			"Cannot get CSI2 transmitter's link frequency\n");
+		return -EINVAL;
+	}
+
+	phy_mipi_dphy_get_default_config_for_hsclk(link_freq, num_lanes, dphy_cfg);

[Severity: High]
If camss_get_link_freq() returns 0, phy_mipi_dphy_get_default_config_for_hsclk()
will return -EINVAL. Does this code ignore the return value, leaving dphy_opts
uninitialized? 

Passing an all-zero struct to phy_configure() could cause the PHY driver to 
attempt a division by hs_clk_rate (which is 0), leading to a kernel panic.

+
+	phy_set_mode(csiphy->phy, PHY_MODE_MIPI_DPHY);
+
+	ret = phy_configure(csiphy->phy, &dphy_opts);
+	if (ret) {
+		dev_err(dev, "failed to configure MIPI D-PHY\n");
+		goto error;
+	}
+
+	return phy_power_on(csiphy->phy);
+
+error:
+	return ret;
+}

[ ... ]

[ ... ]

 		if (IS_ERR(csd)) {
 			ret = PTR_ERR(csd);
 			goto err_cleanup;
 		}
 
 		ret = camss_parse_endpoint_node(dev, ep, csd);

[Severity: Critical]
This is a pre-existing issue, but does camss_parse_endpoint_node() lack bounds
checking on the device tree port number?

When camss_parse_endpoint_node() reads the port number into
csd->interface.csiphy_id, it appears to be used as an index into the
camss->csiphy array in camss_subdev_notifier_bound() without verifying it is
less than camss->res->csiphy_num:

camss_subdev_notifier_bound() {
    u8 id = csd->interface.csiphy_id;
    struct csiphy_device *csiphy = &camss->csiphy[id];
    ...
    csiphy->cfg.csi2 = &csd->interface.csi2;
}

Can a malformed device tree with a large port number cause a heap out-of-bounds
write here?

 		if (ret < 0)
 			goto err_cleanup;
 	}
 
 	return 0;

[ ... ]

[Severity: Medium]
If msm_csiphy_subdev_init() fails on a subsequent iteration (or if a later probe
step fails and jumps to err_genpd_cleanup in camss_probe), do we leak the
initialized PHY state?

msm_csiphy_subdev_init() explicitly initializes the PHY with phy_init(), which
establishes the requirement for a subsequent phy_exit(). When returning here,
camss_unregister_entities() is skipped, which appears to leak the PHY resources
initialized in earlier loop iterations.

 		}
 	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260904-b4-linux-next-25-03-13-dtsi-x1e80100-camss-v15-0-68b7916e7a3f@linaro.org?part=4

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