Re: [PATCH v15 12/12] media: mediatek: jpeg: add jpeg smmu sid setting

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Permanent increment of `hw_index` and `max_hw_count` on probe failure leads to Buffer Overflow and Use-After-Free.
- [High] Missing `pm_runtime_put_noidle()` when `pm_runtime_get_sync()` fails in the encoder worker.
- [High] Race condition between HW IRQ handler and timeout worker leading to double buffer completion.
--

commit 4f42651eeefeb9f2b2cb2fedba4846b1c45e369e
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: add jpeg smmu sid setting

This commit adds a configuration to set the JPEG decoder and encoder SMMU SID.
It maps the SMMU register base from the device tree and updates the SID
settings during worker initialization.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> index 5a485e905787d..2e079a6e5d5d3 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
[ ... ]
> @@ -666,6 +685,10 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>  	dev->master_dev = master_dev;
>  	master_dev->max_hw_count++;
>  
> +	ret = mtk_jpegdec_smmu_init(dev);
> +	if (ret)
> +		return ret;
> +

[Severity: Critical]
This is a pre-existing issue, but can this early return cause an out-of-bounds
array write on subsequent probe attempts?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c:mtk_jpegdec_hw_probe(),
the counters are incremented prior to this block:

	i = atomic_add_return(1, &master_dev->hw_index) - 1;
	master_dev->dec_hw_dev[i] = dev;
	master_dev->reg_decbase[i] = dev->reg_base;
	dev->master_dev = master_dev;
	master_dev->max_hw_count++;

If mtk_jpegdec_smmu_init() (or the pre-existing devm_clk_bulk_get() call below
it) returns an error such as -EPROBE_DEFER, the probe aborts without
decrementing master_dev->hw_index or max_hw_count. 

When the system retries probing, these counters will be permanently
incremented, rapidly exceeding the bounds of the dec_hw_dev array and
leading to memory corruption.

>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  	ret = devm_clk_bulk_get(dev->dev,
> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> index 1cfdfac98829b..0001d69ae7bab 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
[ ... ]
> @@ -390,6 +409,10 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	dev->master_dev = master_dev;
>  	master_dev->max_hw_count++;
>  
> +	ret = mtk_jpegenc_smmu_init(dev);
> +	if (ret)
> +		return ret;
> +

[Severity: Critical]
This isn't a bug introduced by this patch, but could this early return also
lead to an out-of-bounds write here?

Just like in the decoder, if mtk_jpegenc_smmu_init() fails in
drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c:mtk_jpegenc_hw_probe(),
the function returns without rolling back master_dev->hw_index and
max_hw_count. Subsequent probe retries will use an out-of-bounds index for
the enc_hw_dev array.

>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  	ret = devm_clk_bulk_get(dev->dev,

[Severity: High]
This isn't a bug introduced by this patch, but does this error handling path
leak the PM runtime usage counter?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:mtk_jpegenc_worker():

	ret = pm_runtime_get_sync(comp_jpeg[hw_id]->dev);
	if (ret < 0) {
		dev_err(jpeg->dev, "%s : %d, pm_runtime_get_sync fail !!!\n",
			__func__, __LINE__);
		goto enc_end;
	}

If pm_runtime_get_sync() fails, the code jumps directly to enc_end without
calling pm_runtime_put_noidle() to drop the acquired reference, potentially
preventing the device from ever entering a low-power state.

[Severity: High]
This is a pre-existing issue, but is there a race condition between the
hardware IRQ handler and the timeout worker?

In drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c:mtk_jpegenc_hw_irq_handler():

	cancel_delayed_work(&jpeg->job_timeout_work);

	ctx = jpeg->hw_param.curr_ctx;
	src_buf = jpeg->hw_param.src_buffer;
	dst_buf = jpeg->hw_param.dst_buffer;

If the hardware times out, mtk_jpegenc_timeout_work() executes and
unconditionally completes the buffers and drops PM references:

	v4l2_m2m_buf_done(src_buf, buf_state);
	mtk_jpegenc_put_buf(cjpeg);
	pm_runtime_put(cjpeg->dev);

If the hardware interrupt fires concurrently on another CPU,
cancel_delayed_work() will return false without waiting. Both contexts will
proceed to process the completion, potentially leading to double buffer
completion and a double PM runtime put.

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