Re: [PATCH v3 6/8] drm/msm/dpu: clear the DSC blocks left by a previous reservation

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Uninitialized stack memory dereference due to stale `hw_cwb` pointers.
- [High] Stale `hw_cdm` pointer leads to hardware state corruption.
--

commit 21c6c86adbc8ec970742702720e70e53d2f08f5c
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

drm/msm/dpu: clear the DSC blocks left by a previous reservation

This commit fixes an issue in dpu_encoder_virt_atomic_mode_set() where stale
DSC blocks from previous reservations were not cleared by writing NULL to the
remaining elements in the hw_dsc array.

> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> index 1f20695f81e35..50c49259a23e9 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder.c
> @@ -1222,9 +1222,10 @@ static void dpu_encoder_virt_atomic_mode_set(struct drm_encoder *drm_enc,
>  	num_dsc = dpu_rm_get_assigned_resources(&dpu_kms->rm, global_state,
>  						drm_enc->crtc, DPU_HW_BLK_DSC,
>  						hw_dsc, ARRAY_SIZE(hw_dsc));
> -	for (i = 0; i < num_dsc; i++) {
> -		dpu_enc->hw_dsc[i] = to_dpu_hw_dsc(hw_dsc[i]);
> -		dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
> +	for (i = 0; i < MAX_CHANNELS_PER_ENC; i++) {
> +		dpu_enc->hw_dsc[i] = i < num_dsc ? to_dpu_hw_dsc(hw_dsc[i]) : NULL;
> +		if (dpu_enc->hw_dsc[i])
> +			dsc_mask |= BIT(dpu_enc->hw_dsc[i]->idx - DSC_0);
>  	}
>  
>  	dpu_enc->dsc_mask = dsc_mask;

[Severity: High]
This is a pre-existing issue, but does the hw_cwb array suffer from the same
problem just above this block?

In dpu_encoder_virt_atomic_mode_set(), hw_cwb is populated up to num_cwb:

	for (i = 0; i < num_cwb; i++) {
		dpu_enc->hw_cwb[i] = to_dpu_hw_cwb(hw_cwb[i]);
		...
	}

Elements beyond num_cwb are not cleared to NULL. Later, in
dpu_encoder_helper_phys_setup_cwb(), it iterates over the entire
MAX_CWB_PER_ENC array:

	for (int i = 0; i < MAX_CWB_PER_ENC; i++) {
		hw_cwb = dpu_enc->hw_cwb[i];
		if (!hw_cwb)
			continue;
		...
		if (enable) {
			struct dpu_hw_pingpong *hw_pp =
					to_dpu_hw_pingpong(rt_pp_list[i]);
			cwb_cfg.pp_idx = hw_pp->idx;

Could a stale hw_cwb pointer bypass the !hw_cwb check and access
rt_pp_list[i]? If i >= num_pp, it seems this would lead to dereferencing
uninitialized stack memory.

[Severity: High]
This isn't a bug introduced by this patch, but does hw_cdm have a similar
lifecycle issue in the same function?

Just below the hw_dsc clearing, dpu_encoder_virt_atomic_mode_set() handles
hw_cdm:

	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 drops the writeback_job, this block is skipped entirely, leaving
the old pointer in dpu_enc->cur_master->hw_cdm.

Later, dpu_encoder_helper_phys_cleanup() unconditionally uses this pointer:

	if (phys_enc->hw_cdm) {
		if (phys_enc->hw_cdm->ops.bind_pingpong_blk && phys_enc->hw_pp)
			phys_enc->hw_cdm->ops.bind_pingpong_blk(phys_enc->hw_cdm,
								PINGPONG_NONE);
		...
	}

Can this stale pointer cause the cleanup path to alter a hardware block that
might now be owned by a different CRTC?

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