Re: [PATCH] drm/mediatek: Validate gamma_lut size

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] drm/mediatek: NULL pointer dereference in mtk_crtc_atomic_check()
--

commit 9fd51aa9fb3e494d751bd1b62a30a2b909f46ef6
Author: Fei Shao <fshao@chromium.org>

drm/mediatek: Validate gamma_lut size

This commit adds mtk_crtc_atomic_check() to validate that any non-NULL
gamma_lut blob matches the hardware LUT size of the CRTC's DDP components,
preventing out-of-bounds reads in mtk_gamma_set() and mtk_aal_gamma_set().

> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 2920f3198ef6a..fd9b51fd00eaa 100644
> --- a/drivers/gpu/drm/mediatek/mtk_crtc.c
> +++ b/drivers/gpu/drm/mediatek/mtk_crtc.c
> @@ -880,6 +880,26 @@ static void mtk_crtc_atomic_flush(struct drm_crtc *crtc,
>  	mtk_crtc_update_config(mtk_crtc, !!mtk_crtc->event);
>  }
>  
> +static int mtk_crtc_atomic_check(struct drm_crtc *crtc, struct drm_atomic_commit *state)
> +{
> +	struct mtk_crtc *mtk_crtc = to_mtk_crtc(crtc);
> +	struct drm_crtc_state *crtc_state = drm_atomic_get_new_crtc_state(state, crtc);
> +	struct drm_property_blob *gamma_lut = crtc_state->gamma_lut;
> +	unsigned int lut_size;
> +	int i;
> +
> +	if (!gamma_lut)
> +		return 0;
> +
> +	for (i = 0; i < mtk_crtc->ddp_comp_nr; i++) {
> +		lut_size = mtk_ddp_gamma_get_lut_size(mtk_crtc->ddp_comp[i]);
> +		if (lut_size && gamma_lut->length != lut_size * sizeof(struct drm_color_lut))
> +			return -EINVAL;
> +	}

[Severity: High]
Can mtk_crtc->ddp_comp[i] be NULL here when conn_routes is used?

In mtk_crtc_create(), when conn_routes is non-NULL, mtk_crtc->ddp_comp is
allocated with path_len + 1 entries and mtk_crtc->ddp_comp_nr is incremented
at the end:

drivers/gpu/drm/mediatek/mtk_crtc.c:mtk_crtc_create() {
	...
	if (conn_routes) {
		...
		mtk_crtc->num_conn_routes = num_conn_routes;
		mtk_crtc->conn_routes = conn_routes;

		/* increase ddp_comp_nr at the end of mtk_crtc_create */
		mtk_crtc->ddp_comp_nr++;
	}
}

The last slot in mtk_crtc->ddp_comp[mtk_crtc->ddp_comp_nr - 1] remains NULL
until mtk_crtc_update_output() populates it during mtk_crtc_atomic_enable().

Because mtk_crtc_atomic_check() runs during the atomic check phase before
mtk_crtc_atomic_enable() (for example on the initial modeset, a
DRM_MODE_ATOMIC_TEST_ONLY commit, or when setting GAMMA_LUT while the CRTC is
disabled), passing a valid gamma_lut blob will reach the final iteration of
the loop and pass NULL to mtk_ddp_gamma_get_lut_size():

drivers/gpu/drm/mediatek/mtk_ddp_comp.h:mtk_ddp_gamma_get_lut_size() {
	if (comp->funcs && comp->funcs->gamma_get_lut_size)
		return comp->funcs->gamma_get_lut_size(comp->dev);

	return 0;
}

Unlike mtk_ddp_comp_mode_valid(), which checks if comp is non-NULL before
dereferencing comp->funcs, mtk_ddp_gamma_get_lut_size() dereferences comp
unconditionally. Should mtk_ddp_gamma_get_lut_size() or
mtk_crtc_atomic_check() guard against a NULL comp pointer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261105091054.3993611-1-fshao@chromium.org?part=1