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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 12 potential issue(s) to consider:
- [Critical] Job abort and `stop_streaming` return buffers to userspace without stopping the hardware, causing DMA into freed or repurposed memory.
- [High] The driver lacks a `.remove` callback and performs an illegal devm `kfree()` in the `v4l2_dev.release` callback, leaking `/dev/videoX` and causing a SLUB crash.
- [High] The driver configures its videobuf2 queues with `vb2_dma_sg_memops` but only programs the hardware with the first scatterlist chunk's DMA address, leading to Out-Of-Bounds memory accesses.
- [High] Race condition between the threaded IRQ handler and `fop_jpeg_file_release` leads to a Use-After-Free of `jenc_context`.
- [High] Calling `VIDIOC_CREATE_BUFS` overwrites the JPEG header cache with zeroes without resetting `quality_programmed`, leading to a divide-by-zero panic in `jpeg_upload_dmi_table`.
- [High] Unsynchronized hardware power and clock transitions during concurrent `start_streaming` and `stop_streaming` cause a race condition.
- [High] The context is freed while a work item is actively queued, leading to a Use-After-Free in the workqueue handler.
- [High] `jpeg_stop` is invoked in the threaded IRQ handler after completing the previous job, creating a race that halts the newly started job.
- [High] The hardware write engine limits are not adjusted for the JPEG header offset, allowing DMA to write past the end of the destination buffer.
- [Medium] Buffers are permanently leaked to the driver if `start_streaming` fails to acquire the hardware.
- [Medium] The driver synchronizes the capture buffer with `DMA_BIDIRECTIONAL` while it was mapped as `DMA_FROM_DEVICE`, violating the DMA API.
- [Medium] The write engine's DMA pointer is programmed with an unaligned address, risking AXI/AHB burst access faults.
--

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

media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

Add a Qualcomm JPEG encoder driver implemented on top of the V4L2 mem2mem 
framework.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> index e4472a30b412d..0000000000000 100644
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> @@ -267,8 +267,7 @@ static const struct of_device_id qcom_jpeg_of_match[] = {
>  MODULE_DEVICE_TABLE(of, qcom_jpeg_of_match);
>  
>  static struct platform_driver qcom_jpeg_platform_driver = {
>  	.probe  = qcom_jpeg_probe,
>  	.driver = {

[Severity: High]
Does this leave the video device registered after module unload? 
It appears the platform driver lacks a .remove callback, which means
video_unregister_device() might never be called when the module is removed,
leaving a stale device node.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> index b756cf10e5c03..0000000000000 100644
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> @@ -348,6 +348,6 @@ static int jpeg_store_fe_next(struct jenc_context *ectx, struct vb2_buffer *vb2)
>  		buff->plns[pln].sgt = vb2_dma_sg_plane_desc(vb2, pln);
>  		if (!buff->plns[pln].sgt || !buff->plns[pln].sgt->sgl)
>  			return -EINVAL;
>  
>  		buff->plns[pln].dma = sg_dma_address(buff->plns[pln].sgt->sgl);

[Severity: High]
By using vb2_dma_sg_memops during initialization, we allow fragmented buffers,
but here we only extract the DMA address of the very first scatterlist chunk.
Will the hardware assume a contiguous physical buffer and perform out-of-bounds
DMA past the first fragment?

[ ... ]

> @@ -487,6 +487,6 @@ static void jpeg_upload_dmi_table(struct jenc_context *ectx)
>  	/* DMI Luma upload - values are stored in zigzag order in hdr_cache */
>  	for (i = 0; i < ARRAY_SIZE(t81k1_dct_luma_table); i++) {
>  		reg_val = div_u64(U16_MAX + 1U, luma_qt[i]);

[Severity: High]
Can this lead to a divide-by-zero panic? If a user triggers VIDIOC_CREATE_BUFS
after a frame is encoded, it re-initializes the header cache with zeroes. 
When the next frame encodes, if quality_programmed matches quality_requested, 
the cache update is skipped, and luma_qt[i] would be zero when used as a 
denominator here.

[ ... ]

> @@ -612,12 +612,12 @@ static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue
>  
>  	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: Medium]
Since the size of the emitted JPEG header is added directly to the DMA base
address, the resulting pointer might not be cache-line or burst aligned. If
the hardware performs burst writes, could this unaligned address cause AXI or
AHB bus faults?

>  	qcom_jenc_dqts_emit(&ectx->hdr_cache, mptr);
>  
>  	/*
>  	 * Flush CPU writes to the header before handing the buffer to the
>  	 * hardware DMA engine.
>  	 */
>  	jpeg_sync_sg(jenc->dev, frame, DMA_BIDIRECTIONAL, true);

[Severity: Medium]
The queue initializes the capture buffer without the bidirectional flag, so it
is implicitly mapped for DMA_FROM_DEVICE. Synchronizing the buffer explicitly
with DMA_BIDIRECTIONAL here violates the DMA API. Could this trigger kernel
warnings if CONFIG_DMA_API_DEBUG is enabled?

[ ... ]

> @@ -665,6 +665,6 @@ static int jpeg_setup_we_size(struct jenc_context *ectx, struct qcom_jenc_queue
>  	for (pln = 0; pln < QCOM_JPEG_MAX_PLANES; pln++)
>  		jpeg_rw_bits(jenc, jpeg_v420_we_stride[pln], JPEG_V420_PLN_WR_STRIDE_STRIDE_MSK, 0);
>  
>  	jpeg_io_write(jenc, JPEG_V420_REG_WE_BSIZE_0, dfmt->plane_fmt[0].sizeimage);

[Severity: High]
Will this allow the hardware to write past the end of the destination buffer?
The DMA pointer was advanced by header_size in jpeg_apply_we_addr(), but the 
hardware's buffer size limit is programmed with the full original sizeimage, 
potentially extending the write by header_size bytes beyond the allocation.

[ ... ]

> @@ -806,12 +806,12 @@ static irqreturn_t op_jpeg_irq_bot(int irq, void *data)
>  
>  		if (ctx && ctx->bufq[JENC_DST_QUEUE].buff_id >= 0) {
>  			struct qcom_jenc_queue *dq = &ctx->bufq[JENC_DST_QUEUE];
>  			struct qcom_jpeg_buff *frame = &dq->buff[dq->buff_id];
>  			size_t out_size = jpeg_rd_bits(jenc, JPEG_V420_REG_ENC_OUT_SIZE,
>  						       JPEG_V420_STATUS_OUT_SIZE_BYTES_MSK);
>  
>  			dev_dbg(jenc->dev, "complete idx:%d addr=%pad size=%zu\n",
>  				dq->buff_id, &frame->plns[0].dma, out_size);
>  
>  			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_DONE,
>  					    out_size + ctx->hdr_cache.size);
>  		}

[Severity: High]
Can this result in a use-after-free? The threaded IRQ handler drops the
spinlock right before invoking enc_hw_irq_cb(), so fop_jpeg_file_release()
could run concurrently, acquire the lock, see actx is already NULL, and free
the context while the IRQ handler is still using it.

>  		jpeg_stop(jenc);

[Severity: High]
Does this unconditionally stop the hardware even if the next job has already
started? The enc_hw_irq_cb() schedules a worker that could immediately submit
a new job via jpeg_exec(). If this threaded IRQ handler resumes and blindly
calls jpeg_stop(), it might halt the newly started job mid-flight.

[ ... ]

> @@ -912,6 +912,6 @@ static int op_jpeg_queue_setup(struct jenc_context *ectx, enum qcom_enc_qid id)
>  	if (id == JENC_SRC_QUEUE) {
>  		struct qcom_jenc_queue *q = &ectx->bufq[id];
>  
>  		rc = qcom_jenc_header_init(&ectx->hdr_cache, q->vf.pixelformat);

[Severity: High]
Is it intentional that VIDIOC_CREATE_BUFS calls queue_setup, which resets 
the header cache with zeroes, but doesn't reset quality_programmed? This seems
to be what later causes the divide-by-zero panic in the DMI upload.

[ ... ]

> @@ -1556,6 +1556,6 @@ static int op_jpeg_acquire(struct jenc_context *ectx, struct vb2_queue *q)
>  		}
>  
>  		enable_irq(jenc->irq);
>  
>  		rc = jpeg_init(jenc);

[Severity: High]
Is it safe for this hardware enablement to potentially race with teardown? 
If another thread concurrently stops a stream and calls hw_release() without 
holding dev_mutex, it could lead to hardware initialization running at the same 
time as teardown, clobbering the state and causing unbalanced IRQ enablement.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> index 6cb162f8eef90..0000000000000 100644
> --- a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> @@ -338,12 +338,12 @@ static void jpeg_v4l2_work_stop(struct jenc_context *ctx, enum vb2_buffer_state
>  	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);
>  		}
>  	}

[Severity: Critical]
Do we return the buffers to userspace before the underlying hardware has
actually stopped? If v4l2_m2m_buf_done() is called here during a streamoff, 
the buffer might be reused by userspace, but the hardware may still be actively 
executing the job and corrupting that repurposed memory.

>  	while ((vb = v4l2_m2m_src_buf_remove(ctx->fh.m2m_ctx)))
>  		v4l2_m2m_buf_done(vb, buff_state);
>  
>  	/* Always call job_finish to let m2m schedule the next job. */
>  	schedule_work(&ctx->finish_work);

[Severity: High]
Could this lead to a use-after-free in the workqueue? The release function
cancels finish_work and then calls v4l2_m2m_ctx_release(), which eventually
invokes jpeg_v4l2_work_stop(). Re-queuing finish_work right here leaves a 
pending work item on memory that is about to be freed.

[ ... ]

> @@ -796,6 +796,6 @@ static int bop_jpeg_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
>  	} else {
>  		rc = 0;
>  	}
>  
>  	mutex_unlock(&jenc->dev_mutex);
>  
>  	return rc;

[Severity: Medium]
If hw_acquire() fails above, the error is returned immediately. Doesn't this
permanently leak the buffers to the driver since they are not returned to the
vb2 framework using vb2_buffer_done() with VB2_BUF_STATE_QUEUED?

[ ... ]

> @@ -837,12 +837,12 @@ static void bop_jpeg_vb2_stop_streaming(struct vb2_queue *q)
>  
>  	mutex_lock(&jenc->dev_mutex);
>  
>  	jpeg_v4l2_work_stop(ectx, VB2_BUF_STATE_ERROR);
>  
>  	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]
Is it safe to drop dev_mutex before invoking hw_release()? If another thread
concurrently starts a stream and calls hw_acquire() under the lock, it could 
run hardware enablement at the same time as teardown, potentially clobbering 
hardware state and causing unbalanced IRQ enablement.

[ ... ]

> @@ -1105,6 +1105,6 @@ 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 safe to manually kfree(jenc) here when the structure was originally 
allocated using devm_kzalloc() during probe? It looks like this would trigger
a double-free SLUB violation.

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