Re: [PATCH 09/11] drm/msm/dpu: unwind a failed dpu_kms_hw_init() step by step

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm/dpu: use-after-free on hw_vbif during bind retry
--

commit 010fe925950b01a30ca06eec05c2a4db435b69ed
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dpu: unwind a failed dpu_kms_hw_init() step by step

This commit replaces the blanket _dpu_kms_hw_destroy() call in the error
path of dpu_kms_hw_init() with step-by-step unwinding. Only the completed
steps are undone, in reverse order.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 65ba8fa697e94..2de4d881d13d9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
[ ... ]
> @@ -1263,7 +1263,7 @@ static int dpu_kms_hw_init(struct dpu_kms *dpu_kms)
>  	rc = _dpu_kms_drm_obj_init(dpu_kms);
>  	if (rc) {
>  		DPU_ERROR("modeset init failed: %d\n", rc);
> -		goto err_pm_put;
> +		goto err_mmu_destroy;
>  	}
>  
>  	dpu_vbif_init_memtypes(dpu_kms);
> @@ -1272,10 +1272,12 @@ static int dpu_kms_hw_init(struct dpu_kms *dpu_kms)
>  
>  	return 0;
>  
> +err_mmu_destroy:
> +	_dpu_kms_mmu_destroy(dpu_kms);

[Severity: High]
If initialization fails after dpu_kms->hw_vbif is allocated, such as when
_dpu_kms_drm_obj_init() fails, the error path jumps to err_mmu_destroy. Can
this regression leave a dangling pointer in dpu_kms->hw_vbif?

Since the memory for dpu_kms->hw_vbif is allocated via drmm, it will be freed
when the component bind fails and the DRM device is torn down. However,
dpu_kms itself persists because it is allocated via devm.

Could a subsequent bind retry trigger a use-after-free?

When a deferred probe or bind retry occurs, dpu_kms_hw_init() calls
pm_runtime_resume_and_get(), which triggers the runtime PM resume callback
and accesses the dangling pointer:

dpu_runtime_resume()
  dpu_vbif_init_memtypes()
    vbif = dpu_kms->hw_vbif;
    if (vbif && vbif->cap && ...) 

This would result in a use-after-free on the previously freed vbif memory.

Is it necessary to explicitly clear dpu_kms->hw_vbif in the unwind path to
prevent it from being accessed during bind retries?

>  err_pm_put:
>  	pm_runtime_put_sync(&dpu_kms->pdev->dev);
> -error:
> -	_dpu_kms_hw_destroy(dpu_kms);
> +err_global_obj_fini:
> +	dpu_kms_global_obj_fini(dpu_kms);
>  
>  	return rc;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261003-msm-kms-destroy-fixes-v1-0-e062b7dae77f@oss.qualcomm.com?part=9