Re: [PATCH v6 7/8] media: meson: vdec: Fix NULL pointer dereference in ISR handlers

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Freeing `core->m2m_dev` in `vdec_remove` causes a use-after-free vulnerability when userspace keeps file descriptors open.
- [High] Returning IRQ_NONE without clearing the hardware interrupt status when `core->cur_sess` is NULL leaves the level-triggered hardware interrupt unacknowledged, causing an interrupt storm.
- [High] Bailing out early in the ISR prevents updating `sess->last_irq_jiffies`, completely bypassing the hardware quiescence wait.
- [High] Prematurely releasing `core->lock` and clearing `core->cur_sess` in `vdec_stop_streaming()` allows another session to start streaming concurrently while the hardware is still being powered off.

Pre-existing issues:
- [High] The patch fails to resolve the underlying race condition, leaving a Time-of-Check to Time-of-Use (TOCTOU) vulnerability that causes Use-After-Free and bus aborts.
--

commit c640aebabd0e51f1ec94916e63bc19e2fc312eb7
Author: Anand Moon <linux.amoon@gmail.com>

media: meson: vdec: Fix NULL pointer dereference in ISR handlers

This commit attempts to fix a NULL pointer dereference in the vdec
interrupt handlers by using READ_ONCE() to take an atomic snapshot of
core->cur_sess. If the session has already been torn down and the
pointer is NULL, it returns IRQ_NONE to prevent dereferencing it.

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 f99335effe176..3897c75b19c8f 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
> @@ -996,17 +996,36 @@ 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;
> +	irqreturn_t ret = IRQ_HANDLED;
> +
> +	/*
> +	 * Use READ_ONCE to secure an atomic snapshot of the pointer,
> +	 * protecting against concurrent clearing during streaming
> +	 * teardowns.
> +	 */
> +	sess = READ_ONCE(core->cur_sess);
> +	if (!sess)
> +		return IRQ_NONE;

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

In vdec_stop_streaming(), core->cur_sess is set to NULL before the hardware
is actually stopped. If an active hardware interrupt fires during this window,
the handler observes a NULL sess and returns IRQ_NONE early.

Because the interrupt is not cleared in the hardware registers, the
level-triggered interrupt remains asserted. Will this trigger an infinite
interrupt loop until the kernel's spurious interrupt detector permanently
disables the IRQ line?

[Severity: High]
Does bailing out early prevent updating sess->last_irq_jiffies?

The teardown sequence calls vdec_wait_inactive(), which sleeps until 50ms
have elapsed since sess->last_irq_jiffies. By returning early when
core->cur_sess is NULL, the handler no longer updates this timestamp for
trailing interrupts.

Could vdec_wait_inactive() observe a stale timestamp and return immediately
instead of waiting for the hardware to quiesce? This might allow the driver
to forcefully disable device clocks while the hardware is still actively
processing.

>  
>  	sess->last_irq_jiffies = get_jiffies_64();
> +	ret = sess->fmt_out->codec_ops->isr(sess);
>  
> -	return sess->fmt_out->codec_ops->isr(sess);
> +	return ret;
>  }
>  
>  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 = READ_ONCE(core->cur_sess);

[Severity: High]
This is a pre-existing issue, but does using READ_ONCE() here fully resolve
the underlying race condition?

If the thread is preempted after passing the !sess check, could
vdec_stop_streaming() concurrently clear core->cur_sess, disable hardware
clocks via vdec_poweroff(), and free sess->priv?

When the thread resumes, it would dereference the freed sess->priv and access
disabled hardware. Should the driver use synchronize_irq() to ensure
in-flight handlers have completed before tearing down session resources?

> +	if (!sess)
> +		return IRQ_NONE;
>  
>  	return sess->fmt_out->codec_ops->threaded_isr(sess);
>  }

[Severity: High]
Does prematurely releasing core->lock and clearing core->cur_sess in
vdec_stop_streaming() allow another session to start streaming concurrently
while the hardware is still being powered off?

vdec_stop_streaming() acquires core->lock to set core->cur_sess = NULL, but
then releases the lock before stopping the hardware via vdec_poweroff().

Could a concurrent vdec_start_streaming() on a different session successfully
claim core->cur_sess, release the lock, and call vdec_poweron(), breaking
hardware exclusion?

[Severity: Critical]
Does freeing core->m2m_dev in vdec_remove() cause a use-after-free
vulnerability when userspace keeps file descriptors open?

The driver allocates a shared m2m_dev in vdec_probe() and explicitly frees it
via v4l2_m2m_release(core->m2m_dev) in vdec_remove(). However, vdec_remove()
is executed on device unbind, which does not wait for userspace to close
active file descriptors.

If a user holds an open file descriptor when the device is unbound, m2m_dev is
freed immediately. When the user later closes the file descriptor, vdec_close()
calls v4l2_m2m_ctx_release(sess->m2m_ctx), which internally dereferences the
already-freed m2m_dev.

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

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