Re: [PATCH v6 06/12] media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The driver checks the machine root compatible string to infer device-specific hardware parameters instead of using a device match table.

Pre-existing issues:
- [High] The driver saves OF endpoint nodes into a persistent list without acquiring a reference, leading to a use-after-free and double-free during probe.
--

commit c91e2880e308dc3e7795189aa50b13e996251309
Author: Balakrishnan Sambath <balakrishnan.s@microchip.com>

media: microchip-isc: set SAM9X7 maximum resolution to 2560x1920

This patch updates the microchip-sama7g5-isc driver to support the SAM9X7
SoC. It reduces the maximum horizontal and vertical resolutions due to
smaller internal line buffer constraints on the SAM9X7 compared to the
SAMA7G5.

> diff --git a/drivers/media/platform/microchip/microchip-sama7g5-isc.c b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> index ca23e8adecbdc..4119cfe12cdf2 100644
> --- a/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> +++ b/drivers/media/platform/microchip/microchip-sama7g5-isc.c
> @@ -432,8 +435,13 @@ static int microchip_xisc_probe(struct platform_device *pdev)
>  	isc->gamma_table = isc_sama7g5_gamma_table;
>  	isc->gamma_max = 0;
>  
> -	isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
> -	isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
> +	if (of_machine_is_compatible("microchip,sam9x7")) {

[Severity: Medium]
Is checking the machine root compatible string the best approach here?

Typically, a device driver relies on its own device node's compatible
string to infer device-specific hardware parameters, rather than the root
machine's compatible string.

Could this be implemented by introducing a new compatible string for the
SAM9X7 XISC (e.g., microchip,sam9x7-isc) to the driver's of_match_table,
and associating the specific resolution limits using
of_device_get_match_data()?

> +		isc->max_width = ISC_SAM9X7_MAX_SUPPORT_WIDTH;
> +		isc->max_height = ISC_SAM9X7_MAX_SUPPORT_HEIGHT;
> +	} else {
> +		isc->max_width = ISC_SAMA7G5_MAX_SUPPORT_WIDTH;
> +		isc->max_height = ISC_SAMA7G5_MAX_SUPPORT_HEIGHT;
> +	}
>  
>  	isc->config_dpc = isc_sama7g5_config_dpc;

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the probe
sequence, I noticed a potential reference counting issue with the endpoint
nodes.

In xisc_parse_dt(), the endpoint node is assigned without taking a
reference:

xisc_parse_dt() {
    ...
    for_each_endpoint_of_node(np, epn) {
        ...
        subdev_entity->epn = epn;
    ...
}

Since for_each_endpoint_of_node() drops the reference on the current epn
when it advances, subdev_entity->epn might point to a node whose
reference count has already been decremented.

Later in microchip_xisc_probe(), this endpoint is accessed and explicitly
freed:

microchip_xisc_probe() {
    ...
    list_for_each_entry(subdev_entity, &isc->subdev_entities, list) {
        struct fwnode_handle *fwnode =
            of_fwnode_handle(subdev_entity->epn);
        ...
        of_node_put(subdev_entity->epn);
    ...
}

Could this lead to a use-after-free when registering the async remote,
and a double-free when of_node_put() is called? Should the assignment
in xisc_parse_dt() use of_node_get(epn)?

A similar omission also appears to be present in the SAMA5D2 driver
variant in isc_parse_dt().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260603-microchip-isc-fixes-v6-0-8c3d7474a768@microchip.com?part=6