Re: [PATCH v3 5/8] drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Stale pointers in dpu_enc->hw_cwb array causing uninitialized stack memory dereference
- [High] Stale pointer in dpu_enc->cur_master->hw_cdm causing hardware state corruption
--

commit 50e668cb47f95334a8bcced939d0916fe0531def
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dpu: clear the DSPP pointer when no DSPP is assigned

dpu_crtc_assign_resources() only stores mixers[i].hw_dspp when the new
topology has a DSPP for that mixer, and dpu_crtc_duplicate_state() copies
the state with kmemdup(), so a crtc which once used a DSPP keeps a pointer
to it for the rest of its life -- across reservations which no longer own
that block.

Assign NULL when there is no DSPP for the mixer.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> index bf593020e8e45..5dc0c9aeaa5d7 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_crtc.c
> @@ -1480,8 +1480,8 @@ static int dpu_crtc_assign_resources(struct drm_crtc *crtc,
>  
>  		cstate->mixers[i].hw_lm = to_dpu_hw_mixer(hw_lm[i]);
>  		cstate->mixers[i].lm_ctl = to_dpu_hw_ctl(hw_ctl[ctl_idx]);
> -		if (i < num_dspp)
> -			cstate->mixers[i].hw_dspp = to_dpu_hw_dspp(hw_dspp[i]);
> +		cstate->mixers[i].hw_dspp = i < num_dspp ?
> +			to_dpu_hw_dspp(hw_dspp[i]) : NULL;

[Severity: High]
This is a pre-existing issue, but does this same stale pointer problem exist
for hw_cwb in dpu_encoder_virt_atomic_mode_set() when the number of CWB
blocks decreases during a modeset?

If userspace submits a DRM commit that assigns fewer CWB blocks than the
previous state, trailing elements in dpu_enc->hw_cwb are not cleared:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_mode_set() {
    ...
	for (i = 0; i < num_cwb; i++) {
		dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
		cwb_mask |= BIT(dpu_enc->hw_cwb[i]->idx - CWB_0);
	}
    ...
}

Later, when dpu_encoder_helper_phys_setup_cwb() is called, it iterates over
MAX_CWB_PER_ENC and processes any non-NULL pointer. Could this cause the
driver to read from rt_pp_list[i] (an uninitialized stack array beyond
num_pp), cast the garbage value to a pointer, and dereference it via
hw_pp->idx, leading to a kernel crash?

[Severity: High]
This isn't a bug introduced by this patch, but there appears to be another
stale pointer issue in dpu_encoder_virt_atomic_mode_set() with hw_cdm:

drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c:dpu_encoder_virt_atomic_mode_set() {
    ...
	if ((dpu_enc->disp_info.intf_type == INTF_WB && conn_state->writeback_job) ||
	    dpu_enc->disp_info.intf_type == INTF_DP) {
		struct dpu_hw_blk *hw_cdm = NULL;

		dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
					      drm_enc->crtc, DPU_HW_BLK_CDM,
					      &hw_cdm, 1);
		dpu_enc->cur_master->hw_cdm = hw_cdm ? to_dpu_hw_cdm(hw_cdm) : NULL;
	}
    ...
}

If a modeset occurs where the encoder had a CDM block (e.g., from an active
writeback job), but the new state lacks a writeback job, the if-condition
evaluates to false and the pointer remains uncleared.

During dpu_encoder_helper_phys_cleanup(), the driver attempts to unbind the
stale CDM block by calling bind_pingpong_blk(..., PINGPONG_NONE) and
configures a flush. Can this corrupt the hardware state of a block that no
longer belongs to this reservation and may now be assigned to another display?

>  	}
>  
>  	cstate->num_mixers = num_lm;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260912-fd-kms-fix-smmu-v3-0-a7ddc6fe2032@oss.qualcomm.com?part=5