[PATCH v7 10/19] media: meson: vdec: Fix race conditions and leaks in esparser pipeline

[sashiko-bot at kernel.org]

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

New issues:
- [High] Infinite loop and CPU soft-lockup in `esparser_queue_all_src` due to inverted `processed_frame` logic.
- [High] Self-deadlock in `vdec_start_streaming` and `vdec_stop_streaming` due to recursive acquisition of `core->lock`.
- [High] AB-BA deadlock in `vdec_stop_streaming` caused by calling `cancel_work_sync` while holding `sess->lock`.
- [High] AB-BA lock inversion deadlock between `core->lock` and `sess->lock` in the esparser workqueue.
- [High] System-wide V4L2 stall due to sleeping for up to 200ms while holding the driver-wide `core->lock`.
- [High] Silent omission of start code padding for unmapped buffers leads to hardware out-of-bounds DMA.

Pre-existing issues:
- [Critical] Integer overflow in `vp9_update_header` allows massive out-of-bounds heap corruption from untrusted payload data.
- [High] `esparser_pad_start_code` executes an out-of-bounds memory write before evaluating bounds checks.
--

--- Patch [10]: [PATCH v7 10/19] media: meson: vdec: Fix race conditions and leaks in esparser pipeline ---
commit f978165e2ef967ec3f207c80310d474efee3c9e8
Author: Anand Moon <linux.amoon at gmail.com>
Subject: media: meson: vdec: Fix race conditions and leaks in esparser pipeline

This commit fixes a circular locking dependency and potential race conditions
within the asynchronous esparser queue. It moves hardware ownership validation
inside the core->lock mutex region and properly cleans up timestamp trackers.
The VP9 payload evaluation logic is also refined.

> diff --git a/drivers/staging/media/meson/vdec/esparser.c b/drivers/staging/media/meson/vdec/esparser.c
> index edbfc829e2da8..b9f36fef4be12 100644
> --- a/drivers/staging/media/meson/vdec/esparser.c
> +++ b/drivers/staging/media/meson/vdec/esparser.c
> @@ -199,12 +199,16 @@ static int vp9_update_header(struct amvdec_core *core, struct vb2_buffer *buf)
>   * the ESPARSER interrupt.
>   */
>  static u32 esparser_pad_start_code(struct amvdec_core *core,
> +				   struct amvdec_session *sess,
>  				   struct vb2_buffer *vb,
>  				   u32 payload_size)
>  {
>  	u32 pad_size = 0;
>  	u8 *vaddr = vb2_plane_vaddr(vb, 0);
>  
> +	if (!sess || READ_ONCE(sess->should_stop) || !sess->priv || !vaddr)
> +		return 0;

[Severity: High]
Does returning 0 here skip the start code injection and cause an
out-of-bounds DMA scan?

By bypassing the injection of the SEARCH_PATTERN start code, the caller
esparser_queue() might interpret the 0 as success and instruct the DMA
engine to fetch the buffer. 

Since the search pattern is missing, could the hardware parser scan
indefinitely past the valid buffer into adjacent physical memory, resulting
in an out-of-bounds DMA read?

>  	if (payload_size < ESPARSER_MIN_PACKET_SIZE) {
>  		pad_size = ESPARSER_MIN_PACKET_SIZE - payload_size;
>  		memset(vaddr + payload_size, 0, pad_size);
>  	}

[Severity: High]
This is a pre-existing issue, but does this memset() write out of bounds?

If a small buffer is queued, this calculation executes before the
overall bounds check is evaluated. 

Could this overwrite adjacent kernel memory with zeroes before validating 
the size?

[ ... ]

> @@ -349,24 +356,38 @@ esparser_queue(struct amvdec_session *sess, struct vb2_v4l2_buffer *vbuf)
>  	vbuf->sequence = sess->sequence_out++;
>  
>  	if (sess->fmt_out->pixfmt == V4L2_PIX_FMT_VP9) {
> -		payload_size = vp9_update_header(core, vb);
> +		int res = vp9_update_header(core, vb);

[Severity: Critical]
This is a pre-existing issue, but does vp9_update_header() have an integer
overflow?

By extracting untrusted payload bytes and shifting them left up to 24 bits
into a signed integer, can malicious payloads construct negative frame sizes?

If a highly negative signed integer is produced and passed to memmove() as an
unsigned size_t, could this cause massive out-of-bounds reads and writes on
the kernel heap or DMA memory?

>  
> -		if (payload_size == 0) {
> -			dev_err(core->dev, "esparser: VP9 header update failed\n");
> +		if (res <= 0) {
> +			dev_err(core->dev,
> +				"esparser: VP9 header update failed (%d)\n",
> +				res);
>  			amvdec_remove_ts(sess, vb->timestamp);
>  			return -EBADMSG;
>  		}
> +		payload_size = res;
> +	}
> +
> +	pad_size = esparser_pad_start_code(core, sess, vb, payload_size);
> +
> +	/* Protect hardware register writes under core->lock */
> +	mutex_lock(&core->lock);

[Severity: High]
Does this new mutex_lock() introduce an AB-BA lock inversion?

The background workqueue thread esparser_queue_all_src() acquires sess->lock 
first (via scoped_guard) and then calls esparser_queue(). 

By adding mutex_lock(&core->lock) here, could this conflict with V4L2 ioctls
that implicitly acquire core->lock before sess->lock, resulting in an AB-BA
deadlock?

> +	if (core->cur_sess != sess || READ_ONCE(sess->should_stop)) {
> +		mutex_unlock(&core->lock);
> +		amvdec_remove_ts(sess, vb->timestamp);
> +		return -ESHUTDOWN;
>  	}
>  
> -	pad_size = esparser_pad_start_code(core, vb, payload_size);
>  	ret = esparser_write_data(core, phy, payload_size + pad_size);

[Severity: High]
Could sleeping inside esparser_write_data() block all other V4L2 ioctls?

Because core->lock is mapped to vdev->lock, sleeping via 
wait_event_interruptible_timeout() while waiting for the hardware interrupt 
blocks all other V4L2 ioctls across the entire device for the duration of 
the fetch. 

Can this severely degrade system performance?

>  
>  	if (ret <= 0) {
>  		dev_warn(core->dev, "esparser: input parsing error\n");
>  		amvdec_remove_ts(sess, vb->timestamp);
>  		amvdec_write_parser(core, PARSER_FETCH_CMD, 0);
> +		mutex_unlock(&core->lock);
>  		return -EIO;
>  	}
> +	mutex_unlock(&core->lock);
>  
>  	atomic_inc(&sess->esparser_queued_bufs);
>  

I also have some questions about related code paths outside the diff:

[Severity: High]
Is there an infinite loop in esparser_queue_all_src()?

When looking at the worker thread:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
    ...
		bool processed_frame = false;
        ...
		if (processed_frame)
			break;
    ...
}

Because the break statements inside the scoped_guard macro only exit the 
internal macro loop, processed_frame remains false when the queue is empty. 

Does the inverted logic cause the thread to bypass the outer break and loop 
infinitely, soft-locking the CPU?

[Severity: High]
Do vdec_start_streaming() and vdec_stop_streaming() self-deadlock?

In the current code:

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

Because the device's vdev->lock is mapped directly to &core->lock, the V4L2 
core framework automatically acquires it before dispatching standard ioctls. 

Does explicitly calling mutex_lock(&core->lock) inside these callbacks 
result in a permanent self-deadlock?

[Severity: High]
Is there an AB-BA deadlock during stream teardown?

During streamoff:

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

The ioctl thread acquires sess->lock before invoking this callback. 
Concurrently, the background workqueue thread esparser_queue_all_src() 
blocks waiting for sess->lock. 

Does calling cancel_work_sync() here cause the ioctl thread to block forever 
waiting for the worker thread, resulting in an AB-BA deadlock?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713120840.17427-1-linux.amoon@gmail.com?part=10