[PATCH v2 0/2] drm: display: Fix atomic check for HDMI connector disablement

[PATCH v2 0/2] drm: display: Fix atomic check for HDMI connector disablement

[Liu Ying]

Hi,

This patch series fixes a potential NULL pointer dereference when using
drm_atomic_helper_connector_hdmi_check() to check a DRM atomic commit
which tries to disable a HDMI connector.

Patch 1 adds necessary checks to avoid the potential NULL pointer dereference.
Patch 2 adds a KUnit test case to make sure that atomic check succeeds when
disabling a HDMI connector.

Patch 1 is tested with i.MX8MP imx-lcdif, but not tested with sun4i and
rockchip due to no HW access.

v2:
* Trim backtrace in patch 1's commit message. (Dmitry)
* Drop timestamps from backtrace in patch 1's commit message. (Dmitry)
* Move the necessary checks from drm_bridge_connector_atomic_check() to
  drm_atomic_helper_connector_hdmi_check(). (Dmitry)
* Add the KUnit test case in patch 2. (Dmitry)

Liu Ying (2):
  drm/connector: hdmi: Do atomic check when necessary
  drm/tests: hdmi: Add connector disablement test

 .../gpu/drm/display/drm_hdmi_state_helper.c   |  3 ++
 .../drm/tests/drm_hdmi_state_helper_test.c    | 52 +++++++++++++++++++
 2 files changed, 55 insertions(+)

-- 
2.34.1

[PATCH v2 1/2] drm/connector: hdmi: Do atomic check when necessary

[Liu Ying]

It's ok to pass atomic check successfully if an atomic commit tries to
disable the display pipeline which the connector belongs to. That is,
when the crtc or the best_encoder pointers in struct drm_connector_state
are NULL, drm_atomic_helper_connector_hdmi_check() should return 0.
Without the check against the NULL pointers, drm_default_rgb_quant_range()
called by drm_atomic_helper_connector_hdmi_check() would dereference
the NULL pointer to_match in drm_match_cea_mode().

Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
Call trace:
 drm_default_rgb_quant_range+0x0/0x4c (P)
 drm_bridge_connector_atomic_check+0x20/0x2c
 drm_atomic_helper_check_modeset+0x488/0xc78
 drm_atomic_helper_check+0x20/0xa4
 drm_atomic_check_only+0x4b8/0x984
 drm_atomic_commit+0x48/0xc4
 drm_framebuffer_remove+0x44c/0x530
 drm_mode_rmfb_work_fn+0x7c/0xa0
 process_one_work+0x150/0x294
 worker_thread+0x2dc/0x3dc
 kthread+0x130/0x204
 ret_from_fork+0x10/0x20

Fixes: 8ec116ff21a9 ("drm/display: bridge_connector: provide atomic_check for HDMI bridges")
Fixes: 84e541b1e58e ("drm/sun4i: use drm_atomic_helper_connector_hdmi_check()")
Fixes: 65548c8ff0ab ("drm/rockchip: inno_hdmi: Switch to HDMI connector")
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
Tested with i.MX8MP imx-lcdif.
sun4i and rockchip are not tested due to no HW access.

v2:
* Trim backtrace in commit message. (Dmitry)
* Drop timestamps from backtrace commit message. (Dmitry)
* Move the necessary checks from drm_bridge_connector_atomic_check() to
  drm_atomic_helper_connector_hdmi_check(). (Dmitry)

 drivers/gpu/drm/display/drm_hdmi_state_helper.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/display/drm_hdmi_state_helper.c b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
index cfc2aaee1da0..daaf68b80e5f 100644
--- a/drivers/gpu/drm/display/drm_hdmi_state_helper.c
+++ b/drivers/gpu/drm/display/drm_hdmi_state_helper.c
@@ -503,6 +503,9 @@ int drm_atomic_helper_connector_hdmi_check(struct drm_connector *connector,
 		connector_state_get_mode(new_conn_state);
 	int ret;
 
+	if (!new_conn_state->crtc || !new_conn_state->best_encoder)
+		return 0;
+
 	new_conn_state->hdmi.is_limited_range = hdmi_is_limited_range(connector, new_conn_state);
 
 	ret = hdmi_compute_config(connector, new_conn_state, mode);
-- 
2.34.1

Re: [PATCH v2 1/2] drm/connector: hdmi: Do atomic check when necessary

[Dmitry Baryshkov]

On Fri, Jan 10, 2025 at 04:48:20PM +0800, Liu Ying wrote:
> It's ok to pass atomic check successfully if an atomic commit tries to
> disable the display pipeline which the connector belongs to. That is,
> when the crtc or the best_encoder pointers in struct drm_connector_state
> are NULL, drm_atomic_helper_connector_hdmi_check() should return 0.
> Without the check against the NULL pointers, drm_default_rgb_quant_range()
> called by drm_atomic_helper_connector_hdmi_check() would dereference
> the NULL pointer to_match in drm_match_cea_mode().
> 
> Unable to handle kernel NULL pointer dereference at virtual address 0000000000000000
> Call trace:
>  drm_default_rgb_quant_range+0x0/0x4c (P)
>  drm_bridge_connector_atomic_check+0x20/0x2c
>  drm_atomic_helper_check_modeset+0x488/0xc78
>  drm_atomic_helper_check+0x20/0xa4
>  drm_atomic_check_only+0x4b8/0x984
>  drm_atomic_commit+0x48/0xc4
>  drm_framebuffer_remove+0x44c/0x530
>  drm_mode_rmfb_work_fn+0x7c/0xa0
>  process_one_work+0x150/0x294
>  worker_thread+0x2dc/0x3dc
>  kthread+0x130/0x204
>  ret_from_fork+0x10/0x20
> 
> Fixes: 8ec116ff21a9 ("drm/display: bridge_connector: provide atomic_check for HDMI bridges")
> Fixes: 84e541b1e58e ("drm/sun4i: use drm_atomic_helper_connector_hdmi_check()")
> Fixes: 65548c8ff0ab ("drm/rockchip: inno_hdmi: Switch to HDMI connector")
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> Tested with i.MX8MP imx-lcdif.
> sun4i and rockchip are not tested due to no HW access.
> 
> v2:
> * Trim backtrace in commit message. (Dmitry)
> * Drop timestamps from backtrace commit message. (Dmitry)
> * Move the necessary checks from drm_bridge_connector_atomic_check() to
>   drm_atomic_helper_connector_hdmi_check(). (Dmitry)
> 
>  drivers/gpu/drm/display/drm_hdmi_state_helper.c | 3 +++
>  1 file changed, 3 insertions(+)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

[PATCH v2 2/2] drm/tests: hdmi: Add connector disablement test

[Liu Ying]

Atomic check should succeed when disabling a connector. Add a test
case drm_test_check_disabling_connector() to make sure of this.

Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Liu Ying <victor.liu@nxp.com>
---
v2:
* New patch to add the test case. (Dmitry)

 .../drm/tests/drm_hdmi_state_helper_test.c    | 52 +++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
index c3b693bb966f..8f7a39c9a1bb 100644
--- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
@@ -1568,6 +1568,57 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes
 	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
 }
 
+/* Test that atomic check succeeds when disabling a connector. */
+static void drm_test_check_disabling_connector(struct kunit *test)
+{
+	struct drm_atomic_helper_connector_hdmi_priv *priv;
+	struct drm_modeset_acquire_ctx *ctx;
+	struct drm_connector_state *conn_state;
+	struct drm_crtc_state *crtc_state;
+	struct drm_atomic_state *state;
+	struct drm_display_mode *preferred;
+	struct drm_connector *conn;
+	struct drm_device *drm;
+	struct drm_crtc *crtc;
+	int ret;
+
+	priv = drm_kunit_helper_connector_hdmi_init(test,
+						    BIT(HDMI_COLORSPACE_RGB),
+						    8);
+	KUNIT_ASSERT_NOT_NULL(test, priv);
+
+	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
+
+	conn = &priv->connector;
+	preferred = find_preferred_mode(conn);
+	KUNIT_ASSERT_NOT_NULL(test, preferred);
+
+	drm = &priv->drm;
+	crtc = priv->crtc;
+	ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+
+	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
+
+	crtc_state = drm_atomic_get_crtc_state(state, crtc);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
+
+	crtc_state->active = false;
+	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	conn_state = drm_atomic_get_connector_state(state, conn);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
+
+	ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
+	KUNIT_EXPECT_EQ(test, ret, 0);
+
+	ret = drm_atomic_check_only(state);
+	KUNIT_ASSERT_EQ(test, ret, 0);
+}
+
 static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
 	KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode),
 	KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode_vic_1),
@@ -1605,6 +1656,7 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
 	 * picked up aside from changing the BPC or mode which would
 	 * already trigger a mode change.
 	 */
+	KUNIT_CASE(drm_test_check_disabling_connector),
 	{ }
 };
 
-- 
2.34.1

Re: [PATCH v2 2/2] drm/tests: hdmi: Add connector disablement test

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3359 bytes --]

On Fri, Jan 10, 2025 at 04:48:21PM +0800, Liu Ying wrote:
> Atomic check should succeed when disabling a connector. Add a test
> case drm_test_check_disabling_connector() to make sure of this.
> 
> Suggested-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
> Signed-off-by: Liu Ying <victor.liu@nxp.com>
> ---
> v2:
> * New patch to add the test case. (Dmitry)
> 
>  .../drm/tests/drm_hdmi_state_helper_test.c    | 52 +++++++++++++++++++
>  1 file changed, 52 insertions(+)
> 
> diff --git a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> index c3b693bb966f..8f7a39c9a1bb 100644
> --- a/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> +++ b/drivers/gpu/drm/tests/drm_hdmi_state_helper_test.c
> @@ -1568,6 +1568,57 @@ static void drm_test_check_output_bpc_format_display_8bpc_only(struct kunit *tes
>  	KUNIT_EXPECT_EQ(test, conn_state->hdmi.output_format, HDMI_COLORSPACE_RGB);
>  }
>  
> +/* Test that atomic check succeeds when disabling a connector. */
> +static void drm_test_check_disabling_connector(struct kunit *test)
> +{
> +	struct drm_atomic_helper_connector_hdmi_priv *priv;
> +	struct drm_modeset_acquire_ctx *ctx;
> +	struct drm_connector_state *conn_state;
> +	struct drm_crtc_state *crtc_state;
> +	struct drm_atomic_state *state;
> +	struct drm_display_mode *preferred;
> +	struct drm_connector *conn;
> +	struct drm_device *drm;
> +	struct drm_crtc *crtc;
> +	int ret;
> +
> +	priv = drm_kunit_helper_connector_hdmi_init(test,
> +						    BIT(HDMI_COLORSPACE_RGB),
> +						    8);
> +	KUNIT_ASSERT_NOT_NULL(test, priv);
> +
> +	ctx = drm_kunit_helper_acquire_ctx_alloc(test);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, ctx);
> +
> +	conn = &priv->connector;
> +	preferred = find_preferred_mode(conn);
> +	KUNIT_ASSERT_NOT_NULL(test, preferred);
> +
> +	drm = &priv->drm;
> +	crtc = priv->crtc;
> +	ret = light_up_connector(test, drm, crtc, conn, preferred, ctx);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +
> +	state = drm_kunit_helper_atomic_state_alloc(test, drm, ctx);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, state);
> +
> +	crtc_state = drm_atomic_get_crtc_state(state, crtc);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, crtc_state);
> +
> +	crtc_state->active = false;
> +	ret = drm_atomic_set_mode_for_crtc(crtc_state, NULL);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +	conn_state = drm_atomic_get_connector_state(state, conn);
> +	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, conn_state);
> +
> +	ret = drm_atomic_set_crtc_for_connector(conn_state, NULL);
> +	KUNIT_EXPECT_EQ(test, ret, 0);
> +
> +	ret = drm_atomic_check_only(state);
> +	KUNIT_ASSERT_EQ(test, ret, 0);
> +}
> +
>  static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>  	KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode),
>  	KUNIT_CASE(drm_test_check_broadcast_rgb_auto_cea_mode_vic_1),
> @@ -1605,6 +1656,7 @@ static struct kunit_case drm_atomic_helper_connector_hdmi_check_tests[] = {
>  	 * picked up aside from changing the BPC or mode which would
>  	 * already trigger a mode change.
>  	 */
> +	KUNIT_CASE(drm_test_check_disabling_connector),

I've changed slightly that test name (s/disabling/disable/) to make it
consistent with the rest when applying, and ordered it alphabetically.
Thanks!

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

Re: [PATCH v2 0/2] drm: display: Fix atomic check for HDMI connector disablement

[Maxime Ripard]

On Fri, 10 Jan 2025 16:48:19 +0800, Liu Ying wrote:
> This patch series fixes a potential NULL pointer dereference when using
> drm_atomic_helper_connector_hdmi_check() to check a DRM atomic commit
> which tries to disable a HDMI connector.
> 
> Patch 1 adds necessary checks to avoid the potential NULL pointer dereference.
> Patch 2 adds a KUnit test case to make sure that atomic check succeeds when
> disabling a HDMI connector.
> 
> [...]

Applied to misc/kernel.git (drm-misc-next-fixes).

Thanks!
Maxime