Re: [PATCH v5 2/3] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] Active DMA buffers are returned to userspace while hardware is still running, leading to memory corruption and IOMMU faults.
- [High] Missing synchronize_irq() during file release causes a Use-After-Free of `jenc_context` in the IRQ thread.
- [High] V4L2 device release callback manually frees devres-managed memory, causing a double-free.
- [High] Dropping dev_mutex during stream shutdown allows racing hardware initialization and teardown, corrupting system state.
- [High] Asynchronous job completion scheduling after file close cancellation leads to Use-After-Free.
- [High] Hardware write engine buffer size limit calculation permits out-of-bounds DMA writes.
- [Medium] DMA API violation: Syncing a DMA_FROM_DEVICE capture buffer as DMA_BIDIRECTIONAL.
- [Medium] 32-bit arithmetic overflow in clock frequency calculation causes severely downgraded performance.
- [Medium] Transient errors permanently desynchronize queue sequences, freezing the encoder context.
- [Low] Undefined macro `kzalloc_obj` prevents compilation.
--

--- Patch [2]: [PATCH v5 2/3] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder ---
commit f8b765e57f128de496c47a3df983a6b13fbd2b0f
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>
Subject: media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

This commit adds a Qualcomm JPEG encoder driver implemented on top of the
V4L2 mem2mem framework. The driver handles vb2 queues, format negotiation,
JPEG header emission, interrupt-driven job completion, and runtime PM for
the standalone JPEG encode hardware block.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
[ ... ]
> @@ -534,14 +534,16 @@ static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue 
>  	jpeg_upload_dmi_table(ectx);
>  	mutex_unlock(&ectx->quality_mutex);
>  
>  	/*
>  	 * Invalidate stale CPU cache lines before writing the JPEG header
>  	 * with the CPU into the destination buffer.
>  	 */
>  	jpeg_sync_sg(jenc->dev, frame, DMA_BIDIRECTIONAL, false);

[Severity: Medium]
The destination buffer is provided by the videobuf2 capture queue, meaning it
is mapped as DMA_FROM_DEVICE. Is it valid to sync it here using
DMA_BIDIRECTIONAL? Doing so might violate the DMA API and cause cache
incoherency.

>  
>  	header_size = min_t(size_t, vb->planes[0].length, ectx->hdr_cache.size);
>  	header_size = qcom_jenc_header_emit(&ectx->hdr_cache, mptr,
>  					    (u32)header_size, (u16)q->vf.width,
>  					    (u16)q->vf.height);
>  
>  	dma += header_size;

[Severity: High]
When the DMA base pointer is advanced by header_size here, does the hardware
limit need to be updated as well? The hardware buffer size is set to the full
sizeimage in jpeg_setup_we_size(). If the pointer is offset by header_size
without reducing the writable size, could the hardware write past the physical
boundary of the buffer?

>  	qcom_jenc_dqts_emit(&ectx->hdr_cache, mptr);
[ ... ]
> @@ -1560,6 +1560,8 @@ static unsigned long jpeg_calc_optimal_clk(struct qcom_jenc_dev *jenc,
>  
>  	ref_pixel_rate = res->ref_throughput_mpps * 1000000UL;
>  	pixel_rate = (unsigned long)width * height * fps;
>  
>  	optimal_clk = DIV_ROUND_UP_ULL(pixel_rate * res->ref_clk_hz, ref_pixel_rate);

[Severity: Medium]
Can this throughput calculation overflow on 32-bit architectures? The
width, height, and fps are multiplied together as a 32-bit unsigned long
before the product is multiplied by res->ref_clk_hz. For large resolutions
and high framerates, should this be cast to a 64-bit type to prevent
wrapping around?

>  
>  	return min_t(unsigned long, jenc->max_freq, optimal_clk);
>  }
[ ... ]
> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
[ ... ]
> @@ -245,14 +245,13 @@ static void jpeg_v4l2_work_stop(struct jenc_context *ctx, enum vb2_buffer_state 
>  
>  	/* Drain CAPTURE queue; signal EOS on last buffer if V4L2_ENC_CMD_STOP. */
>  	while ((vb = v4l2_m2m_dst_buf_remove(ctx->fh.m2m_ctx))) {
>  		if (was_stopping && !v4l2_m2m_last_dst_buf(ctx->fh.m2m_ctx)) {
>  			vb2_set_plane_payload(&vb->vb2_buf, 0, 0);
>  			v4l2_m2m_last_buffer_done(ctx->fh.m2m_ctx, vb);
>  		} else {
>  			v4l2_m2m_buf_done(vb, buff_state);
>  		}
>  	}
>  
>  	while ((vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx)))
>  		v4l2_m2m_buf_done(vb, buff_state);

[Severity: High]
When returning active DMA buffers to userspace through VB2_BUF_STATE_ERROR
here, does the hardware stop immediately? If the hardware continues performing
DMA transfers on these buffers after they are handed back, is there a risk of
memory corruption or IOMMU faults?

>  
>  	/* Always call job_finish to let m2m schedule the next job. */
>  	schedule_work(&ctx->finish_work);

[Severity: High]
Can scheduling this work item cause a use-after-free on the context?
After cancelling finish_work in fop_jpeg_file_release(), calling
v4l2_m2m_ctx_release() cascades into mop_jpeg_m2m_job_abort() and then here
to jpeg_v4l2_work_stop(), which re-schedules the finish_work item. The work
item will remain pending while the release function completes and frees the
context memory.

>  }
[ ... ]
> @@ -465,6 +465,9 @@ static void bop_jpeg_vb2_stop_streaming(struct vb2_queue *q)
>  	do_release = ectx->hw_acquired;
>  	ectx->hw_acquired = false;
>  
>  	mutex_unlock(&jenc->dev_mutex);
>  
>  	/*
>  	 * hw_release() calls jpeg_deinit() which issues HW_STOP and waits for
>  	 * the STOP_ACK interrupt completion.  It must be called outside
>  	 * dev_mutex to avoid a deadlock: the threaded IRQ handler acquires
>  	 * dev_mutex before signalling the completion.
>  	 */
>  	if (do_release)
>  		hw->hw_release(ectx, q);

[Severity: High]
By dropping dev_mutex before calling hw_release() (which issues hardware
teardown like jpeg_deinit, disable_irq, and pm_runtime_put_sync), can this
race with another thread executing STREAMON? If op_jpeg_acquire() runs
concurrently, it could try to power on and initialize the hardware while
this thread is actively shutting it down.

>  }
[ ... ]
> @@ -641,10 +641,12 @@ static void mop_jpeg_m2m_job_run(void *priv)
>  	if (hw->hw_prepare(jenc))
>  		goto err_stop;
>  
>  	sq = hw->get_queue(ectx, TYPE2QID(src_vb->vb2_buf.type));
>  	src_vb->sequence = sq->sequence++;
>  	if (hw->process_exec(jenc, ectx, &src_vb->vb2_buf))
>  		goto err_stop;
>  
>  	dq = hw->get_queue(ectx, TYPE2QID(dst_vb->vb2_buf.type));
>  	dst_vb->sequence = dq->sequence++;

[Severity: Medium]
If hw->process_exec() fails for the source buffer and jumps to err_stop, will
the queue sequences become permanently misaligned? The source queue sequence
is incremented before the destination queue sequence. Subsequent encode jobs
might stall silently because the check for matching sequences will fail.

>  	if (hw->process_exec(jenc, ectx, &dst_vb->vb2_buf))
>  		goto err_stop;
[ ... ]
> @@ -1034,6 +1034,7 @@ static int fop_jpeg_file_open(struct file *file)
>  	struct jenc_context *ectx;
>  	int rc;
>  
>  	ectx = kzalloc_obj(*ectx, GFP_KERNEL);

[Severity: Low]
Does the kzalloc_obj macro exist in the kernel? It looks like this might
cause a build failure and should perhaps use kzalloc().

>  	if (!ectx)
>  		return -ENOMEM;
[ ... ]
> @@ -996,6 +996,9 @@ static int fop_jpeg_file_release(struct file *file)
>  	spin_lock_irqsave(&jenc->hw_lock, flags);
>  	if (jenc->actx == ectx)
>  		jenc->actx = NULL;
>  	spin_unlock_irqrestore(&jenc->hw_lock, flags);
>  
>  	cancel_work_sync(&ectx->stop_work);
>  	cancel_work_sync(&ectx->finish_work);
>  
>  	v4l2_m2m_ctx_release(ectx->fh.m2m_ctx);
>  	v4l2_m2m_put(m2m_dev);
>  	v4l2_ctrl_handler_free(&ectx->ctrl_hdl);
>  	v4l2_fh_del(&ectx->fh, file);
>  	v4l2_fh_exit(&ectx->fh);
>  	mutex_destroy(&ectx->quality_mutex);
>  	mutex_destroy(&ectx->ctx_lock);
>  	kfree(ectx);

[Severity: High]
Could this lead to a use-after-free if another file descriptor is still open?
When the context is freed here, the driver does not invoke synchronize_irq().
The threaded IRQ handler in op_jpeg_irq_bot() might have already cached the
actx pointer before it was cleared under the spinlock. Should
synchronize_irq() be called before freeing the context?

>  
>  	return 0;
>  }
[ ... ]
> @@ -1115,6 +1115,8 @@ static void jenc_v4l2_dev_release(struct v4l2_device *v4l2_dev)
>  {
>  	struct qcom_jenc_dev *jenc = container_of(v4l2_dev, struct qcom_jenc_dev, v4l2_dev);
>  
>  	v4l2_device_unregister(&jenc->v4l2_dev);
>  	v4l2_m2m_put(jenc->m2m_dev);
>  	kfree(jenc);

[Severity: High]
Is it intentional to manually kfree() jenc here? Because it was originally
allocated with devm_kzalloc() in qcom_jpeg_probe(), freeing it manually will
cause a double-free during the devres teardown sequence.

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260728082548.2511694-1-atanas.filipov@oss.qualcomm.com?part=2