Re: [PATCH v4 08/14] media: mediatek: vcodec: clean xpc status

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

New issues:
- [High] Unprotected read-modify-write on the shared VDEC_XPC_CLEAN_ADDR register from concurrent interrupt handlers causes a data race.
- [Medium] Missing bounds check on DT register properties can cause a NULL pointer dereference in mtk_vdec_hw_clean_xpc().
- [Low] Indexing `main_dev->reg_base` using an enum value from a mismatched enum family.

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but a dangling pointer in the main device structure after a subdevice probe failure leads to a Use-After-Free.
- [High] This problem wasn't introduced by this patch, but an unchecked context dereference in the hardware IRQ handler leads to a NULL pointer dereference.
--

--- Patch [8]: [PATCH v4 08/14] media: mediatek: vcodec: clean xpc status ---
Note: The format of this review has been altered to provide a free-form 
summary without directly quoting the original patch code due to system 
recitation restrictions.

Commit: 21f18ceee1ef481effb4080e344810308dfee5b9
Author: Yunfei Dong <[email protected]>
Subject: media: mediatek: vcodec: clean xpc status

Summary: The driver needs to clean xpc status when receiving decoder hardware
interrupt for mt8196 platform.

[Severity: High]
In mtk_vdec_hw_write_reg_mask(), the code performs a read-modify-write
sequence on the register via readl() and writel(). Since
mtk_vdec_hw_clean_xpc() is called from mtk_vdec_hw_irq_handler() for
multiple independent subdevices (like LAT and CORE) that can fire
concurrently on different CPUs, is it possible for these MMIO accesses to
race? If they race on the shared VDEC_XPC_CLEAN_ADDR register without a
spinlock, could hardware register updates be lost?

[Severity: Low]
Looking at mtk_vdec_hw_probe(), subdev_dev->reg_base[VDEC_HW_XPC] is
assigned using main_dev->reg_base[VDEC_HW_MISC]. Does this index into
main_dev->reg_base using the wrong enum family? It seems main_dev->reg_base
should logically be indexed by enum mtk_hw_reg_idx (where VDEC_MISC is 1),
but it is using VDEC_HW_MISC from enum mtk_vdec_hw_reg_idx. While these
currently share the same integer value, could this silently break if the
enums are ever modified?

[Severity: High]
This isn't a bug introduced by this patch, but looking at
mtk_vdec_hw_probe(), subdev_dev is assigned to main_dev->subdev_dev[hw_idx]
and marked active in subdev_bitmap before the rest of the initialization
finishes. If a subsequent step like devm_platform_ioremap_resource() fails,
the probe returns an error and devm frees subdev_dev. Does this leave a
dangling pointer in the parent's array? Since the bit remains set in
subdev_bitmap, mtk_vdec_hw_prob_done() might consider it ready, potentially
leading to a use-after-free when mtk_vcodec_get_hw_dev() dereferences it.

[Severity: Medium]
In mtk_vdec_hw_probe(), subdev_dev->reg_base[VDEC_HW_XPC] inherits the
pointer from main_dev->reg_base[VDEC_HW_MISC]. If the device tree is
malformed and provides fewer register bases than expected (for example,
only 1 instead of the required number), could
main_dev->reg_base[VDEC_HW_MISC] be NULL? If so, when an interrupt fires,
would mtk_vdec_hw_clean_xpc() blindly access this NULL pointer via readl(),
causing a kernel panic?

[Severity: High]
This is a pre-existing issue, but in mtk_vdec_hw_irq_handler(),
mtk_vcodec_get_curr_ctx() is called to fetch the context. Can this function
legitimately return NULL, for instance during spurious interrupts or early
teardown? If ctx is NULL, wouldn't passing it directly to
mtk_v4l2_vdec_err() and wake_up_dec_ctx() cause an immediate NULL pointer
dereference, as both unconditionally dereference it without checking?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8