Re: [PATCH v15 01/12] media: mediatek: jpeg: fix jpeg cores' amounts setting

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] Unbounded max_hw_count growth on probe failure causes stack and heap buffer overflows.
- [High] Uninitialized stack memory access in worker threads upon retry.
- [High] Mismatched tracking of available hardware causes an infinite busy-loop and job failure.
- [High] Data race and missing memory barriers between child probe and worker threads cause NULL pointer dereferences.

Pre-existing issues:
- [High] Use-After-Free during device removal due to asymmetric devres workqueue teardown.
--

commit 0ee6ba9604510a9c4d1afe2cbb648113729e18a6
Author: Kyrie Wu <kyrie.wu@mediatek.com>

media: mediatek: jpeg: fix jpeg cores' amounts setting

This commit updates the driver to track the number of available hardware
cores dynamically using a max_hw_count variable instead of relying on a
hardcoded macro. It modifies the probe routines to increment this counter
and bounds the iterations in the worker threads based on the new variable.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index d147ec4830811..1e014fd698ae0 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c

[ ... ]

> @@ -1592,7 +1592,7 @@ static void mtk_jpegenc_worker(struct work_struct *work)
>  		jpeg_work);
>  	struct mtk_jpeg_dev *jpeg = ctx->jpeg;
>  
> -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++)
> +	for (i = 0; i < jpeg->max_hw_count; i++)
>  		comp_jpeg[i] = jpeg->enc_hw_dev[i];
>  	i = 0;

[Severity: Critical]
Since max_hw_count can increment on probe retries (as noted below in the
probe routines), could this unbounded count exceed the fixed size of the
comp_jpeg stack array and cause a buffer overflow?

[Severity: High]
Since max_hw_count and the enc_hw_dev pointers are updated during child
probes without locks or memory barriers, could this worker thread observe
an incremented max_hw_count but read a NULL or uninitialized enc_hw_dev
pointer if the operations are reordered?

[Severity: High]
If a deferred child probe completes while waiting, and a subsequent call to
mtk_jpegenc_get_hw() returns a newly probed hw_id, does the retry loop use
an uninitialized pointer from the comp_jpeg array?

mtk_jpegenc_worker() {
    ...
retry_select:
    hw_id = mtk_jpegenc_get_hw(ctx);
    if (hw_id < 0) {
        ret = wait_event_interruptible(...);
        ...
        goto retry_select;
    }
    ...
    ret = pm_runtime_get_sync(comp_jpeg[hw_id]->dev);
}

Because the comp_jpeg array initialization loop happens before retry_select,
it is not re-run to pick up the newly assigned pointers.

[Severity: High]
Does the mismatch between hw_rdy and max_hw_count cause an infinite busy
loop when active cores are busy?

The wait condition relies on hw_rdy, which is initialized to the total
number of child nodes during the master probe:

    ret = wait_event_interruptible(jpeg->hw_wq,
                                   atomic_read(&jpeg->hw_rdy) > 0);

If a child node defers probing, max_hw_count will be less than hw_rdy.
When active cores are busy, this overcount could cause the worker to wake
up immediately and retry until it exhausts the retry limit and fails.

[Severity: High]
This is a pre-existing issue, but does device removal have a use-after-free
regression risk with the worker threads? 

During unbind, mtk_jpeg_remove() synchronously releases the m2m device:

mtk_jpeg_remove()
    ...
    v4l2_m2m_release(jpeg->m2m_dev);

However, the workqueue teardown is registered via devres and executes after
remove completes. If a worker thread is running during unbind, could it
call v4l2_m2m_job_finish() and access the already-freed m2m_dev context?

> 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 b6f5b2249f1f2..2765dafab4ad4 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -386,6 +386,7 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	master_dev->enc_hw_dev[i] = dev;
>  	master_dev->reg_encbase[i] = dev->reg_base;
>  	dev->master_dev = master_dev;
> +	master_dev->max_hw_count++;
>  
>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);

[Severity: Critical]
If a failure occurs later in the probe routine, such as in
mtk_jpegenc_smmu_init(), are the master_dev->max_hw_count increment and
pointer assignments rolled back? 

Without a rollback, a subsequent probe retry would increment max_hw_count
again and leak the previous hw_index, potentially overflowing counters and
corrupting adjacent memory structures.

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