Re: [PATCH v2 3/3] media: ipu-bridge: Add Sony IMX681

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Leaked ACPI device reference and dangling secondary fwnode pointers on error paths.
--

commit 4b67afbc2a1ae41061ad9ba083bbebdbbe1112a4
Author: Sergey Lebedev <lsa.uz@pm.me>

media: ipu-bridge: Add Sony IMX681
    
The Microsoft Surface Pro 11 for Business (Intel Lunar Lake, IPU7) carries
a Sony IMX681 as its user-facing sensor, enumerated as ACPI device
SONY0681. Add it so ipu_bridge_connect_sensors() builds its half of the
graph; without the entry the sensor is never connected, whatever driver is
present.

> diff --git a/drivers/media/pci/intel/ipu-bridge.c b/drivers/media/pci/intel/ipu-bridge.c
> index 7e65491faf6dc..68c2eb326f1dc 100644
> --- a/drivers/media/pci/intel/ipu-bridge.c
> +++ b/drivers/media/pci/intel/ipu-bridge.c
> @@ -97,6 +97,8 @@ static const struct ipu_sensor_config ipu_supported_sensors[] = {
>  	IPU_SENSOR_CONFIG("OVTI5675", 1, 450000000),
>  	/* Omnivision OV8856 */
>  	IPU_SENSOR_CONFIG("OVTI8856", 3, 180000000, 360000000, 720000000),
> +	/* Sony IMX681 */
> +	IPU_SENSOR_CONFIG("SONY0681", 1, 969600000),

[Severity: High]
This is a pre-existing issue, but while reviewing the addition of this new
sensor, I noticed potential leaks and dangling pointers in the error handling
paths of ipu_bridge_connect_sensor() and ipu_bridge_unregister_sensors().

If ipu_bridge_instantiate_ivsc() fails:

drivers/media/pci/intel/ipu-bridge.c:ipu_bridge_connect_sensor() {
	...
		sensor->adev = ACPI_PTR(acpi_dev_get(adev));

		primary = acpi_fwnode_handle(adev);
		primary->secondary = fwnode;

		ret = ipu_bridge_instantiate_ivsc(sensor);
		if (ret)
			goto err_free_swnodes;
	...
err_free_swnodes:
	software_node_unregister_node_group(sensor->group);
err_put_ivsc:
	put_device(sensor->csi_dev);
	acpi_dev_put(sensor->ivsc_adev);
err_put_adev:
	acpi_dev_put(adev);
	return ret;
}

Since bridge->n_sensors is not incremented before the failure, the caller
ipu_bridge_unregister_sensors() iterates up to the previously successful sensor
and skips this failing sensor. Does this mean the acpi_dev_get(adev) reference
held by sensor->adev is permanently leaked?

Furthermore, primary->secondary is assigned but never cleared on this error
path, leaving a dangling pointer.

Additionally, in the cleanup function itself:

drivers/media/pci/intel/ipu-bridge.c:ipu_bridge_unregister_sensors() {
	for (i = 0; i < bridge->n_sensors; i++) {
		sensor = &bridge->sensors[i];
		software_node_unregister_node_group(sensor->group);
		acpi_dev_put(sensor->adev);
		put_device(sensor->csi_dev);
		acpi_dev_put(sensor->ivsc_adev);
	}
}

Does this code leave dangling secondary fwnode pointers? The function
unregisters and frees the software nodes, but it doesn't appear to clear the
secondary fwnode pointers on the ACPI or CSI devices first (e.g., setting
acpi_fwnode_handle(sensor->adev)->secondary = NULL or calling
set_secondary_fwnode(sensor->csi_dev, NULL)).

If the ACPI or CSI devices are later queried for fwnode properties, could
this trigger a use-after-free?

>  	/* Sony IMX471 */
>  	IPU_SENSOR_CONFIG("SONY471A", 1, 200000000),
>  	/* Sony IMX471 (found on Lenovo X1 Carbon G14) */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909193953.86192-1-lsa.uz@pm.me?part=3