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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: mtk-vcodec: NULL pointer deref in mtk_vdec_hw_irq_handler()
- [High] media: mtk-vcodec: NULL pointer dereference in mtk_vdec_hw_clean_xpc() on incomplete DT
--

commit 48b9203929193f7043af88ad2f6320fd0d5acb54
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
> @@ -61,6 +61,34 @@ static int mtk_vdec_hw_prob_done(struct mtk_vcodec_dec_dev *vdec_dev)
>  	return 0;
>  }
>  
> +static void mtk_vdec_hw_write_reg_mask(void __iomem *reg_base, u32 reg_offset, u32 val, u32 mask)
> +{
> +	void __iomem *reg_addr = reg_base + reg_offset;
> +	u32 reg_val;
> +
> +	reg_val =  readl(reg_addr);

[Severity: High]
Does this code dereference a NULL pointer if the parent device tree node
provides only one reg entry?

If the MT8196 parent device tree node provides only one reg entry (which is
permitted by the DT bindings' minItems: 1), mtk_vcodec_get_reg_bases() will map
only VDEC_SYS and leave main_dev->reg_base[VDEC_MISC] as NULL.

This NULL pointer is propagated to subdev_dev->reg_base[VDEC_HW_XPC] during
mtk_vdec_hw_probe():

    subdev_dev->reg_base[VDEC_HW_XPC] = main_dev->reg_base[VDEC_HW_MISC];

When the hardware interrupt fires, mtk_vdec_hw_clean_xpc() is unconditionally
called for MT8196 and passes this NULL base to mtk_vdec_hw_write_reg_mask(),
which dereferences it here in readl().

Would it be better to validate the register base before use, or enforce
reg_num >= 2 for MT8196 during probe?

> +	reg_val &= ~mask;
> +	reg_val |= (val & mask);
> +	writel(reg_val, reg_addr);
> +}

[ ... ]

> @@ -88,6 +116,8 @@ static irqreturn_t mtk_vdec_hw_irq_handler(int irq, void *priv)
>  	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);

[Severity: High]
Can ctx be NULL here during the context teardown race window?

If a delayed or spurious interrupt fires during the teardown path window
(after mtk_vcodec_set_curr_ctx(dev, NULL, MTK_VDEC_CORE) clears the current
context, but before mtk_vcodec_dec_disable_irq() is called), or if hardware
continues generating events before IRQs are disabled, mtk_vdec_hw_irq_handler()
fetches a NULL ctx.

If the HW is inactive, the NULL ctx is unconditionally dereferenced earlier by
the mtk_v4l2_vdec_err macro to log an error. If the HW is active, it is
unconditionally dereferenced here to wake up the context queue.

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