media: meson: Fix memory leak in error path in vdec

drivers/staging/media/meson/vdec/vdec.c static int vdec_open(struct file *file) · line 880

[Severity: Medium]
Is this error unwinding logic in the correct order? Jumps to err_fh_del here will call v4l2_fh_exit() on sess->fh before it has been initialized further down in vdec_open().

drivers/staging/media/meson/vdec/vdec.c static int vdec_open(struct file *file) · line 885

[Severity: Medium]
Does skipping err_fh_del here lead to a memory leak? If vdec_init_ctrls() fails, jumping directly to err_free_sess bypasses freeing the successfully allocated sess->m2m_ctx.

[ ... ]

drivers/staging/media/meson/vdec/vdec.c static int vdec_close(struct file *file) · line 920

[Severity: High]
Can this result in a use-after-free if the user holds the file descriptor open after the driver is unbound?

During driver unbind, vdec_remove() explicitly frees core->m2m_dev and devres cleans up the amvdec_core structure.

If a user subsequently closes an open file descriptor, vdec_close() calls v4l2_m2m_ctx_release(), which will dereference the freed m2m_dev pointer inside v4l2_m2m_cancel_job().

Additionally, any pending jobs may trigger vdec_m2m_job_abort(), which dereferences the freed sess->core and sess->core->m2m_dev.

drivers/staging/media/meson/vdec/vdec.c static void vdec_stop_streaming(struct vb2_queue *q) · line 430

[Severity: High]
Does prematurely clearing core->cur_sess here open a race window for concurrent hardware accesses?

Since the lock is dropped immediately after this block, another session can call vdec_start_streaming(), acquire the lock, claim core->cur_sess, and call vdec_poweron().

Meanwhile, the old session continues executing vdec_stop_streaming() and reaches vdec_poweroff(sess), which calls vdec_wait_inactive(sess) and can sleep for up to 50ms while the hardware shuts down.

Does this result in vdec_poweron() and vdec_poweroff() executing concurrently on the same hardware without synchronization?

(Note: A later commit in this series, "media: meson: vdec: Fix NULL pointer dereference in ISR handlers", addresses an ISR dereference during this window, but it does not appear to resolve this underlying concurrent initialization and teardown race.)

drivers/staging/media/meson/vdec/vdec.c static void vdec_stop_streaming(struct vb2_queue *q) · line 431

[Severity: High]
Does unconditionally setting sess->status = STATUS_STOPPED break dynamic resolution changes (DRC)?

During DRC, the hardware sets sess->status = STATUS_NEEDS_RESUME, and userspace responds by stopping the capture queue (STREAMOFF). If we overwrite sess->status to STATUS_STOPPED here, the subsequent STREAMON from userspace will check this in vdec_start_streaming():

    if (sess->status == STATUS_NEEDS_RESUME &&
        q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE &&
        sess->changed_format) {
        codec_ops->resume(sess);
        sess->status = STATUS_RUNNING;
        return 0;
    }

Because the status is now STATUS_STOPPED, does this bypass the fast resume path and fall through to a full re-initialization?

This appears to cause the reallocation of the 16MB vififo DMA buffer (leaking the original 16MB allocation which was never freed) and spawn a duplicate recycle_thread (leaking the running thread).

drivers/staging/media/meson/vdec/vdec.c static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) · line 374

[Severity: Critical]
Does this explicitly call codec_ops->stop() a second time, resulting in a double-free of the codec DMA memory?

Looking at vdec_poweroff() called just above, it internally delegates to vdec_ops->stop(), which invokes codec_ops->stop(). Codec stop callbacks free their DMA buffers via dma_free_coherent() but do not set the pointers to NULL. If it is called again here, wouldn't it attempt to free them a second time?

drivers/staging/media/meson/vdec/vdec.c static void vdec_stop_streaming(struct vb2_queue *q) · line 459

[Severity: Critical]
This is a pre-existing issue, but could clearing the hardware ownership token before executing vdec_poweroff() create a data race with concurrent sessions?

Looking earlier in vdec_stop_streaming(), core->cur_sess = NULL is set inside the mutex, but vdec_poweroff(sess) is executed here after the mutex is dropped:

	mutex_lock(&core->lock);
	if (core->cur_sess == sess)
		core->cur_sess = NULL;
	mutex_unlock(&core->lock);
    ...
	vdec_poweroff(sess);

This allows a concurrent vdec_start_streaming() from another context to acquire the lock, claim ownership, and call vdec_poweron(). Since vdec_poweron() and vdec_poweroff() would execute concurrently on different sessions, is it possible they will read and write to the exact same global hardware registers and corrupt the shared hardware state?

drivers/staging/media/meson/vdec/vdec.c static void vdec_stop_streaming(struct vb2_queue *q) · line 468

[Severity: High]
This isn't a bug introduced by this patch, but does the recent change to concurrent locking destroy the STATUS_NEEDS_RESUME state?

Earlier in vdec_stop_streaming(), sess->status is unconditionally overwritten to STATUS_STOPPED inside the mutex lock:

	mutex_lock(&core->lock);
	old_status = sess->status;
	sess->status = STATUS_STOPPED;
	mutex_unlock(&core->lock);

When the CAPTURE queue is stopped during a source resolution change, the hardware cleanup block is intentionally skipped because old_status is STATUS_NEEDS_RESUME. However, when vdec_start_streaming() is later called to resume the CAPTURE queue, it expects sess->status == STATUS_NEEDS_RESUME. Since it was forced to STATUS_STOPPED, will the resume check fail and cause the driver to incorrectly fall through to full initialization, executing dma_alloc_coherent() over the existing sess->vififo_vaddr and leaking memory?

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

drivers/staging/media/meson/vdec/vdec.c static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) · line 387

[Severity: Critical]
Can this error path cause a double-free of the codec DMA buffers?

Looking at the error handling above this diff in vdec_start_streaming():

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
    [ ... ]
err_cleanup:
	vdec_free_canvas(sess);
	vdec_poweroff(sess);
	if (codec_ops && codec_ops->stop && sess->priv) {
		codec_ops->stop(sess);
    [ ... ]
}

When vdec_poweroff(sess) is called, it cascades to __vdec_1_stop(), which calls codec_ops->stop(sess). The cleanup block then explicitly calls codec_ops->stop(sess) a second time.

If the codec stop function (such as codec_h264_stop) frees the DMA memory allocations without nullifying the pointers, could passing the exact same DMA addresses to dma_free_coherent() a second time result in a double-free and heap corruption?

[Severity: High]
This is a pre-existing issue, but does this bypass the cleanup of the session context and permanently leak memory?

During vdec_poweron(sess), the driver allocates the sess->priv context and codec DMA buffers. If a subsequent initialization step fails, vdec_poweron propagates the error, and vdec_start_streaming() jumps directly to vififo_free:

drivers/staging/media/meson/vdec/vdec.c:vdec_start_streaming() {
    [ ... ]
	ret = vdec_poweron(sess);
	if (ret)
		goto vififo_free;
    [ ... ]
err_cleanup:
    [ ... ]
		kfree(sess->priv);
    [ ... ]
vififo_free:
    [ ... ]
bufs_done:
    [ ... ]
	sess->status = STATUS_STOPPED;
}

This bypasses the err_cleanup: label and the kfree(sess->priv) cleanup. Because the session status is then explicitly set to STATUS_STOPPED right here at bufs_done, won't later calls to vdec_stop_streaming() evaluate old_status == STATUS_STOPPED and skip the entire cleanup block, permanently leaking the memory?

[Severity: Critical]
Does the newly introduced locking fix in vdec_stop_streaming() lead to a Use-After-Free during a dynamic resolution change?

When the decoder detects a resolution change, it sets sess->status to STATUS_NEEDS_RESUME. When userspace handles this by calling STREAMOFF on the capture queue (the first call), vdec_stop_streaming() unconditionally sets the status to STATUS_STOPPED at the start of the function:

drivers/staging/media/meson/vdec/vdec.c:vdec_stop_streaming() {
    [ ... ]
	mutex_lock(&core->lock);
	old_status = sess->status;
    [ ... ]
	sess->status = STATUS_STOPPED;
	mutex_unlock(&core->lock);

	if (old_status == STATUS_RUNNING ||
	    old_status == STATUS_INIT ||
	    (old_status == STATUS_NEEDS_RESUME &&
	     (!sess->streamon_out || !sess->streamon_cap))) {
    [ ... ]
}

Because sess->streamon_cap is only cleared at the end of the function, both streams are still active on the first STREAMOFF, so the hardware cleanup condition evaluates to false.

When the second STREAMOFF is called on the output queue, old_status is now STATUS_STOPPED, so cleanup is skipped again. Since cleanup is completely bypassed, vififo memory and the recycle_thread are leaked.

If the user then calls STREAMON, won't it spawn a duplicate recycle_thread, overwriting the old pointer? When the session is eventually closed, could vdec_close() free the session structure while the orphaned thread continues executing, resulting in a Use-After-Free?

drivers/staging/media/meson/vdec/vdec.c static int vdec_start_streaming(struct vb2_queue *q, unsigned int count) · line 384

[Severity: High]
Does this code introduce a deadlock? The V4L2 core acquires the queue lock (sess->lock) before calling vdec_start_streaming() or vdec_stop_streaming().

If cancel_work_sync() is called here while the ioctl thread holds sess->lock, and the worker thread is currently executing:

drivers/staging/media/meson/vdec/esparser.c:esparser_queue_all_src() {
    ...
    mutex_lock(&sess->lock);
    ...

Could the worker thread block indefinitely waiting for sess->lock, while the ioctl thread blocks indefinitely in cancel_work_sync() waiting for the worker to complete?

drivers/staging/media/meson/vdec/vdec.c static void vdec_stop_streaming(struct vb2_queue *q) · line 453

[Severity: Critical]
This isn't a bug introduced by this patch, but does prematurely clearing core->cur_sess here introduce a race condition?

Since core->cur_sess is set to NULL under core->lock, but the hardware teardown via vdec_poweroff() executes later after releasing the lock, could a concurrent STREAMON from a different session acquire core->cur_sess and execute vdec_poweron()?

This sequence would cause vdec_poweroff() of the old session and vdec_poweron() of the new session to execute simultaneously, potentially leading to conflicting hardware accesses and disabling clocks for the concurrent session.

drivers/staging/media/meson/vdec/vdec.c static int vdec_probe(struct platform_device *pdev) · line 1065

[Severity: High]
This isn't a bug introduced by this patch, but can we experience a use-after-free here if the canvas provider unbinds?

Looking at meson_canvas_get(), the reference to the provider device is dropped immediately before returning the internal canvas pointer:

drivers/soc/amlogic/meson-canvas.c:meson_canvas_get() {
    ...
    put_device(&canvas_pdev->dev);
    return canvas;
}

Since the vdec driver stores this pointer in core->canvas but does not establish a device link or hold a module reference, what prevents the canvas memory from being freed by devres while the vdec driver is still using it?

drivers/staging/media/meson/vdec/vdec.c static int vdec_probe(struct platform_device *pdev) · line 1067

[Severity: Critical]
Will setting a 64-bit DMA mask cause silent physical address truncation?

The Meson VDEC hardware natively uses 32-bit registers for DMA addresses, and the driver's helper functions strictly accept 32-bit values:

drivers/staging/media/meson/vdec/vdec_helpers.h:
void amvdec_write_dos(struct amvdec_core *core, u32 reg, u32 val);

If the DMA API allocates memory buffers at physical addresses above 4GB, their 64-bit addresses will be implicitly truncated to 32 bits when passed to the hardware registers.

Can this lead to the hardware reading or writing to incorrect physical memory locations in the lower 4GB?

drivers/staging/media/meson/vdec/vdec.c static const struct v4l2_file_operations vdec_fops = { · line 1009

[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.

drivers/staging/media/meson/vdec/vdec.c static const struct v4l2_file_operations vdec_fops = { · line 1026

[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?

drivers/staging/media/meson/vdec/vdec.c static const struct v4l2_file_operations vdec_fops = { · line 1031

[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/[email protected]?part=7

drivers/gpu/drm/meson/meson_drv.c static int meson_drv_bind_master(struct device *dev, bool has_components) · line 248

[Severity: High]
This is a pre-existing issue, but does the error path in meson_drv_bind_master() cause a NULL pointer dereference?

If devm_kzalloc() fails earlier in this function:

drivers/gpu/drm/meson/meson_drv.c:meson_drv_bind_master() {
    ...
	priv = devm_kzalloc(dev, sizeof(*priv), GFP_KERNEL);
	if (!priv) {
		ret = -ENOMEM;
		goto free_drm;
	}
    ...
}

It jumps to the free_drm label with priv set to NULL. The cleanup path sequentially calls meson_encoder_dsi_remove(priv), meson_encoder_hdmi_remove(priv), and meson_encoder_cvbs_remove(priv).

Can this cause a kernel panic when these functions unconditionally dereference priv, such as here in meson_encoder_dsi_remove()?

drivers/gpu/drm/meson/meson_encoder_dsi.c:meson_encoder_dsi_remove() {
    ...
	if (priv->encoders[MESON_ENC_DSI]) {
    ...
}

[Severity: High]
This is a pre-existing issue, but does meson_canvas_get() leave the system vulnerable to a use-after-free?

Looking at meson_canvas_get() in drivers/soc/amlogic/meson-canvas.c, the device reference is dropped after extracting the canvas pointer:

drivers/soc/amlogic/meson-canvas.c:meson_canvas_get() {
    ...
	canvas = dev_get_drvdata(&canvas_pdev->dev);
	put_device(&canvas_pdev->dev);
	if (!canvas)
    ...
}

Since no device_link_add() is used to track the dependency between the DRM driver and the canvas driver, the driver core permits unbinding the amlogic-canvas driver.

Would doing so free its devm-managed memory and leave meson-drm with a dangling pointer that causes a use-after-free when dereferenced later, such as when locking canvas->lock in meson_canvas_alloc()?

[Severity: High]
This is a pre-existing issue, but does meson_encoder_cvbs_probe() leak a bridge reference on error paths?

In drivers/gpu/drm/meson/meson_encoder_cvbs.c, the bridge is added globally:

drivers/gpu/drm/meson/meson_encoder_cvbs.c:meson_encoder_cvbs_probe() {
    ...
	drm_bridge_add(&meson_encoder_cvbs->bridge);

	meson_encoder_cvbs->priv = priv;

	/* Encoder */
	ret = drm_simple_encoder_init(priv->drm, &meson_encoder_cvbs->encoder,
				      DRM_MODE_ENCODER_TVDAC);
	if (ret)
		return dev_err_probe(priv->dev, ret,
				     "Failed to init CVBS encoder\n");
    ...
}

If a subsequent initialization step fails, the function returns an error without calling drm_bridge_remove(). The master cleanup function fails to remove it because priv->encoders[MESON_ENC_CVBS] is only populated upon successful probe completion.

During devres teardown, the reference count drops to 1, but the bridge is not freed. Can this leave the bridge accessible in the global bridge_list with dangling pointers to the freed priv structure, leading to memory corruption if accessed by other DRM subsystems?