[PATCH v11 4/6] decoder: Add V4L2 stateless H.264 decoder driver
sashiko-bot at kernel.org
sashiko-bot at kernel.org
Mon Oct 12 00:20:47 PDT 2026
Thank you for your contribution! Sashiko AI review found 19 potential issue(s) to consider:
- [Critical] media: amlogic: out-of-bounds array access in aml_vdec_init_dec_inst allows control flow hijack
- [Critical] media: amlogic: concurrent STREAMOFF frees codec context while worker thread is waiting
- [High] media: amlogic: uaf on device removal due to devm_kzalloc and v4l2_m2m_release
- [High] media: amlogic: get_canvas_index maps stale dangling DMA addresses to hardware
- [High] media: amlogic: stale one_col_buf_size causes DMA buffer overflow on resolution change
- [High] media: amlogic: missing error handling in pm_vdec_power_domain_power_on causes leaks and MMIO aborts
- [High] media: amlogic: aml_stop_vdec_hw resets hardware concurrently with threaded ISR
- [High] media: amlogic: unprotected dos_enable in device_run races with aml_h264_exit
- [High] media: amlogic: vb2_find_buffer called from threaded ISR without holding queue lock
- [High] media: amlogic: video device registered before hardware power domain is enabled
- [Medium] media: amlogic: irq registered before hardware reset and without handler
- [Medium] media: amlogic: failed canvas config silently compresses hardware reference list
- [Medium] media: amlogic: per-instance power domain device state corrupts static global array
- [Medium] media: amlogic: ref_list1 incorrectly checked against ref_list0 valid flag
- [Medium] media: amlogic: poc_is_in_dpb ignores V4L2_H264_DPB_ENTRY_FLAG_VALID leaking resources
- [Low] media: amlogic: double completion of media request triggers WARN_ON
- [Low] media: amlogic: vdec_s_fmt_output falsely rejects identical format changes with -EBUSY
- [Low] media: amlogic: hardware stride configured using SPS width instead of v4l2 bytesperline
- [Low] media: amlogic: implicit declaration of kzalloc_obj causes build failure
--
commit da274125c14d2da90bb83a7ffc30b57d96da9323
Author: Zhentao Guo <zhentao.guo at amlogic.com>
Subject: decoder: Add V4L2 stateless H.264 decoder driver
This commit adds initial support for a V4L2 stateless video decoder driver on
the Amlogic S4 platform. It supports 8-bit H.264 progressive bitstreams in
this first phase.
> 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
> @@ -133,6 +133,8 @@ static void m2mops_vdec_device_run(void *m2m_priv)
> 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;
> ctx->curr_frm_err_flag = 0;
>
> if (ctx->curr_dec_type == CODEC_TYPE_H264)
> aml_vdec_reset_core(dev->dec_hw);
[Severity: High]
Does this execute hardware setup before curr_ctx is assigned and without
cross-instance locking?
If another decoding instance is concurrently stopped via VIDIOC_STREAMOFF, its
aml_h264_exit() could see (!dec_hw->curr_ctx || dec_hw->curr_ctx == h264_ctx)
evaluate to true, locking ctx_mutex and shutting down the hardware while this
instance is simultaneously trying to initialize it.
> @@ -148,6 +148,8 @@ static void m2mops_vdec_device_run(void *m2m_priv)
> v4l2_m2m_buf_copy_metadata(src_buf, dst_buf);
> if (src_req)
> v4l2_ctrl_request_complete(src_req, &ctx->ctrl_handler);
[Severity: Low]
Is this explicit call to v4l2_ctrl_request_complete() triggering a double
completion warning?
Later in this function, v4l2_m2m_buf_done_and_job_finish() will call
vb2_buffer_done(), which invokes the buf_request_complete callback defined in
aml_vdec_vb2_ops. That callback also calls v4l2_ctrl_request_complete() on the
same request.
[ ... ]
> @@ -231,6 +231,10 @@ static int aml_vdec_init_dec_inst(struct aml_vdec_ctx *ctx)
> struct aml_video_fmt *fmt_out = &ctx->dec_fmt[AML_FMT_SRC];
> int ret = -1;
>
> ctx->codec_ops = &dev->pvdec_data->codec_ops[fmt_out->codec_type];
> /*
> * The codec instance is only released when both queues are
> * streamed off: do not allocate a new one (leaking the previous
> * instance) when the output queue is restarted while the
> * capture queue is still streaming.
> */
> if (ctx->codec_ops->init && !ctx->codec_priv) {
> ret = ctx->codec_ops->init(ctx);
[Severity: Critical]
Does this permit an out-of-bounds array access if fmt_out->codec_type is
untrusted?
If unprivileged userspace calls VIDIOC_S_FMT on the output queue with a
capture-only format (e.g. V4L2_PIX_FMT_NV12), aml_vdec_get_video_fmt() could
return that format and record its codec_type as CODEC_TYPE_FRAME (1). When
STREAMON resolves the codec operations here, index 1 reads past the end of the
aml_S4_dec_ops array and dereferences a wild function pointer.
[ ... ]
> @@ -311,6 +311,10 @@ static int vdec_s_fmt_output(struct aml_vdec_ctx *ctx, struct v4l2_format *f)
> peer_vq = v4l2_m2m_get_vq(ctx->m2m_ctx, V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE);
> if (vb2_is_busy(peer_vq) &&
> (pix_mp->width != cap_fmt->width ||
> pix_mp->height != cap_fmt->height))
> return -EBUSY;
[Severity: Low]
Could this falsely reject identical format changes with -EBUSY?
The cap_fmt structure contains the active capture format, which has already
been aligned to hardware constraints. The pix_mp structure contains the
unaligned user-requested dimensions. If userspace requests the exact same
unaligned format while the queue is busy, this check fails and falsely rejects
a legitimate no-op format setting.
>
> out_fmt = aml_vdec_get_video_fmt(ctx->dev, pix_mp->pixelformat);
> if (out_fmt) {
> ctx->dec_fmt[AML_FMT_SRC] = *out_fmt;
[Severity: Critical]
Can this bind a capture-only format to the output queue?
The aml_vdec_get_video_fmt() function ignores fmt->type during lookup. This
means a format like V4L2_PIX_FMT_NV12 can be applied to the output queue,
which eventually leads to the out-of-bounds access in aml_vdec_init_dec_inst().
[ ... ]
> @@ -522,6 +522,8 @@ static void vb2ops_vdec_stop_streaming(struct vb2_queue *q)
> }
>
> if (!ctx->is_output_streamon && !ctx->is_cap_streamon)
> aml_vdec_release_instance(ctx);
> }
[Severity: Critical]
Does this free the codec context without synchronizing against the active
worker thread?
If STREAMOFF runs while the device_run worker thread is blocked in
wait_event_interruptible_timeout(), aml_h264_exit() will free ctx->codec_priv.
When the worker thread wakes up, it will write to the freed memory.
> 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
> @@ -23,6 +23,7 @@ static int fops_vcodec_open(struct file *file)
> struct aml_vdec_ctx *ctx = NULL;
> int ret = 0;
>
> ctx = kzalloc_obj(*ctx, GFP_KERNEL);
[Severity: Low]
Is kzalloc_obj() an undefined macro/function here?
It looks like this might have meant to be kzalloc(sizeof(*ctx), GFP_KERNEL),
and will cause a build failure as written.
[ ... ]
> @@ -68,6 +69,7 @@ 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);
[Severity: High]
Does this access the M2M device after it has been freed?
If the device is unbound via sysfs while a userspace process holds the file
descriptor open, aml_vdec_drv_remove() runs and calls v4l2_m2m_release().
Since the driver structures were allocated with devm_kzalloc, they are freed
immediately. When userspace eventually closes the file descriptor, this delayed
call accesses the freed m2m_dev_dec.
[ ... ]
> @@ -168,6 +170,8 @@ static int aml_vdec_drv_probe(struct platform_device *pdev)
> goto err_m2m_dev_init;
> }
>
> ret = video_register_device(vfd_dec, VFL_TYPE_VIDEO, -1);
> if (ret) {
> v4l2_err(&dev->v4l2_dev, "Failed to register video device");
> @@ -189,6 +193,7 @@ static int aml_vdec_drv_probe(struct platform_device *pdev)
> v4l2_err(&dev->v4l2_dev, "Failed to register media device");
> goto err_media_dev_register;
> }
> vdec_enable(dev->dec_hw);
[Severity: High]
Is the video device registered and exposed to userspace before the hardware
power domain is enabled?
A fast userspace process can immediately open the device and start streaming,
causing the driver to execute MMIO reads/writes on unpowered registers,
resulting in synchronous external aborts.
[ ... ]
> @@ -233,6 +238,8 @@ static void aml_vdec_drv_remove(struct platform_device *pdev)
> 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]
Does releasing the M2M device here create a use-after-free risk for open file
descriptors?
Since dev and dec_hw use devm_kzalloc, releasing them here before all fds are
closed means that fops_vcodec_release() will later dereference freed memory.
> 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
> @@ -179,6 +179,8 @@ static int pm_vdec_power_domain_init(struct aml_vdec_hw *hw)
>
> 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);
> pd[i].dev = NULL;
> continue;
> }
>
> pd[i].link = device_link_add(hw->dev, pd[i].dev,
[Severity: Medium]
Does storing per-device pointers into the static global 'vdec_domain_data'
array corrupt state for multiple instances?
If the driver unbinds and rebinds, or if multiple instances probe, the global
array will be overwritten, causing teardown routines to operate on corrupted
pointers.
[ ... ]
> @@ -283,6 +285,8 @@ static void pm_vdec_power_domain_power_on(struct aml_vdec_hw *hw, int id)
>
> if (!ret && dev) {
> ret = pm_runtime_resume_and_get(dev);
> if (ret < 0)
> dev_err(hw->dev, "failed to power on %s domain: %d\n",
> pm->pd_data[id].name, ret);
> else
> dev_dbg(dev, "dev: %p link %p the %-15s power on\n",
> dev, pm->pd_data[id].link, pm->pd_data[id].name);
> }
>
> /*
> * Do not touch the DOS registers when the clock or the power
> * domain could not be enabled, the MMIO access would abort.
> */
> if (!ret)
> dos_local_config(hw, 1, id);
[Severity: High]
If pm_runtime_resume_and_get() fails, does this leave the clock enabled?
Because it doesn't clean up the clk_prepare_enable() call, the clock leaks.
Also, if either gate_node or dev is NULL, ret remains 0, which bypasses the
enablement but allows dos_local_config() to execute unpowered MMIO writes,
causing a bus crash.
[ ... ]
> @@ -343,6 +347,8 @@ static irqreturn_t vdec_threaded_isr_handler(int irq, void *priv)
>
> mutex_lock(&hw->ctx_mutex);
> if (hw->hw_ops.irq_threaded_func && hw->curr_ctx)
> ret = hw->hw_ops.irq_threaded_func(irq, priv);
> mutex_unlock(&hw->ctx_mutex);
[Severity: High]
Does this threaded ISR program hardware registers while relying solely on
ctx_mutex?
During a decoder timeout, aml_h264_dec_run() calls aml_stop_vdec_hw() without
acquiring ctx_mutex. This causes the hardware to be reset concurrently while
the ISR might still be actively programming its memory and registers.
[ ... ]
> @@ -428,6 +434,8 @@ static irqreturn_t vdec_irq_handler(int irq, void *priv)
> irqreturn_t ret = IRQ_NONE;
>
> if (hw->hw_ops.irq_handler)
> ret = hw->hw_ops.irq_handler(irq, priv);
>
> return ret;
> }
[ ... ]
> @@ -479,6 +487,8 @@ int dev_request_hw_resources(void *priv)
> dev_err(&pdev->dev, "get irq failed\n");
> return hw->dec_irq;
> }
> ret = devm_request_threaded_irq(&pdev->dev, hw->dec_irq, vdec_irq_handler,
[Severity: Medium]
Could a pending interrupt cause the IRQ line to be permanently disabled?
The IRQ is registered before the hardware has been explicitly reset, and
hw->hw_ops.irq_handler is NULL at this point. If an interrupt fires,
vdec_irq_handler() will repeatedly return IRQ_NONE, causing the kernel to
detect a spurious IRQ storm and disable the line.
> 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
> @@ -423,6 +423,8 @@ static int poc_is_in_dpb(int poc, const struct v4l2_h264_dpb_entry *dpb)
> int ret = 0;
>
> for (i = 0; i < V4L2_H264_NUM_DPB_ENTRIES; i++) {
> if (poc == dpb[i].top_field_order_cnt) {
> ret = 1;
> break;
[Severity: Medium]
Should this check V4L2_H264_DPB_ENTRY_FLAG_VALID before comparing the POC?
If it compares against inactive or stale entries with lingering zeros, it can
falsely return 1, preventing clear_unused_col_buf() from releasing the
associated canvas positions and colocate buffers.
[ ... ]
> @@ -494,6 +496,8 @@ static int config_decode_canvas(struct aml_vdec_hw *hw,
> /* config y canvas */
> meson_canvas_config(hw->canvas,
> buf_spec->y_canvas_index, buf_spec->y_dma_addr,
> mb_width << 4, mb_height << 4,
[Severity: Low]
Does this hardcode the hardware stride to the bitstream-derived width instead
of the negotiated bytesperline?
V4L2 semantics require using the bytesperline set via VIDIOC_S_FMT. If the
negotiated stride differs from the tightly-packed SPS width, the resulting
image will suffer from visual corruption.
[ ... ]
> @@ -609,6 +613,9 @@ static int alloc_colocate_cma(struct aml_h264_ctx *h264_ctx,
> gfp_t gfp = GFP_KERNEL | GFP_DMA32;
>
> if (h264_ctx->collated_cma_vaddr)
> return 0;
>
> hw = vdec_get_hw(ctx->dev);
[Severity: High]
Does this bypass updating one_col_buf_size if the buffer was already allocated?
If the stream dynamically increases in resolution, h264_ctx->one_col_buf_size
will remain at the stale, smaller size.
[ ... ]
> @@ -838,6 +845,9 @@ static int h264_config_decode_buf(struct aml_vdec_hw *hw,
> 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;
> /* bit 0:3 canvas_pos bit 5:6 frame struct cfg */
> ref_cfg_once = (canvas_pos & 0x1f) | (type_cfg << 5);
[Severity: Medium]
If a reference picture is invalid, does skipping it without incrementing 'j'
shift all subsequent valid entries into the wrong hardware slots?
This `continue` statement silently bypasses the hardware index packing,
corrupting the decoding of B-frames or P-frames.
[ ... ]
> @@ -865,6 +875,8 @@ static int h264_config_decode_buf(struct aml_vdec_hw *hw,
> ref_cfg = 0;
>
> for (i = 0; i < h264_ctx->list_size[1]; i++) {
> if (!h264_ctx->ref_list0[i].valid)
> continue;
> canvas_pos = h264_ctx->ref_list1[i].canvas_pos;
[Severity: Medium]
Is ref_list1 incorrectly checked against ref_list0's valid flag here?
If ref_list0 has fewer valid entries than ref_list1, the loop will erroneously
skip valid ref_list1 entries.
[ ... ]
> @@ -896,6 +908,9 @@ static int h264_config_decode_buf(struct aml_vdec_hw *hw,
> 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_wr_adr + h264_ctx->one_col_buf_size >
> (unsigned int)h264_ctx->collated_cma_addr_end) {
[Severity: High]
Does this bounds check permit memory corruption because it verifies against
the old smaller one_col_buf_size?
The hardware is programmed with the new larger dimensions, writing
new_mb_total * 96 bytes starting at colocate_wr_adr, overflowing the spacing
defined by the stale one_col_buf_size.
[ ... ]
> @@ -1570,6 +1585,8 @@ static int h264_config_ref_spec(struct aml_h264_ctx *h264_ctx,
> return -1;
> }
>
> vb = vb2_find_buffer(vq, dpb->reference_ts);
[Severity: High]
Is it safe to iterate over the vb2 queue buffers from the threaded ISR without
holding the v4l2_intf_lock?
If a concurrent VIDIOC_REQBUFS or VIDIOC_STREAMOFF modifies the queue while
this is executing, it could lead to use-after-free or out-of-bounds accesses.
> if (!vb) {
> dev_dbg(&ctx->dev->plat_dev->dev,
> "ref pic for ts %llu lost\n", dpb->reference_ts);
> buf_spec->valid = 0;
> return -1;
[Severity: High]
Does this leave the stale physical addresses in the buffer spec?
When combined with the loop in get_canvas_index(), which ignores the valid
flag:
drivers/media/platform/amlogic/vdec/h264.c:get_canvas_index() {
...
for (i = 0; i < h264_ctx->list_size[0]; i++) {
buf = &h264_ctx->ref_list0[i];
ret = config_decode_canvas(hw, buf, h264_ctx->mb_width,
...
}
this will configure the hardware to map and use the stale, dangling y_dma_addr
for the current frame's decoding if reference picture validation fails.
[ ... ]
> @@ -1665,6 +1682,9 @@ static int get_canvas_index(struct aml_vdec_hw *hw, struct aml_vdec_ctx *ctx)
> if (ret)
> return ret;
>
> if (h264_ctx->list_size[0] > 0) {
> for (i = 0; i < h264_ctx->list_size[0]; i++) {
> buf = &h264_ctx->ref_list0[i];
> ret = config_decode_canvas(hw, buf, h264_ctx->mb_width,
> h264_ctx->mb_height);
[Severity: High]
Does this iterate over all reference entries and configure canvases without
checking if the entry is valid?
This ignores buf->valid and programs potentially stale or failed DMA addresses
into the hardware.
[ ... ]
> @@ -2015,6 +2035,8 @@ void aml_h264_exit(void *priv)
>
> synchronize_irq(dec_hw->dec_irq);
> mutex_lock(&dec_hw->ctx_mutex);
> if (ctx->dos_clk_en &&
> (!dec_hw->curr_ctx || dec_hw->curr_ctx == h264_ctx))
> aml_stop_vdec_hw(dec_hw);
[Severity: High]
Can this shut down the hardware concurrently while another instance is being
initialized?
If m2mops_vdec_device_run() is currently executing dos_enable() for a new
instance, dec_hw->curr_ctx might still be NULL. This condition evaluates to
true, resulting in aml_stop_vdec_hw() running and resetting the hardware
clocks/registers out from under the other context.
[ ... ]
> @@ -2092,6 +2114,8 @@ int aml_h264_dec_run(void *priv)
> dev_err(&ctx->dev->plat_dev->dev, "dec inter fail\n");
> }
>
> h264_ctx->init_flag = 0;
> aml_vdec_set_curr_ctx(dec_hw, NULL);
[Severity: Critical]
Does this write to the freed context after returning from
wait_event_interruptible_timeout()?
If a concurrent STREAMOFF has already freed h264_ctx, setting init_flag here
results in a use-after-free.
>
> return ret;
[ ... ]
> @@ -2164,6 +2188,8 @@ int aml_h264_dec_run(void *priv)
> * h264_release_decode_spec() concurrently.
> */
> aml_stop_vdec_hw(dec_hw);
> synchronize_irq(dec_hw->dec_irq);
[Severity: High]
Does this call hardware reset operations concurrently without the ctx_mutex and
before synchronizing the ISR?
If the threaded ISR is active, aml_stop_vdec_hw() will reset the hardware while
the ISR is actively programming its memory and registers, leading to bus
lockups.
--
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-b4-s4-vdec-upstream-v11-0-25939f85ada5@amlogic.com?part=4
More information about the linux-amlogic
mailing list