Re: [PATCH v3 6/8] media: platform: Add NXP Neoisp Image Signal Processor

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 14 potential issue(s) to consider:
- [Critical] The driver validates ISP parameters directly in the user-mapped shared memory buffer without a kernel copy, exposing a critical TOCTOU vulnerability.
- [High] Using asynchronous `pm_runtime_put()` before `pm_runtime_disable()` leaves the hardware permanently powered on.
- [High] Using standard `memcpy` on MMIO mapped memory causes architectural faults.
- [High] Missing NULL pointer checks for virtual addresses returned by videobuf2.
- [High] Invalid V4L2 teardown order in `neoisp_destroy_devices()` causes Use-After-Free of `v4l2_dev`.
- [High] Missing `media_entity_cleanup()` for video nodes causes memory leaks of media pads/links.
- [High] Unbounded wait for hardware completion in `neoisp_node_stop_streaming()` deadlocks and leaks buffers.
- [High] The IRQ handler can fire after `neoisp_destroy_devices()` has freed DMA buffers, causing a Use-After-Free.
- [High] The driver exposes video devices to userspace before initializing `hw_lock` and the hardware.
- [High] `start_streaming` prepares only a single job, stranding any other initially queued buffers and stalling the pipeline.
- [High] `start_streaming` modifies the shared `neoispd->context` without locking, causing a data race with the active IRQ handler.
- [High] The IRQ handler accesses hardware registers without checking if the device is active, violating runtime PM safety.
- [High] `VIDIOC_S_SELECTION` modifies `node->crop` locklessly while the IRQ handler reads it, causing address corruption.
- [High] `neoisp_core_media_register` lacks synchronization, allowing concurrent calls to corrupt media graph initialization.
--

commit 0f5211be403174984a3657c54a249e9f22caa901
Author: Antoine Bouyer <antoine.bouyer@nxp.com>
Date:   Fri Jun 12 13:20:37 2026 +0000

    media: platform: Add NXP Neoisp Image Signal Processor

    This commit adds the first version of the NXP neoisp driver for processing
    RAW images. It supports single context and multiple M2M operations,
    including generic v4l2-isp parameters and statistics.

> diff --git a/drivers/media/platform/nxp/neoisp/neoisp_ctx.c b/drivers/media/platform/nxp/neoisp/neoisp_ctx.c
> new file mode 100644
> index 0000000000000..9d13eb7b732d2
> --- /dev/null
> +++ b/drivers/media/platform/nxp/neoisp/neoisp_ctx.c

[ ... ]

> +static inline void
> +local_mem_blk_write(struct neoisp_dev_s *neoispd, enum isp_block_map_e map, void *src)
> +{
> +	u32 offset, count;
> +
> +	get_offsize(map, &offset, &count);
> +	memcpy(neoispd->local_mem + offset, src, count);
> +}
> +
> +static inline void
> +local_mem_blk_read(struct neoisp_dev_s *neoispd, enum isp_block_map_e map, void *dst)
> +{
> +	u32 offset, count;
> +
> +	get_offsize(map, &offset, &count);
> +	memcpy(dst, neoispd->local_mem + offset, count);
> +}

[Severity: High]
Does this usage of memcpy() on an MMIO region trigger architectural faults?

The neoispd->local_mem region is mapped via devm_ioremap_resource_wc(),
returning an I/O memory pointer. Using standard string routines like
memcpy() on MMIO violates kernel API rules and can cause unaligned access
faults on certain architectures.

[ ... ]

> +void neoisp_ctx_update_buf_addr(struct neoisp_dev_s *neoispd)
> +{
> +	struct neoisp_job_s *job = &neoispd->queued_job;
> +	struct neoisp_pipe_conf_s *cfg = &neoispd->context->hw.pipe_conf;
> +	struct neoisp_buffer_s *buf_inp0 = job->buf[NEOISP_INPUT0_NODE];
> +	struct neoisp_buffer_s *buf_inp1 = job->buf[NEOISP_INPUT1_NODE];
> +	struct neoisp_buffer_s *buf_out = job->buf[NEOISP_FRAME_NODE];
> +	struct neoisp_buffer_s *buf_ir = job->buf[NEOISP_IR_NODE];
> +	struct neoisp_node_s *nd;
> +	u32 ibpp, inp0_stride, inp1_stride;
> +	dma_addr_t inp0_addr, inp1_addr;
> +
> +	/* Input0 specific */
> +	nd = &neoispd->node[NEOISP_INPUT0_NODE];
> +	ibpp = (nd->neoisp_format->bit_depth + 7) / 8;
> +	inp0_stride = nd->format.fmt.pix_mp.plane_fmt[0].bytesperline;
> +
> +	/* Input0 - Take crop into account if any */
> +	inp0_addr = get_addr(buf_inp0, 0) + (nd->crop.left * ibpp) + (nd->crop.top * inp0_stride);

[Severity: High]
Is there a data race on nd->crop here?

In neoisp_s_selection(), the node->crop values are updated without checking
vb2_is_busy() while only holding the queue_lock. The hardirq handler runs
this code to calculate the active DMA address without synchronization,
potentially resulting in an invalid address if crop coordinates change
during active streaming.

[ ... ]

> +void neoisp_ctx_update_w_user_params(struct neoisp_dev_s *neoispd)
> +{
> +	struct neoisp_buffer_s *buf = neoispd->queued_job.buf[NEOISP_PARAMS_NODE];
> +	struct v4l2_isp_buffer *params;
> +	size_t block_offset = 0, max_offset;
> +
> +	if (IS_ERR_OR_NULL(buf))
> +		return;
> +
> +	params = (struct v4l2_isp_buffer *)get_vaddr(buf);
> +
> +	if (!params || params->data_size == 0)
> +		/* No relevant parameters in this buffer */
> +		return;
> +
> +	max_offset = params->data_size;
> +
> +	/*
> +	 * Walk the list of parameter blocks and process them. No
> +	 * validation is done here, as the content of the parameters
> +	 * buffer is already checked when the buffer is queued.
> +	 */
> +	while (block_offset < max_offset) {
> +		const struct neoisp_block_handler_s *block_handler;
> +		const union neoisp_params_block_u *block;
> +
> +		block = (const union neoisp_params_block_u *)
> +			&params->data[block_offset];
> +		block_offset += block->header.size;
> +
> +		block_handler = &neoisp_block_handlers[block->header.type];
> +		block_handler->handler(neoispd->context, block);
> +	}
> +}

[Severity: Critical]
Is this vulnerable to a TOCTOU (Time-Of-Check to Time-Of-Use) race?

The parameters are validated during QBUF but never copied into private
kernel memory. Here, in the hardware interrupt context, the driver
dereferences block->header.type directly from the user-mapped buffer to
index the neoisp_block_handlers array. 

An attacker modifying the mapped buffer after validation could cause an
out-of-bounds access on the function pointer array and obtain arbitrary
code execution.

[ ... ]

> +void neoisp_ctx_upload_context(struct neoisp_dev_s *neoispd)
> +{
> +	struct neoisp_context_s *ctx = neoispd->context;
> +	u32 *src = (u32 *)&ctx->hw.pipe_conf.img_conf;
> +
> +	reg_blk_write(neoispd, NEO_PIPE_CONF_IMG_CONF_CAM0, src,
> +		      NEO_AUTOFOCUS_ROI0_SUM0_CAM0 - NEO_PIPE_CONF_IMG_CONF_CAM0);

[Severity: High]
Could this copy a partially updated context to the hardware?

This is executed in the hardirq handler, while neoisp_node_start_streaming()
modifies neoispd->context concurrently without holding hw_lock. If another
node is streaming, this could write torn configuration data to MMIO.

[ ... ]

> +void neoisp_ctx_get_stats(struct neoisp_dev_s *neoispd, struct neoisp_buffer_s *buf)
> +{
> +	struct neoisp_node_s *node = &neoispd->node[NEOISP_STATS_NODE];
> +	struct v4l2_isp_buffer *stats;
> +	u32 *blk_list, count;
> +
> +	/* Check if stats node link is enabled */
> +	if (!neoisp_node_link_is_enabled(node))
> +		return;
> +
> +	if (IS_ERR_OR_NULL(buf)) {
> +		dev_err(neoispd->dev, "Error: stats pointer\\n");
> +		return;
> +	}
> +
> +	stats = (struct v4l2_isp_buffer *)get_vaddr(buf);
> +	v4l2_isp_stats_init_buffer(stats, V4L2_ISP_VERSION_V1);

[Severity: High]
Can get_vaddr() return NULL here and cause an oops?

vb2_plane_vaddr() can return NULL if the memory isn't mapped, which gets
passed blindly to v4l2_isp_stats_init_buffer() resulting in a dereference
crash.

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

[ ... ]

> +static int neoisp_params_node_buf_prepare(struct vb2_buffer *vb)
> +{
> +	struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> +	struct neoisp_node_s *node = vb2_get_drv_priv(vb->vb2_queue);
> +	struct neoisp_dev_s *neoispd = node->neoisp;
> +	struct v4l2_isp_params_buffer *params = vb2_plane_vaddr(&vbuf->vb2_buf, 0);
> +	int ret;
> +
> +	ret = v4l2_isp_params_validate_buffer_size(neoispd->dev, vb,
> +						   node->format.fmt.meta.buffersize);
> +	if (ret)
> +		return ret;
> +
> +	ret = v4l2_isp_params_validate_buffer(neoispd->dev, vb,
> +					      params, neoisp_params_block_types_info,
> +					      ARRAY_SIZE(neoisp_params_block_types_info));

[Severity: High]
Should we check if params is NULL before passing it to the validation?

vb2_plane_vaddr() can return NULL, and v4l2_isp_params_validate_buffer()
will crash if it tries to dereference a NULL pointer.

[ ... ]

> +static int neoisp_node_start_streaming(struct vb2_queue *q, u32 count)
> +{
> +	struct neoisp_node_s *node = vb2_get_drv_priv(q);
> +	struct neoisp_dev_s *neoispd = node->neoisp;
> +	struct neoisp_buffer_s *buf, *tmp;
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(neoispd->dev);
> +	if (ret < 0)
> +		goto error;
> +
> +	ret = neoisp_prepare_node_streaming(node);
> +	if (ret < 0)
> +		goto error_streaming;
> +
> +	scoped_guard(spinlock_irq, &neoispd->hw_lock) {
> +		neoispd->streaming_map |= BIT(node->id);
> +		neoispd->frame_sequence = 0;
> +	}
> +
> +	dev_dbg(neoispd->dev, "%s: for node %s (count %u)\\n",
> +		__func__, NODE_NAME(node), count);
> +	dev_dbg(neoispd->dev, "Nodes streaming now 0x%x\\n",
> +		neoispd->streaming_map);
> +
> +	/* Update queued job context with current driver configuration */
> +	neoisp_ctx_update_packetizer(neoispd);
> +	neoisp_ctx_update_pipe_conf(neoispd);

[Severity: High]
Could neoispd->context be corrupted here?

Updating the context configuration is performed here without locking, which
can race with the hardware interrupt if the device is already streaming on
another node.

> +
> +	/* Maybe we're ready to run. */
> +	if (!neoisp_prepare_job(neoispd))
> +		neoisp_schedule(neoispd, false);

[Severity: High]
Is a single neoisp_prepare_job() call sufficient here?

If userspace has queued multiple buffers before starting the stream,
preparing only one job leaves the rest stranded in the ready_queue, which
stalls the processing pipeline.

[ ... ]

> +static void neoisp_node_stop_streaming(struct vb2_queue *q)
> +{
> +	struct neoisp_node_s *node = vb2_get_drv_priv(q);
> +	struct neoisp_dev_s *neoispd = node->neoisp;
> +	struct neoisp_job_desc_s *job, *temp;
> +	struct neoisp_buffer_s *buf;
> +	LIST_HEAD(tmp_list);
> +
> +	/*
> +	 * Now this is a bit awkward. In a simple M2M device we could just wait
> +	 * for all queued jobs to complete, but here there's a risk that a
> +	 * partial set of buffers was queued and cannot be run. For now, just
> +	 * cancel all buffers stuck in the "ready queue", then wait for any
> +	 * running job.
> +	 *
> +	 * This may return buffers out of order.
> +	 */
> +	dev_dbg(neoispd->dev, "%s: for node %s\\n", __func__, NODE_NAME(node));
> +	do {
> +		buf = list_first_entry_or_null(&node->ready_queue,
> +					       struct neoisp_buffer_s,
> +					       ready_list);
> +		if (buf) {
> +			list_del(&buf->ready_list);
> +			vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
> +		}
> +	} while (buf);
> +
> +	vb2_wait_for_all_buffers(&node->queue);

[Severity: High]
Can this lead to a permanent deadlock?

If the hardware has stalled or an interrupt is missed,
vb2_wait_for_all_buffers() will block indefinitely because the hardware is
never forcibly stopped.

> +
> +	if (node->id == NEOISP_INPUT0_NODE)
> +		neoisp_set_default_context(neoispd);
> +
> +	spin_lock_irq(&neoispd->hw_lock);
> +	neoispd->streaming_map &= ~BIT(node->id);
> +
> +	if (neoispd->streaming_map == 0) {
> +		/*
> +		 * If all nodes have stopped streaming release all jobs
> +		 * without holding the lock.
> +		 */
> +		list_splice_init(&neoispd->job_queue, &tmp_list);
> +	}
> +	spin_unlock_irq(&neoispd->hw_lock);
> +
> +	list_for_each_entry_safe(job, temp, &tmp_list, queue) {
> +		list_del(&job->queue);
> +		kfree(job);
> +	}

[Severity: High]
Do these freed jobs leak their associated buffers?

Any buffers tracked inside the discarded job->buffers arrays are not
returned to vb2, resulting in a permanent buffer leak.

[ ... ]

> +static irqreturn_t neoisp_irq_handler(int irq, void *dev_id)
> +{
> +	struct neoisp_dev_s *neoispd = (struct neoisp_dev_s *)dev_id;
> +	struct neoisp_buffer_s **buf = neoispd->queued_job.buf;
> +	u64 ts = ktime_get_ns();
> +	u32 irq_status = 0;
> +	u32 irq_clear = 0;
> +	bool done = false;
> +	int i;
> +
> +	irq_status = neoisp_rd(neoispd, NEO_PIPE_CONF_INT_STAT0);

[Severity: High]
Does this access the hardware without ensuring it is powered?

The device supports runtime PM, but the IRQ handler does not call
pm_runtime_get_if_active() before accessing NEO_PIPE_CONF_INT_STAT0. A
spurious or shared interrupt while the device is suspended could trigger a
fatal bus error.

[ ... ]

> +static void neoisp_destroy_devices(struct neoisp_dev_s *neoispd)
> +{
> +	int i;
> +
> +	if (neoispd->context) {
> +		dma_free_coherent(neoispd->dev,
> +				  sizeof(struct neoisp_context_s),
> +				  neoispd->context,
> +				  neoispd->params_dma_addr);
> +	}
> +
> +	v4l2_device_unregister(&neoispd->v4l2_dev);
> +
> +	if (standalone_mdev)
> +		media_device_unregister(&neoispd->mdev);
> +	else if (!neoispd->media_registered)
> +		return;
> +
> +	dev_dbg(neoispd->dev, "Unregister from media controller\\n");
> +
> +	v4l2_ctrl_handler_free(&neoispd->hdl);
> +	media_entity_cleanup(&neoispd->sd.entity);
> +
> +	for (i = NEOISP_NODES_COUNT - 1; i >= 0; i--) {
> +		struct neoisp_node_s *node = &neoispd->node[i];
> +
> +		video_unregister_device(&node->vfd);
> +		vb2_queue_release(&node->queue);
> +	}

[Severity: High]
Is the v4l2 unregistration sequence safe?

v4l2_device_unregister() is called before the video devices are unregistered
with video_unregister_device(). The V4L2 core requires video nodes to be
unregistered first to prevent Use-After-Free bugs if userspace is still
interacting with an open node.

[Severity: High]
Are we leaking the media pads allocated for the video nodes?

The video nodes had their pads dynamically initialized via
media_entity_pads_init() during setup, but media_entity_cleanup() is never
called for node->vfd.entity before releasing them.

[ ... ]

> +int neoisp_core_media_register(struct device *dev, struct v4l2_subdev *sd)
> +{
> +	struct neoisp_dev_s *neoispd = dev_get_drvdata(dev);
> +	struct media_device *mdev = sd->v4l2_dev->mdev;
> +	int ret;
> +
> +	if (!neoispd)
> +		return -EINVAL;
> +
> +	if (neoispd->media_registered || standalone_mdev)
> +		return 0;
> +
> +	ret = neoisp_init_group(neoispd, mdev);
> +	if (ret)
> +		return ret;
> +
> +	neoispd->media_registered++;
> +	return 0;
> +}

[Severity: High]
Is neoispd->media_registered protected from concurrent accesses?

This exported function can be called asynchronously. Without locking, two
callers can bypass the check concurrently, causing double-initialization of
the video nodes and a corrupted media device graph.

[ ... ]

> +static int neoisp_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct neoisp_dev_s *neoispd;
> +	struct resource *r;
> +	int ret, irq;

[ ... ]

> +	ret = neoisp_init_devices(neoispd);
> +	if (ret)
> +		goto err_pm_runtime_suspend;
> +
> +	spin_lock_init(&neoispd->hw_lock);
> +	neoisp_init_hw(neoispd);
> +	neoisp_set_default_context(neoispd);

[Severity: High]
Does this order expose uninitialized locks to userspace?

neoisp_init_devices() registers the /dev/videoX nodes. If userspace
immediately opens a node and starts streaming, it could hit an uninitialized
hw_lock or interact with unconfigured hardware before neoisp_init_hw()
runs.

[ ... ]

> +err_pm_runtime_suspend:
> +	pm_runtime_put(dev);
> +err_pm_runtime_disable:
> +	pm_runtime_dont_use_autosuspend(dev);
> +	pm_runtime_disable(dev);
> +err_irq:
> +	dev_err(dev, "probe: error %d\\n", ret);
> +	return ret;
> +}

[Severity: High]
Does this sequence properly handle the asynchronous pm_runtime_put()?

Calling pm_runtime_disable() immediately after the asynchronous
pm_runtime_put() will cancel the pending suspend, permanently stranding the
hardware in a powered-on state. A synchronous put should be used before
disabling.

> +static void neoisp_remove(struct platform_device *pdev)
> +{
> +	struct neoisp_dev_s *neoispd = platform_get_drvdata(pdev);
> +
> +	neoisp_destroy_devices(neoispd);
> +
> +	if (standalone_mdev)
> +		media_device_cleanup(&neoispd->mdev);
> +
> +	pm_runtime_dont_use_autosuspend(neoispd->dev);
> +	pm_runtime_disable(neoispd->dev);
> +}

[Severity: High]
Could the hardware IRQ fire after structures are freed?

The IRQ handler is managed by devm_request_irq() and will remain active
until the devm teardown phase. However, neoisp_destroy_devices() frees
neoispd->context. A late or spurious interrupt could trigger a
use-after-free in the handler. Should the hardware interrupts be explicitly
disabled here?

[Severity: High]
Should we use a synchronous PM put here?

Similar to the probe error path, failing to transition the device to suspend
before calling pm_runtime_disable() can leave the hardware permanently
powered on.

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