Re: [PATCH v4 1/2] media: sun6i-csi: bridge: Use V4L2 subdev active state

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

Pre-existing issues:
- [High] Missing synchronize_irq() in runtime PM suspend callback.
- [High] Spurious IRQ hardware access in shared interrupt handler without active PM check.
- [High] Missing v4l2_async_unregister_subdev() in cleanup leads to UAF in global V4L2 async list.
--

--- Patch [1]: [PATCH v4 1/2] media: sun6i-csi: bridge: Use V4L2 subdev active state ---
commit 1d64b20b941162e1616fdc42a52770e51b6cdb58
Author: Arash Golgol <[email protected]>

media: sun6i-csi: bridge: Use V4L2 subdev active state

Use the V4L2 subdev active state API to store the active format.
This simplifies the driver not only by dropping the bridge mbus_format
field, but it also allows dropping the bridge lock, replaced with
the state lock.

Signed-off-by: Arash Golgol <[email protected]>

> diff --git a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> index d006d9dd01708..43a85bcc2ba28 100644
> --- a/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c
> +++ b/drivers/media/platform/sunxi/sun6i-csi/sun6i_csi_bridge.c

[ ... ]

> @@ -868,5 +847,7 @@ void sun6i_csi_bridge_cleanup(struct sun6i_csi_device *csi_dev)
>  
>  	v4l2_device_unregister_subdev(subdev);
>  
> +	v4l2_subdev_cleanup(subdev);
> +
>  	media_entity_cleanup(&subdev->entity);
>  }

[Severity: High]
This is a pre-existing issue, but is it possible we're missing a call to
v4l2_async_unregister_subdev() here?

During probe, if isp_available is true, v4l2_async_register_subdev() is
called. However, this cleanup function unconditionally calls
v4l2_device_unregister_subdev().

Since the subdev remains linked in the global subdev_list within the V4L2
core, could this lead to a use-after-free when the driver unbinds and the
memory backing csi_dev is freed?


While reviewing this subsystem, I also noticed two other pre-existing issues
in drivers/media/platform/sunxi/sun6i-csi/sun6i_csi.c:

In sun6i_csi_suspend():

static int sun6i_csi_suspend(struct device *dev)
{
        struct sun6i_csi_device *csi_dev = dev_get_drvdata(dev);

        reset_control_assert(csi_dev->reset);
        clk_disable_unprepare(csi_dev->clock_ram);
        clk_disable_unprepare(csi_dev->clock_mod);

        return 0;
}

[Severity: High]
This isn't a bug introduced by this patch, but shouldn't we call
synchronize_irq() before asserting resets and disabling clocks?

Since the IRQ line is registered as shared (IRQF_SHARED), if another device
on the same line triggers an interrupt just as the PM core invokes
sun6i_csi_suspend(), the suspend callback might complete and power off the
hardware before the concurrent IRQ handler finishes executing.

Could this cause a bus fault or external abort when the processor accesses
unclocked MMIO registers?


Also in sun6i_csi_interrupt():

        regmap_read(regmap, SUN6I_CSI_CH_INT_STA_REG, &status);
        regmap_read(regmap, SUN6I_CSI_CH_INT_EN_REG, &enable);

[Severity: High]
This is another pre-existing issue, but does this IRQ handler need to
check if the device is active before accessing hardware registers?

Because this is a shared interrupt, the kernel might invoke
sun6i_csi_interrupt() when the device is suspended and its clocks are
disabled.

Without calling pm_runtime_get_if_active() before reading these MMIO
registers, could this result in an external abort or return 0xffffffff,
leading the handler to incorrectly assume all interrupts are pending?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1