Re: [PATCH v18 01/13] media: mediatek: jpeg: fix jpeg cores' amounts setting

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] media: mtk-jpeg: Parent probe resets hw_rdy and hw_index after child probes
- [High] media: mtk-jpeg: Stack buffer overflow from unbounded max_hw_count
- [High] media: mtk-jpeg: NULL pointer dereference on deferred or failed child probes
--

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

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

Different ICs have different amounts of cores,
use a variable to set the cores' amounts.

> diff --git a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> index d147ec4830811..41c4cf8dc2626 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_core.c
> @@ -1325,7 +1325,8 @@ static int mtk_jpeg_probe(struct platform_device *pdev)
>  		for_each_child_of_node(pdev->dev.of_node, child)
>  			num_child++;
>  
> -		atomic_set(&jpeg->hw_rdy, num_child);
> +		jpeg->max_hw_count = num_child;

[Severity: High]
Does this code overflow the comp_jpeg[] stack arrays in mtk_jpegenc_worker()
and mtk_jpegdec_worker()?

The child node count from the device tree is assigned to max_hw_count without
verifying it fits within the driver's hardcoded limits (MTK_JPEGENC_HW_MAX or
MTK_JPEGDEC_HW_MAX).

If a malformed device tree provides more nodes than these limits, the worker
functions will perform out-of-bounds writes on their fixed-size stack arrays.

> +		atomic_set(&jpeg->hw_rdy, 0);
>  		atomic_set(&jpeg->hw_index, 0);

[Severity: High]
Is it safe to reset these atomic variables here?

Earlier in mtk_jpeg_probe(), devm_of_platform_populate() synchronously probes
available child devices. When those child probes succeed, they increment hw_rdy
and hw_index, as seen in the changes to mtk_jpegdec_hw_probe() and
mtk_jpegenc_hw_probe() below.

By unconditionally resetting hw_rdy and hw_index to 0 here, the state
established by the successfully probed children is wiped out. This leaves hw_rdy
at 0 despite having ready cores.

When workers subsequently try to consume hardware, they will decrement hw_rdy to
a negative value, causing other workers calling wait_event_interruptible() to
block indefinitely, leading to a deadlock.

Also, deferred child probes will mistakenly receive index 0, overwriting pointers
of previously probed children.

[ ... ]

> @@ -1462,7 +1463,7 @@ static int mtk_jpegenc_get_hw(struct mtk_jpeg_ctx *ctx)
>  	int i;
>  
>  	spin_lock_irqsave(&jpeg->hw_lock, flags);
> -	for (i = 0; i < MTK_JPEGENC_HW_MAX; i++) {
> +	for (i = 0; i < jpeg->max_hw_count; i++) {
>  		comp_jpeg = jpeg->enc_hw_dev[i];
>  		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {

[Severity: High]
Can this dereference a NULL pointer if a child node fails to probe or is
disabled?

max_hw_count is the total number of child nodes in the device tree, but
enc_hw_dev[] is only populated when a child driver successfully probes. If a
child node has status = "disabled" or fails to probe, its corresponding slot in
enc_hw_dev[] remains NULL.

Iterating up to max_hw_count and unconditionally dereferencing comp_jpeg will
cause a NULL pointer dereference in this scenario.

>  			hw_id = i;

[ ... ]

> @@ -1509,7 +1510,7 @@ static int mtk_jpegdec_get_hw(struct mtk_jpeg_ctx *ctx)
>  	int i;
>  
>  	spin_lock_irqsave(&jpeg->hw_lock, flags);
> -	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++) {
> +	for (i = 0; i < jpeg->max_hw_count; i++) {
>  		comp_jpeg = jpeg->dec_hw_dev[i];
>  		if (comp_jpeg->hw_state == MTK_JPEG_HW_IDLE) {

[Severity: High]
Similarly, can this trigger a NULL pointer dereference if a decoder child node
fails to probe or is disabled?

>  			hw_id = i;

[ ... ]

> @@ -1592,7 +1593,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];

[Severity: High]
As mentioned earlier, does this overflow the fixed-size comp_jpeg stack array
if max_hw_count exceeds MTK_JPEGENC_HW_MAX?

Additionally, this lockless copy will cache NULL pointers if there were failed
child probes, leading to a crash later in the worker when
pm_runtime_get_sync(comp_jpeg[hw_id]->dev) is called.

>  	i = 0;

[ ... ]

> @@ -1687,7 +1688,7 @@ static void mtk_jpegdec_worker(struct work_struct *work)
>  	struct mtk_jpeg_fb fb;
>  	unsigned long flags;
>  
> -	for (i = 0; i < MTK_JPEGDEC_HW_MAX; i++)
> +	for (i = 0; i < jpeg->max_hw_count; i++)
>  		comp_jpeg[i] = jpeg->dec_hw_dev[i];

[Severity: High]
Similarly, does this overflow the comp_jpeg stack array if max_hw_count exceeds
MTK_JPEGDEC_HW_MAX, and can it cache NULL pointers?

>  	i = 0;

[ ... ]

> 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 32372781daf57..9ca68cde45a48 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_dec_hw.c
> @@ -668,6 +668,8 @@ static int mtk_jpegdec_hw_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	atomic_inc(&master_dev->hw_rdy);
> +
>  	return 0;
>  }
>  
> 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 b312a15d707b2..44a064dc7b3d0 100644
> --- a/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> +++ b/drivers/media/platform/mediatek/jpeg/mtk_jpeg_enc_hw.c
> @@ -389,6 +389,8 @@ static int mtk_jpegenc_hw_probe(struct platform_device *pdev)
>  	platform_set_drvdata(pdev, dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> +	atomic_inc(&master_dev->hw_rdy);
> +
>  	return 0;
>  }

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