Re: [PATCH v2 7/8] media: platform: neoisp: Add debugfs support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The probe error path skips cleanup of previously allocated devices and debugfs files, leading to memory leaks and Use-After-Free.
- [High] Direct pointer dereference of `__iomem` mapped memory causes undefined behavior and potential faults.
- [Medium] PM runtime usage counter leak when `enable_debugfs` is true.
--

commit 8ed96f76ce4a3c28d7472942ee6dafd2b5be2f9a
Author: Antoine Bouyer <antoine.bouyer@nxp.com>

media: platform: neoisp: Add debugfs support

Add debugfs entries to dump ISP registers and internal memory regions used
to store Vignetting and DRC coefficients. Debug mode is conditionally
activated to avoid runtime suspend which blocks register access.

> diff --git a/drivers/media/platform/nxp/neoisp/neoisp_debugfs.c b/drivers/media/platform/nxp/neoisp/neoisp_debugfs.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/media/platform/nxp/neoisp/neoisp_debugfs.c

[ ... ]

> +static inline int neoisp_dump_memory(struct seq_file *m, enum isp_block_map_e map, int wsize)
> +{
> +	struct neoisp_dev_s *neoispd = m->private;
> +	union udata_t data;
> +	u32 addr;
> +	u32 *src = (u32 *)(uintptr_t)neoispd->mmio_tcm;
> +	u32 offset = ISP_GET_OFF(map) / sizeof(u32);
> +	u32 size = ISP_GET_SZ(map) / sizeof(u32);
> +	int i, j;
> +
> +	for (i = 0; i < size; i++) {
> +		addr = (offset + i) * sizeof(u32);
> +		data.word = src[offset + i];

Is it safe to directly dereference __iomem pointers here? It looks like
mmio_tcm is mapped I/O memory, so bypassing I/O accessors like readl() or
ioread32() might cause alignment faults or undefined behavior on ARM64.

[ ... ]

> diff --git a/drivers/media/platform/nxp/neoisp/neoisp_main.c b/drivers/media/platform/nxp/neoisp/neoisp_main.c
> --- a/drivers/media/platform/nxp/neoisp/neoisp_main.c
> +++ b/drivers/media/platform/nxp/neoisp/neoisp_main.c

[ ... ]

> @@ -1758,9 +1763,18 @@ static int neoisp_probe(struct platform_device *pdev)
>  	neoisp_init_hw(neoispd);
>  	neoisp_set_default_context(neoispd);
>  
> +	if (enable_debugfs) {
> +		neoisp_debugfs_init(neoispd);
> +		/* Increase pm_runtime counter to prevent suspend */
> +		ret = pm_runtime_resume_and_get(dev);
> +		if (ret)
> +			goto err_pm_runtime_suspend;
> +	}
>  
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_put_autosuspend(dev);

If this pm_runtime_resume_and_get() fails and we take the goto
err_pm_runtime_suspend path, does this leak the devices initialized by
neoisp_init_devices() earlier in the probe function?

Since neoispd is allocated via devm_kzalloc(), it will be freed when probe
fails. If the V4L2 devices and debugfs files aren't cleaned up here, could
userspace accessing the leaked files trigger a use-after-free on the freed
neoispd pointer?

[ ... ]

> @@ -1777,6 +1791,9 @@ static void neoisp_remove(struct platform_device *pdev)
>  {
>  	struct neoisp_dev_s *neoispd = platform_get_drvdata(pdev);
>  
> +	if (neoispd->regset)
> +		neoisp_debugfs_exit(neoispd);
> +
>  	neoisp_destroy_devices(neoispd);
>  
>  	if (standalone_mdev)

Since we incremented the PM runtime usage counter with
pm_runtime_resume_and_get() in probe when enable_debugfs is true, do we
need to drop that extra reference here in the remove path?

It seems like failing to do so could leak the usage counter and keep the
device permanently awake after driver removal.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511132629.1300868-1-antoine.bouyer@nxp.com?part=7