Re: [PATCH] media: coda: Fix potential use-after-free after VDOA removal

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] media: coda: unstopped DMA and job desync on timeout in
  vdoa_stop()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Medium] media: coda: resource leaks in coda_open() error paths [1]
--

commit 641eafc2847290538ca328a1783a635a0c449aa1
Author: Guangshuo Li <liguangshuo@mail.sdu.edu.cn>

media: coda: Fix potential use-after-free after VDOA removal

This commit reference-counts VDOA driver data across CODA consumers and
contexts, serializes VDOA hardware operations against driver removal, and
destroys the VDOA context if coda_open() fails.

> diff --git a/drivers/media/platform/chips-media/coda/coda-common.c b/drivers/media/platform/chips-media/coda/coda-common.c
> index be37ea568bfe5..b519cf8541ded 100644
> --- a/drivers/media/platform/chips-media/coda/coda-common.c
> +++ b/drivers/media/platform/chips-media/coda/coda-common.c

[ ... ]

> @@ -2721,6 +2719,8 @@ static int coda_open(struct file *file)
>  err_clk_enable:
>  	pm_runtime_put_sync(dev->dev);
>  err_pm_get:
> +	if (ctx->vdoa)
> +		vdoa_context_destroy(ctx->vdoa);
>  	v4l2_fh_del(&ctx->fh, file);
>  	v4l2_fh_exit(&ctx->fh);
>  err_coda_name_init:

[Severity: Medium]
This problem wasn't introduced by this patch, but does this error path
still leak ctx->debugfs_entry and ctx->ctrls [1]?

coda_open() creates ctx->debugfs_entry with debugfs_create_dir() before
v4l2_fh_init() and initializes ctx->ctrls in coda_ctrls_setup(), but
neither debugfs_remove() nor v4l2_ctrl_handler_free() is called during
error unwind, leaving stale debugfs directories and leaked control handler
memory when opening the device fails.

[ ... ]

> diff --git a/drivers/media/platform/chips-media/coda/imx-vdoa.c b/drivers/media/platform/chips-media/coda/imx-vdoa.c
> index cd085c1c73f42..d9249c943dfa2 100644
> --- a/drivers/media/platform/chips-media/coda/imx-vdoa.c
> +++ b/drivers/media/platform/chips-media/coda/imx-vdoa.c

[ ... ]

> @@ -142,17 +150,43 @@ int vdoa_wait_for_completion(struct vdoa_ctx *ctx)
>  
>  	return 0;
>  }
> +
> +int vdoa_wait_for_completion(struct vdoa_ctx *ctx)
> +{
> +	struct vdoa_data *vdoa = ctx->vdoa;
> +	int ret;
> +
> +	mutex_lock(&vdoa->lock);
> +	ret = vdoa->removed ? -ENODEV : vdoa_wait(ctx);
> +	mutex_unlock(&vdoa->lock);
> +
> +	return ret;
> +}
>  EXPORT_SYMBOL(vdoa_wait_for_completion);
>  
> +static void vdoa_stop(struct vdoa_data *vdoa)
> +{
> +	if (!vdoa->curr_ctx)
> +		return;
> +
> +	if (vdoa_wait(vdoa->curr_ctx))
> +		writel(VDOASRR_SWRST, vdoa->regs + VDOASRR);
> +	writel(0, vdoa->regs + VDOAIE);
> +	synchronize_irq(vdoa->irq);
> +	vdoa->curr_ctx = NULL;
> +}

[Severity: Medium]
When a transfer times out in vdoa_wait(), does vdoa_wait_for_completion()
leave DMA running while vdoa_stop() leaves submitted_job and completed_job
permanently out of sync?

When vdoa_wait_for_completion() returns -ETIMEDOUT to coda_finish_decode(),
the caller immediately releases the internal source frame and returns
dst_buf to videobuf2 without VDOASRR_SWRST or VDOAIE being written, so
VDOA DMA can still write to the returned buffer.  Furthermore, because
vdoa->curr_ctx is still set and completed_job is never updated to match
submitted_job when resetting the hardware in vdoa_stop(), the next
vdoa_stop() call waits another 300ms on the same timed-out transfer, and
every subsequent job on that context sees submitted_job != completed_job
after completion and stalls for 300ms before resetting the hardware.

[1] https://sashiko.dev/#/bug/linux-28a3755d-8800-42ad-8451-7f7421db4955

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-0d0c29e10a-media-coda-1@mail.sdu.edu.cn?part=1