Re: [PATCH v7 04/19] media: meson: vdec: Fix use-after-free race between teardown and ISR routines

[sashiko-bot@kernel.org]

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

New issues:
- [High] Returning `IRQ_HANDLED` for a NULL session in the hardware interrupt handler causes an infinite interrupt storm.

Pre-existing issues:
- [High] Calling `synchronize_irq()` before stopping hardware interrupts fails to fix the use-after-free race condition.
- [High] Unbind operations destroy devm-managed structures and the m2m device while open file descriptors remain, causing UAF on closure.
--

--- Patch [4]: [PATCH v7 04/19] media: meson: vdec: Fix use-after-free race between teardown and ISR routines ---
commit 7f1533ed7c182234db28cc4195417115cb635bb9
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix use-after-free race between teardown and ISR routines

This patch attempts to prevent race conditions and null pointer dereferences
by synchronizing interrupt service routines during session teardown and file
descriptor release phases. It adds synchronize_irq() calls and uses
smp_load_acquire() to safely capture the current session in the ISR handlers.

Link: 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 7fc73d5cdebbf..7ae3d5a9dd6ab 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -484,6 +484,9 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  			}
>  		}
>  
> +		/* Synchronize and flush pending hardware interrupt service routines */
> +		synchronize_irq(core->vdec_irq);
> +

[Severity: High]
This isn't a bug introduced by this patch, but does calling synchronize_irq()
before vdec_poweroff() actually prevent the race condition?

Since synchronize_irq() only waits for currently executing handlers to finish
and does not disable the IRQ line or prevent new hardware interrupts, it seems
new interrupts can still fire immediately after it returns.

Because the hardware isn't stopped until vdec_poweroff() runs, could a newly
triggered ISR observe a valid session pointer and execute concurrently with the
rest of the teardown sequence?

Should the hardware be stopped via vdec_poweroff() before synchronizing IRQs?

>  		vdec_poweroff(sess);
>  		vdec_free_canvas(sess);
>  

[ ... ]

> @@ -989,6 +992,9 @@ static int vdec_close(struct file *file)
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  	struct amvdec_core *core = sess->core;
>  
> +	/* Synchronize and flush pending hardware interrupt service routines */
> +	synchronize_irq(core->vdec_irq);
> +

[Severity: High]
This is a pre-existing issue, but does the driver handle unbind safely if
user-space keeps the video device file open?

If the driver is unbound, vdec_remove() executes:

vdec_remove()
    video_unregister_device(core->vdev_dec);
    v4l2_m2m_release(core->m2m_dev);
    v4l2_device_unregister(&core->v4l2_dev);

Since devres automatically frees the amvdec_core structure upon unbind
completion, and video_unregister_device() does not forcefully close open file
descriptors, could a later call to vdec_close() dereference the freed core
structure?

For instance, this newly added synchronize_irq(core->vdec_irq) call would
use the freed core pointer. Similarly, v4l2_m2m_ctx_release(sess->m2m_ctx)
later in vdec_close() would use the freed core->m2m_dev.

>  	if (!IS_ERR_OR_NULL(sess->recycle_thread)) {
>  		kthread_stop(sess->recycle_thread);
>  		sess->recycle_thread = NULL;

[ ... ]

> @@ -1038,7 +1044,12 @@ static const struct v4l2_file_operations vdec_fops = {
>  static irqreturn_t vdec_isr(int irq, void *data)
>  {
>  	struct amvdec_core *core = data;
> -	struct amvdec_session *sess = core->cur_sess;
> +	struct amvdec_session *sess;
> +
> +	/* Secure an atomic acquire snapshot to protect against concurrent teardown */
> +	sess = smp_load_acquire(&core->cur_sess);
> +	if (!sess)
> +		return IRQ_HANDLED;

[Severity: High]
Can returning IRQ_HANDLED here cause an infinite interrupt storm?

If the handler evaluates the session to NULL and returns IRQ_HANDLED without
acknowledging or clearing the interrupt at the hardware level, the generic IRQ
framework will unmask the interrupt line.

Since the hardware interrupt source hasn't been cleared, won't the interrupt
controller immediately re-trigger the interrupt, resulting in a soft lockup or
CPU stall?

Should this return IRQ_NONE so the IRQ subsystem can detect it as spurious and
eventually disable the IRQ line?

>  
>  	sess->last_irq_jiffies = get_jiffies_64();
>  
> @@ -1048,7 +1059,12 @@ static irqreturn_t vdec_isr(int irq, void *data)
>  static irqreturn_t vdec_threaded_isr(int irq, void *data)
>  {
>  	struct amvdec_core *core = data;
> -	struct amvdec_session *sess = core->cur_sess;
> +	struct amvdec_session *sess;
> +
> +	/* Prevent late-stage threaded interrupts from dereferencing a NULL session */
> +	sess = smp_load_acquire(&core->cur_sess);
> +	if (!sess)
> +		return IRQ_HANDLED;

[Severity: High]
Does returning IRQ_HANDLED here also incorrectly propagate the same interrupt
storm behavior?

>  
>  	return sess->fmt_out->codec_ops->threaded_isr(sess);
>  }

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

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