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

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Missing cleanup of media devices and debugfs files in the `neoisp_probe` error path leads to Resource Leaks and Use-After-Free vulnerabilities.
- [High] The device's PM usage counter is leaked if `CONFIG_DEBUG_FS` is disabled or if debugfs allocation fails.
- [High] Invalid direct pointer dereference of I/O memory (`__iomem`) in `neoisp_dump_memory()`.
- [Low] The `enable_debugfs` module parameter is defined with `0644` (read-write) permissions, but it is only evaluated once during module probe.

Pre-existing issues:
- [High] Race condition in Runtime PM state due to asynchronous `pm_runtime_put()` before `pm_runtime_disable()`.
- [High] Double unregister of the v4l2_device if neoisp_init_group() fails when called from neoisp_core_media_register().
--

commit 15c2086c65406ec4a2f05196f174786dbe315997
Author: Antoine Bouyer <antoine.bouyer@nxp.com>
Subject: media: platform: neoisp: Add debugfs support

Add debugfs entries to dump ISP registers, and some internal memory
regions used to store Vignetting, DRC global and DRC local coefficients.

Debug mode is activated with the enable_debugfs module parameter.

> diff --git a/drivers/media/platform/nxp/neoisp/neoisp_debugfs.c b/drivers/media/platform/nxp/neoisp/neoisp_debugfs.c
> new file mode 100644
> index 0000000000000..e730569184b3d
> --- /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 *)neoispd->local_mem;
> +	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 * sizeof(u32);
> +		data.word = src[offset++];

[Severity: High]
Can this directly dereference I/O memory?

neoispd->local_mem is mapped using devm_ioremap_resource_wc(), which returns
an iomem pointer. Here it is cast to a regular u32 pointer and accessed
directly:

		data.word = src[offset++];

Accessing I/O memory via direct pointer dereferencing bypasses required
compiler memory barriers and architecture-specific I/O memory instructions.
It invites unsafe compiler optimizations and can cause CPU faults.

Should this code use readl() or ioread32() to safely access the MMIO region
and ensure correct CPU endianness conversion?

[ ... ]

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

[ ... ]

> @@ -41,6 +42,10 @@ static int standalone_mdev;
>  module_param_named(standalone_mdev, standalone_mdev, uint, 0644);
>  MODULE_PARM_DESC(standalone_mdev, " Create standalone neoisp media device, default is 0 (off)");
>  
> +static int enable_debugfs;
> +module_param_named(enable_debugfs, enable_debugfs, uint, 0644);

[Severity: Low]
Since the driver cannot respond to runtime changes of this parameter, could
the permissions be set to 0444 (read-only) instead of 0644?

Users may attempt to toggle debugfs support dynamically via sysfs after the
module is loaded, but changing this value at runtime will not dynamically
create or remove the debugfs entries, nor will it alter the device PM state.

> +MODULE_PARM_DESC(enable_debugfs, " Turn on/off debugfs, default is 0 (off)");
> +
>  static inline bool node_desc_is_output(const struct neoisp_node_desc_s *desc)

[ ... ]

> @@ -1766,9 +1771,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;

[Severity: Critical]
Does this error path leave dangling media device and debugfs registrations?

If pm_runtime_resume_and_get() fails, the execution jumps to
err_pm_runtime_suspend. This error path only cleans up the initial runtime
PM state.

It bypasses calling neoisp_destroy_devices() to unregister the previously
initialized V4L2 and media devices, and it fails to call
neoisp_debugfs_exit().

Since returning an error from probe causes devres to free the neoispd
structure, the V4L2 device nodes and debugfs files will remain alive and
point to freed memory, leading to use-after-free vulnerabilities if accessed
from user-space.

> +	}
> +
>  	pm_runtime_mark_last_busy(dev);
>  	pm_runtime_put_autosuspend(dev);

[ ... ]

> @@ -1785,6 +1799,11 @@ static void neoisp_remove(struct platform_device *pdev)
>  {
>  	struct neoisp_dev_s *neoispd = platform_get_drvdata(pdev);
>  
> +	if (neoispd->regset) {
> +		neoisp_debugfs_exit(neoispd);
> +		pm_runtime_put(neoispd->dev);
> +	}

[Severity: High]
Does this unconditionally leak the PM usage counter if debugfs allocation
fails or is disabled?

In neoisp_probe(), if the enable_debugfs parameter is true, the driver calls
neoisp_debugfs_init() and unconditionally increments the PM counter via
pm_runtime_resume_and_get() to keep the device awake.

However, in neoisp_debugfs_init(), if devm_kzalloc() fails or if
CONFIG_DEBUG_FS is disabled, neoispd->regset remains NULL.

Later, neoisp_remove() incorrectly checks if (neoispd->regset) before calling
pm_runtime_put(). Since regset is NULL, the put is entirely skipped. This
permanently leaks the PM usage counter and prevents the device from
suspending.

[Severity: High]
This is a pre-existing issue, but does the use of pm_runtime_put() here
create a race condition with device removal?

pm_runtime_put() queues an asynchronous idle notification. However, at the
end of neoisp_remove(), pm_runtime_disable() is called:

	pm_runtime_dont_use_autosuspend(neoispd->dev);
	pm_runtime_disable(neoispd->dev);

The pending async suspend work can be canceled before it executes by
pm_runtime_disable(), leaving the hardware powered on in an incorrect
RPM_ACTIVE state.

Should this use pm_runtime_put_sync() instead to ensure the device powers
down correctly? The same pre-existing pattern appears in the
err_pm_runtime_suspend error path in neoisp_probe().

> +
>  	neoisp_destroy_devices(neoispd);

[Severity: High]
This is a pre-existing issue, but does neoisp_destroy_devices() cause a
double unregister of the v4l2_device?

If another driver calls neoisp_core_media_register() and invokes
neoisp_init_group(), an error in neoisp_init_group() will trigger its
error path to call v4l2_device_unregister().

However, the core neoisp device remains actively probed. When the driver is
later unbound, neoisp_remove() calls neoisp_destroy_devices(), which
unconditionally calls v4l2_device_unregister() a second time on the same
v4l2_dev, causing a use-after-free.

>  
>  	if (standalone_mdev)

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