Re: [PATCH v5 4/6] media: meson: vdec: Handle kthread error and free codec private data

[sashiko-bot@kernel.org]

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

New issues:
- [High] Calling `codec_ops->stop()` before `vdec_poweroff()` causes a hardware DMA use-after-free vulnerability.
- [Medium] The error path fails to free the dynamically allocated codec context (`sess->priv`), leading to a permanent memory leak.

Pre-existing issues:
- [High] The error path fails to free the hardware canvases allocated during `vdec_poweron()`.
--

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

media: meson: vdec: Handle kthread error and free codec private data

This commit adds an error handling path in vdec_start_streaming() when
kthread_run() fails. It introduces an err_cleanup label to invoke the
codec's stop callback and power off the hardware, aiming to fix memory leaks.

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 52ace4de967cb..b31bf08af88e6 100644
> --- a/drivers/staging/media/meson/vdec/vdec.c
> +++ b/drivers/staging/media/meson/vdec/vdec.c
[ ... ]
> @@ -345,13 +345,25 @@ static int vdec_start_streaming(struct vb2_queue *q, unsigned int count)
[ ... ]
>  	schedule_work(&sess->esparser_queue_work);
>  	return 0;
>  
> +err_cleanup:
> +	if (codec_ops && codec_ops->stop && sess->priv) {
> +		codec_ops->stop(sess);

[Severity: High]
Does calling codec_ops->stop() before vdec_poweroff() risk a hardware DMA use
after free?

The codec_ops->stop() callbacks unmap and free DMA memory used by the
firmware, but vdec_poweroff() hasn't been called yet to halt the hardware. The
hardware could still be powered on with its DMA pointers pointing to the freed
memory.

Could this lead to IOMMU faults or memory corruption if the active hardware
issues any memory transactions before it is powered off?

> +		sess->priv = NULL;

[Severity: Medium]
Does zeroing out sess->priv without freeing it cause a memory leak?

The codec_ops->stop() implementations typically only release internal DMA
buffers, but do not free the sess->priv structure itself. Normally, this
memory is freed via kfree() in vdec_stop_streaming().

Does setting it to NULL here permanently orphan the structure?

> +	}
> +	vdec_poweroff(sess);

[Severity: High]
This is a pre-existing issue, but does this error path also need to free the
hardware canvases?

During vdec_poweron(), codecs allocate hardware canvas IDs. The normal
shutdown path calls vdec_free_canvas() to release them.

Since the hardware has a limited global pool of canvas IDs, does omitting this
call risk rendering the video decoder subsystem unusable by leaking the canvas
IDs?

>  vififo_free:
>  	mutex_lock(&core->lock);
>  	core->cur_sess = NULL;

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

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