Re: [PATCH v7 12/19] media: meson: vdec: Coordinate m2m task execution inside async loop

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:

New issues:
- [Critical] An AB-BA deadlock occurs between the V4L2 `VIDIOC_STREAMOFF` ioctl and the workqueue worker `esparser_queue_all_src` over `sess->lock`.
- [Critical] A recursive mutex self-deadlock occurs because `core->lock` is manually acquired inside functions already protected by the V4L2 core.
- [High] The `esparser_queue_all_src` workqueue thread enters a 100% CPU infinite busy-loop on empty queues, end-of-stream buffers, or hardware FIFO full conditions.
- [High] The V4L2 M2M framework permanently stalls due to a leaked job context when `esparser_queue_all_src` exits early or hits loop break conditions without calling `v4l2_m2m_job_finish()`.
- [High] Closing any secondary file descriptor unconditionally clears `core->cur_sess`, stripping hardware ownership from the active session and causing permanent DMA memory leaks.

Pre-existing issues:
- [Critical] An unconditional out-of-bounds heap write occurs when padding artificially small, user-controlled buffers in `esparser_pad_start_code()`.
- [Critical] An integer overflow during VP9 sub-frame length parsing allows attackers to bypass size boundaries and trigger an out-of-bounds `memmove` copy.
--

--- Patch [12]: [PATCH v7 12/19] media: meson: vdec: Coordinate m2m task execution inside async loop ---
commit 8f8ffb4492573d6d82ee209ecb382a9f5fbed9e4
Author: Anand Moon <[email protected]>

media: meson: vdec: Coordinate m2m task execution inside async loop

This commit introduces proper V4L2 Memory-to-Memory (m2m) scheduler pipeline 
tracking by handling job finalisation inside the asynchronous esparser 
workqueue. It defers the scheduling call to v4l2_m2m_job_finish() to execute 
within the worker routine.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index b9f36fef4be12..939b239c2af47 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
[ ... ]
> @@ -437,6 +438,9 @@ void esparser_queue_all_src(struct work_struct *work)
>  			else
>  				v4l2_m2m_buf_done(vbuf, VB2_BUF_STATE_DONE);
>  
> +			/* Safely notify the V4L2 core sub-framework */
> +			v4l2_m2m_job_finish(core->m2m_dev, sess->m2m_ctx);
> +
>  			/* Set tracking flag indicating transaction completion */
>  			processed_frame = true;
>  		}

[Severity: High]
Does this worker thread run into an infinite busy-loop on empty queues or 
end-of-stream buffers?

In esparser_queue_all_src(), the code uses a scoped_guard for the lock:

	while (1) {
		...
		scoped_guard(mutex, &sess->lock) {
			if (!vbuf)
				break;
		}
	}

Since scoped_guard expands into a hidden for-loop, the break statement only 
exits the scoped_guard's loop, not the outer while (1) loop. This could trap 
execution indefinitely, consuming 100% CPU when the queue is empty, hits an 
EOS buffer, or the hardware FIFO is full.

[Severity: High]
Could the V4L2 M2M job context permanently leak if the session shuts down?

Looking at esparser_queue_all_src():

	scoped_guard(mutex, &sess->lock) {
		/* Safe atomic tracking check: exit loop if session is shutting down */
		if (READ_ONCE(sess->should_stop))
			return;

Returning early directly bypasses the newly added v4l2_m2m_job_finish() call.
This could permanently hang the V4L2 memory-to-memory framework because it 
considers the device busy forever, preventing further decoding jobs.

[Severity: Critical]
Is there a risk of an AB-BA deadlock between the V4L2 VIDIOC_STREAMOFF ioctl 
and the workqueue worker?

The V4L2 core framework acquires sess->lock during VIDIOC_STREAMOFF and then 
calls vdec_stop_streaming(), which waits for the active worker thread:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
	...
	cancel_work_sync(&sess->esparser_queue_work);
}

Concurrently, the worker thread esparser_queue_all_src spins up and blocks 
attempting to acquire the identical sess->lock:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
	...
	scoped_guard(mutex, &sess->lock) {
}

This sequence would entirely deadlock the system when stopping the video stream.

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 0eb39aa6014ee..b3e1d99e8889f 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -149,6 +149,17 @@ vdec_queue_recycle(struct amvdec_session *sess, struct vb2_buffer *vb)
>  static void vdec_m2m_device_run(void *priv)
>  {
>  	struct amvdec_session *sess = priv;
> +	struct amvdec_core *core = sess->core;
> +
> +	if (READ_ONCE(sess->should_stop)) {
> +		v4l2_m2m_job_finish(core->m2m_dev, sess->m2m_ctx);
> +		return;
> +	}
> +
> +	mutex_lock(&core->lock);
> +	if (!core->cur_sess)
> +		core->cur_sess = sess;
> +	mutex_unlock(&core->lock);
>  
>  	schedule_work(&sess->esparser_queue_work);
>  }

[Severity: Critical]
Will acquiring core->lock manually here and in vdec_start_streaming result 
in a recursive mutex self-deadlock?

Because vdev->queue is intentionally left NULL for M2M devices, the V4L2 core 
ioctl wrapper unconditionally acquires vdev->lock (which is mapped to 
&core->lock) before delegating execution to the ioctl implementations.

The underlying driver callback vdec_start_streaming then attempts to manually 
acquire it again:

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
	...
	mutex_lock(&core->lock);
	if (core->cur_sess && core->cur_sess != sess) {
}

This would immediately self-deadlock the kernel thread processing 
VIDIOC_STREAMON or VIDIOC_STREAMOFF.

[Severity: High]
Does closing a secondary file descriptor unintentionally strip hardware 
ownership from the active session?

In vdec_close(), the state modification is placed outside the active session 
check block:

drivers/staging/media/meson/vdec/vdec.c:vdec_close() {
	...
	/* Safely clear hardware ownership since we were confirmed as the owner */
	smp_store_release(&core->cur_sess, NULL);
}

If a user-space process opens the video node twice, streams on the first 
session, and then closes the second session, this clears core->cur_sess 
unconditionally. This destroys the ownership state of the active session, 
bypassing its cleanup sequence and permanently leaking DMA buffers and canvases.

[Severity: Critical]
This is a pre-existing issue, but can esparser_pad_start_code() cause an 
out-of-bounds heap write when handling artificially small buffers?

If userspace negotiates a small sizeimage and queues a tiny buffer, the padding 
function computes pad_size based on ESPARSER_MIN_PACKET_SIZE and executes 
a memset before verifying the vb2_plane_size:

drivers/staging/media/meson/vdec/esparser.c:esparser_pad_start_code() {
	...
		pad_size = ESPARSER_MIN_PACKET_SIZE - payload_size;
		memset(vaddr + payload_size, 0, pad_size);
	}

	if ((payload_size + pad_size + SEARCH_PATTERN_LEN) >
						vb2_plane_size(vb, 0)) {
}

This could allow a local user to overwrite memory with zeroes by passing a 
payload_size of 0, resulting in a 4096-byte memset.

[Severity: Critical]
This is a pre-existing issue, but might an integer overflow in 
vp9_update_header() allow attackers to trigger an out-of-bounds memmove?

The parser reconstructs frame lengths by bit-shifting byte payloads:

drivers/staging/media/meson/vdec/esparser.c:vp9_update_header() {
	...
				frame_size[cur_frame] |=
					(dp[mag_ptr] << (cur_mag * 8));
}

If dp[mag_ptr] is >= 128 and cur_mag is 3, the unsigned 8-bit value promotes 
to an int and shifts into the sign bit, resulting in a large negative integer.

Later, this negative frame_size is passed to memmove():

drivers/staging/media/meson/vdec/esparser.c:vp9_update_header() {
	...
		memmove(fdata + VP9_HEADER_SIZE, old_framedata, framesize);
}

The negative framesize implicitly converts to a massive size_t, which could 
corrupt arbitrary kernel memory.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=12

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic