Re: [PATCH v18 13/13] media: mediatek: jpeg: add jpeg smmu sid setting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] media: mtk-jpeg: UAF and OOB write on smmu_init probe failure
- [High] media: mtk-jpeg: stack and heap overflow due to unbounded DT child nodes
--

commit 5c85022fd040c3aae35226e60e232c6d2771df6e
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.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
[ ... ]
> @@ -665,6 +684,10 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>  	master_dev->reg_decbase[i] = dev->reg_base;
>  	dev->master_dev = master_dev;
>  
> +	ret = mtk_jpegdec_smmu_init(dev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
If mtk_jpegdec_smmu_init() returns an error (such as -EPROBE_DEFER
when the SMMU isn't ready), the probe function returns immediately.

Will this leave a dangling pointer in the global master_dev->dec_hw_dev[i]
array after devm frees the dev structure?

Additionally, master_dev->hw_index is incremented via atomic_add_return()
before this initialization step. Because it is not decremented on this
error path, could subsequent probe retries keep incrementing hw_index and
eventually cause an out-of-bounds write on the fixed-size dec_hw_dev array?

>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->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
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
[ ... ]
> @@ -388,6 +407,10 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	master_dev->reg_encbase[i] = dev->reg_base;
>  	dev->master_dev = master_dev;
>  
> +	ret = mtk_jpegenc_smmu_init(dev);
> +	if (ret)
> +		return ret;
> +

[Severity: High]
Similar to the decoder probe, will returning early here on SMMU init
failure cause a use-after-free by leaving the freed dev pointer in
master_dev->enc_hw_dev[i]?

Also, are there any checks to ensure the index 'i' doesn't exceed the bounds
of the master_dev->enc_hw_dev array?

The device tree child nodes are counted and assigned to jpeg->max_hw_count
without validation. Later, in mtk_jpegenc_worker(), it appears this count is
used to populate a fixed-size stack array:

drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c:mtk_jpegenc_worker() {
...
	struct mtk_jpegenc_comp_dev *comp_jpeg[MTK_JPEGENC_HW_MAX];
...
	for (i = 0; i < jpeg->max_hw_count; i++)
		comp_jpeg[i] = jpeg->enc_hw_dev[i];
...
}

Could an excessively large number of DT child nodes cause a heap buffer
overflow during probe, and subsequently a stack buffer overflow here?

>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);

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