Re: [PATCH v10 4/6] decoder: Add V4L2 stateless H.264 decoder driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 20 potential issue(s) to consider:
- [Critical] media: amlogic: arbitrary physical memory read via unvalidated first_mb_in_slice
- [Critical] media: amlogic: heap buffer overflow in reference list reordering
- [High] media: amlogic: soft lockup via atomic busy-wait in aml_stop_vdec_hw()
- [High] media: amlogic: WARN_ON due to double completion of V4L2 control requests
- [High] media: amlogic: memory leak of h264 context and DMA buffers on STREAMON
- [High] media: amlogic: shifting hardware RefPicList indices on invalid reference
- [High] media: amlogic: incorrect base address for collocated buffers due to 8x8 shift
- [High] media: amlogic: VIFIFO write pointer programmed beyond ring buffer bounds
- [High] media: amlogic: infinite poll timeout deadlocks the threaded IRQ handler
- [High] media: amlogic: out-of-bounds DMA write due to undersized buffers for interlaced
- [High] media: amlogic: UAF when VIDIOC_STREAMOFF frees context concurrently
- [High] media: amlogic: OOB DMA write of collocated MVs during dynamic resolution change
- [High] media: amlogic: UAF of M2M device and devm resources when fd outlives remove
- [High] media: amlogic: threaded IRQ accesses powered-off registers during remove
- [High] media: amlogic: context cleanup unconditionally resets shared hardware
- [High] media: amlogic: early device registration exposes unpowered hardware to userspace
- [High] media: amlogic: missing checks on clock and power domain enablement
- [Medium] media: amlogic: allocation failure for large firmware using kzalloc
- [Medium] media: amlogic: signed integer overflow in collocated buffer bitmask
- [Low] media: amlogic: out-of-bounds read of reorder_cmd array due to off-by-one check
--

commit d745d294471b0d176edba67b04bee8a0c6b95c04
Author: Zhentao Guo <zhentao.guo@amlogic.com>

decoder: Add V4L2 stateless H.264 decoder driver

This commit adds initial support for the v4l2 stateless video decoder driver
on the Amlogic S4 (S805X2) platform, supporting 8-bit H.264 progressive 
bitstream decoding.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec.c b/drivers/media/platform/amlogic/vdec/aml_vdec.c
> --- a/drivers/media/platform/amlogic/vdec/aml_vdec.c
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec.c
[ ... ]
> @@ m2mops_vdec_device_run @@
>  	if (src_req)
>  		v4l2_ctrl_request_complete(src_req, &ctx->ctrl_handler);
>  	if (ret < 0 || ctx->curr_frm_err_flag)
>  		goto err_cancel_job;
>  	v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx,
>  					 VB2_BUF_STATE_DONE);
[ ... ]
> @@ vb2ops_vdec_buf_request_complete @@
>  static void vb2ops_vdec_buf_request_complete(struct vb2_buffer *vb)
>  {
>  	struct aml_vdec_ctx *ctx = vb2_get_drv_priv(vb->vb2_queue);
>  
>  	v4l2_ctrl_request_complete(vb->req_obj.req, &ctx->ctrl_handler);
>  }

[Severity: High]
Will this sequence cause a double completion of v4l2 control requests? 

Since m2mops_vdec_device_run() manually calls v4l2_ctrl_request_complete() 
for the source request and then marks the buffer done, the videobuf2 core 
automatically invokes vb2ops_vdec_buf_request_complete(). This secondary 
callback invokes v4l2_ctrl_request_complete() again on the same request, 
which triggers a warn_on() in the media request core.

[ ... ]
> @@ vb2ops_vdec_start_streaming @@
>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>  		struct vb2_v4l2_buffer *src_buf;
>  
>  		if (aml_vdec_init_dec_inst(ctx) < 0) {
>  			while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx)))
[ ... ]
> @@ vb2ops_vdec_stop_streaming @@
>  	if (!ctx->is_output_streamon && !ctx->is_cap_streamon)
>  		aml_vdec_release_instance(ctx);

[Severity: High]
Could this leak the h264 context and dma buffers on stream toggle?

Calling vidioc_streamon for the output queue triggers 
vb2ops_vdec_start_streaming(), which blindly allocates a new context via 
aml_h264_init(). If a user calls vidioc_streamoff on the output queue while 
the capture queue remains active, vb2ops_vdec_stop_streaming() skips freeing 
the context because both queues are not stopped. A subsequent streamon on 
the output queue will allocate a new context, irreversibly dropping the 
pointers to the old context and its dma allocations.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c b/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c
> --- a/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c
[ ... ]
> @@ fops_vcodec_release @@
>  static int fops_vcodec_release(struct file *file)
>  {
>  	struct aml_vdec_ctx *ctx = fh_to_dec_ctx(file);
>  
>  	v4l2_m2m_ctx_release(ctx->m2m_ctx);
[ ... ]
> @@ aml_vdec_drv_remove @@
>  	vdec_disable(dev->dec_hw);
>  	media_device_unregister(&dev->mdev);
>  	v4l2_m2m_unregister_media_controller(dev->m2m_dev_dec);
>  	video_unregister_device(dev->vfd);
>  	media_device_cleanup(&dev->mdev);
>  
>  	v4l2_m2m_release(dev->m2m_dev_dec);
>  	dev->pvdec_data->destroy_hw_resource(dev);

[Severity: High]
Can this lead to a use-after-free if the file descriptor outlives the 
device removal? 

In aml_vdec_drv_remove(), v4l2_m2m_release() explicitly frees the m2m device.
Shortly after, the devm infrastructure frees the aml_vdec_dev structure. 
Because video_unregister_device() does not forcibly close open file 
descriptors, an open file descriptor closed later triggers 
fops_vcodec_release(), which accesses the manually freed m2m_dev and the 
devm-freed ctx->dev.

[Severity: High]
Does this expose unpowered hardware to userspace during registration and 
removal?

In aml_vdec_drv_probe(), video_register_device() exposes the video node 
to userspace before vdec_enable() powers on the hardware domains. Userspace 
could open the device and attempt to start jobs. 

Similarly, in aml_vdec_drv_remove(), vdec_disable() shuts down hardware 
clocks before video_unregister_device() and v4l2_m2m_release() safely 
cancel ongoing m2m jobs. Accessing unclocked or unpowered mmio registers 
often results in a synchronous external abort.

[Severity: High]
Could the threaded irq handler execute and access unpowered registers here?

The hardware is shut down via vdec_disable() before the interrupt requested 
via devm_request_threaded_irq() is disabled and synchronized. If an 
interrupt fires just before vdec_disable(), the threaded isr is scheduled. 
The running h264_threaded_isr_func() will unconditionally read from DOS_BUS 
registers like DPB_STATUS_REG, triggering a synchronous external abort.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c b/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c
> --- a/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c
[ ... ]
> @@ aml_stop_vdec_hw @@
>  	ret = read_poll_timeout_atomic(read_dos_reg, reg_val,
>  				       !(reg_val & 0x8000),
>  				       10, 100000, true,
>  				       hw, IMEM_DMA_CTRL);
>  
>  	ret = read_poll_timeout_atomic(read_dos_reg, reg_val,
>  				       !(reg_val & 0x8000),
>  				       10, 100000, true,
>  				       hw, LMEM_DMA_CTRL);
>  
>  	ret = read_poll_timeout_atomic(read_dos_reg, reg_val,
>  				       !(reg_val & 0xfff),
>  				       10, 300000, true,
>  				       hw, WRRSP_LMEM);

[Severity: High]
Does this stall the cpu for hundreds of milliseconds?

The atomic variants of read_poll_timeout internally loop with udelay(), 
forcing the cpu to busy-wait continuously. Waiting up to 300ms atomically 
is highly likely to trigger rcu stalls and the soft lockup watchdog. 
Since the callers such as aml_h264_exit() execute in sleepable contexts, 
the non-atomic read_poll_timeout() should be used instead.

[ ... ]
> @@ pm_vdec_power_domain_power_on @@
>  	if (gate_node) {
>  		clk_prepare_enable(gate_node->clk);
[ ... ]
>  	if (dev) {
>  		pm_runtime_get_sync(dev);

[Severity: High]
Are the return values for clock and power domain enablement intentionally 
ignored here?

If these calls fail to enable the hardware resources, proceeding to access 
mmio registers in dos_local_config() will result in a synchronous external 
abort, crashing the system.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c b/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c
> --- a/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c
[ ... ]
> @@ get_firmware @@
>  	/* get rid of the first 32K bytes plaintext */
>  	*size = fw->size - UCODE_HEADER_SIZE;
>  	buf = kzalloc(*size, GFP_KERNEL);
>  	if (!buf) {

[Severity: Medium]
Will this kzalloc() succeed for large video firmware blobs on a fragmented 
system? 

Video firmware is typically large, and kzalloc() requires physically 
contiguous pages. Since this buffer is only used to stage data copying 
into tee shared memory in chunks, physical contiguity seems unnecessary. 
Using vmalloc() or kvzalloc() might be more reliable.

> diff --git a/drivers/media/platform/amlogic/vdec/h264.c b/drivers/media/platform/amlogic/vdec/h264.c
> --- a/drivers/media/platform/amlogic/vdec/h264.c
> +++ b/drivers/media/platform/amlogic/vdec/h264.c
[ ... ]
> @@ h264_prepare_input @@
>  	regmap_write(hw->map[DOS_BUS], VLD_MEM_VIFIFO_RP,
>  		     round_down(src_dma, VDEC_FIFO_ALIGN));
>  	dummy = payload_size + VLD_PADDING_SIZE;
>  	regmap_write(hw->map[DOS_BUS], VLD_MEM_VIFIFO_WP,
>  		     round_down((src_dma + dummy), VDEC_FIFO_ALIGN));

[Severity: High]
Can this ring buffer write pointer exceed the defined boundaries?

The hardware bitstream ring buffer is configured with START_PTR at the start 
of the dma buffer and END_PTR strictly at the end of the payload. 
Programming WP past END_PTR causes the hardware to attempt an out-of-bounds 
physical memory read. Also, because the read pointer mathematically wraps 
at END_PTR, it will never equal the out-of-bounds WP, causing the hardware 
to believe the fifo is infinitely full and spin endlessly.

[ ... ]
> @@ config_sps_params @@
>  	h264_ctx->frame_width = (sps->pic_width_in_mbs_minus1 + 1) << 4;
>  	h264_ctx->frame_height = (sps->pic_height_in_map_units_minus1 + 1) << 4;

[Severity: High]
Is there a risk of undersized buffer allocations for interlaced streams?

The driver computes frame_height directly from pic_height_in_map_units_minus1 
without checking the frame_mbs_only_flag. For interlaced content, this 
specifies half the actual frame height. This results in alloc_colocate_cma() 
allocating a half-sized buffer, causing the hardware to perform an 
out-of-bounds dma write during decoding. Progressive-only drivers typically 
reject streams missing the V4L2_H264_SPS_FLAG_FRAME_MBS_ONLY flag.

[ ... ]
> @@ reorder_short_term @@
>  	memcpy(&ref_list_reordered[*ref_idx_lx], pic_lx, sizeof(struct h264_decode_buf_spec));
>  	dev_dbg(&ctx->dev->plat_dev->dev, "%s : RefPicListX[%d ] = pic %p pic_num(%d)\n", __func__,
>  		*ref_idx_lx, pic_lx, ref_list_reordered[*ref_idx_lx].dpb->pic_num);
>  	*ref_idx_lx = *ref_idx_lx + 1;
[ ... ]
> @@ reorder_long_term @@
>  	memcpy(&ref_list[*ref_idx_lx], pic_lt, sizeof(struct h264_decode_buf_spec));
>  	dev_dbg(&ctx->dev->plat_dev->dev, "%s : RefPicListX[%d ] = pic %p pic_num(%d)\n", __func__,
>  		*ref_idx_lx, pic_lt, ref_list[*ref_idx_lx].dpb->pic_num);
>  	*ref_idx_lx = *ref_idx_lx + 1;
[ ... ]
> @@ reorder_pics @@
>  	pic_num_lx_pred = curr_pic_num;
>  	for (i = 0; i < REORDERING_COMMAND_MAX_SIZE && modification_of_pic_nums_idc[i] != 3; i++) {

[Severity: Critical]
Could this loop overflow the ref_list arrays?

The ref_list arrays in aml_h264_ctx are statically sized to 17 elements. 
However, the loop processes up to REORDERING_COMMAND_MAX_SIZE (33) 
reordering commands. Because there is no bounds check ensuring 
*ref_idx_lx < 17 before the memcpy operations in reorder_short_term and 
reorder_long_term, a crafted bitstream with excessive reorder commands can 
trigger a deterministic out-of-bounds write of h264_decode_buf_spec structs 
into adjacent heap memory.

[ ... ]
> @@ alloc_colocate_cma @@
>  	if (h264_ctx->collated_cma_vaddr)
>  		return 0;
>  
>  	hw = vdec_get_hw(ctx->dev);
>  	if (!hw)
>  		return -1;
>  
>  	/* 96 :col buf size for each mb */
>  	h264_ctx->one_col_buf_size = h264_ctx->mb_total * 96;

[Severity: High]
Does this logic correctly update the collocated buffer size during dynamic 
resolution changes?

When a stream dynamically increases in resolution, h264_ctx->mb_total 
increases. However, the early return in alloc_colocate_cma() prevents 
one_col_buf_size from updating. When the hardware later decodes a larger 
frame, it processes the new mb_total macroblocks but the pointer 
calculations rely on the old size, causing an out-of-bounds dma write 
outside the cma buffer bounds.

[ ... ]
> @@ allocate_colocate_buf @@
>  	for (i = 0; i < h264_ctx->colocated_buf_num; i++) {
>  		if (((h264_ctx->colocated_buf_map >> i) & 0x1) == 0) {
>  			h264_ctx->colocated_buf_map |= (1 << i);
>  			break;
>  		}
>  	}
[ ... ]
> @@ release_colocate_buf @@
>  		} else {
>  			if (((h264_ctx->colocated_buf_map >> index) & 0x1) == 0x1) {
>  				h264_ctx->colocated_buf_map &= (~(1 << index));

[Severity: Medium]
Can shifting by 31 bits trigger signed integer overflow?

The expression (1 << i) uses a signed 32-bit integer. If the 31st 
collocated buffer is accessed, it overflows into the sign bit, resulting 
in undefined behavior. Using 1U << i might be safer.

[ ... ]
> @@ get_modification_cmd @@
>  			i++;
>  
>  			if (i >= REORDERING_COMMAND_MAX_SIZE) {
>  				curr_slice->ref_pic_list_reordering_flag[list] = 0;
>  				break;
>  			};
>  			if (j > REORDER_CMD_MAX) {
>  				curr_slice->ref_pic_list_reordering_flag[list] = 0;
>  				break;
>  			};
>  		} while (val != 3);

[Severity: Low]
Is the loop termination condition correct here?

Checking if (j > REORDER_CMD_MAX) fails to break the loop when j reaches 
the array size (66). In the next iteration, the code reads exactly one 
element past the end of the reorder_cmd array.

[ ... ]
> @@ h264_config_decode_buf @@
>  	for (i = 0; i < h264_ctx->list_size[0]; i++) {
>  		if (!h264_ctx->ref_list0[i].valid)
>  			continue;
>  		canvas_pos = h264_ctx->ref_list0[i].canvas_pos;

[Severity: High]
Does skipping invalid reference entries corrupt the hardware's expected 
index array?

If a reference frame is marked as invalid, using continue skips 
accumulating ref_cfg_once and prevents j from incrementing. This compacts 
the reference list, causing all subsequent valid references to shift into 
earlier slots. Shifting the entries breaks temporal prediction and results 
in severe visual corruption.

[ ... ]
> @@ h264_config_decode_buf @@
>  		colocate_wr_adr = (unsigned int)h264_ctx->collated_cma_addr +
>  		    ((h264_ctx->one_col_buf_size *
>  		      h264_ctx->curr_spec.col_buf_index) >> (use_mode_8x8_flag ? 2 : 0));
>  		if (colocate_adr_offset > h264_ctx->one_col_buf_size ||
[ ... ]
>  			colocate_rd_adr = (unsigned int)h264_ctx->collated_cma_addr +
>  			    ((h264_ctx->one_col_buf_size *
>  			      colocate_pic->col_buf_index) >> (use_mode_8x8_flag
>  							       ? 2 : 0));

[Severity: High]
Is the 8x8 shift applied to the correct part of the offset?

By shifting the entire base address computation instead of just the 
intra-buffer offset, the physical base addresses diverge. If a reference 
frame was written with 8x8 mode but read by a current frame without 8x8 
mode, the hardware will read from mismatched physical memory offsets, 
fetching garbage motion vectors.

[ ... ]
> @@ h264_config_decode_buf @@
>  	read_poll_timeout(read_dos_reg, reg_val,
>  			  !(reg_val & 0x800),
>  			  10, 0, true, hw, H264_CO_MB_RW_CTL);

[Severity: High]
Will a timeout parameter of 0 create an infinite loop?

Using read_poll_timeout with a timeout_us argument of 0 creates an infinite 
loop. If the hardware bit fails to clear, the threaded irq handler will spin 
forever. Because it holds the driver's global hw->ctx_mutex, this will 
permanently deadlock the decoder.

[ ... ]
> @@ h264_config_decode_buf @@
>  	colocate_adr_offset = COL_SIZE_FOR_ONE_MB;
>  	if (use_mode_8x8_flag)
>  		colocate_adr_offset >>= 2;
>  	colocate_adr_offset *= curr_slice->first_mb_in_slice;
[ ... ]
>  		colocate_wr_adr = (unsigned int)h264_ctx->collated_cma_addr +
>  		    ((h264_ctx->one_col_buf_size *
>  		      h264_ctx->curr_spec.col_buf_index) >> (use_mode_8x8_flag ? 2 : 0));
>  		if (colocate_adr_offset > h264_ctx->one_col_buf_size ||
[ ... ]
>  			val  = ((colocate_rd_adr_offset + colocate_rd_adr) >> 3) |
>  				(cur_colocate_ref_type << 29) |
>  				(l10_structure << 30);
>  			regmap_write(hw->map[DOS_BUS], H264_CO_MB_RD_ADDR, val);

[Severity: Critical]
Is it safe to use first_mb_in_slice without validation?

This value is parsed directly from the untrusted h264 bitstream and 
multiplied to compute colocate_rd_adr_offset. While there is a bounds 
check for colocate_rd_adr + one_col_buf_size, there is no validation that 
colocate_rd_adr_offset itself remains within the buffer bounds before 
programming the hardware read address. A malicious bitstream supplying a 
massive first_mb_in_slice could cause arbitrary physical memory reads.

[ ... ]
> @@ aml_h264_exit @@
>  	if (ctx->dos_clk_en)
>  		aml_stop_vdec_hw(dec_hw);
>  
>  	if (h264_ctx->collated_cma_vaddr) {
[ ... ]
>  	kfree(ctx->codec_priv);
>  	ctx->codec_priv = NULL;
>  }
[ ... ]
> @@ aml_h264_dec_run @@
>  		aml_stop_vdec_hw(dec_hw);
>  		synchronize_irq(dec_hw->dec_irq);
>  		h264_release_decode_spec(dec_hw, ctx);
>  	}
>  
>  	h264_ctx->init_flag = 0;
>  	aml_vdec_set_curr_ctx(dec_hw, NULL);

[Severity: High]
Could a concurrent vidioc_streamoff trigger a use-after-free?

If userspace calls vidioc_streamoff, aml_h264_exit() frees ctx->codec_priv. 
However, if aml_h264_dec_run() is actively sleeping on 
wait_event_interruptible_timeout() in the m2m worker thread, the forced 
hardware stop causes the wait to return. The worker thread will immediately 
dereference the freed h264_ctx without any synchronization.

[Severity: High]
Does this teardown sequence unconditionally reset shared hardware?

The aml_vdec_hw block is a global resource shared by all active v4l2 decoder 
instances. During teardown, aml_h264_exit() unconditionally calls 
aml_stop_vdec_hw(dec_hw) which issues soft resets to the shared DOS_BUS 
registers. There is no check ensuring that the tearing-down context actually 
owns the hardware. If another context is actively decoding, resetting the 
hardware underneath it will cause timeouts and state corruption.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260924-b4-s4-vdec-upstream-v10-0-1289303a58a4@amlogic.com?part=4

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic