Re: [PATCH v6 12/14] media: mediatek: vcodec: clean xpc status

[sashiko-bot@kernel.org]

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

New issues:
- [Low] Enum type mismatch when indexing parent device's register array.

Pre-existing issues:
- [High] Null pointer dereference on spurious hardware interrupts.
- [High] Use-after-free and dirty state on probe error path.
- [High] Unconditional NULL pointer dereference in the hardware IRQ handler on platforms using syscon for VDECSYS.
--

--- Patch [12]: [PATCH v6 12/14] media: mediatek: vcodec: clean xpc status ---
commit ff6c78cd026b1536229c06cdffd12b2498c8aeb9
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: vcodec: clean xpc status

The driver need to clean xpc status when receive decoder hardware
interrupt for mt8196 platform.

> diff --git a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> index 881d5de41e050..46ac3e41c401c 100644
> --- a/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> +++ b/drivers/media/platform/mediatek/vcodec/decoder/mtk_vcodec_dec_hw.c
> @@ -88,6 +116,8 @@ static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv)

[Severity: High]
This is a pre-existing issue, but does mtk_vdec_hw_irq_handler() in
mtk_vcodec_dec_hw.c properly handle spurious interrupts where
mtk_vcodec_get_curr_ctx() returns NULL?

If there is no active context when the interrupt fires, ctx will be NULL.
The handler subsequently passes this NULL context to mtk_v4l2_vdec_err(),
which dereferences ctx->dev directly:

mtk_vdec_hw_irq_handler() {
    ...
    ctx = mtk_vcodec_get_curr_ctx(dev->main_dev, dev->hw_idx);

    /* check if HW active or not */
    cg_status = readl(dev->reg_base[VDEC_HW_SYS] + VDEC_HW_ACTIVE_ADDR);
    if (cg_status & VDEC_HW_ACTIVE_MASK) {
        mtk_v4l2_vdec_err(ctx, "vdec active is not 0x0 (0x%08x)", cg_status);
        return IRQ_HANDLED;
    }
    ...
}

Could this trigger a kernel panic on spurious interrupts?

[Severity: High]
This is also a pre-existing issue, but will dev->reg_base[VDEC_HW_SYS] be
NULL in mtk_vdec_hw_irq_handler() on platforms using syscon for VDECSYS
(like MT8195 and MT8196)?

On platforms utilizing the mediatek,vdecsys syscon, main_dev->reg_base[0] is
left NULL. The subdevice probe copies this NULL pointer to the subdevice.
When an interrupt fires, the handler performs an unconditional readl() from
this NULL address in the snippet shown above, instead of using the
vdecsys_regmap fallback.

Could this cause an unconditional panic when an interrupt fires on affected
platforms?

>  	writel(dec_done_status | VDEC_IRQ_CFG, vdec_misc_addr);
>  	writel(dec_done_status & ~VDEC_IRQ_CLR, vdec_misc_addr);
>  
> +	mtk_vdec_hw_clean_xpc(dev);
> +
>  	wake_up_dec_ctx(ctx, MTK_INST_IRQ_RECEIVED, dev->hw_idx);
>  
>  	mtk_v4l2_vdec_dbg(3, ctx, "wake up ctx %d, dec_done_status=%x",

[ ... ]

> @@ -166,6 +196,7 @@ static int mtk_vdec_hw_probe(struct platform_device *pdev)
>  	subdev_dev->hw_idx = hw_idx;
>  	subdev_dev->main_dev = main_dev;
>  	subdev_dev->reg_base[VDEC_HW_SYS] = main_dev->reg_base[VDEC_HW_SYS];
> +	subdev_dev->reg_base[VDEC_HW_XPC] = main_dev->reg_base[VDEC_HW_MISC];

[Severity: Low]
Is mtk_vdec_hw_probe() indexing the main_dev->reg_base array with the
correct enum?

It looks like it is using VDEC_HW_MISC (from enum mtk_vdec_hw_reg_idx)
instead of the expected VDEC_MISC (from enum mtk_hw_reg_idx). While they
both currently evaluate to 1, could this cause out-of-bounds access or
incorrect address mappings if the enums drift?

>  	set_bit(subdev_dev->hw_idx, main_dev->subdev_bitmap);
>  
>  	if (IS_SUPPORT_VDEC_HW_IRQ(hw_idx)) {

[Severity: High]
This is a pre-existing issue, but does the mtk_vdec_hw_probe() error path
leave dangling pointers in the parent device?

If mtk_vdec_hw_init_irq() fails here, mtk_vdec_hw_probe() returns an error:

mtk_vdec_hw_probe() {
    ...
    if (IS_SUPPORT_VDEC_HW_IRQ(hw_idx)) {
        ret = mtk_vdec_hw_init_irq(subdev_dev);
        if (ret)
            return ret;
    }
    ...
}

Since subdev_dev was allocated with devres, it will be automatically freed.
However, the parent (main_dev) will retain the freed pointer in
main_dev->subdev_dev[hw_idx] and the bit in main_dev->subdev_bitmap remains
set.

Could this lead to a use-after-free if another component iterates over the
subdevices?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720012056.1026551-1-kyrie.wu@mediatek.com?part=12