Re: [PATCH 04/11] drm/msm: let the kms drivers clean up a failed kms_init()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/msm: NULL pointer dereference in snapshot worker due to inverted teardown order
--

commit 5b99326f79a79593a3cb18892305a7e107cff364
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm: let the kms drivers clean up a failed kms_init()

A failed priv->kms_init() or ->hw_init() is cleaned up by the ->destroy()
callback, which makes ->destroy() responsible for tearing down a KMS in
any state between "not initialised at all" and "fully initialised". The
kernel convention is the opposite: a function which fails undoes its own
steps, and the teardown only ever sees a fully set up object.

> diff --git a/drivers/gpu/drm/msm/msm_kms.c b/drivers/gpu/drm/msm/msm_kms.c
> index f3e39c3907a93..f65774b04c6fa 100644
> --- a/drivers/gpu/drm/msm/msm_kms.c
> +++ b/drivers/gpu/drm/msm/msm_kms.c
[ ... ]
> @@ -344,9 +348,10 @@ int msm_drm_kms_init(struct device *dev, const struct drm_driver *drv)
>  err_destroy_event_threads:
>  	msm_drm_kms_destroy_event_threads(kms);
>  err_destroy_kms:
> -	msm_disp_snapshot_destroy(ddev);
>  	if (kms->funcs)
>  		kms->funcs->destroy(kms);
> +err_destroy_snapshot:
> +	msm_disp_snapshot_destroy(ddev);
>  
>  	return ret;
>  }

[Severity: High]
Does this inverted teardown order cause a NULL pointer dereference in the
snapshot worker?

If an error occurs after kms_init() succeeds in msm_drm_kms_init() (for
example, if drm_vblank_init() fails), the error path jumps to
err_destroy_event_threads. If a concurrent IOMMU fault or underrun triggers
msm_disp_snapshot_state() at this time, it queues the snapshot work.

With this patch, the teardown order is swapped so that
kms->funcs->destroy(kms) is called before msm_disp_snapshot_destroy(ddev).
The destroy() callback (such as dpu_kms_destroy() calling
_dpu_kms_hw_destroy()) tears down the hardware and sets pointers like
dpu_kms->catalog = NULL.

When msm_disp_snapshot_destroy() is subsequently called, it invokes
kthread_destroy_worker(), which flushes the worker and executes the pending
snapshot work. The snapshot work calls kms->funcs->snapshot(), which then
dereferences the nulled hardware state, resulting in a kernel oops in
dpu_kms_mdp_snapshot() when reading cat->ctl_count.

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