Re: [PATCH v5 2/6] media: meson: vdec: Protect session exclusivity check with lock

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch fails to resolve the concurrent STREAMON race condition, leaving a Time-Of-Check to Time-Of-Use (TOCTOU) vulnerability.

Pre-existing issues:
- [High] Unprotected modification of `core->cur_sess` in `vdec_stop_streaming` leads to a data race.
- [High] Missing NULL check for `core->cur_sess` in interrupt handlers risks a kernel panic from spurious interrupts.
--

commit 7f55f473f3e7d75722e346666034bf541752f2d3
Author: Anand Moon <linux.amoon@gmail.com>
Subject: media: meson: vdec: Protect session exclusivity check with lock

This commit attempts to fix a race condition during concurrent STREAMON calls.
It wraps the session exclusivity check in vdec_start_streaming() with a
mutex to prevent multiple threads from bypassing the active session check
and concurrently triggering hardware initialization.

Closes: https://lore.kernel.org/all/20260521090944.F35401F00A3D@smtp.kernel.org/

> diff --git a/drivers/staging/media/meson/vdec/vdec.c b/drivers/staging/media/meson/vdec/vdec.c
> index 18a22b79e835e..e72f54af026e3 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -286,10 +286,13 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  	struct vb2_v4l2_buffer *buf;
>  	int ret;
>  
> +	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);

[Severity: High]
Does this completely resolve the concurrent STREAMON race condition?

While the lock protects the check itself, it is released immediately
afterward. This leaves a window where two threads could sequentially acquire
the lock, observe core->cur_sess as NULL, and then concurrently proceed
with hardware initialization in vdec_poweron().

A later patch in the series wraps the eventual core->cur_sess = sess
assignment in the lock, but does that still leave an unprotected gap between
this check and the assignment?

[Severity: High]
This is a pre-existing issue, but does core->cur_sess need lock protection
in vdec_stop_streaming() as well?

Since V4L2 queue locks are used, STREAMON and STREAMOFF on different file
descriptors can execute concurrently. If vdec_stop_streaming() sets
core->cur_sess to NULL without acquiring core->lock, could this create a
data race on the global pointer?

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for
core->cur_sess to be NULL when an interrupt fires?

Since core->cur_sess is set to NULL when no decoding session is active,
could a spurious hardware interrupt arriving during an idle period cause
a NULL pointer dereference in vdec_isr() or vdec_threaded_isr() if they
blindly dereference core->cur_sess without checking?

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

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