Re: [PATCH v7 02/19] media: meson: vdec: Fix STREAMON / STREAMOFF race conditions and session teardown

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

New issues:
- [High] The vdec_recycle_thread is conditionally skipped from being stopped but its resources are unconditionally freed during a partial STREAMOFF, leading to Use-After-Free and list corruption.
- [High] Failing start_streaming with -EBUSY permanently leaks streamon flags, causing state inconsistency and cross-session hardware corruption.

Pre-existing issues:
- [High] The return value of `kthread_run()` is not checked, leading to a kernel crash when `kthread_stop()` is called on an `ERR_PTR`.
- [High] vdec_reset_timestamps accesses the timestamps list without holding ts_spinlock, leading to list corruption.
- [High] Missing cancel_work_sync for esparser_queue_work causes UAF and unclocked register access.
--

--- Patch [2]: [PATCH v7 02/19] media: meson: vdec: Fix STREAMON / STREAMOFF race conditions and session teardown ---
commit c8f8b26e4a120933b5b1d329b3cc6e97ea21778d
Author: Anand Moon <[email protected]>
Subject: media: meson: vdec: Fix STREAMON / STREAMOFF race conditions and session teardown

This commit addresses race conditions during concurrent STREAMON / STREAMOFF
calls and dynamic resolution changes. It restructures initialization paths,
error handling, and state tracking to prevent hardware state corruption.

Closes: https://lore.kernel.org/all/[email protected]/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c

[ ... ]

> @@ -308,9 +303,29 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	}
>  
>  	if (sess->status == STATUS_RUNNING ||
> -	    sess->status == STATUS_NEEDS_RESUME ||
> -	    sess->status == STATUS_INIT)
> +	    sess->status == STATUS_NEEDS_RESUME)
>  		return 0;
>  
> +	/*
> +	 * Secure the core hardware lock before checking availability
> +	 * and updating session states to prevent STREAMON race conditions.
> +	 */
> +	mutex_lock(&core->lock);
> +	if (core->cur_sess && core->cur_sess != sess) {
> +		ret = -EBUSY;
> +		mutex_unlock(&core->lock);
> +		goto err_unlock_no_hw;
> +	}

[Severity: High]
If we jump to err_unlock_no_hw here, doesn't it bypass the
err_cleanup_session label that clears the streamon flags?

If session B calls VIDIOC_STREAMON on its OUTPUT queue and hits this path,
sess->streamon_out will remain 1.

Could a subsequent VIDIOC_QBUF from session B cause vdec_vb2_buf_queue to
schedule esparser_queue_work, potentially overwriting the active hardware
registers of the current session?

[ ... ]

>  	sess->sequence_cap = 0;
>  	sess->sequence_out = 0;
> +
>  	if (vdec_codec_needs_recycle(sess))
>  		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
>  						   "vdec_recycle");

[Severity: High]
This is a pre-existing issue, but does this code properly handle thread
creation failure?

If kthread_run fails due to memory constraints, it returns an ERR_PTR.
Later in vdec_stop_streaming, this pointer is passed unconditionally to
kthread_stop.

Could this cause a kernel panic when kthread_stop attempts to dereference
the ERR_PTR?

> -	sess->status = STATUS_INIT;
> -	core->cur_sess = sess;
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;

[Severity: High]
This is a pre-existing issue, but does closing the file descriptor safely
clean up the workqueue scheduled here?

When the client queues a buffer and then closes the file descriptor,
vdec_close destroys mutexes and frees the amvdec_session memory via kfree
without cancelling the pending esparser_queue_work.

When the workqueue executes vdec_esparser_queue_work, could it access the
freed session memory and attempt to take a destroyed mutex?

[ ... ]

> @@ -399,30 +424,13 @@ static void vdec_stop_streaming(struct vb2_queue *q)

[ ... ]

> +	if (full_cleanup) {
> +		if ((q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE ||
> +		     !sess->streamon_out) && vdec_codec_needs_recycle(sess)) {
> +			kthread_stop(sess->recycle_thread);
> +		}
> +
> +		vdec_poweroff(sess);

[Severity: High]
If a userspace client starts streaming on both queues and then calls
VIDIOC_STREAMOFF on the CAPTURE queue, the newly added condition evaluating
q->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE or !sess->streamon_out
will evaluate to false, skipping kthread_stop.

Since full_cleanup is true, the code proceeds to tear down resources via
vdec_reset_bufs_recycle and kfree of sess->priv.

Could this lead to a use-after-free or list corruption if the recycle thread
continues executing while its backing resources are freed?

> +		vdec_free_canvas(sess);
> +
> +		if (sess->vififo_vaddr) {
> +			dma_free_coherent(sess->core->dev, sess->vififo_size,
> +					  sess->vififo_vaddr, sess->vififo_paddr);
> +			sess->vififo_vaddr = NULL;
> +			sess->vififo_paddr = 0;
> +		}
> +
> +		vdec_reset_timestamps(sess);

[Severity: High]
This is a pre-existing issue, but does vdec_reset_timestamps safely access
the timestamps list here?

The function iterates and frees elements of the sess->timestamps list
without holding ts_spinlock.

If a concurrent threaded ISR executes amvdec_dst_buf_done_idx and calls
amvdec_remove_ts, it will acquire ts_spinlock and attempt to remove an
element.

Could this race condition lead to linked list corruption and use-after-free?

> +		vdec_reset_bufs_recycle(sess);
> +		core->cur_sess = NULL;
> +
> +		kfree(sess->priv);
> +		sess->priv = NULL;
> +	} else {
> +		if (sess->status == STATUS_NEEDS_RESUME)
> +			sess->changed_format = 0;
> +	}
> +	mutex_unlock(&core->lock);
>  }

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

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