Re: [PATCH v7 5/9] media: chips-media: wave6: Add Wave6 core driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [Critical] V4L2 lifecycle mismatch causes Use-After-Free of `core` structure when file descriptors are closed after driver unbind or probe error.
- [High] Polling a reference count with a timeout guarantees a Use-After-Free under scheduling delays.
- [High] Hardware interrupt events are dropped due to early returns and incomplete bitmask processing, causing instances to hang.
- [High] Delayed work `task_timer` is leaked on module remove, leading to a use-after-free.
- [High] The IRQ handler and thread can execute after driver teardown destroys critical resources, leading to Use-After-Free.
- [High] Hardware registers are accessed in the interrupt handler without validating that the device is not in a runtime PM suspended state.
- [High] Requesting the IRQ before enabling clocks and runtime PM can lead to a bus error if a spurious or pending interrupt fires.
- [High] DMA memory `core->temp_vbuf` is freed while the hardware may still be running.
- [Medium] Storing a debugfs dentry pointer after dropping its reference leads to a dangling pointer and Use-After-Free.
- [Medium] Debugfs directory is never removed, causing a memory leak.
--

commit 7b3f41c4df55c9ee33568cd7f9d838b5f4de81b9
Author: Nas Chung <nas.chung@chipsnmedia.com>

media: chips-media: wave6: Add Wave6 core driver
    
Add the core driver for the Chips&Media Wave6 video codec IP.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c b/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c
> new file mode 100644
> index 0000000000000..89cce27802cfe
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu-core.c
[ ... ]
> +static irqreturn_t wave6_vpu_core_irq(int irq, void *dev_id)
> +{
> +	struct vpu_core_device *core = dev_id;
> +	struct vpu_irq irq_info;
> +
> +	if (!vpu_read_reg(core, W6_VPU_VPU_INT_STS))
> +		return IRQ_NONE;

[Severity: High]
Does this code risk triggering a system bus error? The interrupt handler
unconditionally reads W6_VPU_VPU_INT_STS without first checking if the
device is runtime suspended. Subsystem guidelines recommend using
pm_runtime_get_if_active() before accessing hardware registers in an IRQ
handler to ensure clocks are enabled.

> +
> +	irq_info.status = vpu_read_reg(core, W6_VPU_VINT_REASON);
> +	irq_info.inst_idc = vpu_read_reg(core, W6_RET_INT_INSTANCE_INFO);
> +
> +	vpu_write_reg(core, W6_RET_INT_INSTANCE_INFO, INT_INSTANCE_INFO_CLEAR);
> +	vpu_write_reg(core, W6_VPU_VINT_REASON_CLEAR, irq_info.status);
> +	vpu_write_reg(core, W6_VPU_VINT_CLEAR, VINT_CLEAR);
> +
> +	trace_wave6_vpu_irq(core, irq_info.status, irq_info.inst_idc);
> +
> +	if (irq_info.status & BIT(W6_INT_BIT_REQ_WORK_BUF)) {
> +		if (core->vpu)
> +			core->vpu->req_work_buffer(core->vpu, core);
> +
> +		return IRQ_HANDLED;
> +	}

[Severity: High]
If W6_INT_BIT_REQ_WORK_BUF is set, this returns IRQ_HANDLED immediately.
Doesn't this discard any other pending interrupt events (like INIT_SEQ)
that might be present in irq_info.status, potentially causing other
instances to hang?

> +
> +	if (kfifo_in(&core->irq_fifo, &irq_info, sizeof(struct vpu_irq)) !=
> +	    sizeof(struct vpu_irq))
> +		dev_err(core->dev, "IRQ overflow status 0x%x idc 0x%x\n",
> +			irq_info.status, irq_info.inst_idc);
> +
> +	return IRQ_WAKE_THREAD;
> +}
> +
> +static struct vpu_instance *wave6_vpu_core_get_instance(struct vpu_core_device *core,
> +							u32 inst_idc)
> +{
> +	struct vpu_instance *inst;
> +
> +	guard(spinlock)(&core->inst_lock);
> +
> +	list_for_each_entry(inst, &core->instances, list) {
> +		if ((BIT(inst->id) & inst_idc) && inst->enable) {
> +			atomic_inc(&inst->refcount);
> +			return inst;
> +		}
> +	}

[Severity: High]
This loops through the instances and returns the first one that matches
a set bit in inst_idc. Since inst_idc is a bitmask, what happens if
multiple instances complete simultaneously? The IRQ thread will process
only this first instance and ignore the other set bits, causing them to hang.

> +
> +	return NULL;
> +}
[ ... ]
> +void wave6_vpu_disable_instance(struct vpu_instance *inst)
> +{
> +	int count;
> +
> +	scoped_guard(spinlock, &inst->dev->inst_lock)
> +		inst->enable = false;
> +
> +	if (read_poll_timeout(atomic_read, count, !count,
> +			      W6_VPU_POLL_DELAY_US,
> +			      W6_VPU_POLL_TIMEOUT,
> +			      true, &inst->refcount))
> +		dev_dbg(inst->dev->dev, "[%d] disable timeout\n", inst->id);
> +}

[Severity: High]
Is it safe to return from here if the polling times out? If the IRQ thread
is preempted and delayed, this timeout can expire while the IRQ thread still
holds a reference to the instance. The caller will assume the instance is
disabled and free it, leading to a use-after-free when the delayed IRQ
thread resumes and calls inst->ops->finish_process().

[ ... ]
> +static int wave6_vpu_core_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	ret = devm_request_threaded_irq(&pdev->dev, irq,
> +					wave6_vpu_core_irq,
> +					wave6_vpu_core_irq_thread,
> +					0, "vpu_irq", core);

[Severity: High]
Can requesting the IRQ here lead to a crash? The IRQ is requested before
pm_runtime_enable() is called and before the device clocks are powered on.
If a pending or spurious interrupt fires immediately, the handler will
read MMIO registers while the hardware clocks are disabled.

> +	if (ret) {
> +		dev_err(&pdev->dev, "failed to request IRQ: %d\n", ret);
> +		return ret;
> +	}
[ ... ]
> +	core->debugfs = debugfs_lookup(WAVE6_VPU_DEBUGFS_DIR, NULL);
> +	if (!IS_ERR_OR_NULL(core->debugfs))
> +		dput(core->debugfs);
> +	else
> +		core->debugfs = debugfs_create_dir(WAVE6_VPU_DEBUGFS_DIR, NULL);

[Severity: Medium]
Does this create a dangling pointer? The reference obtained from
debugfs_lookup() is immediately dropped via dput(), but the raw pointer is
retained in core->debugfs for later use. If the debugfs directory is removed
manually, subsequent uses of this pointer will cause a use-after-free.

[Severity: Medium]
Additionally, it appears the debugfs directory created here is never
tracked for removal. Repeatedly loading and unloading the driver, or failing
probe after this point, seems to permanently leak the debugfs directory.

> +
> +	pm_runtime_enable(&pdev->dev);
> +
> +	if (core->res->codec_types & WAVE6_IS_DEC) {
> +		ret = wave6_vpu_dec_register_device(core);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to register video_dev_dec: %d\n", ret);
> +			goto err_temp_vbuf_free;
> +		}
> +	}
> +	if (core->res->codec_types & WAVE6_IS_ENC) {
> +		ret = wave6_vpu_enc_register_device(core);
> +		if (ret) {
> +			dev_err(&pdev->dev,
> +				"failed to register video_dev_enc: %d\n", ret);
> +			goto err_dec_unreg;
> +		}
> +	}

[Severity: Critical]
Does this create a use-after-free condition via V4L2 lifecycle mismatch?
The core structure is allocated earlier using devm_kzalloc(), tying its
lifetime to the device binding. However, wave6_vpu_dec_register_device()
exposes the decoder to userspace here.

If wave6_vpu_enc_register_device() fails on the next step (or if the driver
is later unbound via sysfs), devres will free the core structure. If a
userspace process has already opened the /dev/videoX node and holds a file
descriptor, closing it later will invoke V4L2 release callbacks that
dereference the freed core structure.

[ ... ]
> +static void wave6_vpu_core_remove(struct platform_device *pdev)
> +{
> +	struct vpu_core_device *core = dev_get_drvdata(&pdev->dev);
> +
> +	wave6_vpu_enc_unregister_device(core);
> +	wave6_vpu_dec_unregister_device(core);
> +	pm_runtime_disable(&pdev->dev);
> +	wave6_vdi_free_dma(&core->temp_vbuf);

[Severity: High]
Is it safe to free this shared DMA memory without first instructing the
hardware to halt or waiting for active jobs to finish? If the VPU is still
running, it will continue DMA operations into the freed pages.

[Severity: High]
Also, does this function leak the delayed work? The driver schedules
core->task_timer during streaming, but cancel_delayed_work_sync() is never
called here. If the work is pending when the device unbinds, it will
execute later and dereference the freed core structure.

> +	wave6_vpu_release_m2m_dev(core);
> +	v4l2_device_unregister(&core->v4l2_dev);

[Severity: High]
Can the devres-managed IRQ thread execute concurrently with or after this
teardown? The IRQ was requested with devm_request_threaded_irq(), meaning
it remains active until after this remove() function returns. However,
wave6_vpu_release_m2m_dev() destroys M2M structures here. If an interrupt
fires during or just after this step, the still-active IRQ thread will
dereference the destroyed M2M and V4L2 structures.

> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1788496816.git.nas.chung@chipsnmedia.com?part=5