Re: [PATCH v6 8/9] media: chips-media: wave6: Add Wave6 control driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 13 potential issue(s) to consider:
- [Critical] Concurrent access to work buffer pool without synchronization causes data races and array out-of-bounds access.
- [Critical] Self-deadlock (ABBA) during VPU core unregistration due to holding vpu->lock.
- [Critical] Missing hardware quiescence during device removal allows the VPU to continue DMAing into freed memory.
- [High] Asynchronous firmware loading callback causes a use-after-free if the device is unbound before completion.
- [High] Pointer type mismatch when calling gen_pool_dma_alloc() leads to memory corruption and invalid DMA mapping.
- [High] Disabling PM runtime prematurely leaks clock enable references on the parent device.
- [High] Silent truncation of the firmware code buffer during memory remapping.
- [High] Calling of_irq_get() inside an asynchronous firmware load callback prevents handling -EPROBE_DEFER.
- [High] Failures in wave6_vpu_init_work_buf() are ignored, leading to a broken VPU boot state.
- [High] wave6_vpu_detach_pm_domains() destroys PM domain virtual devices before the devm-managed thermal cooling device is unregistered, creating a UAF window.
- [High] The driver frees the IRQ kfifo before disabling the IRQ during core removal, leading to Use-After-Free.
- [Medium] Conditionally calling pm_runtime_put_sync() based on pm_runtime_suspended() leaks the usage counter.
- [Medium] Checking dma_addr against 0 to determine validity skips unmapping on platforms where 0 is a valid DMA address.
--

commit e4a2fe67f6aa37393c77f5308ac691953a00a6e8
Author: Nas Chung <nas.chung@chipsnmedia.com>
Subject: media: chips-media: wave6: Add Wave6 control driver

This commit adds the control driver for the Chips&Media Wave6 video codec IP.
It handles the control region and manages shared resources such as firmware
loading, memory allocation, and the creation of wave6-core child devices.

> diff --git a/drivers/media/platform/chips-media/wave6/wave6-vpu.c b/drivers/media/platform/chips-media/wave6/wave6-vpu.c
> new file mode 100644
> index 0000000000000..c6efc45781833
> --- /dev/null
> +++ b/drivers/media/platform/chips-media/wave6/wave6-vpu.c
[ ... ]
> +static void wave6_vpu_remap_code_buf(struct wave6_vpu_device *vpu)
> +{
> +	dma_addr_t code_base = vpu->code_buf.dma_addr;
> +	u32 i, reg_val;
> +
> +	for (i = 0; i < wave6_vpu_get_code_buf_size(vpu) / W6_MAX_REMAP_PAGE_SIZE; i++) {

[Severity: High]
Does this code silently truncate the firmware code buffer during memory
remapping?

If the buffer size is not an exact multiple of W6_MAX_REMAP_PAGE_SIZE,
the integer division will discard the remainder and the tail end of the
buffer will not be mapped. If the firmware requires the unmapped tail,
could the VPU encounter a memory fault?

[ ... ]
> +static int wave6_vpu_init_vpu(struct wave6_vpu_device *vpu,
> +			      struct vpu_core_device *core)
> +{
[ ... ]
> +init_done:
> +	wave6_vpu_init_work_buf(vpu, core);
> +	wave6_vpu_set_state(vpu, WAVE6_VPU_STATE_ON);
> +
> +	return 0;
> +}

[Severity: High]
Is the return value of wave6_vpu_init_work_buf() intentionally ignored here?

If wave6_vpu_wait_busy() or wave6_vpu_check_result() fails inside
wave6_vpu_init_work_buf(), it will return early without setting
vpu->work_buffers_avail. Because the failure is ignored, the VPU state
transitions to WAVE6_VPU_STATE_ON anyway, and later requests for work
buffers might feed uninitialized addresses to the hardware.

[ ... ]
> +static void wave6_vpu_put(struct wave6_vpu_device *vpu,
> +			  struct vpu_core_device *core)
> +{
[ ... ]
> +	wave6_vpu_sleep(vpu, core);
> +
> +	if (!pm_runtime_suspended(vpu->dev))
> +		pm_runtime_put_sync(vpu->dev);
> +}

[Severity: Medium]
Could this conditional check cause the runtime PM usage counter to leak?

If the device's status is forcefully set to suspended, for example via
pm_runtime_force_suspend() during system sleep, pm_runtime_suspended()
will return true and this check will skip the put operation, leaving the
counter permanently incremented.

[ ... ]
> +static void wave6_vpu_require_work_buffer(struct wave6_vpu_device *vpu,
> +					  struct vpu_core_device *core)
> +{
[ ... ]
> +	if (WARN_ON(vpu->work_buffers_avail == 0))
> +		goto exit;
> +
> +	vpu->work_buffers_avail--;
> +	vb = &vpu->work_buffers[vpu->work_buffers_avail];

[Severity: Critical]
Is there a data race when accessing the work buffers pool?

Since wave6_vpu_require_work_buffer() modifies vpu->work_buffers_avail
without a lock, and wave6_vpu_core_irq() can call this from a hardirq
context across multiple child cores concurrently, could this result in
an array out-of-bounds access or assigning the exact same DMA buffer to
multiple instances?

[ ... ]
> +static int wave6_vpu_create_cores(struct wave6_vpu_device *vpu)
> +{
[ ... ]
> +		irq = of_irq_get(child, 0);
> +		if (irq < 0) {
> +			dev_warn(vpu->dev, "%pOF: missing interrupts property\n", child);
> +			continue;
> +		}

[Severity: High]
Does this error handling prevent the driver from properly handling probe
deferral?

Because wave6_vpu_create_cores() executes in the asynchronous firmware loader
worker context, returning -EPROBE_DEFER here won't defer the probe process.
If the IRQ controller probes later, the core device might be permanently
ignored instead of waiting for the controller.

[ ... ]
> +static void wave6_vpu_release(struct wave6_vpu_device *vpu)
> +{
> +	guard(mutex)(&vpu->lock);
> +
> +	vpu->fw_available = false;
> +	wave6_vpu_destroy_cores(vpu);
> +	wave6_vpu_free_work_buffers(vpu);

[Severity: Critical]
Can holding vpu->lock during core destruction result in a self-deadlock?

When wave6_vpu_destroy_cores() unregisters the core devices, the child
removal triggers pm_runtime_disable() which blocks via __pm_runtime_barrier()
waiting for ongoing PM transitions. If another thread is executing
wave6_vpu_put() or wave6_vpu_get() inside a runtime PM callback, it will
block trying to acquire vpu->lock, causing a deadlock.

[Severity: Critical]
Are the work buffers and SRAM DMA memory freed while the VPU hardware
might still be accessing them?

This function explicitly frees the DMA memory, but it doesn't appear to send
a sleep command, disable the PM runtime properly, or assert a hardware reset
first. If the hardware is still active, could it continue performing DMA into
the kernel memory that was just released?

[Severity: High]
When wave6_vpu_destroy_cores() tears down the core devices, does the
sub-driver wave6-vpu-core.c free the IRQ kfifo before disabling the IRQ?

Looking at wave6_vpu_core_remove(), it calls kfifo_free(&core->irq_fifo)
manually. Since devm_request_threaded_irq() is used, the IRQ is only
disabled after the remove routine completes. If an interrupt fires during
this window, the IRQ handler might call kfifo_in() on a freed structure.

[ ... ]
> +	if (vpu->code_buf.dma_addr)
> +		dma_unmap_resource(vpu->dev,
> +				   vpu->code_buf.dma_addr,

[Severity: Medium]
Is it safe to check the DMA address against 0 to determine if it is valid?

On architectures where 0 is a valid DMA address, this condition will
evaluate to false, which would skip the dma_unmap_resource() call and
permanently leak the mapping.

[ ... ]
> +static void wave6_vpu_load_firmware(const struct firmware *fw, void *context)
> +{
> +	struct wave6_vpu_device *vpu = context;
> +
> +	guard(mutex)(&vpu->lock);

[Severity: High]
Can this asynchronous callback trigger a use-after-free if the device is
unbound?

If the device is unbound via sysfs while the firmware load is still pending,
devres will free the devm-allocated vpu structure. Because there is no
cancellation mechanism on unbind, the callback might execute later and
dereference the freed vpu pointer when trying to acquire the lock.

[ ... ]
> +static int wave6_vpu_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	vpu->sram_pool = of_gen_pool_get(pdev->dev.of_node, "sram", 0);
> +	if (vpu->sram_pool) {
> +		vpu->sram_buf.size = vpu->res->sram_size;
> +		vpu->sram_buf.vaddr = gen_pool_dma_alloc(vpu->sram_pool,
> +							 vpu->sram_buf.size,
> +							 &vpu->sram_buf.phys_addr);

[Severity: High]
Is the third argument to gen_pool_dma_alloc() using the correct pointer type?

The function gen_pool_dma_alloc() expects a dma_addr_t pointer for its
third argument, but here it receives a phys_addr_t pointer. On 32-bit
platforms where phys_addr_t and dma_addr_t have different sizes, could this
overwrite adjacent struct members? Furthermore, it returns a device DMA
address which is later passed to dma_map_resource() as if it were a physical
address.

[ ... ]
> +	ret = firmware_request_nowait_nowarn(THIS_MODULE,
> +					     vpu->res->fw_name,
> +					     &pdev->dev,
> +					     GFP_KERNEL,
> +					     vpu,
> +					     wave6_vpu_load_firmware);

[Severity: High]
Does this firmware request need a cancellation mechanism on unbind?

As noted above in wave6_vpu_load_firmware(), without a way to cancel
the pending firmware request or hold a reference to the vpu structure,
an unbind operation could lead to a use-after-free when the callback
eventually runs.

[ ... ]
> +static void wave6_vpu_remove(struct platform_device *pdev)
> +{
> +	struct wave6_vpu_device *vpu = dev_get_drvdata(&pdev->dev);
> +
> +	pm_runtime_disable(vpu->dev);
> +	wave6_vpu_release(vpu);
> +	wave6_vpu_detach_pm_domains(vpu);
> +}

[Severity: High]
Does disabling the PM runtime prematurely here leak clock enable references?

When pm_runtime_disable() is called before the child cores are released via
wave6_vpu_release(), the parent's PM runtime is disabled. When the child
cores subsequently drop their references to the parent device, the parent's
runtime_suspend callback won't execute, which might leave the hardware
clocks running indefinitely.

[Severity: High]
Does detaching the PM domains here create a use-after-free window for
the thermal cooling device?

wave6_vpu_detach_pm_domains() destroys the PM domain virtual devices.
However, the thermal cooling device, which uses one of these virtual devices
as its parent, was registered with devm_ and won't be cleaned up until after
wave6_vpu_remove() returns. If a thermal event fires in that window, could
it access the freed virtual device?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260624072043.238-1-nas.chung@chipsnmedia.com?part=8