Re: [PATCH v2] staging: media: meson: vdec: fix advanced concurrency and UAF bugs

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

New issues:
- [High] Use-After-Free in vdec_close() due to cancel_work_sync() ordered after v4l2_m2m_ctx_release().
- [High] Structural Use-After-Free of core->m2m_dev on driver unbind due to improper lifecycle management.
- [Medium] Memory leak of sess->priv on the kthread_run() error path in vdec_start_streaming().
- [Low] Compilation error in vdec_m2m_job_abort() due to removed sess->m2m_dev.

Pre-existing issues:
- [High] TOCTOU race in vdec_stop_streaming() places synchronize_irq() after freeing session data, risking UAF and hardware crashes.
--

--- Patch [1]: [PATCH v2] staging: media: meson: vdec: fix advanced concurrency and UAF bugs ---
commit 3fdf8cd968dfefcd7c68b91efa837b4576ff3069
Author: Alfie Varghese <[email protected]>

staging: media: meson: vdec: fix advanced concurrency and UAF bugs

This commit fixes several deep concurrency and race condition issues identified
in the Amlogic video decoder driver.

> 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
[ ... ]
> @@ -337,9 +337,16 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
>  
>  	sess->sequence_cap = 0;
>  	sess->sequence_out = 0;
> -	if (vdec_codec_needs_recycle(sess))
> +	if (vdec_codec_needs_recycle(sess)) {
>  		sess->recycle_thread = kthread_run(vdec_recycle_thread, sess,
>  						   "vdec_recycle");
> +		if (IS_ERR(sess->recycle_thread)) {
> +			ret = PTR_ERR(sess->recycle_thread);
> +			sess->recycle_thread = NULL;
> +			vdec_poweroff(sess);
> +			goto vififo_free;

[Severity: Medium]
Does jumping to vififo_free here leak sess->priv?

Because sess->status is not set to STATUS_INIT until after this block,
it remains STATUS_STOPPED. The subsequent cleanup path in
vdec_stop_streaming() evaluates the status condition as false and skips
its teardown block entirely, leaving sess->priv allocated.

[ ... ]
> @@ -416,6 +423,7 @@ static void vdec_stop_streaming(struct vb2_queue *q)
>  		kfree(sess->priv);
>  		sess->priv = NULL;
>  		core->cur_sess = NULL;
> +		synchronize_irq(core->irq);

[Severity: High]
This isn't a bug introduced by this patch, but does the new synchronize_irq()
call actually close the TOCTOU race window?

The commit message states it fixes a TOCTOU race by ensuring running
interrupt handlers finish before freeing session data. However, if an
interrupt fires just before vdec_poweroff(sess) is called in this function,
the ISR reads core->cur_sess locklessly and evaluates it as valid.

The thread executing vdec_stop_streaming() then continues, disables clocks
in vdec_poweroff(), and frees sess->priv. Since synchronize_irq() is placed
at the very end of the block, the concurrent ISR will access unclocked
registers or dereference the freed sess->priv.

[ ... ]
> @@ -927,10 +926,12 @@ static int vdec_close(struct file *file)
>  	struct amvdec_session *sess = file_to_amvdec_session(file);
>  
>  	v4l2_m2m_ctx_release(sess->m2m_ctx);
> -	v4l2_m2m_release(sess->m2m_dev);
> +	cancel_work_sync(&sess->esparser_queue_work);

[Severity: High]
If the esparser_queue_work is concurrently executing or scheduled when
vdec_close() is called, will this cause a use-after-free?

The M2M context is first released and freed by v4l2_m2m_ctx_release().
Immediately after, if the work function esparser_queue_all_src() runs,
it iterates using v4l2_m2m_for_each_src_buf_safe(sess->m2m_ctx, ...),
which dereferences the already-freed memory. Should cancel_work_sync()
be called before v4l2_m2m_ctx_release()?

[ ... ]
> @@ -1105,6 +1126,7 @@ static void vdec_remove(struct platform_device *pdev)
>  	struct amvdec_core *core = platform_get_drvdata(pdev);
>  
>  	video_unregister_device(core->vdev_dec);
> +	v4l2_m2m_release(core->m2m_dev);

[Severity: High]
Is it safe to unconditionally free core->m2m_dev here during driver unbind?

If a userspace process holds the /dev/videoX file descriptor open during
driver unbind, video_unregister_device() will return but the file remains
open.

When the user later closes the file descriptor, vdec_close() invokes
v4l2_m2m_ctx_release(sess->m2m_ctx), which internally accesses the now-freed
m2m_ctx->m2m_dev to acquire its job spinlock.

> diff --git a/drivers/staging/media/meson/vdec/vdec.h b/drivers/staging/media/meson/vdec/vdec.h
> --- a/drivers/staging/media/meson/vdec/vdec.h
> +++ b/drivers/staging/media/meson/vdec/vdec.h
[ ... ]
> @@ -230,7 +233,6 @@ struct amvdec_session {
>  	struct amvdec_core *core;
>  
>  	struct v4l2_fh fh;
> -	struct v4l2_m2m_dev *m2m_dev;

[Severity: Low]
Does removing this field cause a compilation error in vdec_m2m_job_abort()?

It looks like vdec_m2m_job_abort() in vdec.c was not updated and might
still reference sess->m2m_dev, which would break the build.

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

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