[PATCH] drm/mediatek: Validate gamma_lut size

[PATCH] drm/mediatek: Validate gamma_lut size

[Fei Shao]

The DRM core only checks that a GAMMA_LUT blob size is a multiple of
sizeof(struct drm_color_lut), not that its entry count matches the
hardware LUT size. mtk_gamma_set() and mtk_aal_gamma_set() then read
the blob up to the hardware LUT size, which is an OOB read when the blob
is shorter.

Add mtk_crtc_atomic_check() and reject gamma_lut with invalid length.

Fixes: 2f3f4dda747c ("drm/mediatek: Add gamma correction.")
Signed-off-by: Fei Shao <fshao@chromium.org>
---

 drivers/gpu/drm/mediatek/mtk_crtc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
index 2920f3198ef6..fd9b51fd00ea 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;
+	}
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs mtk_crtc_funcs = {
 	.set_config		= drm_atomic_helper_set_config,
 	.page_flip		= drm_atomic_helper_page_flip,
@@ -899,6 +919,7 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
 	.atomic_flush	= mtk_crtc_atomic_flush,
 	.atomic_enable	= mtk_crtc_atomic_enable,
 	.atomic_disable	= mtk_crtc_atomic_disable,
+	.atomic_check	= mtk_crtc_atomic_check,
 };
 
 static int mtk_crtc_init(struct drm_device *drm, struct mtk_crtc *mtk_crtc,
-- 
2.56.0.455.g6c5a503b38-goog

[PATCH] drm/mediatek: Validate gamma_lut size

[Fei Shao]

The DRM core only checks that a GAMMA_LUT blob size is a multiple of
sizeof(struct drm_color_lut), not that its entry count matches the
hardware LUT size. mtk_gamma_set() and mtk_aal_gamma_set() then read
the blob up to the hardware LUT size, which is an OOB read when the blob
is shorter.

Add mtk_crtc_atomic_check() and reject gamma_lut with invalid length.

Fixes: 2f3f4dda747c ("drm/mediatek: Add gamma correction.")
Signed-off-by: Fei Shao <fshao@chromium.org>
---

 drivers/gpu/drm/mediatek/mtk_crtc.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
index 2920f3198ef6..fd9b51fd00ea 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;
+	}
+
+	return 0;
+}
+
 static const struct drm_crtc_funcs mtk_crtc_funcs = {
 	.set_config		= drm_atomic_helper_set_config,
 	.page_flip		= drm_atomic_helper_page_flip,
@@ -899,6 +919,7 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
 	.atomic_flush	= mtk_crtc_atomic_flush,
 	.atomic_enable	= mtk_crtc_atomic_enable,
 	.atomic_disable	= mtk_crtc_atomic_disable,
+	.atomic_check	= mtk_crtc_atomic_check,
 };
 
 static int mtk_crtc_init(struct drm_device *drm, struct mtk_crtc *mtk_crtc,
-- 
2.56.0.455.g6c5a503b38-goog

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

[sashiko-bot]

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

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

[AngeloGioacchino Del Regno]

On 11/5/26 10:10, Fei Shao wrote:
> The DRM core only checks that a GAMMA_LUT blob size is a multiple of
> sizeof(struct drm_color_lut), not that its entry count matches the
> hardware LUT size. mtk_gamma_set() and mtk_aal_gamma_set() then read
> the blob up to the hardware LUT size, which is an OOB read when the blob
> is shorter.
> 
> Add mtk_crtc_atomic_check() and reject gamma_lut with invalid length.
> 
> Fixes: 2f3f4dda747c ("drm/mediatek: Add gamma correction.")
> Signed-off-by: Fei Shao <fshao@chromium.org>
> ---
> 
>   drivers/gpu/drm/mediatek/mtk_crtc.c | 21 +++++++++++++++++++++
>   1 file changed, 21 insertions(+)
> 
> diff --git a/drivers/gpu/drm/mediatek/mtk_crtc.c b/drivers/gpu/drm/mediatek/mtk_crtc.c
> index 2920f3198ef6..fd9b51fd00ea 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++) {

Yet another iterator in an array of components, slowing down this driver...

I know it's not merged yet, but please, do it on top of my restructuring for
mediatek-drm...

Cheers,
Angelo

> +		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;
> +	}
> +
> +	return 0;
> +}
> +
>   static const struct drm_crtc_funcs mtk_crtc_funcs = {
>   	.set_config		= drm_atomic_helper_set_config,
>   	.page_flip		= drm_atomic_helper_page_flip,
> @@ -899,6 +919,7 @@ static const struct drm_crtc_helper_funcs mtk_crtc_helper_funcs = {
>   	.atomic_flush	= mtk_crtc_atomic_flush,
>   	.atomic_enable	= mtk_crtc_atomic_enable,
>   	.atomic_disable	= mtk_crtc_atomic_disable,
> +	.atomic_check	= mtk_crtc_atomic_check,
>   };
>   
>   static int mtk_crtc_init(struct drm_device *drm, struct mtk_crtc *mtk_crtc,