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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 31 potential issue(s) to consider:
- [Critical] Asymmetrical stop/start streaming triggers a Use-After-Free and NULL pointer dereference on the decoder context.
- [Critical] Unvalidated reference list sizes and PPS parameters lead to massive heap/array buffer overflows.
- [Critical] Global context pointers are unconditionally overwritten on open(), corrupting state for concurrent users.
- [Critical] Multi-planar capture formats (e.g. NV12M) are broken because `plane_num` is hardcoded, causing severe DMA memory corruption.
- [Critical] The driver does not validate the video dimensions specified in the SPS against the actual V4L2 destination buffer size, allowing arbitrary DMA out-of-bounds writes.
- [Critical] Colocation buffer physical address calculations suffer from integer truncations and overflows, bypassing bounds checks and causing arbitrary DMA memory corruption.
- [High] Incorrect error checking for `of_parse_phandle()` leads to a NULL pointer dereference and wrong error code.
- [High] `S_FMT` on the OUTPUT queue explicitly clobbers the CAPTURE queue's pixel format.
- [High] Hardware resources and PM domains are permanently leaked in probe error paths.
- [High] The `dos` clock is requested but never prepared or enabled.
- [High] `copy_ref_list` incorrectly duplicates `ref_list0` into `ref_list1_unreordered`.
- [High] Metadata copying and request completion occur after buffers are handed back to userspace.
- [High] Synchronous blocking in `device_run` allows double-finishing of V4L2 M2M jobs on timeout.
- [High] Missing bounds check on firmware size causes integer underflow during buffer allocation.
- [High] `TRY_FMT` ignores user-requested pixel formats, directly returning the active configuration.
- [High] Bypassing clock enabling when the power domain is active breaks PM usage counts and clock balancing.
- [High] Device lifetime mismatch with `devm_kzalloc` causes Use-After-Free on driver unbind.
- [High] Failed allocations overwrite error results, and always-true `u8` checks cause unowned canvas release.
- [High] Hardware stream parser is programmed to read 1024 bytes past the user-supplied payload, causing OOB DMA reads.
- [High] IDR picture checks erroneously use strict equality against a bitmask field.
- [High] Spurious hardware interrupts result in a guaranteed kernel crash by dereferencing a NULL `curr_ctx`.
- [High] The DPB preparation loop incorrectly terminates early if a sparse entry is encountered.
- [High] Failing to return queued buffers when `start_streaming` fails causes videobuf2 warnings and leaks.
- [High] Missing video_unregister_device() on probe error path leaves a dangling character device.
- [High] Premature job finish notification causes a race between `aml_stop_vdec_hw` and `aml_start_vdec_hw`.
- [High] Missing job finish notification in the ISR error path permanently hangs the v4l2_m2m queue.
- [High] `ERR_PTR` stored in power domain array causes a kernel panic when dereferenced by PM API.
- [High] `read_poll_timeout_atomic` used with large timeouts stalls the CPU.
- [Medium] Selecting MESON_SM without satisfying its dependencies causes unmet dependency warnings.
- [Medium] Failure during power domain initialization leaks successfully attached domains.
- [Medium] `h264_ctx` is leaked during initialization failure.
--

--- Patch [4]: [PATCH v6 4/6] decoder: Add V4L2 stateless H.264 decoder driver ---
commit afc65ff0c18671a19fba53cb7a5d5a671420c2d6
Author: Zhentao Guo <zhentao.guo@amlogic.com>

decoder: Add V4L2 stateless H.264 decoder driver

Add initial support for V4L2 stateless video decoder driver on Amlogic S4
(S805X2) platform. In phase 1, it supports 8bit H.264 bitstreams decoding. 
Currently only progressive streams are supported.

> diff --git a/drivers/media/platform/amlogic/vdec/Kconfig b/drivers/media/platform/amlogic/vdec/Kconfig
> new file mode 100644
> index 0000000000000..d392967c77436
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/Kconfig
> @@ -0,0 +1,18 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR MIT)
> +
> +config VIDEO_AMLOGIC_VDEC
> +    tristate "Amlogic Video Decoder Driver"
> +    depends on ARCH_MESON || COMPILE_TEST
> +    depends on VIDEO_DEV
> +    depends on V4L_MEM2MEM_DRIVERS
> +    depends on TEE
> +    select VIDEOBUF2_DMA_CONTIG
> +    select V4L2_H264
> +    select V4L2_MEM2MEM_DEV
> +    select MESON_CANVAS
> +    select MESON_SM

[Severity: Medium]
Does selecting MESON_SM here bypass architecture-specific dependencies?
If this driver is built with COMPILE_TEST on architectures that lack the
dependencies for MESON_SM, it could trigger Kconfig unmet dependency warnings
and build failures.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec.c b/drivers/media/platform/amlogic/vdec/aml_vdec.c
> new file mode 100644
> index 0000000000000..d4dcd0180d2d0
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec.c
> @@ -0,0 +1,736 @@

[ ... ]

> +static void m2mops_vdec_device_run(void *m2m_priv)
> +{
> +	struct aml_vdec_ctx *ctx = m2m_priv;
> +	struct aml_vdec_dev *dev = ctx->dev;
> +	struct vb2_v4l2_buffer *src_buf, *dst_buf;
> +	struct media_request *src_req;
> +	int ret = 0;
> +
> +	src_buf = v4l2_m2m_next_src_buf(ctx->fh.m2m_ctx);
> +	dst_buf = v4l2_m2m_next_dst_buf(ctx->fh.m2m_ctx);

[ ... ]

> +	ret = ctx->codec_ops->run(ctx);
> +
> +	v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
> +	if (src_req)
> +		v4l2_ctrl_request_complete(src_req, &ctx->ctrl_handler);

[Severity: High]
Could v4l2_m2m_buf_copy_metadata and v4l2_ctrl_request_complete be accessing
buffers after they have been returned to userspace?

The synchronous wait_event in the run callback completes when the hardware ISR
calls v4l2_m2m_buf_done_and_job_finish. By the time run returns here, the
buffers might have already been returned to userspace and potentially unmapped
or freed, leading to a use-after-free.

[ ... ]

> +static int vdec_try_fmt_mp(struct aml_vdec_ctx *ctx, enum v4l2_buf_type type,
> +			   struct v4l2_pix_format_mplane *pix_mp)
> +{
> +	struct aml_video_fmt *dec_fmt;
> +	int i, align;
> +
> +	if (V4L2_TYPE_IS_OUTPUT(type))
> +		dec_fmt = &ctx->dec_fmt[AML_FMT_SRC];
> +	else
> +		dec_fmt = &ctx->dec_fmt[AML_FMT_DST];
> +
> +	pix_mp->field = V4L2_FIELD_NONE;
> +
> +	if (V4L2_TYPE_IS_OUTPUT(type)) {
> +		pix_mp->num_planes = dec_fmt->num_planes;
> +		pix_mp->pixelformat = dec_fmt->fourcc;

[Severity: High]
Does this unconditionally overwrite the user-requested pixel format with the
current active format? This prevents userspace applications from querying
supported formats using VIDIOC_TRY_FMT since their requested format is
aggressively overridden.

[ ... ]

> +static int vdec_s_fmt_output(struct aml_vdec_ctx *ctx, struct v4l2_format *f)
> +{
> +	struct v4l2_pix_format_mplane *pix_mp = &f->fmt.pix_mp;
> +	const struct aml_video_fmt *out_fmt;
> +	struct vb2_queue *vq;
> +	int ret;
> +
> +	vq = v4l2_m2m_get_vq(ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE);
> +	if (vb2_is_busy(vq) &&
> +	    pix_mp->pixelformat != ctx->pix_fmt[AML_FMT_SRC].pixelformat)
> +		return -EBUSY;
> +
> +	out_fmt = aml_vdec_get_video_fmt(ctx->dev, pix_mp->pixelformat);
> +	if (out_fmt)
> +		ctx->dec_fmt[AML_FMT_SRC] = *out_fmt;
> +	else
> +		dev_dbg(&ctx->dev->plat_dev->dev,
> +			"%s fmt %d not supported, use default\n", __func__,
> +			pix_mp->pixelformat);
> +
> +	ret = vdec_try_fmt_mp(ctx, f->type, pix_mp);
> +	set_pic_info(ctx, pix_mp, f->type);
> +
> +	ctx->pix_fmt[AML_FMT_SRC] = *pix_mp;
> +	ctx->pix_fmt[AML_FMT_DST] = *pix_mp;

[Severity: High]
Can this S_FMT call on the OUTPUT queue corrupt the CAPTURE queue's state?
By directly assigning to ctx->pix_fmt[AML_FMT_DST], configuring the bitstream
format clobbers the raw uncompressed format already configured on the CAPTURE
queue.

[ ... ]

> +static int vb2ops_vdec_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct aml_vdec_ctx *ctx = vb2_get_drv_priv(q);
> +
> +	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> +		ctx->is_output_streamon = 1;
> +		if (aml_vdec_init_dec_inst(ctx) < 0)
> +			return -EINVAL;

[Severity: High]
Should this failure path return the queued buffers to the framework?
When start_streaming fails, the videobuf2 framework requires the driver to
return all queued buffers (e.g. using vb2_buffer_done with
VB2_BUF_STATE_QUEUED), otherwise the queue stalls and videobuf2 warns.

[ ... ]

> +static void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
> +{
> +	struct aml_vdec_ctx *ctx = vb2_get_drv_priv(q);
> +	struct vb2_v4l2_buffer *src_buf = NULL, *dst_buf = NULL;
> +
> +	aml_vdec_release_instance(ctx);

[Severity: Critical]
Is it safe to release the codec instance here when only one of the queues
is stopped?

If userspace stops the CAPTURE queue, aml_vdec_release_instance is called,
which frees the codec context and nullifies codec_priv. If the OUTPUT queue
is still streaming, subsequent hardware interrupts or V4L2 jobs will
dereference the freed or NULL codec_priv, causing a kernel panic.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c b/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c
> new file mode 100644
> index 0000000000000..d63cbd4f9e26d
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_drv.c
> @@ -0,0 +1,239 @@

[ ... ]

> +static int fops_vcodec_open(struct file *file)
> +{
> +	struct aml_vdec_dev *dec_dev = video_drvdata(file);
> +	struct aml_vdec_ctx *ctx = NULL;
> +	int ret = 0;
> +
> +	ctx = kzalloc_obj(*ctx, GFP_KERNEL);
> +	if (!ctx)
> +		return -ENOMEM;
> +
> +	mutex_lock(&dec_dev->dev_mutex);
> +	dec_dev->dec_ctx = ctx;

[Severity: Critical]
Does this unconditionally overwrite the global device context pointer?
If multiple programs open the video device, the global pointer is overwritten.
Since the threaded ISR unconditionally uses these global pointers to look
up the active context, an interrupt generated by one client will be processed
using another client's context, leading to state corruption and use-after-free
crashes.

[ ... ]

> +static int fops_vcodec_release(struct file *file)
> +{
> +	struct aml_vdec_dev *dec_dev = video_drvdata(file);
> +	struct aml_vdec_ctx *ctx = fh_to_dec_ctx(file);
> +
> +	mutex_lock(&dec_dev->dev_mutex);

[Severity: High]
Is the lifetime of dec_dev properly managed here?
In aml_vdec_drv_probe, the aml_vdec_dev struct is allocated with
devm_kzalloc, tying it to the platform device. If the driver is unbound
while userspace still holds an open file descriptor, the memory is freed.
When userspace later closes the file descriptor, fops_vcodec_release will
dereference the freed memory when attempting to take the mutex.

[ ... ]

> +static int aml_vdec_drv_probe(struct platform_device *pdev)
> +{
> +	struct aml_vdec_dev *dev;
> +	struct video_device *vfd_dec;
> +	struct aml_vdec_hw *hw;
> +	int ret = 0;
> +
> +	dev = devm_kzalloc(&pdev->dev, sizeof(*dev), GFP_KERNEL);

[Severity: High]
Could tying this structure to the platform device using devm_kzalloc
lead to use-after-free bugs as described in the release fop above?

[ ... ]

> +	dev->pvdec_data = of_device_get_match_data(&pdev->dev);
> +	ret = dev->pvdec_data->req_hw_resource(dev);
> +	if (ret < 0)
> +		goto err_hw_init;
> +
> +	dev->m2m_dev_dec = v4l2_m2m_init(&aml_vdec_m2m_ops);
> +	if (IS_ERR(dev->m2m_dev_dec)) {
> +		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem dec device\n");
> +		ret = PTR_ERR((__force void *)dev->m2m_dev_dec);
> +		goto err_hw_init;
> +	}
> +
> +	ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
> +	if (ret) {
> +		v4l2_err(&dev->v4l2_dev, "Failed to register video device");
> +		goto err_vid_dev_register;
> +	}

[ ... ]

> +err_media_dev_register:
> +	v4l2_m2m_unregister_media_controller(dev->m2m_dev_dec);
> +error_m2m_mc_register:
> +	media_device_cleanup(&dev->mdev);
> +err_vid_dev_register:
> +	v4l2_m2m_release(dev->m2m_dev_dec);
> +err_hw_init:
> +	dev->dec_hw = NULL;

[Severity: High]
Does this error path leak the requested hardware resources?
If probe fails after req_hw_resource completes successfully, jumping to
err_hw_init clears the pointer without invoking the corresponding
destroy_hw_resource callback, permanently leaking power domains, device
links, clocks, and IRQs.

> +err_dec_mem_init:
> +	video_device_release(vfd_dec);
> +err_device_alloc:
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	return ret;
> +}

[Severity: High]
Is a call to video_unregister_device missing on the error paths after
video_register_device succeeds?
If probe fails late (like during media_device_register), the error cleanup
invokes video_device_release which immediately frees the memory, but
leaves the active video node registered. If userspace opens it, it will access
the deallocated structure.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c b/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c
> new file mode 100644
> index 0000000000000..29a195eb68721
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c

[ ... ]

> +void aml_stop_vdec_hw(struct aml_vdec_hw *hw)
> +{
> +	u32 reg_val = 0;
> +	int ret;
> +
> +	regmap_write(hw->map[DOS_BUS], MPSR, 0);
> +	regmap_write(hw->map[DOS_BUS], CPSR, 0);
> +
> +	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, 800000, true,
> +				       hw, WRRSP_LMEM);

[Severity: High]
Could this atomic poll timeout cause a soft lockup?
read_poll_timeout_atomic uses udelay to busily spin the processor without
yielding. Spinning for up to 800ms severely violates kernel latency constraints
and will trigger CPU stalls and soft lockup warnings.

[ ... ]

> +static int pm_vdec_power_domain_init(struct aml_vdec_hw *hw)
> +{
> +	int i, err;
> +	const struct power_manager_s *pm = hw->pm;
> +	struct pm_pd_s *pd = pm->pd_data;
> +
> +	mutex_init(&hw->pm_mutex);
> +
> +	for (i = 0; i < VDEC_MAX; i++) {
> +		pd[i].dev = dev_pm_domain_attach_by_name(hw->dev, pd[i].name);
> +		if (IS_ERR_OR_NULL(pd[i].dev)) {
> +			err = PTR_ERR(pd[i].dev);
> +			dev_dbg(hw->dev, "Get %s failed, pm-domain: %d\n",
> +				pd[i].name, err);
> +			continue;
> +		}
> +
> +		pd[i].link = device_link_add(hw->dev, pd[i].dev,
> +					     DL_FLAG_PM_RUNTIME |
> +					     DL_FLAG_STATELESS);
> +		if (IS_ERR_OR_NULL(pd[i].link)) {
> +			dev_err(hw->dev, "Adding %s device link failed!\n", pd[i].name);
> +			return -ENODEV;
> +		}

[Severity: High]
Can pd[i].dev end up storing an ERR_PTR if dev_pm_domain_attach_by_name fails?
If an ERR_PTR is returned, IS_ERR_OR_NULL catches it, but pd[i].dev retains
the error pointer. Later, in pm_vdec_power_domain_power_on, the code checks
if (dev). Since ERR_PTR is non-NULL, the check passes, and the error pointer
is sent directly into pm_runtime_get_sync, causing a kernel panic.

[Severity: Medium]
Does this error path leak previously attached domains?
If device_link_add fails on the second iteration, returning -ENODEV directly
bypasses cleanup (device_link_del / dev_pm_domain_detach) for the successful
first iteration.

[ ... ]

> +static void pm_vdec_power_domain_power_on(struct aml_vdec_hw *hw, int id)
> +{
> +	const struct power_manager_s *pm = hw->pm;
> +	struct device *dev = pm->pd_data[id].dev;
> +	struct clk_bulk_data *gate_node = NULL;
> +
> +	if (id == VDEC)
> +		gate_node = vdec_get_clk_by_name(hw, "vdec");
> +	else if (id == HVDEC)
> +		gate_node = vdec_get_clk_by_name(hw, "hevcf");

[Severity: High]
Is the dos clock missing from being enabled here?
The dos clock is fetched in vdec_clock_gate_init, but this function only
checks for vdec and hevcf to enable. If the dos clock remains disabled,
subsequent accesses to the DOS_BUS could stall the system bus.

[ ... ]

> +static void vdec_poweron(struct aml_vdec_hw *hw, enum vdec_type_e core)
> +{
> +	if (core >= VDEC_MAX)
> +		return;
> +
> +	mutex_lock(&hw->pm_mutex);
> +	if (!hw->pm->pd_data[core].dev)
> +		goto out;
> +
> +	hw->pm->pd_data[core].ref_count++;
> +	if (hw->pm->pd_data[core].ref_count > 1)
> +		goto out;
> +
> +	if (hw->pm->power_state(hw, core))
> +		goto out;
> +
> +	hw->pm->power_on(hw, core);
> +
> +out:
> +	mutex_unlock(&hw->pm_mutex);
> +}

[Severity: High]
Does skipping power_on here break the PM usage counts and clock balancing?
If hw->pm->power_state is true, the driver jumps to out without calling
power_on. However, power_on is responsible for clk_prepare_enable and
pm_runtime_get_sync. By bypassing it, the clock isn't prepared, but
vdec_poweroff will unconditionally execute power_off, disabling the
clock and triggering a PM put underflow.

[ ... ]

> +int dev_request_hw_resources(void *priv)
> +{
> +	struct aml_vdec_dev *dev = (struct aml_vdec_dev *)priv;
> +	struct aml_vdec_hw *hw;
> +	struct platform_device *pdev;
> +	struct device_node *sm_np;
> +	void __iomem *reg_base[MAX_REG_BUS];
> +	struct resource res;
> +	int i;
> +	int ret = -1;
> +
> +	if (!dev || !dev->dec_hw)
> +		return -1;
> +
> +	pdev = dev->plat_dev;
> +	hw = dev->dec_hw;
> +	hw->dev = &pdev->dev;

[ ... ]

> +	sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
> +	if (IS_ERR(sm_np))
> +		return PTR_ERR(hw->canvas);

[Severity: High]
Does of_parse_phandle return NULL on failure rather than an ERR_PTR?
If the secure-monitor property is missing, sm_np is NULL, meaning IS_ERR
evaluates to false. The driver will then pass NULL into meson_sm_get, which
likely dereferences it, resulting in a kernel panic.

> diff --git a/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c b/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c
> new file mode 100644
> index 0000000000000..ad4156249f558
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_tee_fw.c

[ ... ]

> +static int get_firmware(const char *path, void **data, size_t *size)
> +{
> +	const struct firmware *fw = NULL;
> +	int ret;
> +	void *buf;
> +
> +	ret = request_firmware(&fw, FIRMWARE_PATH, NULL);
> +	if (ret)
> +		return ret;
> +
> +	/* get rid of the first 32K bytes plaintext */
> +	buf = kzalloc((fw->size - UCODE_HEADER_SIZE), GFP_KERNEL);

[Severity: High]
Should there be a bounds check on fw->size before subtracting?
If the loaded firmware file is smaller than UCODE_HEADER_SIZE (32KB), this
subtraction underflows to a massive value, leading to a huge kzalloc failure
or out-of-bounds heap operations.

> diff --git a/drivers/media/platform/amlogic/vdec/h264.c b/drivers/media/platform/amlogic/vdec/h264.c
> new file mode 100644
> index 0000000000000..7584d5eb20ec8
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/h264.c

[ ... ]

> +static int h264_prepare_input(struct aml_vdec_ctx *ctx)
> +{

[ ... ]

> +	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]
Does this cause the hardware to read out-of-bounds memory?
Adding VLD_PADDING_SIZE (1024 bytes) to the payload size implicitly
tells the hardware stream parser to read past the end of the user-provided
V4L2 buffer payload. If the payload size aligns closely with the end of
the IOMMU/VMA mapping, this causes OOB DMA reads and IOMMU faults.

[ ... ]

> +static void config_sps_params(struct aml_h264_ctx *h264_ctx,
> +			      unsigned short *sps_base,
> +			      const struct v4l2_ctrl_h264_sps *sps)
> +{
> +	struct aml_vdec_ctx *ctx = h264_ctx->v4l2_ctx;
> +	struct aml_vdec_hw *hw = vdec_get_hw(ctx->dev);
> +	u32 cfg_tmp = 0;
> +	u32 frame_size;
> +	u32 offset = 0;
> +	unsigned short data_tmp[0x100];
> +	int i, ii;
> +
> +	memset(sps_base, 0, 0x100);
> +
> +	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: Critical]
Are the dimensions provided in the untrusted SPS validated against the
actual allocated V4L2 destination buffer size?

The user-supplied SPS dimensions are translated to canvas dimensions here.
If userspace provides massive SPS dimensions but queues a small destination
buffer, the hardware decoder will write out of bounds during decoding,
corrupting adjoining kernel physical memory.

[ ... ]

> +static void config_decode_canvas(struct aml_vdec_hw *hw,
> +				 struct h264_decode_buf_spec *buf_spec,
> +				 u32 mb_width, u32 mb_height)
> +{
> +	int canvas_alloc_result = 0;
> +	int blkmode = 0x0;
> +
> +	canvas_alloc_result = meson_canvas_alloc(hw->canvas, &buf_spec->y_canvas_index);
> +	canvas_alloc_result = meson_canvas_alloc(hw->canvas, &buf_spec->u_canvas_index);

[Severity: High]
Does the second assignment overwrite the allocation result of the first?
If the allocation of the y canvas fails but the u canvas succeeds, the
failure is masked, and the hardware is configured with an uninitialized
y_canvas_index.

[ ... ]

> +static void reorder_short_term(struct slice *curr_slice, int cur_list,
> +			       int pic_num_lx, int *ref_idx_lx)
> +{
> +	struct aml_h264_ctx *h264_ctx =
> +	    container_of(curr_slice, struct aml_h264_ctx, mslice);
> +	struct aml_vdec_ctx *ctx = h264_ctx->v4l2_ctx;
> +	int c_idx, n_idx;
> +	int num_ref_idx_lx_active;
> +	struct h264_decode_buf_spec *pic_lx = NULL;
> +	struct h264_decode_buf_spec *ref_list_reordered;

[ ... ]

> +	for (c_idx = num_ref_idx_lx_active; c_idx > *ref_idx_lx; c_idx--)
> +		memcpy(&ref_list_reordered[c_idx], &ref_list_reordered[c_idx - 1],
> +		       sizeof(struct h264_decode_buf_spec));

[Severity: Critical]
Is num_ref_idx_lx_active properly validated before this backwards memcpy?
num_ref_idx_l0_active_minus1 is parsed from the untrusted bitstream or
user-supplied PPS, and can be up to 65535. get_ref_list_size returns this
value, which is then used as a loop boundary for memcpy over the
fixed 17-element ref_list0 array, causing massive heap buffer overflows.

[ ... ]

> +static void copy_ref_list(struct aml_h264_ctx *h264_ctx, int curr_list)
> +{
> +	if (curr_list == 0)
> +		memcpy(h264_ctx->ref_list0_unreordered, h264_ctx->ref_list0,
> +		       sizeof(h264_ctx->ref_list0));
> +	else
> +		memcpy(h264_ctx->ref_list1_unreordered, h264_ctx->ref_list0,
> +		       sizeof(h264_ctx->ref_list1));
> +}

[Severity: High]
Does this memcpy copy from the wrong source list?
It copies h264_ctx->ref_list0 into ref_list1_unreordered instead of
h264_ctx->ref_list1, breaking B-frame reference list reordering.

[ ... ]

> +static void clear_unused_col_buf(struct aml_h264_ctx *h264_ctx,
> +				 struct v4l2_ctrl_h264_decode_params *decode)
> +{
> +	int i, col_poc;
> +
> +	/* flush all col buffers when IDR */
> +	if (decode->flags == V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC) {

[Severity: High]
Can this equality check fail because flags is a bitmask?
If decode->flags has another bit set (like V4L2_H264_DECODE_PARAM_FLAG_FIELD_PIC)
alongside V4L2_H264_DECODE_PARAM_FLAG_IDR_PIC, the strict equality fails. This
prevents the necessary flushing of colocated buffers on IDR frames.

[ ... ]

> +static void h264_config_decode_spec(struct aml_vdec_hw *hw, struct aml_vdec_ctx *ctx)
> +{
> +	struct aml_h264_ctx *h264_ctx = (struct aml_h264_ctx *)hw->curr_ctx;
> +	struct vdec_h264_stateless_ctrl_ref *ctrls = &h264_ctx->ctrl_ref;
> +	struct v4l2_ctrl_h264_decode_params *decode =
> +	    (struct v4l2_ctrl_h264_decode_params *)ctrls->decode;
> +	struct h264_decode_buf_spec *buf_spec_l0, *buf_spec_l1;

[ ... ]

> +		buf_spec_l0 = find_spec_by_dpb_index(h264_ctx, i, 0);
> +		if (buf_spec_l0) {
> +			buf_spec_l0->canvas_pos =
> +			    get_canvas_pos_by_poc(h264_ctx,
> +						  dpb->top_field_order_cnt);

[ ... ]

> +			buf_spec_l0->y_dma_addr =
> +				vb2_dma_contig_plane_dma_addr(vb, 0);
> +			if (ctx->pic_info.plane_num > 1)
> +				buf_spec_l0->c_dma_addr =
> +				    vb2_dma_contig_plane_dma_addr(vb, 1);
> +			else
> +				buf_spec_l0->c_dma_addr =
> +				    buf_spec_l0->y_dma_addr +
> +				    ctx->pic_info.fb_size[0];

[Severity: Critical]
Is ctx->pic_info.plane_num accurate here for multi-planar capture formats?
In vdec_try_fmt_mp and set_pic_info, plane_num is hardcoded to 1 when
formatting the output queue. When userspace sets a multi-planar format
(like NV12M) on the capture queue, plane_num remains 1. The code here
subsequently calculates the UV address as y_dma_addr + fb_size[0], pointing
completely out of bounds of the actual Y plane allocation, causing severe
DMA memory corruption.

[ ... ]

> +	for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
> +		struct v4l2_h264_dpb_entry *dpb = &decode->dpb[i];
> +
> +		if (!(dpb->flags & V4L2_H264_DPB_ENTRY_FLAG_ACTIVE))
> +			break;

[Severity: High]
Should this be a continue instead of a break?
V4L2 DPB arrays can be sparse. If an empty slot is encountered early in the
iteration, break exits the loop completely, skipping configuration for
all subsequent valid reference frames in the array.

[ ... ]

> +static int alloc_colocate_cma(struct aml_h264_ctx *h264_ctx,
> +			      struct aml_vdec_ctx *ctx)
> +{
> +	int alloc_size = 0;
> +	int i;
> +	struct aml_vdec_hw *hw;
> +
> +	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;
> +	alloc_size = PAGE_ALIGN(h264_ctx->one_col_buf_size *
> +			(h264_ctx->max_num_ref_frames + COL_BUFFER_MARGIN));
> +	h264_ctx->collated_cma_vaddr = dma_alloc_coherent(hw->dev, alloc_size,
> +							  &h264_ctx->collated_cma_addr, GFP_KERNEL);

[Severity: Critical]
Can this allocation size wrap around?
Because the SPS parameters used to compute one_col_buf_size and
max_num_ref_frames are unvalidated, the multiplication can wrap around in
32-bit space, allocating a tiny CMA buffer.

[ ... ]

> +static int h264_config_decode_buf(struct aml_vdec_hw *hw,
> +				  struct aml_vdec_ctx *ctx)
> +{

[ ... ]

> +	if (h264_ctx->curr_spec.col_buf_index >= 0 &&
> +	    h264_ctx->curr_spec.col_buf_index < h264_ctx->colocated_buf_num) {
> +		colocate_wr_adr = h264_ctx->collated_cma_addr +
> +		    ((h264_ctx->one_col_buf_size *
> +		      h264_ctx->curr_spec.col_buf_index) >> (use_mode_8x8_flag ? 2 : 0));

[Severity: Critical]
Is colocate_wr_adr incorrectly typed as 32-bit (unsigned int)?
Assigning the 64-bit collated_cma_addr (dma_addr_t) to a 32-bit integer
truncates the upper bits. The subsequent bounds check
(colocate_wr_adr + one_col_buf_size > collated_cma_addr_end) wraps around in
32-bit space, allowing out-of-bounds DMA programming.

[ ... ]

> +static void release_canvas_index(struct aml_vdec_hw *hw,
> +				 struct h264_decode_buf_spec *buf)
> +{
> +	if (buf->y_canvas_index >= 0) {
> +		dev_dbg(hw->dev, "free y_canvas %d\n", buf->y_canvas_index);
> +		meson_canvas_free(hw->canvas, buf->y_canvas_index);
> +		buf->y_canvas_index = -1;
> +	}

[Severity: High]
Does this negative bounds check work on a u8 variable?
buf->y_canvas_index is declared as a u8. The check >= 0 will always be
true, causing it to blindly invoke meson_canvas_free(hw->canvas, 255)
when it hasn't been allocated, which corrupts other subsystem canvases.

[ ... ]

> +static irqreturn_t h264_threaded_isr_func(int irq, void *priv)
> +{
> +	u32 dec_status;
> +	struct aml_vdec_dev *dev = (struct aml_vdec_dev *)priv;
> +	struct aml_h264_ctx *h264_ctx = (struct aml_h264_ctx *)dev->dec_hw->curr_ctx;
> +	struct aml_vdec_ctx *ctx = (struct aml_vdec_ctx *)dev->dec_ctx;
> +	struct aml_vdec_hw *hw = vdec_get_hw(ctx->dev);
> +	unsigned short *p = (unsigned short *)h264_ctx->lmem_addr;

[Severity: High]
Is it safe to blindly dereference h264_ctx here?
When the stream is torn down in aml_h264_exit, curr_ctx is assigned NULL.
If a spurious or delayed hardware interrupt fires shortly after, this will
trigger a NULL pointer dereference panic.

[ ... ]

> +		if (h264_config_decode_buf(hw, ctx) < 0) {
> +			h264_release_decode_spec(hw, ctx);
> +			ctx->int_cond = 1;
> +			wake_up_interruptible(&ctx->queue);
> +			goto irq_handled;
> +		}

[Severity: High]
Is the v4l2_m2m_buf_done_and_job_finish call missing from this error path?
The code wakes up the waiter thread, but skips signalling job completion to
the M2M framework. The framework will never register the job as finished,
permanently hanging the M2M queue for any subsequent jobs.

> +		if (h264_ctx->new_pic_flag == 1) {
> +			regmap_write(hw->map[DOS_BUS], DPB_STATUS_REG, H264_ACTION_DECODE_NEWPIC);
> +			dev_dbg(&dev->plat_dev->dev, "action decode new pic\n");
> +			h264_ctx->new_pic_flag = 0;
> +		} else {
> +			regmap_write(hw->map[DOS_BUS], DPB_STATUS_REG, H264_ACTION_DECODE_SLICE);
> +			dev_dbg(&dev->plat_dev->dev, "action decode new slice\n");
> +		}
> +		break;
> +	case H264_SLICE_DATA_DONE:
> +		h264_release_decode_spec(hw, ctx);
> +		h264_ctx->decode_pic_count++;
> +		ctx->int_cond = 1;
> +		v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx,
> +						 VB2_BUF_STATE_DONE);
> +		wake_up_interruptible(&ctx->queue);
> +		break;

[Severity: High]
Could marking the job finished here cause a race with aml_stop_vdec_hw?
Calling v4l2_m2m_buf_done_and_job_finish signals the framework to
immediately dequeue and start the next job. Concurrently, the awoken
thread inside aml_h264_dec_run proceeds to issue an aml_stop_vdec_hw,
shutting down the hardware unit out from under the newly scheduled job.

[ ... ]

> +int aml_h264_init(void *priv)
> +{
> +	struct aml_vdec_ctx *ctx = (struct aml_vdec_ctx *)priv;
> +	struct aml_vdec_hw *dec_hw;
> +	struct aml_h264_ctx *h264_ctx;
> +	int ret = 0;
> +
> +	h264_ctx = kzalloc_obj(*h264_ctx, GFP_KERNEL);
> +	if (!h264_ctx)
> +		return -ENOMEM;
> +
> +	h264_ctx->v4l2_ctx = ctx;
> +	dec_hw = vdec_get_hw(ctx->dev);
> +	if (!dec_hw)
> +		return -1;

[Severity: Medium]
Does this error path leak the freshly allocated h264_ctx structure?
If vdec_get_hw returns NULL, the function returns -1 without calling
kfree on h264_ctx.

[ ... ]

> +int aml_h264_dec_run(void *priv)
> +{
> +	struct aml_vdec_ctx *ctx = (struct aml_vdec_ctx *)priv;
> +	struct aml_h264_ctx *h264_ctx = (struct aml_h264_ctx *)ctx->codec_priv;
> +	struct aml_vdec_hw *dec_hw = vdec_get_hw(ctx->dev);
> +	int ret = -1;
> +	int i;

[ ... ]

> +	aml_start_vdec_hw(dec_hw);
> +	h264_ctx->init_flag = 1;
> +
> +	regmap_write(dec_hw->map[DOS_BUS], DPB_STATUS_REG, H264_ACTION_SEARCH_HEAD);
> +
> +	ret = wait_event_interruptible_timeout(ctx->queue, ctx->int_cond,
> +					       msecs_to_jiffies(DECODER_TIMEOUT_MS));

[Severity: High]
Does synchronously blocking inside device_run violate the M2M Request API?
If this timeout triggers, device_run returns and callers typically assume
the job failed and may finish it. However, the delayed hardware interrupt
could subsequently fire and call v4l2_m2m_buf_done_and_job_finish for the
same job, double-finishing the context and corrupting M2M framework state.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260724-b4-s4-vdec-upstream-v6-0-6072a7ee7428@amlogic.com?part=4

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