Re: [PATCH v0 2/4] media: chips-media: wave5: Set inst->std during default format initialization

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Error paths in the open handlers improperly call `ida_free()` with unallocated or negative IDs, corrupting the IDA state and risking instance ID aliasing.
- [High] The V4L2 M2M context is leaked on error paths during instance open because `v4l2_m2m_ctx_release` is never called.
- [High] The file release handler `wave5_vpu_release_device` can abort early on pending signals, permanently leaking the instance and leaving a dangling pointer.
- [High] The condition to free the device SRAM buffer is logically impossible to satisfy, permanently leaking the `gen_pool` allocation for non-WAVE515 chips.
- [High] `inst->state` is read and modified concurrently without any locks in the encoder, leading to race conditions and invalid state transitions.
- [High] Concurrent file opens race to allocate the shared device SRAM buffer, causing memory leaks and pointer corruption.
--

commit 4066d4d2cd1d6dd5099164b5a1d648c7737c2049
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: Set inst->std during default format initialization

This commit fixes an issue where inst->std remains uninitialized during the
default capture format setup, causing STREAMON to fail if S_FMT hasn't been
called. It resolves this by setting inst->std via wave5_to_vpu_std() in
wave5_set_default_format() to keep the codec type consistent.

Fixes: 9707a6254a8a ("media: chips-media: wave5: Add the v4l2 layer")
Cc: stable@vger.kernel.org
Signed-off-by: Jackson Lee <jackson.lee@chipsnmedia.com>
Signed-off-by: Nas Chung <nas.chung@chipsnmedia.com>

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> index e6c94b6f26711..f9fcdf4c224b3 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-enc.c
[ ... ]
> @@ -1770,7 +1772,7 @@ static int wave5_vpu_open_enc(struct file *filp)
>  	inst->v4l2_fh.ctrl_handler = v4l2_ctrl_hdl;
>  	v4l2_ctrl_handler_setup(v4l2_ctrl_hdl);
>  
> -	wave5_set_default_format(&inst->src_fmt, &inst->dst_fmt);
> +	wave5_set_default_format(inst, &inst->src_fmt, &inst->dst_fmt);
>  	inst->conf_win.width = inst->dst_fmt.width;
>  	inst->conf_win.height = inst->dst_fmt.height;
>  	inst->colorspace = V4L2_COLORSPACE_REC709;

[Severity: High]
This isn't a bug introduced by this patch, but there are multiple issues on
the error paths in wave5_vpu_open_enc() jumping to cleanup_inst.

If an initialization step fails before ida_alloc() is called:

wave5-vpu-enc.c:wave5_vpu_open_enc() {
    ...
	ret = wave5_kfifo_alloc(inst);
	if (ret) {
		dev_err(inst->dev->dev, "failed to allocate fifo\n");
		goto cleanup_inst;
	}
    ...
cleanup_inst:
	wave5_cleanup_instance(inst, filp);
    ...
}

Does this call ida_free() with an unallocated or negative ID?

wave5-helper.c:wave5_cleanup_instance() {
    ...
	ida_free(&inst->dev->inst_ida, inst->id);
    ...
}

Since inst->id is 0-initialized, if it fails before ida_alloc(), this frees
ID 0. If another instance is using ID 0, the next open will reuse it,
causing aliasing. If ida_alloc() fails and returns a negative error code,
does this cause an out-of-bounds access?

Also, wave5_cleanup_instance() does not call v4l2_m2m_ctx_release(). Does
this permanently leak the m2m_ctx allocated earlier in wave5_vpu_open_enc()
and leave a dangling pointer?

[Severity: High]
This is a pre-existing issue, but do concurrent file opens race to allocate
the shared device SRAM buffer?

In wave5_vpu_open_enc(), wave5_vdi_allocate_sram() is called without holding
dev->dev_lock:

wave5-vpu-enc.c:wave5_vpu_open_enc() {
    ...
	wave5_vdi_allocate_sram(inst->dev);

	ret = mutex_lock_interruptible(&dev->dev_lock);
    ...
}

Inside wave5_vdi_allocate_sram():

wave5-vdi.c:wave5_vdi_allocate_sram() {
    ...
	if (!vpu_dev->sram_pool || vb->vaddr)
		return;

	size = min_t(size_t, vpu_dev->sram_size,
	             gen_pool_avail(vpu_dev->sram_pool));
	vaddr = gen_pool_dma_alloc(vpu_dev->sram_pool, size, &daddr);
    ...
}

If multiple processes open the device simultaneously, can they race past the
vb->vaddr check, leak the gen_pool allocation, and corrupt the shared
pointers?

[Severity: High]
This isn't a bug introduced by this patch, but can wave5_vpu_release_device()
abort early and leak the instance?

wave5-helper.c:wave5_vpu_release_device() {
    ...
	ret = mutex_lock_interruptible(&inst->dev->irq_lock);
	if (ret)
		return ret;
    ...
}

If a process is exiting due to a fatal signal, this lock acquisition will
fail and return -ERESTARTSYS. Because this is the file release handler, the
kernel will destroy the file descriptor anyway. Does this permanently leak
the instance resources and leave a dangling pointer in dev->instances?
Should this use mutex_lock() instead?

[Severity: High]
This is a pre-existing issue, but is the condition to free the device SRAM
buffer logically impossible to satisfy?

wave5-helper.c:wave5_vpu_release_device() {
    ...
	list_del_init(&inst->list);
	wave5_cleanup_instance(inst, filp);
    ...
}

wave5-helper.c:wave5_cleanup_instance() {
    ...
	if (list_is_singular(&inst->list) &&
	    inst->dev->product_code != WAVE515_CODE)
		wave5_vdi_free_sram(inst->dev);
    ...
}

Because list_del_init() reinitializes inst->list as empty,
list_is_singular() will always return false. Does this permanently leak the
gen_pool allocation for non-WAVE515 chips? Should this check if
inst->dev->instances is empty instead?

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
inst->state in the encoder?

wave5-vpu-enc.c:switch_state() {
    ...
	inst->state = state;
	return 0;
}

In the encoder, switch_state() directly modifies inst->state without any
locks. Ioctl threads and the V4L2 M2M worker thread can concurrently mutate
this state. Can this cause the worker thread to observe intermediate states,
leading to invalid state transitions? The decoder module seems to protect
state mutations using inst->state_spinlock.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604020155.77-1-jackson.lee@chipsnmedia.com?part=2