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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 22 potential issue(s) to consider:
- [Critical] Global context pointers are unconditionally overwritten, breaking isolation for multiple instances and leading to UAF.
- [High] Accessing V4L2 M2M buffers and request objects after they have been completed by the ISR.
- [High] Missing `v4l2_m2m_buf_done_and_job_finish()` on error path causes a permanent stall of the V4L2 M2M context.
- [High] Probe error paths fail to clean up PM domains and device links.
- [High] Setting the OUTPUT queue format mistakenly overwrites the CAPTURE queue's format with a compressed stream format.
- [High] Multi-planar raw capture formats result in out-of-bounds DMA access due to improperly hardcoded plane counts.
- [High] Redundant hardware canvas allocations and error path failures leak and exhaust the global hardware canvas pool.
- [High] Tautological comparisons on unsigned variables cause bounds-check bypasses and spurious resource frees.
- [High] Missing validation of V4L2 SPS/PPS controls leads to multiple heap buffer overflows, integer overflows, and OOB DMA.
- [High] Teardown in `aml_vdec_drv_remove` frees the driver context while file descriptors can remain open.
- [High] Failing `start_streaming` does not return queued buffers to the VB2 framework.
- [High] Double free and premature free of `ctx->codec_priv` via `vb2ops_vdec_stop_streaming`.
- [High] Bypassing PM runtime acquisition leads to unbalanced clock/PM references.
- [High] Lockless buffer access and unchecked NULL dereference in IRQ handler.
- [High] The DOS clock is never enabled before accessing DOS_BUS registers.
- [Medium] Incorrect error checking of `of_parse_phandle` leads to NULL pointer dereference and wrong error return.
- [Medium] Unmet dependencies when using `select` with architecture-specific symbols in a `COMPILE_TEST` driver.
- [Medium] Missing check for pm_runtime_get_sync return value allows access to unpowered hardware.
- [Medium] The V4L2 control handler memory is leaked if `v4l2_ctrl_handler_setup` fails.
- [Medium] Video device node is exposed to userspace before hardware initialization is complete.
- [Medium] Missing minimum length validation for loaded firmware data.
- [Medium] Missing `dma_rmb()` barrier before reading DMA memory updated by hardware.
--

commit 05d0db2be715fa3e4cc8fc756c957d431e6f5b89
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]
Will selecting architecture-specific symbols like MESON_CANVAS and MESON_SM
cause unmet dependency warnings on unsupported architectures when COMPILE_TEST
is enabled?

[ ... ]
> 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
[ ... ]
> +int aml_vdec_ctrls_setup(struct aml_vdec_ctx *ctx)
> +{
> +	int i;
> +	int ctrls_size = ARRAY_SIZE(controls);
> +
> +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, ctrls_size);
> +	for (i = 0; i < ctrls_size; i++) {
> +		v4l2_ctrl_new_custom(&ctx->ctrl_handler, &controls[i].cfg, NULL);
> +		if (ctx->ctrl_handler.error) {
> +			dev_info(&ctx->dev->plat_dev->dev, "add ctrl for (%d) failed%d\n",
> +				 controls[i].cfg.id, ctx->ctrl_handler.error);
> +			v4l2_ctrl_handler_free(&ctx->ctrl_handler);
> +			return ctx->ctrl_handler.error;
> +		}
> +	}
> +	ctx->fh.ctrl_handler = &ctx->ctrl_handler;
> +	return v4l2_ctrl_handler_setup(&ctx->ctrl_handler);

[Severity: Medium]
If v4l2_ctrl_handler_setup() returns an error, does this code leak the memory
allocated for the control handler? The caller doesn't seem to free it on
this failure path.

> +}
> +
> +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);
> +	dev_dbg(&dev->plat_dev->dev, "device run : src buf : %d dst buf %d\n",
> +		src_buf->vb2_buf.index, dst_buf->vb2_buf.index);
> +	if (WARN_ON_ONCE(!ctx->codec_ops->run))
> +		goto err_cancel_job;
> +
> +	src_req = src_buf->vb2_buf.req_obj.req;
> +	if (src_req)
> +		v4l2_ctrl_request_setup(src_req, &ctx->ctrl_handler);
> +	dos_enable(dev->dec_hw);
> +	/* incase of bus hang in stop_streaming */
> +	ctx->dos_clk_en = 1;
> +
> +	if (ctx->curr_dec_type == CODEC_TYPE_H264)
> +		aml_vdec_reset_core(dev->dec_hw);
> +
> +	if (load_firmware(dev->dec_hw, ctx->curr_dec_type) < 0)
> +		goto err_cancel_job;
> +
> +	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]
Since ctx->codec_ops->run() blocks on wait_event_interruptible_timeout, and
the ISR completes the job by calling v4l2_m2m_buf_done_and_job_finish(), could
src_buf, dst_buf, and src_req already be returned to userspace and potentially
freed by the time this thread wakes up to access them?

> +	if (ret < 0)
> +		goto err_cancel_job;
> +
> +	return;
> +
> +err_cancel_job:
> +	v4l2_m2m_buf_done_and_job_finish(dev->m2m_dev_dec, ctx->m2m_ctx,
> +					 VB2_BUF_STATE_ERROR);
> +}
[ ... ]
> +static void set_pic_info(struct aml_vdec_ctx *ctx,
> +			 struct v4l2_pix_format_mplane *pix_mp,
> +			 enum v4l2_buf_type type)
> +{
> +	if (V4L2_TYPE_IS_OUTPUT(type)) {
> +		ctx->pic_info.output_pix_fmt = pix_mp->pixelformat;
> +		ctx->pic_info.coded_width = ALIGN(pix_mp->width, 64);
> +		ctx->pic_info.coded_height = ALIGN(pix_mp->height, 64);
> +		ctx->pic_info.fb_size[0] =
> +		    ctx->pic_info.coded_width * ctx->pic_info.coded_height;
> +		ctx->pic_info.fb_size[1] = ctx->pic_info.fb_size[0] / 2;
> +		ctx->pic_info.plane_num = 1;

[Severity: High]
If a multi-planar format like NV12M is negotiated, shouldn't plane_num be set
to match the actual planes used? By rigidly setting plane_num to 1 here, could
the driver end up computing out-of-bounds DMA addresses for the C plane later?

> +	}
> +}
[ ... ]
> +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]
Does overwriting ctx->pix_fmt[AML_FMT_DST] here corrupt the capture queue's
pixel format by erroneously assigning it the output queue's compressed
bitstream format?

> +
> +	return ret;
> +}
[ ... ]
> +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]
If aml_vdec_init_dec_inst() fails and returns an error, doesn't the driver
need to return all queued buffers to userspace with state VB2_BUF_STATE_QUEUED
to avoid permanently leaking them?

> +	} else {
> +		ctx->is_cap_streamon = 1;
> +	}
> +
> +	return 0;
> +}
> +
> +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: High]
Since aml_vdec_release_instance() calls aml_h264_exit() which frees
ctx->codec_priv, what happens if userspace stops only the capture queue while
the output queue is actively streaming? Will this cause a use-after-free on
ctx->codec_priv for ongoing decode jobs or incoming IRQs?

> +
> +	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
> +		while ((src_buf = v4l2_m2m_src_buf_remove(ctx->m2m_ctx)))
> +			v4l2_m2m_buf_done(src_buf, VB2_BUF_STATE_ERROR);
> +		ctx->is_output_streamon = 0;
> +	} else {
> +		while ((dst_buf = v4l2_m2m_dst_buf_remove(ctx->m2m_ctx)))
> +			v4l2_m2m_buf_done(dst_buf, VB2_BUF_STATE_ERROR);
> +		ctx->is_cap_streamon = 0;
> +	}
> +}
[ ... ]
> 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
[ ... ]
> +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]
Is it safe to assign the newly allocated context to the global
dec_dev->dec_ctx? If multiple file handles are opened concurrently, won't this
overwrite the pointer and cause the ISR to route events only to the most
recently opened instance?

> +	ctx->dev = dec_dev;
> +	v4l2_fh_init(&ctx->fh, video_devdata(file));
> +	file->private_data = &ctx->fh;
> +	v4l2_fh_add(&ctx->fh, file);
> +	dec_dev->filp = file;
> +	mutex_init(&ctx->v4l2_intf_lock);
> +	init_waitqueue_head(&ctx->queue);
> +	ctx->int_cond = 0;
[ ... ]
> +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);
> +	if (!dev)
> +		return -ENOMEM;
> +
> +	dev->plat_dev = pdev;
> +	mutex_init(&dev->dev_mutex);
> +
> +	ret = v4l2_device_register(&pdev->dev, &dev->v4l2_dev);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "v4l2_device_register err\n");
> +
> +	vfd_dec = video_device_alloc();
> +	if (!vfd_dec) {
> +		v4l2_err(&dev->v4l2_dev, "Failed to allocate video device\n");
> +		ret = -ENOMEM;
> +		goto err_device_alloc;
> +	}
> +	*vfd_dec = dec_dev;
> +	vfd_dec->v4l2_dev = &dev->v4l2_dev;
> +	vfd_dec->lock = &dev->dev_mutex;
> +	video_set_drvdata(vfd_dec, dev);
> +	dev->vfd = vfd_dec;
> +	platform_set_drvdata(pdev, dev);
> +
> +	hw = devm_kzalloc(&pdev->dev, sizeof(*hw), GFP_KERNEL);
> +	if (!hw) {
> +		ret = -ENOMEM;
> +		goto err_dec_mem_init;
> +	}
> +	dev->dec_hw = hw;
> +
> +	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;
> +	}
> +
> +	dev->mdev.dev = &pdev->dev;
> +	strscpy(dev->mdev.model, AML_VDEC_DRV_NAME, sizeof(dev->mdev.model));
> +	media_device_init(&dev->mdev);
> +	dev->mdev.ops = &aml_m2m_media_ops;
> +	dev->v4l2_dev.mdev = &dev->mdev;
> +
> +	ret = v4l2_m2m_register_media_controller(dev->m2m_dev_dec, vfd_dec,
> +						 MEDIA_ENT_F_PROC_VIDEO_DECODER);
> +	if (ret) {
> +		v4l2_err(&dev->v4l2_dev, "Failed to init mem2mem media controller\n");
> +		goto error_m2m_mc_register;
> +	}
> +
> +	ret = media_device_register(&dev->mdev);
> +	if (ret) {
> +		v4l2_err(&dev->v4l2_dev, "Failed to register media device");
> +		goto err_media_dev_register;
> +	}
> +	vdec_enable(dev->dec_hw);

[Severity: Medium]
By calling video_register_device() before vdec_enable(), isn't the device node
exposed to userspace before the hardware is powered up and initialized? Could
this allow userspace to issue ioctls on unpowered hardware?

> +	return 0;
> +
> +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 forget to call dev->pvdec_data->destroy_hw_resource(dev)?
Failing to clean up hardware resources here might permanently leak attached PM
domains and device links.

> +err_dec_mem_init:
> +	video_device_release(vfd_dec);
> +err_device_alloc:
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +	return ret;
> +}
> +
> +static void aml_vdec_drv_remove(struct platform_device *pdev)
> +{
> +	struct aml_vdec_dev *dev = platform_get_drvdata(pdev);
> +
> +	vdec_disable(dev->dec_hw);
> +
> +	if (media_devnode_is_registered(dev->mdev.devnode)) {
> +		media_device_unregister(&dev->mdev);
> +		media_device_cleanup(&dev->mdev);
> +	}
> +
> +	if (dev->m2m_dev_dec)
> +		v4l2_m2m_release(dev->m2m_dev_dec);
> +	if (dev->vfd)
> +		video_unregister_device(dev->vfd);
> +	if (dev->dec_hw) {
> +		dev->pvdec_data->destroy_hw_resource(dev);
> +		dev->dec_hw = NULL;
> +	}
> +	v4l2_device_unregister(&dev->v4l2_dev);
> +}

[Severity: High]
If a process keeps the video file descriptor open when the device is unbound,
devm_kzalloc will free the dev structure after this remove function completes.
When the file descriptor is eventually closed, fops_vcodec_release will
execute and attempt to access dev->dev_mutex. Will this lead to a
use-after-free on the dev 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..79ba2a68bde41
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/aml_vdec_hw.c
[ ... ]
> +static int vdec_clock_gate_init(struct aml_vdec_hw *hw)
> +{
> +	hw->gates[CLK_DOS].id = "dos";
> +	hw->gates[CLK_VDEC].id = "vdec";
> +	hw->gates[CLK_HEVCF].id = "hevcf";
> +
> +	return devm_clk_bulk_get(hw->dev, CLK_MAX, hw->gates);
> +}
[ ... ]
> +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_1)
> +		gate_node = vdec_get_clk_by_name(hw, "vdec");
> +	else if (id == VDEC_HEVC)
> +		gate_node = vdec_get_clk_by_name(hw, "hevcf");
> +
> +	if (gate_node) {
> +		clk_prepare_enable(gate_node->clk);

[Severity: High]
While the "vdec" and "hevcf" clocks are enabled here, isn't the "dos" clock
omitted entirely? If the "dos" clock remains gated, could accessing DOS_BUS
registers result in a synchronous external abort?

> +		if (id == VDEC_1) {
> +			clk_set_rate(gate_node->clk, 499999992);
> +			dev_dbg(hw->dev, "after set, vdec clock is %lu Hz\n",
> +				clk_get_rate(gate_node->clk));
> +		}
> +		dev_dbg(hw->dev, "the %-15s clock on\n", gate_node->id);
> +	} else {
> +		dev_info(hw->dev, "clk %d, unreachable\n", id);
> +	}
> +
> +	if (dev) {
> +		pm_runtime_get_sync(dev);
> +		dev_dbg(dev, "dev: %p link %p the %-15s power on\n",
> +			dev, pm->pd_data[id].link, pm->pd_data[id].name);
> +	}
> +
> +	dos_local_config(hw, 1, id);

[Severity: Medium]
Should the return value of pm_runtime_get_sync() be checked before calling
dos_local_config()? If the power domain fails to power up, accessing the
hardware registers could lead to a bus hang.

> +}
[ ... ]
> +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;

[Severity: High]
If the power domain is already active, this code jumps past power_on() but
still increments ref_count. Won't the subsequent teardown decrement ref_count
and call power_off() unconditionally, causing unbalanced clk_disable_unprepare()
and pm_runtime_put_sync() calls?

> +
> +	hw->pm->power_on(hw, core);
> +
> +out:
> +	mutex_unlock(&hw->pm_mutex);
> +}
[ ... ]
> +int dev_request_hw_resources(void *priv)
> +{
[ ... ]
> +	hw->canvas = meson_canvas_get(&pdev->dev);
> +	if (IS_ERR(hw->canvas))
> +		return PTR_ERR(hw->canvas);
> +
> +	sm_np = of_parse_phandle(pdev->dev.of_node, "secure-monitor", 0);
> +	if (IS_ERR(sm_np))
> +		return PTR_ERR(hw->canvas);

[Severity: Medium]
Since of_parse_phandle() returns NULL on failure rather than an ERR_PTR,
doesn't the IS_ERR check fail to catch a missing node? Passing NULL to
meson_sm_get() later might cause issues. Also, why is PTR_ERR(hw->canvas)
returned here instead of an appropriate error code?

> +
> +	hw->sec_fw = meson_sm_get(sm_np);
> +	of_node_put(sm_np);
> +	if (IS_ERR(hw->sec_fw))
> +		return PTR_ERR(hw->sec_fw);
[ ... ]
> diff --git a/drivers/media/platform/amlogic/vdec/h264.c b/drivers/media/platform/amlogic/vdec/h264.c
> new file mode 100644
> index 0000000000000..bd3aef44409f3
> --- /dev/null
> +++ b/drivers/media/platform/amlogic/vdec/h264.c
[ ... ]
> +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: High]
Does this code validate userspace-provided SPS controls? For example, if
max_num_ref_frames is artificially large, could it overflow colocated arrays?
Likewise, could an overly large pic_width_in_mbs_minus1 cause integer overflows
when calculating allocation sizes?

> +
> +	data_tmp[offset] = PARAM_BASE_VAL;
> +	offset += 2;
[ ... ]
> +static int allocate_canvas_pos(struct aml_h264_ctx *h264_ctx, int poc)
> +{
> +	int i;
> +	int ret = -1;
> +	struct aml_vdec_ctx *ctx = h264_ctx->v4l2_ctx;
> +
> +	for (i = 0; i < (V4L2_H264_NUM_DPB_ENTRIES + 1); i++) {
> +		if (((h264_ctx->canvas_pos_map >> i) & 0x1) == 0) {
> +			h264_ctx->canvas_pos_map |= (1 << i);
> +			h264_ctx->ref_canvas[i].poc = poc;
> +			h264_ctx->ref_canvas[i].canvas_pos = i;
> +			ret = i;
> +
> +			dev_dbg(&ctx->dev->plat_dev->dev,
> +				"%s i %d pos_poc %d\n", __func__, i,
> +				h264_ctx->ref_canvas[i].poc);
> +			break;
> +		}
> +	}
> +
> +	return ret;
> +}
> +
> +static void release_canvas_pos(struct aml_h264_ctx *h264_ctx, int index)
> +{
> +	struct aml_vdec_ctx *ctx = h264_ctx->v4l2_ctx;
> +
> +	if (index >= 0) {
> +		if (index > V4L2_H264_NUM_DPB_ENTRIES) {
> +			dev_dbg(&ctx->dev->plat_dev->dev,
> +				"%s error, index %d is bigger than buf count %d\n",
> +				__func__, index, h264_ctx->max_num_ref_frames);
> +		} else {
> +			if (h264_ctx->ref_canvas[index].poc != INVALID_POC &&
> +			    ((h264_ctx->canvas_pos_map >> index) & 0x1) ==
> +			    0x1) {
> +				h264_ctx->canvas_pos_map &= (~(1 << index));
> +				dev_dbg(&ctx->dev->plat_dev->dev,
> +					"%s canvas_pos index %d released poc %d, canvas_pos_map 0x%x\n",
> +					__func__, index, h264_ctx->ref_canvas[index].poc,
> +					h264_ctx->canvas_pos_map);
> +				h264_ctx->ref_canvas[index].poc = INVALID_POC;
> +				h264_ctx->ref_canvas[index].canvas_pos = -1;
> +			}
> +		}
> +	}
> +}
[ ... ]
> +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;
> +	struct vb2_buffer *vb;
> +	struct vb2_v4l2_buffer *vb2_v4l2;
> +	struct vb2_queue *vq;
> +	int i;
> +
> +	clear_unused_col_buf(h264_ctx, decode);
> +
> +	vb2_v4l2 = v4l2_m2m_next_dst_buf(ctx->m2m_ctx);
> +	vb = &vb2_v4l2->vb2_buf;

[Severity: High]
If the destination queue is empty when this ISR runs, won't v4l2_m2m_next_dst_buf
return NULL? This would result in an immediate NULL pointer dereference.

> +
> +	h264_ctx->curr_spec.y_dma_addr = vb2_dma_contig_plane_dma_addr(vb, 0);
> +	if (ctx->pic_info.plane_num > 1)
> +		h264_ctx->curr_spec.c_dma_addr =
> +		    vb2_dma_contig_plane_dma_addr(vb, 1);
> +	else
> +		h264_ctx->curr_spec.c_dma_addr =
> +		    h264_ctx->curr_spec.y_dma_addr + ctx->pic_info.fb_size[0];
> +	h264_ctx->curr_spec.canvas_pos =
> +	    allocate_canvas_pos(h264_ctx, decode->top_field_order_cnt);
> +	if (h264_ctx->curr_spec.canvas_pos < 0)

[Severity: High]
Since canvas_pos is a u32 (inside struct h264_decode_buf_spec), isn't this
comparison (< 0) always false? This causes the code to proceed with an invalid
canvas position if allocate_canvas_pos() fails.

> +		dev_err(&ctx->dev->plat_dev->dev, "curr_spec.canvas error\n");
> +
> +	if (decode->nal_ref_idc)
> +		h264_ctx->curr_spec.col_buf_index =
> +		    allocate_colocate_buf(h264_ctx,
> +					  decode->top_field_order_cnt);
[ ... ]
> +static void get_canvas_index(struct aml_vdec_hw *hw, struct aml_vdec_ctx *ctx)
> +{
> +	struct aml_h264_ctx *h264_ctx = (struct aml_h264_ctx *)hw->curr_ctx;
> +	int i;
> +	struct h264_decode_buf_spec *buf;
> +
> +	config_decode_canvas(hw, &h264_ctx->curr_spec,
> +			     h264_ctx->mb_width, h264_ctx->mb_height);
> +	if (h264_ctx->list_size[0] > 0) {
> +		for (i = 0; i < h264_ctx->list_size[0]; i++) {
> +			buf = &h264_ctx->ref_list0[i];
> +			config_decode_canvas(hw, buf, h264_ctx->mb_width,
> +					     h264_ctx->mb_height);
> +		}
> +	}
> +
> +	if (h264_ctx->list_size[1] > 0) {
> +		for (i = 0; i < h264_ctx->list_size[1]; i++) {
> +			buf = &h264_ctx->ref_list1[i];
> +			config_decode_canvas(hw, buf, h264_ctx->mb_width,
> +					     h264_ctx->mb_height);
> +		}
> +	}

[Severity: High]
If a reference frame is shared between list0 and list1, will config_decode_canvas()
be called twice for the same buffer, causing a redundant canvas allocation
and leaking the hardware canvas pool?

> +}
> +
> +static void release_canvas_index(struct aml_vdec_hw *hw,
> +				 struct h264_decode_buf_spec *buf)
> +{
> +	if (buf->y_canvas_index >= 0) {

[Severity: High]
Because y_canvas_index is declared as u8, won't this check always be true?
If it's previously set to -1, it wraps to 255, causing the driver to spuriously
free canvas 255.

> +		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;
> +	}
> +
> +	if (buf->u_canvas_index >= 0) {
> +		dev_dbg(hw->dev, "free uv_canvas_index %d\n",
> +			buf->u_canvas_index);
> +		meson_canvas_free(hw->canvas, buf->u_canvas_index);
> +		buf->u_canvas_index = -1;
> +		buf->v_canvas_index = -1;
> +	}
> +}
[ ... ]
> +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;
> +	int i, ii;
> +
> +	regmap_read(hw->map[DOS_BUS], DPB_STATUS_REG, &dec_status);
> +	h264_ctx->dec_status = dec_status;
> +	dev_dbg
> +	    (&dev->plat_dev->dev,
> +	     "%s, dec_status 0x%x VIFF_BIT_CNT 0x%x MBY_MBX 0x%x VLD_SHIFT_STATUS 0x%x\n",
> +	     __func__, dec_status, read_dos_reg(hw, VIFF_BIT_CNT),
> +	     read_dos_reg(hw, MBY_MBX), read_dos_reg(hw, VLD_SHIFT_STATUS));
> +
> +	regmap_read(hw->map[DOS_BUS], AV_SCRATCH_F, &h264_ctx->save_avscratch_f);
> +
> +	switch (dec_status) {
> +	case H264_SLICE_HEADER_DONE:
> +		for (i = 0; i < 0x400; i += 4)
> +			for (ii = 0; ii < 4; ii++)
> +				h264_ctx->dpb_param.l.data[i + ii] = p[i + 3 - ii];

[Severity: Medium]
Since lmem_addr holds DMA memory populated by the hardware, should a dma_rmb()
barrier be issued before reading from `p` to ensure the CPU doesn't observe
stale cached data?

> +		save_reg_status(h264_ctx);
> +		h264_get_slice_params(h264_ctx);
> +		if (h264_ctx->mslice.first_mb_in_slice != 0)
> +			h264_release_decode_spec(hw, ctx);
> +
> +		h264_config_decode_spec(hw, ctx);
> +		h264_reorder_reflists(h264_ctx);
> +		get_canvas_index(hw, ctx);
> +
> +		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]
If the execution takes this error path, ctx->int_cond = 1 satisfies the wait
condition in m2mops_vdec_device_run(), which then returns directly without
canceling the job. Does this omit a necessary v4l2_m2m_buf_done_and_job_finish()
call and permanently stall the mem2mem context?

> +		}
> +		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;
[ ... ]
> +static int aml_h264_load_fw_ext(void *priv, const u8 *data, u32 len)
> +{
> +	struct aml_h264_ctx *h264_ctx = (struct aml_h264_ctx *)priv;
> +	struct aml_vdec_ctx *ctx = (struct aml_vdec_ctx *)h264_ctx->v4l2_ctx;
> +	struct aml_vdec_hw *dec_hw;
> +
> +	if (h264_ctx->mc_cpu_loaded)
> +		return 0;
> +
> +	dec_hw = vdec_get_hw(ctx->dev);
> +	if (!dec_hw)
> +		return -1;
> +
> +	if (len > MC_TOTAL_SIZE) {
> +		dev_info(dec_hw->dev, "size of mc_cpu_fw id invalid\n");
> +		return -1;
> +	}

[Severity: Medium]
Should the firmware length also be checked against a minimum bounds before
calling copy_mc_cpu_fw()? Passing truncated firmware could cause out-of-bounds
reads when accessing fixed offsets (e.g., data + 0x6000) inside copy_mc_cpu_fw().

> +
> +	h264_ctx->mc_cpu_vaddr = dma_alloc_coherent(dec_hw->dev, MC_TOTAL_SIZE,
> +						    &h264_ctx->mc_cpu_paddr,
> +						    GFP_KERNEL);
> +	if (!h264_ctx->mc_cpu_vaddr)
> +		return -ENOMEM;
> +
> +	copy_mc_cpu_fw(h264_ctx->mc_cpu_vaddr, data);
> +
> +	h264_ctx->mc_cpu_loaded = true;
> +
> +	dev_dbg(dec_hw->dev, "h264 mccpu fw loaded\n");
> +
> +	return 0;
> +}
> +
> +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;
> +
> +	h264_ctx->mc_cpu_loaded = false;
> +	dec_hw->hw_ops.irq_handler = h264_isr;
> +	dec_hw->hw_ops.irq_threaded_func = h264_threaded_isr_func;
> +	dec_hw->hw_ops.load_firmware_ex = aml_h264_load_fw_ext;
> +
> +	h264_ctx->lmem_addr = dma_alloc_coherent(dec_hw->dev, LMEM_DUMP_SIZE,
> +						 &h264_ctx->lmem_phy_addr,
> +						 GFP_KERNEL);
> +	if (!h264_ctx->lmem_addr) {
> +		ret = -ENOMEM;
> +		goto err_alloc_lmem;
> +	}
> +
> +	h264_ctx->cma_alloc_vaddr =
> +	    dma_alloc_coherent(dec_hw->dev, V_BUF_ADDR_OFFSET,
> +			       &h264_ctx->cma_alloc_addr, GFP_KERNEL);
> +	if (!h264_ctx->cma_alloc_vaddr) {
> +		ret = -ENOMEM;
> +		goto err_alloc_workspace;
> +	}
> +
> +	h264_ctx->workspace_offset = h264_ctx->cma_alloc_addr + DCAC_READ_MARGIN;
> +	h264_ctx->workspace_vaddr = h264_ctx->cma_alloc_vaddr + DCAC_READ_MARGIN;
> +
> +	ctx->codec_priv = h264_ctx;
> +	dec_hw->curr_ctx = h264_ctx;

[Severity: Critical]
Similar to the issue in fops_vcodec_open(), could assigning the context to
dec_hw->curr_ctx here break isolation across multiple concurrent instances?
When multiple streams are active, the ISR will blindly dereference curr_ctx,
potentially targeting the wrong or freed context.

> +	h264_ctx->col_buf_alloc_size = 0;
> +	h264_ctx->init_flag = 0;
> +	h264_ctx->new_pic_flag = 0;
> +	h264_ctx->param_set = 0;
> +	h264_ctx->reg_iqidct_control_init_flag = 0;
> +	h264_ctx->decode_pic_count = 0;
> +
> +	return 0;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260526-b4-s4-vdec-upstream-v5-0-33bc817f93f4@amlogic.com?part=4