linux-kernel - Re: [PATCH] drm/display: bridge_connector: Do atomic check when necessary

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dm4lqvtd4lxqmxruji5eydmbqxoomj6fogiu4uucqfevifqpll@33eeykwlqczx>
Date: Wed, 8 Jan 2025 12:28:39 +0200
From: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To: Liu Ying <victor.liu@....com>
Cc: dri-devel@...ts.freedesktop.org, linux-kernel@...r.kernel.org, 
	andrzej.hajda@...el.com, neil.armstrong@...aro.org, rfoss@...nel.org, 
	Laurent.pinchart@...asonboard.com, jonas@...boo.se, jernej.skrabec@...il.com, 
	maarten.lankhorst@...ux.intel.com, mripard@...nel.org, tzimmermann@...e.de, airlied@...il.com, 
	simona@...ll.ch
Subject: Re: [PATCH] drm/display: bridge_connector: Do atomic check when
 necessary

On Wed, Jan 08, 2025 at 06:13:51PM +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_bridge_connector_atomic_check() should return 0 directly.
> 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().
> 

[..]

> [   46.823581] pc : drm_default_rgb_quant_range+0x0/0x4c [drm]
> [   46.829244] lr : drm_atomic_helper_connector_hdmi_check+0xb0/0x6b0 [drm_display_helper]
> [   46.837293] sp : ffff80008364ba00

[..]

> [   46.911984] Call trace:
> [   46.914429]  drm_default_rgb_quant_range+0x0/0x4c [drm] (P)
> [   46.920106]  drm_bridge_connector_atomic_check+0x20/0x2c [drm_display_helper]
> [   46.927275]  drm_atomic_helper_check_modeset+0x488/0xc78 [drm_kms_helper]
> [   46.934111]  drm_atomic_helper_check+0x20/0xa4 [drm_kms_helper]
> [   46.940063]  drm_atomic_check_only+0x4b8/0x984 [drm]
> [   46.945136]  drm_atomic_commit+0x48/0xc4 [drm]
> [   46.949674]  drm_framebuffer_remove+0x44c/0x530 [drm]
> [   46.954809]  drm_mode_rmfb_work_fn+0x7c/0xa0 [drm]
> [   46.959687]  process_one_work+0x150/0x294
> [   46.963700]  worker_thread+0x2dc/0x3dc
> [   46.967450]  kthread+0x130/0x204
> [   46.970679]  ret_from_fork+0x10/0x20
> [   46.974258] Code: d50323bf d65f03c0 52800041 17ffffe6 (b9400001)
> [   46.980350] ---[ end trace 0000000000000000 ]---

Please trim the backtrace in a way I trimmed it. Also you can drop the
timestamps, they don't have useful information.

> 
> Fixes: 8ec116ff21a9 ("drm/display: bridge_connector: provide atomic_check for HDMI bridges")
> Signed-off-by: Liu Ying <victor.liu@....com>
> ---
>  drivers/gpu/drm/display/drm_bridge_connector.c | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 32108307de66..587020a3f3e8 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -343,6 +343,11 @@ static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
>  {
>  	struct drm_bridge_connector *bridge_connector =
>  		to_drm_bridge_connector(connector);
> +	struct drm_connector_state *new_conn_state =
> +		drm_atomic_get_new_connector_state(state, connector);
> +
> +	if (!new_conn_state->crtc || !new_conn_state->best_encoder)
> +		return 0;

Unfortunately, this is not enough. Other drivers (e.g. sun4i) use
drm_atomic_helper_connector_hdmi_check() at connector's atomic_check()
function. Please move necessary checks to the helper itself. Also please
add corresponding KUnit test: attempt to atomic_check() the unconnected
HDMI connector, there is a test suite for the HDMI connector functions,
but the atomic_check() for the disconnected connecor seems to be missing.

>  
>  	if (bridge_connector->bridge_hdmi)
>  		return drm_atomic_helper_connector_hdmi_check(connector, state);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry