Re: [PATCH v8 4/8] media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential
issues to consider:
- [High] media: qcom: jpeg: unverified contiguous DMA length in
  jpeg_store_fe/we_next()
- [High] media: qcom: jpeg: use-after-free of m2m_ctx in
  jpeg_finish_work()
- [Medium] media: qcom: jpeg: unadjusted WE_BSIZE_0 limit in
  jpeg_setup_we_size()
- [Medium] media: qcom: jpeg: unhandled IRQs unmasked in jpeg_init()
  risk lost IRQ
- [Medium] media: qcom: jpeg: non-exclusive IRQ handling in
  qcom_jenc_v420_irq_bot()
- [Medium] media: qcom: jpeg: disjoint vdev->lock and vq->lock leave
  ioctls unserialized
- [Medium] media: qcom: jpeg: missing job_abort in
  qcom_jpeg_v4l2_m2m_ops blocks streamoff
- [Medium] media: qcom: jpeg: use-after-free on unbind due to
  devm_kzalloc() of jenc
- [Medium] media: qcom: jpeg: broken V4L2_ENC_CMD_START resume and drain
  error handling
- [Low] media: qcom: jpeg: mismatched CAPTURE alignment in
  jpeg_v4l2_set_format()
--

commit 5296b93869b724f0ca5368bbad66d4fa21dc5933
Author: Atanas Filipov <atanas.filipov@oss.qualcomm.com>

media: qcom: jpeg: Add Qualcomm JPEG V4L2 encoder

This commit adds a V4L2 mem2mem JPEG encoder driver for Qualcomm SoCs
targeting the SM8250 and SC7280 platforms.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
> new file mode 100644
> index 0000000000000..dd2200665a1c3
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_dev.c
[ ... ]
> +static int qcom_jpeg_probe(struct platform_device *pdev)
> +{
> +	struct qcom_jenc_dev *jenc;
> +	int rc;
> +
> +	jenc = devm_kzalloc(&pdev->dev, sizeof(*jenc), GFP_KERNEL);
> +	if (!jenc)
> +		return -ENOMEM;
> +
> +	jenc->dev = &pdev->dev;
> +	platform_set_drvdata(pdev, jenc);
> +
> +	rc = devm_mutex_init(&pdev->dev, &jenc->dev_mutex);
> +	if (rc)
> +		goto err_free_jenc;
> +	rc = devm_mutex_init(&pdev->dev, &jenc->hw_ref_lock);
> +	if (rc)
> +		goto err_free_jenc;

[Severity: Medium]
Can unbinding the device while a /dev/videoX file descriptor is open cause a
use-after-free of jenc when that file descriptor is later closed?

Because jenc and its mutexes are managed by devres, unbinding the platform
driver destroys jenc->dev_mutex and jenc->hw_ref_lock, unmaps jenc->jpeg_base,
and frees jenc immediately.  When userspace subsequently closes the open file
descriptor, fop_jpeg_file_release() (and bop_jpeg_vb2_stop_streaming() if the
context was streaming) dereferences ectx->jenc, locks the destroyed mutexes,
and v4l2_release() -> video_put() accesses &jenc->v4l2_dev inside the freed
struct.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c
> new file mode 100644
> index 0000000000000..9e1933957d101
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c
[ ... ]
> +static int jpeg_init(struct qcom_jenc_dev *jenc)
> +{
[ ... ]
> +	jpeg_clear_planes_queues(jenc);
> +
> +	jpeg_io_write(jenc, JPEG_V420_REG_INT_CLR, JPEG_V420_IRQ_STATUS_ALL_BITS);
> +	jpeg_io_write(jenc, JPEG_V420_REG_INT_MASK, JPEG_V420_IRQ_STATUS_ALL_BITS);
> +
> +	return 0;
> +}

[Severity: Medium]
Does unmasking JPEG_V420_IRQ_STATUS_ALL_BITS here risk losing the
JPEG_V420_IRQ_STATUS_SESSION_DONE interrupt or overwriting
jenc->pending_irq_status?

JPEG_V420_IRQ_STATUS_ALL_BITS enables RD_BUF_PLNS_DONE_MSK, WR_BUF_PLN0_DONE,
RD_BUF_PLNS_REQ_ATTN_MSK, and WR_BUF_PLNS_REQ_ATTN_MSK, even though
qcom_jenc_v420_irq_bot() only handles SESSION_DONE and SESION_ERROR_MSK.  In
qcom_jenc_v420_irq_top():

drivers/media/platform/qcom/jpeg/qcom_jenc_v420.c:qcom_jenc_v420_irq_top() {
	irq_status = jpeg_io_read(jenc, JPEG_V420_REG_INT_STATUS);
	jpeg_io_write(jenc, JPEG_V420_REG_INT_CLR, irq_status & JPEG_V420_IRQ_STATUS_ALL_BITS);
    ...
	irq_status &= ~(JPEG_V420_IRQ_STATUS_RESET_ACK | JPEG_V420_IRQ_STATUS_STOP_ACK);
	if (!irq_status) {
		spin_unlock_irqrestore(&jenc->hw_lock, flags);
		return IRQ_HANDLED;
	}

	WRITE_ONCE(jenc->pending_irq_status, irq_status);
}

If a plane-done interrupt fires at the end of a frame and SESSION_DONE asserts
between reading JPEG_V420_REG_INT_STATUS and writing JPEG_V420_REG_INT_CLR,
only the plane-done bit is cleared while SESSION_DONE stays asserted without
generating a new rising edge, wedging the active m2m job.  Even if two
interrupts are delivered, a second unhandled interrupt can overwrite
jenc->pending_irq_status via WRITE_ONCE() before the threaded handler runs.
Only unmasking the interrupt bits the driver actually handles (and
accumulating pending_irq_status with atomic or spinlock-protected OR-and-swap)
would avoid this.

[ ... ]
> +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;
> +
> +		buff->plns[pln].size = vb2_plane_size(vb2, pln);
> +		if (!buff->plns[pln].size)
> +			return -EINVAL;
> +	}
> +
> +	return 0;
> +}

[Severity: High]
Can jpeg_store_fe_next() and jpeg_store_we_next() cause out-of-bounds DMA when
a buffer's scatterlist is mapped into multiple segments?

Both functions take only sg_dma_address(sgt->sgl) and program the hardware to
DMA the entire plane linearly from that address, without checking that
sg_dma_len(sgt->sgl) covers vb2_plane_size(vb2, pln).  Because the queues use
vb2_dma_sg_memops with VB2_USERPTR and VB2_DMABUF enabled, an unaligned
USERPTR (which falls back to per-entry swiotlb bounce mappings on arm64) or a
multi-segment DMABUF produces sg_dma_len(sgt->sgl) < vb2_plane_size(vb2, pln),
so the hardware reads or writes past the end of the first IOVA segment.
Should the driver use vb2_dma_contig_memops instead, or verify that
sg_dma_len(sgt->sgl) >= buff->plns[pln].size?

[ ... ]
> +static int jpeg_apply_we_addr(struct jenc_context *ectx, struct qcom_jenc_queue *q,
> +			      struct vb2_buffer *vb)
> +{
[ ... ]
> +	header_size = ectx->hdr_cache.layout.size;
> +	if (vb->planes[0].length < header_size)
> +		return -EINVAL;
> +
> +	memcpy(mptr, ectx->hdr_cache.data, header_size);
> +	v4l2_jpeg_hdr_set_size(mptr, &ectx->hdr_cache.layout, q->vf.width, q->vf.height);
> +
> +	dma += header_size;
> +
> +	/*
> +	 * 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);
[ ... ]
> +static void 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_MSK, 0);
> +
> +	jpeg_io_write(jenc, JPEG_V420_REG_WE_BSIZE_0, dfmt->plane_fmt[0].sizeimage);
> +
> +	dev_dbg(ectx->dev, "WE size=%u\n", dfmt->plane_fmt[0].sizeimage);
> +}

[Severity: Medium]
Should JPEG_V420_REG_WE_BSIZE_0 be reduced by header_size?

jpeg_apply_we_addr() advances the Write Engine base pointer
JPEG_V420_REG_WE_PNTR_0 by header_size bytes into the capture buffer, but
jpeg_setup_we_size() programs JPEG_V420_REG_WE_BSIZE_0 with the full
dfmt->plane_fmt[0].sizeimage.  If an encoded frame exceeds
sizeimage - header_size, the hardware can DMA up to header_size bytes past the
end of a sizeimage-sized buffer, and qcom_jenc_v420_irq_bot() will pass
out_size + ctx->hdr_cache.layout.size > vb2_plane_size() to
vb2_set_plane_payload().

[ ... ]
> +irqreturn_t qcom_jenc_v420_irq_bot(int irq, void *data)
> +{
> +	struct qcom_jenc_dev *jenc = data;
> +	unsigned long flags;
> +	u32 irq_status;
> +
> +	irq_status = READ_ONCE(jenc->pending_irq_status);
> +
> +	if (FIELD_GET(JPEG_V420_IRQ_STATUS_SESSION_DONE, irq_status)) {
> +		struct jenc_context *ctx;
> +
> +		spin_lock_irqsave(&jenc->hw_lock, flags);
> +		ctx = jenc->actx;
> +		jenc->actx = NULL;
> +		spin_unlock_irqrestore(&jenc->hw_lock, flags);
> +
> +		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);
> +
> +			/*
> +			 * Stop the hardware before enc_hw_irq_cb(): it leads to
> +			 * v4l2_m2m_job_finish(), which can start the next job's device_run().
> +			 */
> +			jpeg_stop(jenc);
> +			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_DONE,
> +					    out_size + ctx->hdr_cache.layout.size);
> +		} else {
> +			jpeg_stop(jenc);
> +		}
> +	}
> +
> +	if (FIELD_GET(JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK, irq_status)) {
> +		struct jenc_context *ctx;
> +
> +		spin_lock_irqsave(&jenc->hw_lock, flags);
> +		ctx = jenc->actx;
> +		jenc->actx = NULL;
> +		spin_unlock_irqrestore(&jenc->hw_lock, flags);
> +
> +		dev_err(jenc->dev, "encoder hardware failure=0x%lx\n",
> +			FIELD_GET(JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK, irq_status));
> +
> +		jpeg_stop(jenc);
> +		if (ctx)
> +			jenc->enc_hw_irq_cb(ctx, VB2_BUF_STATE_ERROR, 0);
> +	}
> +
> +	return IRQ_HANDLED;
> +}

[Severity: Medium]
What happens if both JPEG_V420_IRQ_STATUS_SESSION_DONE and
JPEG_V420_IRQ_STATUS_SESION_ERROR_MSK are set in irq_status?

CPU0 (irq_bot)                    CPU1 (system_wq)
if (SESSION_DONE)
  ctx = jenc->actx;
  jenc->actx = NULL;
  jpeg_stop(jenc);
  enc_hw_irq_cb(..., DONE)
    schedule_work(&finish_work)
                                  jpeg_finish_work()
                                    v4l2_m2m_job_finish()
                                      mop_jpeg_m2m_job_run()
                                        jenc->actx = next_ctx;
                                        jpeg_exec(jenc);
if (SESION_ERROR_MSK)
  ctx = jenc->actx;
  jenc->actx = NULL;
  jpeg_stop(jenc);  <-- stops newly started job
  enc_hw_irq_cb(ctx, ERROR)

Because the two checks are non-exclusive and run outside jenc->dev_mutex, the
first block completes the failed frame with VB2_BUF_STATE_DONE and schedules
finish_work, allowing the next job to start on another CPU before the second
block pops the new jenc->actx, stops the hardware, and aborts the new job.
Checking SESION_ERROR_MSK first and using else if for SESSION_DONE avoids both
reporting a failed job as done and clobbering the next job.

> diff --git a/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
> new file mode 100644
> index 0000000000000..380008cfcb442
> --- /dev/null
> +++ b/drivers/media/platform/qcom/jpeg/qcom_jenc_v4l2.c
[ ... ]
> +static void jpeg_update_dst_plane(const struct jenc_enc_format *ef, struct v4l2_format *v4f)
> +{
> +	struct v4l2_pix_format_mplane *f = &v4f->fmt.pix_mp;
> +	u32 nbx, nby;
> +	u64 size;
> +
> +	f->pixelformat  = ef->fourcc;
> +	f->field	= V4L2_FIELD_NONE;
> +	f->colorspace	= V4L2_COLORSPACE_SRGB;
> +	f->xfer_func	= V4L2_MAP_XFER_FUNC_DEFAULT(f->colorspace);
> +	f->ycbcr_enc	= V4L2_MAP_YCBCR_ENC_DEFAULT(f->colorspace);
> +	f->quantization =
> +		V4L2_MAP_QUANTIZATION_DEFAULT(f->ycbcr_enc == V4L2_YCBCR_ENC_601,
> +					      f->colorspace, f->ycbcr_enc);
> +
> +	f->width  = clamp_t(u32, f->width,  QCOM_JPEG_HW_MIN_WIDTH,  QCOM_JPEG_HW_MAX_WIDTH);
> +	f->width  = ALIGN_DOWN(f->width, QCOM_JPEG_HW_DEF_HSTEP);
> +
> +	f->height = clamp_t(u32, f->height, QCOM_JPEG_HW_MIN_HEIGHT, QCOM_JPEG_HW_MAX_HEIGHT);
[ ... ]
> +static int jpeg_v4l2_set_format(struct jenc_context *ectx, struct v4l2_format *f)
> +{
[ ... ]
> +	if (V4L2_TYPE_IS_CAPTURE(f->type)) {
> +		/* Adjust source size to match capture size */
> +		if (pm->width != sq->vf.width || pm->height != sq->vf.height) {
> +			struct v4l2_format nf = {0};
> +
> +			nf.type			  = jpeg_src_formats->type;
> +			nf.fmt.pix_mp.pixelformat = sq->vf.pixelformat;
> +			nf.fmt.pix_mp.width	  = pm->width;
> +			nf.fmt.pix_mp.height	  = pm->height;
> +
> +			rc = jpeg_v4l2_try_format(ectx, &nf);
> +			if (rc)
> +				return rc;
> +
> +			sq->vf = nf.fmt.pix_mp;
> +
> +			/*
> +			 * OUTPUT TRY_FMT may crop the width to the block size; keep the
> +			 * CAPTURE size equal to the negotiated OUTPUT size.
> +			 */
> +			if (pm->width != nf.fmt.pix_mp.width ||
> +			    pm->height != nf.fmt.pix_mp.height) {
> +				pm->width  = nf.fmt.pix_mp.width;
> +				pm->height = nf.fmt.pix_mp.height;
> +			}
> +		}

[Severity: Low]
Does VIDIOC_S_FMT on the CAPTURE queue leave pm->plane_fmt[0].sizeimage stale
and disagree with VIDIOC_TRY_FMT when the active OUTPUT format is NV12M or
NV21M?

jpeg_update_dst_plane() aligns CAPTURE dimensions to 8x1, whereas
jpeg_update_src_planes() aligns NV12M/NV21M width down to 16 and rounds height
up to 2.  When jpeg_v4l2_set_format() is called on CAPTURE with dimensions
like 1928x1089, the initial jpeg_v4l2_try_format(ectx, f) computes sizeimage
for 1928x1089, and then pm->width and pm->height are overwritten with the
OUTPUT-aligned 1920x1090 without recalculating pm->plane_fmt[0].sizeimage
(and VIDIOC_TRY_FMT on CAPTURE returns 1928x1089 instead of 1920x1090).

[ ... ]
> +static void jpeg_v4l2_return_bufs(struct jenc_context *ctx, enum vb2_buffer_state buff_state)
> +{
> +	bool draining = ctx->fh.m2m_ctx->is_draining;
> +	struct vb2_v4l2_buffer *vb;
> +
> +	/* 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 (draining && !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);
> +}
[ ... ]
> +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;
> +	bool do_release;
> +
> +	mutex_lock(&jenc->dev_mutex);
> +
> +	do_release = ectx->hw_acquired;
> +	ectx->hw_acquired = false;
> +
> +	mutex_unlock(&jenc->dev_mutex);
> +
> +	/*
> +	 * qcom_jenc_v420_release() calls jpeg_deinit() which issues HW_STOP and
> +	 * waits for the STOP_ACK interrupt completion. It runs before any buffer
> +	 * is returned below and outside dev_mutex: the threaded IRQ handler takes
> +	 * dev_mutex, and with IRQF_ONESHOT the STOP_ACK interrupt stays masked
> +	 * until that handler returns.
> +	 */
> +	if (do_release)
> +		qcom_jenc_v420_release(ectx);
[ ... ]
> +static const struct v4l2_m2m_ops qcom_jpeg_v4l2_m2m_ops = {
> +	.device_run	= mop_jpeg_m2m_job_run,
> +};

[Severity: Medium]
Without a .job_abort callback in qcom_jpeg_v4l2_m2m_ops, can VIDIOC_STREAMOFF
or close() ever reach bop_jpeg_vb2_stop_streaming() while a job is running?

Both v4l2_m2m_streamoff() and v4l2_m2m_ctx_release() call
v4l2_m2m_cancel_job() before invoking vb2_streamoff() or vb2_queue_release().
v4l2_m2m_cancel_job() calls m2m_ops->job_abort() if present and then waits
uninterruptibly for TRANS_RUNNING to clear, which only happens when
v4l2_m2m_job_finish() is called.  Because .job_abort is not implemented, the
HW_STOP abort path in bop_jpeg_vb2_stop_streaming() ->
qcom_jenc_v420_release() is unreachable while a job is active, and any stalled
encode job (such as on an unhandled attention interrupt) will hang the calling
task indefinitely in v4l2_m2m_cancel_job().

[ ... ]
> +static int iop_jpeg_encoder_command(struct file *file, void *priv, struct v4l2_encoder_cmd *ec)
> +{
> +	struct jenc_context *ectx = jpeg_file2ctx(file);
> +	struct vb2_queue *vq;
> +
> +	if (ec->cmd == V4L2_ENC_CMD_STOP) {
> +		vq = v4l2_m2m_get_src_vq(ectx->fh.m2m_ctx);
> +		if (!vb2_is_streaming(vq))
> +			return 0;
> +
> +		vq = v4l2_m2m_get_dst_vq(ectx->fh.m2m_ctx);
> +		if (!vb2_is_streaming(vq))
> +			return 0;
> +	}
> +
> +	return v4l2_m2m_ioctl_encoder_cmd(file, priv, ec);
> +}

[Severity: Medium]
Does V4L2_ENC_CMD_START after draining leave the CAPTURE queue stuck returning
-EPIPE on VIDIOC_DQBUF?

When draining finishes and userspace dequeues the V4L2_BUF_FLAG_LAST buffer,
vb2_dqbuf() sets dst_vq->last_buffer_dequeued = true.  On V4L2_ENC_CMD_START,
v4l2_m2m_ioctl_encoder_cmd() clears m2m_ctx->has_stopped, but nothing calls
vb2_clear_last_buffer_dequeued(dst_vq) or v4l2_m2m_try_schedule().
iop_jpeg_encoder_command() also skips v4l2_m2m_ioctl_try_encoder_cmd(), leaving
ec->flags unvalidated and unzeroed.  In addition, jpeg_v4l2_return_bufs() is
only called on job error (VB2_BUF_STATE_ERROR), yet it drains all queued
buffers instead of only the failed job's pair and calls
v4l2_m2m_last_buffer_done() when draining, which marks the last CAPTURE buffer
VB2_BUF_STATE_DONE instead of VB2_BUF_STATE_ERROR.

[ ... ]
> +static int jpeg_v4l2_init_queue(void *priv, struct vb2_queue *sq, struct vb2_queue *dq)
> +{
> +	struct jenc_context *ectx = priv;
> +	int rc;
> +
> +	sq->drv_priv		= ectx;
> +	sq->dev			= ectx->dev;
> +	sq->type		= V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE;
> +	sq->io_modes		= VB2_MMAP | VB2_DMABUF | VB2_USERPTR;
> +	sq->buf_struct_size	= sizeof(struct v4l2_m2m_buffer);
> +	sq->ops			= &qcom_jpeg_v4l2_vb2_ops;
> +	sq->mem_ops		= &vb2_dma_sg_memops;
> +	sq->timestamp_flags	= V4L2_BUF_FLAG_TIMESTAMP_COPY;
> +	sq->lock		= &ectx->ctx_lock;
> +	sq->min_queued_buffers	= 1;
[ ... ]
> +	dq->lock		= &ectx->ctx_lock;

[Severity: Medium]
Aren't queue ioctls and format/encoder_cmd ioctls serialized by two different
mutexes?

jpeg_v4l2_init_queue() sets sq->lock and dq->lock (and thus m2m_ctx->q_lock)
to &ectx->ctx_lock, whereas qcom_jpeg_v4l2_register() sets jenc->vdev->lock to
&jenc->dev_mutex, and nothing in the driver ever takes ectx->ctx_lock
explicitly.  In v4l2_ioctl_get_lock(), ioctls with INFO_FL_QUEUE (REQBUFS,
CREATE_BUFS, PREPARE_BUF, QBUF, DQBUF, STREAMON, STREAMOFF) lock
m2m_ctx->q_lock (&ectx->ctx_lock), while VIDIOC_S_FMT, VIDIOC_G_FMT, and
VIDIOC_ENCODER_CMD fall back to vdev->lock (&jenc->dev_mutex).  As a result,
jpeg_v4l2_set_format() can mutate sq->vf, dq->vf, and ectx->hdr_cache while
bop_jpeg_vb2_queue_setup() or bop_jpeg_vb2_buf_prepare() is running, and
iop_jpeg_encoder_command() -> v4l2_m2m_ioctl_encoder_cmd() can race with
VIDIOC_QBUF and VIDIOC_STREAMON/OFF on m2m_ctx draining state and rdy_queue.

[ ... ]
> +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;
> +
> +	/*
> +	 * Waits until a running job of this context completes: its
> +	 * SESSION_DONE interrupt needs jenc->actx, and finish_work must not
> +	 * be cancelled before, as it calls v4l2_m2m_job_finish().
> +	 */
> +	v4l2_m2m_ctx_release(ectx->fh.m2m_ctx);
> +
> +	/*
> +	 * Ensure the threaded IRQ handler cannot dereference this context
> +	 * after it is freed.  Clear jenc->actx under hw_lock before
> +	 * cancelling work.
> +	 */
> +	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->finish_work);

[Severity: High]
Can jpeg_finish_work() dereference ectx->fh.m2m_ctx after
v4l2_m2m_ctx_release() frees it?

CPU0                              CPU1 (system_wq)
fop_jpeg_file_release()           jpeg_finish_work()
  v4l2_m2m_ctx_release(m2m_ctx)     v4l2_m2m_job_finish(..., m2m_ctx)
    v4l2_m2m_cancel_job()             _v4l2_m2m_job_finish()
      wait_event(!(TRANS_RUNNING))      job_flags &= ~TRANS_RUNNING;
                                        wake_up(&m2m_ctx->finished);
    kfree(m2m_ctx);
                                      v4l2_m2m_schedule_next_job()
                                        __v4l2_m2m_try_queue(m2m_ctx)
                                          <-- use-after-free
  cancel_work_sync(&finish_work);

v4l2_m2m_cancel_job() only waits for TRANS_RUNNING to be cleared (or returns
immediately if a prior VIDIOC_STREAMOFF already cleared job_flags while
finish_work was still queued on system_wq), so v4l2_m2m_ctx_release() frees
m2m_ctx before cancel_work_sync(&ectx->finish_work) runs.  Calling
flush_work(&ectx->finish_work) (or invoking v4l2_m2m_job_finish() directly
from the threaded IRQ callback instead of deferring to a workqueue) before
v4l2_m2m_ctx_release() would prevent this.

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