Re: [PATCH v8 3/3] media: mediatek: jpeg: add compatible for MT8189 SoC

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Missing cancellation of `job_timeout_work` in `mtk_jpeg_remove()` leads to a Use-After-Free.
- [High] Variant structures use globally shared static arrays for clock definitions, which are illegally mutated during device probe by `devm_clk_bulk_get()`.
--

commit 8f115ea6a1de2f80507b4b60d5ec40d3fdf1eb31
Author: Jianhua Lin <jianhua.lin@mediatek.com>

media: mediatek: jpeg: add compatible for MT8189 SoC

The commit adds support for the MT8189 SoC's JPEG encoder and decoder by
introducing new compatible strings. It also adds the associated driver data
structures configuring the hardware for a 34-bit IOVA space and single
clock setup.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index d147ec4830811..14f2991a40535 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1868,6 +1868,10 @@ static struct clk_bulk_data mt8173_jpeg_dec_clocks[] = {
>  	{ .id = "jpgdec" },
>  };
>  
> +static struct clk_bulk_data mtk_jpeg_dec_clocks[] = {
> +	{ .id = "jpgdec" },
> +};
> +
>  static const struct mtk_jpeg_variant mt8173_jpeg_drvdata = {
>  	.clks = mt8173_jpeg_dec_clocks,

[ ... ]

> @@ -1899,6 +1903,38 @@ static const struct mtk_jpeg_variant mtk_jpeg_drvdata = {
>  	.multi_core = false,
>  };
>  
> +static const struct mtk_jpeg_variant mtk8189_jpegenc_drvdata = {
> +	.clks = mtk_jpeg_clocks,

Does this code pass a shared static array to devm_clk_bulk_get()?

During device probe, the global mtk_jpeg_clocks array is passed directly:

mtk_jpeg_probe()
    devm_clk_bulk_get(jpeg->dev, jpeg->variant->num_clks, jpeg->variant->clks)

The devm_clk_bulk_get() API modifies the array in-place by writing the
acquired struct clk pointers into it.

If multiple device instances probe, or if unbind/rebind cycles occur, could
the clock pointers be clobbered by subsequent probes? This might lead to
hardware lockups since clocks wouldn't be enabled or disabled properly, or
use-after-free conditions when devres frees the clocks but dangling pointers
remain in the global array.

Should the driver dynamically allocate a per-device clk_bulk_data array
during probe instead?

> +	.num_clks = ARRAY_SIZE(mtk_jpeg_clocks),
> +	.formats = mtk_jpeg_enc_formats,
> +	.num_formats = MTK_JPEG_ENC_NUM_FORMATS,
> +	.qops = &mtk_jpeg_enc_qops,
> +	.irq_handler = mtk_jpeg_enc_irq,
> +	.hw_reset = mtk_jpeg_enc_reset,
> +	.m2m_ops = &mtk_jpeg_enc_m2m_ops,
> +	.dev_name = "mtk-jpeg-enc",
> +	.ioctl_ops = &mtk_jpeg_enc_ioctl_ops,
> +	.out_q_default_fourcc = V4L2_PIX_FMT_YUYV,
> +	.cap_q_default_fourcc = V4L2_PIX_FMT_JPEG,
> +	.support_34bit = true,
> +};
> +

This isn't a bug in this patch, but looking at the single-core implementation
that this variant uses, mtk_jpeg_single_core_init() sets up the workqueue:

mtk_jpeg_single_core_init()
    INIT_DELAYED_WORK(&jpeg->job_timeout_work, mtk_jpeg_job_timeout_work);

However, mtk_jpeg_remove() doesn't cancel this work:

mtk_jpeg_remove()
    pm_runtime_disable(&pdev->dev);
    video_unregister_device(jpeg->vdev);
    v4l2_m2m_release(jpeg->m2m_dev);
    v4l2_device_unregister(&jpeg->v4l2_dev);

If the driver is unbound while a job is active, mtk_jpeg_remove() will
complete and the devm subsystem will free the jpeg structure. If the timeout
work fires afterwards and executes mtk_jpeg_job_timeout_work(), will this
result in a use-after-free when it dereferences the freed jpeg pointer?

> +static const struct mtk_jpeg_variant mtk8189_jpegdec_drvdata = {
> +	.clks = mtk_jpeg_dec_clocks,

Does this structure also pass a shared static array to devm_clk_bulk_get(),
similar to mtk8189_jpegenc_drvdata above?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260519021726.19137-1-jianhua.lin@mediatek.com?part=3