Re: [PATCH v6 6/8] media: qcom: camss: Add CAMSS Offline Processing Engine driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [High] Permanent deadlock of the OPE scheduler when a single queue is stopped while others are active.
- [High] Race condition during streamoff causing the hardware to perform DMA into a freed buffer returned to userspace.
- [High] Missing pipeline refcounting for V4L2 subdev nodes leading to Use-After-Free.
- [High] Use-After-Free of the main `ope_dev` structure due to devres and file lifetime mismatch.
- [High] The driver fails to abort active hardware DMA during `remove()`, causing fatal IOMMU faults.
- [High] NULL pointer dereference in `ope_run_work` because `ope->pipeline` is freed before the workqueue is canceled.
- [High] Use-After-Free of `ope_ctx` if the hardware reset times out.
- [High] Use-After-Free of `ctx` in `ope_irq()` due to continuing execution after signaling reset completion.
- [High] `ope_release()` fails to clear vb2_queue ownership when an owner file descriptor is closed, causing persistent lockups and resource leaks.
- [Medium] Double invocation of `ope_job_finish` corrupts queue state if `RST_DONE` and `IDLE` interrupts are co-asserted.
- [Medium] Time-Of-Check to Time-Of-Use (TOCTOU) race during video device registration allows uninitialized drvdata access.
- [Medium] Dangling pointer in Gamma LUT cache allows information leak and rendering corruption across sessions.
- [Low] Mixing goto-based error handling and scope-based cleanup helpers (`scoped_guard`) in the same function.
--

commit aab4f4c79719b4532d479995843c0d6e2fac9811
Author: Loic Poulain <loic.poulain@oss.qualcomm.com>
Subject: media: qcom: camss: Add CAMSS Offline Processing Engine driver

This commit adds an image processing driver for the Qualcomm Offline
Processing Engine (OPE), which converts raw Bayer frames to YUV. The
driver exposes three video nodes per pipeline instance and relies on a
static topology builder for media controller setup.

Link: https://github.com/jwrdegoede/libcamera/commits/camss_pipeline_v2.1/
Link: https://github.com/loicpoulain/camss-isp-m2m-test

> diff --git a/drivers/media/platform/qcom/camss/ope/core.c b/drivers/media/platform/qcom/camss/ope/core.c
> --- a/drivers/media/platform/qcom/camss/ope/core.c
> +++ b/drivers/media/platform/qcom/camss/ope/core.c

[ ... ]

> static bool ope_glut_bank_resident(struct ope_dev *ope, u8 bank,
> 				    struct ope_ctx *ctx)
> {
> 	return ope->glut_bank[bank].valid &&
> 	       ope->glut_bank[bank].owner == ctx;
> }

[Severity: Medium]
Might this lead to a dangling pointer in the Gamma LUT cache? If a
non-running context is canceled, it skips the hardware reset and the
ope_glut_cache_invalidate() call. The cache owner field continues pointing to
the destroyed context. If a new session reuses that memory address, it could
bypass the upload and incorrectly reuse the previous session's LUT.

[ ... ]

> static dma_addr_t ope_buf_dma_addr(struct ope_ctx *ctx, unsigned int queue_idx)
> {
> 	struct vb2_v4l2_buffer *vbuf = ope_queue_peek(ctx, queue_idx);
> 
> 	if (!vbuf)
> 		return 0;
> 
> 	return vb2_dma_contig_plane_dma_addr(&vbuf->vb2_buf, 0);
> }

[Severity: High]
Might this cause a race condition where the hardware performs DMA into a freed
buffer? The worker merely peeks at the ready queue to get the DMA address
without taking ownership. Meanwhile, ope_stop_streaming() re-enables the work
queue and calls ope_queue_drain(), which can return the buffer to userspace
while the hardware is programmed to use it.

[ ... ]

> static void ope_sched_cancel(struct ope_ctx *ctx)
> {
> 	struct ope_dev *ope = ctx->ope;
> 	bool running;
> 
> 	disable_work_sync(&ope->run_work);
> 
> 	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> 		running = (ope->curr_ctx == ctx);
> 		ctx->queued = false;
> 	}
> 
> 	if (running) {
> 		reinit_completion(&ope->reset_complete);
> 		ope_write(ope, OPE_TOP_RESET_CMD, OPE_TOP_RESET_CMD_SW);
> 		if (!wait_for_completion_timeout(&ope->reset_complete,
> 						 msecs_to_jiffies(OPE_RESET_TIMEOUT_MS)))
> 			dev_err(ope->dev, "reset timeout during cancel\n");

[Severity: High]
Is there a risk of a use-after-free if the hardware reset times out here? The
timeout error is logged, but ope->hw_ctx is not cleared. The caller will
proceed to free the context memory. If the delayed hardware interrupt
eventually fires, the ISR will read the dangling pointer.

> 	}
> 
> 	enable_work(&ope->run_work);
> }

[Severity: High]
Could this lead to a permanent deadlock of the scheduler? Disabling the
workqueue and triggering the hardware reset interrupts the state machine.
If ope_stop_streaming() is called on a single queue while others are active,
the interrupt will call ope_job_finish(). Because other queues are active,
ope_try_schedule() sees the job as ready, sets ctx->queued = true, and calls
schedule_work(). Since the workqueue is disabled here, the work is silently
dropped, and future try_schedule calls will return immediately.

[ ... ]

> static void ope_try_schedule(struct ope_ctx *ctx)
> {
> 	struct ope_dev *ope = ctx->ope;
> 
> 	if (!ope_job_ready(ctx))
> 		return;
> 
> 	scoped_guard(spinlock_irqsave, &ope->sched_lock) {
> 		if (ctx->queued || ope->curr_ctx)
> 			return;
> 		ctx->queued = true;
> 	}
> 
> 	schedule_work(&ope->run_work);
> }

[Severity: High]
As noted above, if schedule_work() is called while the workqueue is disabled,
it is dropped. Because ctx->queued is already set to true, the scheduler
becomes permanently stuck.

[ ... ]

> static irqreturn_t ope_irq(int irq, void *dev_id)
> {
> 	struct ope_dev *ope = dev_id;
> 	struct ope_ctx *ctx;
> 	u32 status;
> 
> 	/*
> 	 * hw_ctx is safe to read here without a lock: ope_run_job() sets it
> 	 * before calling ope_start(), so the hardware cannot raise an IRQ
> 	 * before hw_ctx is visible.  ope_job_finish() clears it in IRQ context
> 	 * here, so the clear is serialised with this read by the IRQ
> 	 * itself.
> 	 */
> 	ctx = ope->hw_ctx;

[Severity: High]
If the hardware reset timed out earlier during ope_sched_cancel(), could the
delayed interrupt fire later and dereference a freed context here?

> 
> 	status = ope_read(ope, OPE_TOP_IRQ_STATUS);
> 	ope_write(ope, OPE_TOP_IRQ_CLEAR, status);
> 	ope_write(ope, OPE_TOP_IRQ_CMD, OPE_TOP_IRQ_CMD_CLEAR);
> 
> 	if (status & OPE_TOP_IRQ_STATUS_RST_DONE) {
> 		dev_dbg(ope->dev, "reset done ctx=%p\n", ctx);
> 		ope_glut_cache_invalidate(ope);
> 		if (ctx)
> 			ope_job_finish(ctx, VB2_BUF_STATE_ERROR);
> 		complete(&ope->reset_complete);
> 	}

[Severity: High]
Could the ISR access freed memory by continuing execution after this block?
Signaling reset_complete unblocks the thread in ope_sched_cancel(), which
might immediately destroy and free the context. The ISR then continues to
execute down to the WE and IDLE status checks, dereferencing its local copy
of the freed ctx.

> 
> 	if (status & OPE_TOP_IRQ_STATUS_VIOL)
> 		dev_warn(ope->dev, "OPE violation: 0x%08x\n",
> 			 ope_read(ope, OPE_TOP_VIOLATION_STATUS));
> 
> 	if (status & OPE_TOP_IRQ_STATUS_FE)
> 		ope_fe_irq(ope);
> 
> 	if (status & OPE_TOP_IRQ_STATUS_WE) {
> 		if (ope_we_irq(ope, ctx))
> 			return IRQ_HANDLED;
> 	}
> 
> 	if ((status & OPE_TOP_IRQ_STATUS_IDLE) && ctx) {
> 		struct ope_stripe *stripe = ope_current_stripe(ctx);
> 
> 		dev_dbg(ope->dev, "stripe %u done ctx=%p\n", ctx->current_stripe, ctx);
> 
> 		if (unlikely(!stripe)) {
> 			dev_err(ope->dev, "stripe index %u out of range\n", ctx->current_stripe);
> 			ctx->current_stripe = 0;
> 			ope_job_finish(ctx, VB2_BUF_STATE_ERROR);
> 		} else if (ope_stripe_is_last(stripe)) {
> 			ctx->current_stripe = 0;
> 			ope_job_finish(ctx, VB2_BUF_STATE_DONE);
> 		} else {
> 			ctx->current_stripe++;
> 			ope_start(ope);
> 		}
> 	}

[Severity: Medium]
What happens if both the RST_DONE and IDLE interrupts are co-asserted? Since
ctx is not cleared and the routine doesn't return early after handling the
reset block above, it looks like it will fall through here and call
ope_job_finish() a second time, which might prematurely complete unrelated
buffers and corrupt the queue state.

[ ... ]

> static void ope_stop_streaming(struct vb2_queue *q)
> {
> 	struct ope_ctx *ctx = ope_ctx_from_vq(q);
> 	struct video_device *vdev = ope_vdev_from_vq(q);
> 	unsigned int idx = ope_idx_from_vq(q);
> 
> 	dev_dbg(ctx->ope->dev, "streaming stop ctx=%p queue=%u\n", ctx, idx);
> 
> 	if (idx == OPE_QUEUE_FRAME_IN) {
> 		ctx->started = false;
> 		ope_adjust_power(ctx->ope);
> 	}
> 
> 	ope_sched_cancel(ctx);
> 	ope_queue_drain(ctx, idx, VB2_BUF_STATE_ERROR);
> 	pm_runtime_put(ctx->ope->dev);
> 	video_device_pipeline_stop(vdev);
> }

[Severity: High]
As noted with ope_buf_dma_addr(), the context's queues are drained after the
workqueue is re-enabled in ope_sched_cancel(). This allows the hardware
worker to race with the drain and potentially perform DMA into a buffer that
has already been returned to userspace.

[ ... ]

> static struct ope_ctx *ope_ctx_create(struct ope_dev *ope)
> {
> 	struct v4l2_pix_format_mplane pix_in = { .pixelformat = ope_input_fmts[0].fourcc };

[ ... ]

> 	for (i = 0; i < OPE_QUEUE_COUNT; i++) {
> 		INIT_LIST_HEAD(&ctx->queues[i].rdy_queue);
> 		spin_lock_init(&ctx->queues[i].rdy_spinlock);
> 		ret = ope_init_vq(ctx, i);
> 		if (ret)
> 			goto err_release_vq;
> 	}
> 
> 	INIT_LIST_HEAD(&ctx->list);
> 
> 	scoped_guard(spinlock, &ope->ctx_lock)
> 		list_add(&ctx->list, &ope->ctx_list);

[Severity: Low]
Is it safe to mix goto-based error handling and scope-based cleanup helpers
in the same routine? The kernel cleanup guidelines suggest avoiding this
mixture to prevent LIFO ordering issues or confusing ownership semantics.

[ ... ]

> static int ope_release(struct file *file)
> {
> 	struct v4l2_fh *fh = file_to_v4l2_fh(file);
> 	struct ope_ctx *ctx = ope_ctx_from_file(file);
> 	struct ope_dev *ope = ctx->ope;
> 
> 	v4l2_fh_del(fh, file);
> 	v4l2_fh_exit(fh);
> 	kfree(fh);
> 
> 	scoped_guard(mutex, &ope->mutex) {
> 		if (--ope->open_count != 0)
> 			return 0;
> 		ope->shared_ctx = NULL;
> 		scoped_guard(spinlock, &ope->ctx_lock)
> 			list_del(&ctx->list);
> 	}
> 
> 	ope_ctx_destroy(ctx);
> 
> 	return 0;
> }

[Severity: High]
Could this early return on close trap the vb2_queue ownership? If a file
descriptor that acquired queue ownership is closed, returning without calling
vb2_queue_release() or clearing its owner leaves the queue locked to a freed
file descriptor pointer. This could lead to a permanent -EBUSY denial of
service for other open descriptors.

[Severity: High]
Also, is it possible for ope_release() to access freed memory here? Since the
ope_dev structure is allocated with devm_kzalloc(), its lifetime is tied to
device binding. If userspace holds the character device open beyond unbind,
ctx->ope might point to freed memory when the file is eventually closed.

[ ... ]

> static void ope_v4l2_cleanup(struct ope_dev *ope)
> {
> 	if (ope->pipeline) {
> 		camss_isp_pipeline_unregister(ope->pipeline);
> 		camss_isp_pipeline_free(ope->pipeline);
> 		ope->pipeline = NULL;
> 	}
> 	media_device_unregister(&ope->mdev);
> 	v4l2_device_unregister(&ope->v4l2_dev);
> 	media_device_cleanup(&ope->mdev);
> 	synchronize_irq(ope->irq);
> 	cancel_work_sync(&ope->run_work);
> }

[Severity: High]
Could this sequence result in a NULL pointer dereference? The pipeline is
freed and nulled out before the worker thread is fully canceled. If the
worker thread executes concurrently during unbind, it might try to access the
freed pipeline via camss_isp_pipeline_get_subdev().

[Severity: High]
Additionally, should this cleanup routine issue a hardware reset to abort any
active DMA? When the driver unbinds, devres tears down DMA and IOMMU
mappings. If the hardware is still running, couldn't it continue executing
DMA into unmapped memory, triggering fatal bus or IOMMU faults?

[ ... ]

> static int ope_probe(struct platform_device *pdev)
> {
> 	struct device *dev = &pdev->dev;
> 	struct ope_dev *ope;
> 	int ret, irq;
> 
> 	ope = devm_kzalloc(dev, sizeof(*ope), GFP_KERNEL);

[Severity: High]
As noted in ope_release(), allocating this structure with a devres managed
lifetime ties it to the driver bind rather than the file descriptors. This
leads to a use-after-free when userspace closes the file descriptor after the
driver is unbound.

> diff --git a/drivers/media/platform/qcom/camss/ope/pipeline.c b/drivers/media/platform/qcom/camss/ope/pipeline.c
> --- a/drivers/media/platform/qcom/camss/ope/pipeline.c
> +++ b/drivers/media/platform/qcom/camss/ope/pipeline.c

[ ... ]

> static int isp_register_vdev(struct camss_isp_pipeline *pipeline,
> 			     struct camss_isp_pipeline_entity *slot,
> 			     const struct camss_isp_entity_desc *desc,
> 			     struct v4l2_device *v4l2_dev)
> {

[ ... ]

> 	ret = video_register_device(vdev, VFL_TYPE_VIDEO, -1);
> 	if (ret)
> 		return ret;
> 
> 	kref_get(&pipeline->refcount);
> 
> 	video_set_drvdata(vdev, desc->vdev.drvdata);
> 
> 	return 0;
> }

[Severity: Medium]
Does registering the device before setting the driver private data introduce a
time-of-check to time-of-use race condition? If userspace immediately issues
an ioctl on the newly visible device node, video_get_drvdata() could return
uninitialized data before it is set here.

[ ... ]

> static int isp_register_subdev(struct camss_isp_pipeline_entity *slot,
> 			       const struct camss_isp_entity_desc *desc,
> 			       struct v4l2_device *v4l2_dev)
> {

[ ... ]

> 	sd->entity.function = desc->function ?
> 			      desc->function : MEDIA_ENT_F_V4L2_SUBDEV_UNKNOWN;
> 	/* Create a /dev/v4l-subdevN node so userspace can query pad formats */
> 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
> 	if (desc->subdev.internal_ops)

[Severity: High]
Does exposing this subdevice to userspace without matching refcounting on the
parent pipeline risk a use-after-free? Unlike the main video nodes, it seems
the subdevice node doesn't hold a reference to pipeline->refcount. If
userspace keeps it open after driver unbind, the main nodes might release the
pipeline memory, leading to a crash on subsequent subdev operations.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907-camss-isp-ope-v6-0-6b915b9c5131@oss.qualcomm.com?part=6