Re: [PATCH v0 1/4] media: chips-media: wave5: Guard bit depth check with initial_info_obtained

[sashiko-bot@kernel.org]

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

New issues:
- [High] Data race when checking `initial_info_obtained` and `luma_bitdepth` without locks, leading to a TOCTOU-like race that reproduces the original bug.
- [High] Bypassing the bitdepth check for unresolved sequences reintroduces a timeout for unsupported 10-bit HEVC streams on WAVE521C.

Pre-existing issues:
- [Critical] Kernel heap buffer overflow in `wave5_prepare_fb` due to unvalidated `fbc_buf_count` and `linear_num`.
- [Critical] Physical memory corruption (DMA overflow) in `wave5_prepare_fb` due to missing buffer size validation against bitstream dimensions.
- [High] Ignored return value of `pm_runtime_resume_and_get()` leads to unpowered hardware access and usage count underflow.
- [High] Lockless modification of `inst->state` in `wave5_vpu_dec_start_streaming()`.
--

commit 26ff861a58ecc6c2122ff143493bf266fceeafb4
Author: Jackson Lee <jackson.lee@chipsnmedia.com>

media: chips-media: wave5: Guard bit depth check with initial_info_obtained

When CAPTURE STREAMON is called before the VPU has completed sequence
initialization, the initial_info fields contain uninitialized data. The driver
checks luma_bitdepth and rejects anything other than 8-bit, so garbage values
cause STREAMON to fail spuriously. This fix checks initial_info_obtained
before accessing the bit depth fields.

> diff --git a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> index bb2ba9204a836..01d1368b29656 100644
> --- a/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> +++ b/drivers/media/platform/chips-media/wave5/wave5-vpu-dec.c
> @@ -1403,6 +1403,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  	} else if (q->type == V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE) {
>  		struct dec_initial_info *initial_info =
>  			&inst->codec_info->dec_info.initial_info;
> +		struct dec_info *p_dec_info = &inst->codec_info->dec_info;
>  
>  		if (inst->state == VPU_INST_STATE_STOP)
>  			ret = switch_state(inst, VPU_INST_STATE_INIT_SEQ);

[Severity: High]
This is a pre-existing issue, but does this code safely modify inst->state?

The state appears to be protected by inst->state_spinlock, but here it is
read and modified directly without acquiring the lock.

If the M2M worker thread modifies the state asynchronously via
set_instance_state() which takes the lock, could this lack of serialization
result in an inconsistent internal driver state machine?

> @@ -1410,6 +1411,7 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  			goto return_buffers;
>  
>  		if (inst->state == VPU_INST_STATE_INIT_SEQ &&
> +		    p_dec_info->initial_info_obtained &&
>  		    inst->dev->product_code == WAVE521C_CODE) {

[Severity: High]
Can this introduce a data race when checking p_dec_info->initial_info_obtained
and initial_info->luma_bitdepth?

If the M2M worker thread in wave5_vpu_dec_complete_seq_init() updates this
state concurrently:

wave5_vpu_dec_complete_seq_init()
    if (!ret)
        p_dec_info->initial_info_obtained = true;
    ...
    p_dec_info->initial_info = *info;

Could a concurrent ioctl observe initial_info_obtained as true while
luma_bitdepth is still uninitialized, observing garbage and reproducing the
same spurious failure this patch intended to resolve?

[Severity: High]
By bypassing the bitdepth validation here when initial_info_obtained is false,
do we defer the rejection of unsupported 10-bit streams to the worker thread?

If wave5_prepare_fb() catches the 10-bit stream later, its error handler
in wave5_vpu_dec_device_run() simply stops the instance:

wave5_vpu_dec_device_run()
    ret = wave5_prepare_fb(inst);
    if (ret) {
        ...
        set_instance_state(inst, VPU_INST_STATE_STOP);
        break;
    }

Since this doesn't return the queued capture buffers via v4l2_m2m_buf_done(),
could userspace hang indefinitely on DQBUF waiting for buffers that will
never arrive?

>  			if (initial_info->luma_bitdepth != 8) {
>  				dev_info(inst->dev->dev, "%s: no support for %d bit depth",
> @@ -1418,7 +1420,6 @@ static int wave5_vpu_dec_start_streaming(struct vb2_queue *q, unsigned int count
>  				goto return_buffers;
>  			}
>  		}
> -
>  	}
>  	pm_runtime_put_autosuspend(inst->dev->dev);

[Severity: High]
This is a pre-existing issue, but is it safe to ignore the return value of
pm_runtime_resume_and_get() earlier in this function?

If the initial resume fails, it returns a negative error and intrinsically
drops the usage counter.

Proceeding unconditionally and calling pm_runtime_put_autosuspend() here
would cause a double-decrement of the PM usage counter. Could this lead to
an underflow and unpowered hardware access?

>  	return ret;

[ ... ]

Additionally, there are some pre-existing issues in other parts of the driver
that might be worth reviewing:

[Severity: Critical]
This is a pre-existing issue, but does wave5_prepare_fb() properly bound
the number of frame buffers?

The function loops up to non_linear_num (which comes from the bitstream)
and linear_num (userspace queued capture buffers):

wave5_prepare_fb()
    for (i = 0; i < non_linear_num; i++) {
        struct frame_buffer *frame = &inst->frame_buf[i];

    ...
    for (i = 0; i < linear_num; i++) {
        struct frame_buffer *frame = &inst->frame_buf[non_linear_num + i];

The bounds check (num_of_decoding_fbs >= WAVE5_MAX_FBS) seems to happen
after these loops. Since the inst->frame_buf array is fixed size, could a
malicious bitstream or a large number of queued buffers cause an
out-of-bounds write into inst->frame_buf[] here?

[Severity: Critical]
This is a pre-existing issue, but does wave5_prepare_fb() verify the size
of the backing vb2 buffer before computing physical DMA addresses?

When computing the addresses based on dynamic bitstream dimensions:

wave5_prepare_fb()
    buf_addr_y = vb2_dma_contig_plane_dma_addr(vb, 0);
    buf_addr_cb = buf_addr_y + luma_size;
    buf_addr_cr = buf_addr_cb + chroma_size;

If userspace continues streaming with previously queued small buffers, could
the VPU hardware decode a large frame into undersized physical addresses
and cause out-of-bounds DMA writes?

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