Re: [PATCH v5 3/6] media: meson: vdec: Set cur_sess before hardware vdec_poweron()

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Setting `core->cur_sess` before enabling hardware clocks creates a window where an interrupt can trigger hardware access while clocks are gated, causing a system crash.

Pre-existing issues:
- [High] The patch attempts to fix a hardware exclusivity race by protecting the assignment of `core->cur_sess` with a lock, but leaves a critical Time-Of-Check to Time-Of-Use (TOCTOU) gap.
- [High] The interrupt handlers lack a NULL pointer check for `core->cur_sess`, leaving the driver susceptible to panics from spurious or delayed interrupts.
- [High] The global pointers `core->cur_sess` and `sess->status` are cleared in `vdec_stop_streaming()` without acquiring `core->lock`, introducing a data race.
--

commit aa641955fed13a943e3d3bbb36f369ce0d7fb41c
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Set cur_sess before hardware vdec_poweron()

This patch moves the assignment of core->cur_sess and sess->status before
vdec_poweron() in vdec_start_streaming() to prevent a NULL pointer dereference
in the ISR. It also updates the error path and protects the assignments with
core->lock.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/
Fixes: 3e7f51bd9607 ("media: meson: add v4l2 m2m video decoder driver")

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index e72f54af026e3..52ace4de967cb 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -334,6 +334,11 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	atomic_set(&sess->esparser_queued_bufs, 0);
>  	v4l2_ctrl_s_ctrl(sess->ctrl_min_buf_capture, 1);
>  
> +	mutex_lock(&core->lock);
> +	core->cur_sess = sess;
> +	sess->status = STATUS_INIT;
> +	mutex_unlock(&core->lock);
> +
>  	ret = vdec_poweron(sess);

[Severity: Critical]
By setting core->cur_sess before calling vdec_poweron(), could this create a
window where a spurious or shared interrupt triggers a hardware access while
the hardware clocks are still gated?

If the interrupt fires here, the ISR might invoke the active codec's ISR and
perform MMIO writes. Accessing registers of a clock-gated device could result
in a synchronous external abort (bus error or SError) and crash the kernel.


[Severity: High]
This isn't a bug introduced by this patch, but there is a Time-Of-Check to
Time-Of-Use (TOCTOU) race condition regarding core->cur_sess.

Earlier in vdec_start_streaming(), core->lock is held to check if
core->cur_sess is available, but the lock is immediately released:

	mutex_lock(&core->lock);
	if (core->cur_sess && core->cur_sess != sess) {
		mutex_unlock(&core->lock);
		ret = -EBUSY;
		goto bufs_done;
	}
	mutex_unlock(&core->lock);

Multiple concurrent sessions could simultaneously observe
core->cur_sess == NULL, drop the lock, perform memory allocations, and then
sequentially re-acquire the lock here to overwrite core->cur_sess. Both
sessions would then execute vdec_poweron(), bypassing the exclusivity check.

Should the initial check and the assignment be performed atomically within the
same critical section?


[Severity: High]
This is a pre-existing issue, but does this fix address the root cause of the
NULL pointer dereference?

The commit message notes that vdec_isr() can dereference a NULL pointer if an
interrupt fires early. Because the IRQ is registered during probe, it remains
active for the device's lifetime. If a spurious interrupt fires when no
session is active, or if a delayed interrupt triggers immediately after
vdec_stop_streaming() clears core->cur_sess, could the kernel still panic?

Should vdec_isr() and vdec_threaded_isr() be updated to include a proper NULL
check for core->cur_sess?

>  	if (ret)
>  		goto vififo_free;
> @@ -344,12 +349,14 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
>  						   "vdec_recycle");
>  
> -	sess->status = STATUS_INIT;
> -	core->cur_sess = sess;
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
>  vififo_free:
> +	mutex_lock(&core->lock);
> +	core->cur_sess = NULL;
> +	sess->status = STATUS_STOPPED;
> +	mutex_unlock(&core->lock);

[Severity: High]
This is a pre-existing issue, but while core->lock is now used here in
vdec_start_streaming(), is it also required in vdec_stop_streaming()?

If vdec_stop_streaming() clears core->cur_sess and sess->status locklessly, it
could run concurrently with another session's vdec_start_streaming(). This
could break the synchronization model and allow for data races where the
starting session reads an inconsistent hardware state.

>  	dma_free_coherent(sess->core->dev, sess->vififo_size,
>  			  sess->vififo_vaddr, sess->vififo_paddr);
>  bufs_done:

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

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic