Re: [PATCH v4 5/5] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 11 potential issue(s) to consider:
- [Critical] The destination DMA buffer limits configured for the hardware do not account for the prepended JPEG header offset, allowing an out-of-bounds write.
- [Critical] Hardware is not stopped when active jobs are aborted or queues are stopped, leading to ongoing DMA into freed V4L2 buffers.
- [Critical] Context structure `ectx` is freed in the release handler without synchronizing with the threaded IRQ handler.
- [High] Missing platform driver `.remove` hook causes a video device and V4L2 M2M context leak, leading to Use-After-Free on unbind.
- [High] The V4L2 device release callback attempts to call `kfree()` on a `devm_kzalloc` managed pointer.
- [High] Hardware registers for write engine blocks per row/column are swapped.
- [High] Hardware release in `stop_streaming` is executed outside of `dev_mutex`, creating a race condition with concurrent V4L2 contexts acquiring the hardware.
- [High] The driver requires contiguous DMA memory but improperly uses `vb2_dma_sg_memops` and ignores multi-entry scatterlists.
- [High] `jpeg_stop` is issued to the hardware after notifying the m2m framework of job completion, corrupting the subsequent job.
- [High] Bidirectional DMA sync is performed on a buffer that was mapped as `DMA_FROM_DEVICE`.
- [High] If `start_streaming` fails to acquire the hardware, queued buffers are leaked instead of being returned to the vb2 core.
--

commit 1e9c8c45335bfb3f6ed96f07a4bd3645221761b5
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
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c

[ ... ]

> +static struct platform_driver qcom_jpeg_platform_driver = {
> +	.probe  = qcom_jpeg_probe,
> +	.driver = {
> +		.name = QCOM_JPEG_ENC_NAME,
> +		.of_match_table = qcom_jpeg_of_match,
> +		.pm             = pm_ptr(&qcom_jpeg_pm_ops),
> +	},
> +};

[Severity: High]
Can this lead to a memory leak and use-after-free on driver unbind?

The platform driver registers the video device via qcom_jpeg_v4l2_register()
in qcom_jpeg_probe(), but lacks a .remove hook to unregister it. When the
device is unbound, devres will automatically free the jenc structure, but
the video node will remain active and could be accessed by userspace.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_ops.c

[ ... ]

> +static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue *q,
> +			      struct vb2_buffer *vb)
> +{

[ ... ]

> +	dma += header_size;
> +	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);
> +
> +	jpeg_io_write(jenc, jpeg_v420_we_pntr[pln], dma);

[Severity: Critical]
Does this allow the hardware to write past the end of the allocated DMA
mapping?

The DMA start address is advanced by header_size here. However, in
jpeg_setup_we_size() below, the hardware limit is programmed using the full
original buffer capacity (dfmt->plane_fmt[0].sizeimage) instead of the
remaining space.

[ ... ]

> +static int jpeg_setup_we_size(struct jenc_context *ectx, struct qcom_jenc_queue *q)
> +{
> +	struct v4l2_pix_format_mplane *dfmt = &q->vf;
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	u8 pln;
> +
> +	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);

[ ... ]

> +static int jpeg_setup_we_params(struct jenc_context *ectx, struct qcom_jenc_queue *q)
> +{

[ ... ]

> +	dev_dbg(ectx->dev, "WE blk_w=%u blk_h=%u cols=%u rows=%u\n",
> +		blk_w, blk_h, mcu_cols, mcu_rows);
> +
> +	jpeg_rw_bits(jenc, JPEG_V420_REG_WE_BLOCKS_0,
> +		     JPEG_V420_PLN_WR_BLK_CFG_BLOCKS_PER_ROW_MSK, mcu_rows - 1);
> +	jpeg_rw_bits(jenc, JPEG_V420_REG_WE_BLOCKS_0,
> +		     JPEG_V420_PLN_WR_BLK_CFG_BLOCKS_PER_COL_MSK, mcu_cols - 1);

[Severity: High]
Are the horizontal and vertical block counts swapped here?

It appears BLOCKS_PER_ROW_MSK is being assigned the vertical block count
(mcu_rows), and BLOCKS_PER_COL_MSK is receiving the horizontal block count
(mcu_cols).

[ ... ]

> +static int jpeg_store_fe_next(struct jenc_context *ectx, struct vb2_buffer *vb2)
> +{
> +	struct qcom_jenc_queue *q = &ectx->bufq[TYPE2QID(vb2->type)];
> +	struct qcom_jpeg_buff *buff = &q->buff[vb2->index];
> +	u8 pln;
> +
> +	for (pln = 0; pln < q->vf.num_planes; pln++) {
> +		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);
> +		if (!buff->plns[pln].dma)
> +			return -EINVAL;

[Severity: High]
Will this silently cross into unmapped memory for buffers larger than 64KB?

The queue is initialized with vb2_dma_sg_memops, which can generate multiple
scatterlist segments. However, this only extracts the DMA address from the
first segment. Because dma_set_max_seg_size() is not called, large buffers
may map into fragmented IOVA chunks that the linear DMA engine cannot handle.

[ ... ]

> +static irqreturn_t op_jpeg_irq_bot(int irq, void *data)
> +{
> +	struct qcom_jenc_dev *jenc = 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);
> +		}
> +
> +		jpeg_stop(jenc);

[Severity: High]
Could this jpeg_stop() call break the next queued job?

The enc_hw_irq_cb() callback invokes v4l2_m2m_job_finish(), which can
immediately schedule and start the next job on another CPU. If that happens,
this subsequent jpeg_stop() will reset the hardware while the new job is
actively running.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c

[ ... ]

> +static int bop_jpeg_vb2_start_streaming(struct vb2_queue *q, unsigned int count)
> +{

[ ... ]

> +	if (!ectx->hw_acquired) {
> +		rc = hw->hw_acquire(ectx, q);
> +		if (!rc) {
> +			ectx->hw_acquired = true;
> +			hw->hw_get_cap(jenc, &hw_caps);
> +			dev_dbg(ectx->dev, "hw_caps=0x%x\n", hw_caps);
> +		}
> +	} else {
> +		rc = 0;
> +	}
> +
> +	mutex_unlock(&jenc->dev_mutex);
> +
> +	return rc;
> +}

[Severity: High]
If hardware acquisition fails, are the queued buffers leaked?

The videobuf2 framework requires drivers to return all queued buffers via
vb2_buffer_done(..., VB2_BUF_STATE_QUEUED) when start_streaming() returns
an error, but this error path directly returns the error code without draining
the m2m queues.

[ ... ]

> +static void bop_jpeg_vb2_stop_streaming(struct vb2_queue *q)
> +{
> +	struct jenc_context *ectx = vb2_get_drv_priv(q);
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	const struct qcom_jpeg_hw_ops *hw = jenc->res->hw_ops;
> +	bool do_release;
> +
> +	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 there a race condition here exposing hardware teardown to concurrent
contexts?

The dev_mutex is unlocked before calling hw_release(). Concurrently,
another stream could be executing hw_acquire() (which runs under dev_mutex)
and calling jpeg_init(), causing both contexts to write to hardware
registers simultaneously.

[ ... ]

> +static void mop_jpeg_m2m_job_abort(void *priv)
> +{
> +	struct jenc_context *ectx = priv;
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +
> +	mutex_lock(&jenc->dev_mutex);
> +
> +	jpeg_v4l2_work_stop(ectx, VB2_BUF_STATE_ERROR);
> +
> +	mutex_unlock(&jenc->dev_mutex);
> +}

[Severity: Critical]
Does this leave the hardware actively performing DMA into freed memory?

Active buffers are returned to userspace via jpeg_v4l2_work_stop(), but the
hardware engine is never halted. If another context keeps the reference
count above zero, the hardware continues its asynchronous transfers into
V4L2 buffers that userspace might have already freed.

[ ... ]

> +static int jpeg_v4l2_init_queue(void *priv, struct vb2_queue *sq, struct vb2_queue *dq)
> +{

[ ... ]

> +	dq->drv_priv		= ectx;
> +	dq->dev			= ectx->dev;
> +	dq->type		= V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE;
> +	dq->io_modes		= VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> +	dq->buf_struct_size	= sizeof(struct v4l2_m2m_buffer);
> +	dq->ops			= &qcom_jpeg_v4l2_vb2_ops;
> +	dq->mem_ops		= &vb2_dma_sg_memops;
> +	dq->timestamp_flags	= V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +	dq->lock		= &ectx->ctx_lock;
> +	dq->min_queued_buffers	= 1;
> +
> +	rc = vb2_queue_init(dq);

[Severity: High]
Will the bidirectional DMA sync used in jpeg_apply_we_addr() trigger a DMA API
violation?

The capture queue is initialized without setting dq->bidirectional = 1, so
it maps as DMA_FROM_DEVICE. Calling dma_sync_sgtable_for_cpu() or
dma_sync_sgtable_for_device() with DMA_BIDIRECTIONAL on a buffer mapped with
DMA_FROM_DEVICE is invalid and will fail.

[ ... ]

> +static int fop_jpeg_file_release(struct file *file)
> +{
> +	struct jenc_context *ectx = jpeg_file2ctx(file);
> +	struct v4l2_m2m_dev *m2m_dev = ectx->fh.m2m_ctx->m2m_dev;
> +	struct qcom_jenc_dev *jenc = ectx->jenc;
> +	unsigned long flags;
> +
> +	/*
> +	 * Ensure the threaded IRQ handler cannot dereference this context
> +	 * after it is freed.  Clear jenc->actx under hw_lock before
> +	 * cancelling work and releasing the m2m context.
> +	 */
> +	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: Critical]
Is there a missing synchronize_irq() call before freeing the context?

The actx pointer is cleared under a spinlock, but if the threaded IRQ
handler (op_jpeg_irq_bot) has already fetched jenc->actx, it will proceed to
dereference the ectx pointer after it has been freed by kfree(ectx) here.

[ ... ]

> +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]
Does this cause a double free or memory corruption?

The jenc structure is allocated using devm_kzalloc() in qcom_jpeg_probe(),
but this release callback explicitly calls kfree(jenc). Freeing a
devres-managed pointer with kfree() corrupts the managed allocation list.

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