Re: [PATCH v5 3/3] media: qcom: camss: Add support for X1P42100 CAMSS

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: qcom: camss: CSID_LITE power on failure and incorrect VFE mapping due to 1:1 ID assumption
- [High] media: qcom: camss: NULL ptr deref in legacy init on malformed DT
- [High] media: qcom: camss: OOB array access due to missing DT port bounds check
--

commit f107a0f6644c97e69bdf263cd7e5e868d13b9438
Author: Wenmeng Liu <wenmeng.liu@oss.qualcomm.com>

media: qcom: camss: Add support for X1P42100 CAMSS

Add CAMSS support for the Qualcomm X1P42100 (Purwa) SoC.

> diff --git a/drivers/media/platform/qcom/camss/camss.c b/drivers/media/platform/qcom/camss/camss.c
> index c54185c93a8a9..5f0e5cadc06d5 100644
> --- a/drivers/media/platform/qcom/camss/camss.c
> +++ b/drivers/media/platform/qcom/camss/camss.c
> @@ -36,6 +36,13 @@
>  /* Top-level CAMSS version register */
>  #define CAMSS_HW_VERSION		0x0
>  
> +/* CSIPHY resources for PHY API devices, only id and formats are needed */
> +#define CAMSS_CSIPHY_RES_PHY_API(_id, _formats)		\
> +	.csiphy = {					\
> +		.id = (_id),				\
> +		.formats = (_formats),			\
> +	}

[Severity: High]
Does this macro leave the hw_ops pointer uninitialized?

If a device tree is malformed or lacks endpoints, camss_detect_legacy_phy()
will leave camss->legacy_phy as true. This causes camss_init_subdevices() to
use the legacy initialization path:

camss_init_subdevices()
    msm_csiphy_subdev_init_legacy()
        csiphy->res->hw_ops->init(csiphy);

Since this macro does not set hw_ops, will this result in a NULL pointer
dereference during driver probe when legacy_phy defaults to true?

[ ... ]

> +static const struct camss_subdev_resources csiphy_res_x1p42100[] = {
> +	/* CSIPHY0 */
> +	{
> +		CAMSS_CSIPHY_RES_PHY_API(0, &csiphy_formats_sdm845),
> +	},
> +	/* CSIPHY4 */
> +	{
> +		CAMSS_CSIPHY_RES_PHY_API(4, &csiphy_formats_sdm845),
> +	},
> +};

[Severity: High]
Can a device tree mapping for CSIPHY4 cause an out-of-bounds array access?

camss_parse_endpoint_node() assigns the device tree port endpoint directly to
csd->interface.csiphy_id without checking bounds:

    csd->interface.csiphy_id = vep.base.port;

Later, camss_subdev_notifier_bound() uses this unvalidated ID to index the
dynamically allocated camss->csiphy array:

    u8 id = csd->interface.csiphy_id;
    struct csiphy_device *csiphy = &camss->csiphy[id];

Since the X1P42100 SoC has a csiphy_num of 2, mapping CSIPHY4 to port@4 in the
device tree would cause an out-of-bounds access at camss->csiphy[4] during
device probe.

[ ... ]

> +	/* CSID_LITE0 */
> +	{
> +		.regulators = {},
> +		.clock = { "gcc_axi_hf", "cpas_ahb", "cpas_fast_ahb",
> +			   "csid", "csid_csiphy_rx" },
> +		.clock_rate = { { 0 },
> +				{ 64000000, 80000000 },
> +				{ 80000000,  100000000, 200000000,
> +				  300000000, 400000000 },
> +				{ 300000000, 400000000, 480000000 },
> +				{ 300000000, 400000000, 480000000 }, },
> +		.reg = { "csid_lite0" },
> +		.interrupt = { "csid_lite0" },
> +		.csid = {
> +			.is_lite = true,
> +			.hw_ops = &csid_ops_680,
> +			.parent_dev_ops = &vfe_parent_dev_ops,

[Severity: High]
Will CSID_LITE0 fail to power on because its ID is too high?

When powering on a CSID, csid_set_power() calls:
    csid->res->parent_dev_ops->get(camss, csid->id);

For vfe_parent_dev_ops_get(), it checks if the ID is strictly less than
camss->res->vfe_num. The vfe_num for X1P42100 is 3, but CSID_LITE0 has an ID
of 3, and CSID_LITE1 has an ID of 4.

Since 3 < 3 and 4 < 3 are false, vfe_parent_dev_ops_get() will return -EINVAL.
Additionally, this direct 1:1 index mapping seems to incorrectly pair CSID1
and CSID2 with IFE_LITE_0 and IFE_LITE_1 instead of full IFEs.

> +			.formats = &csid_formats_gen2
> +		}
> +	},

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-purwa_camss-v5-0-58c13dc7c4d2@oss.qualcomm.com?part=3