Re: [PATCH RFC] drm/bridge: connector: Support the colorspace property for HDMI

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_bridge_connector_atomic_check() on disabled connector
- [Low] drm: mixed declarations and code in drm_bridge_connector_atomic_check()
--

commit bf56f41939cb53804040ce55667f5edb2cca09b7
Author: Jan Claußen <jan.claussen10@web.de>

drm/bridge: connector: Support the colorspace property for HDMI

This commit adds support for the colorspace property to HDMI bridge
connectors, allowing userspace to signal colorimetry like BT.2020.
It attaches the property during initialization and forces a modeset
when the colorspace changes.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 81f3e26f6fdf5..0639fe094b108 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
> @@ -412,6 +412,20 @@ drm_bridge_connector_mode_valid(struct drm_connector *connector,
>  static int drm_bridge_connector_atomic_check(struct drm_connector *connector,
>  					     struct drm_atomic_commit *state)
>  {
> +	struct drm_connector_state *old_state =
> +		drm_atomic_get_old_connector_state(state, connector);
> +	struct drm_connector_state *new_state =
> +		drm_atomic_get_new_connector_state(state, connector);
> +	struct drm_crtc_state *crtc_state;
> +
> +	if (old_state->colorspace != new_state->colorspace) {
> +		crtc_state = drm_atomic_get_crtc_state(state, new_state->crtc);

[Severity: High]
Can this cause a NULL pointer dereference if the connector is disabled?

Userspace can submit an atomic commit that modifies the colorspace property
on a disconnected or disabled connector. It might also concurrently disable
the connector while modifying the property. In these scenarios,
new_state->crtc will be NULL.

If new_state->crtc is NULL, drm_atomic_get_crtc_state() immediately passes
the pointer to drm_crtc_index(), which dereferences it:

drm_atomic_get_crtc_state(state, crtc) {
    int ret, index = drm_crtc_index(crtc);
    ...

Should the CRTC retrieval be guarded by an if (new_state->crtc) check to
prevent crashes during such commits?

> +		if (IS_ERR(crtc_state))
> +			return PTR_ERR(crtc_state);
> +
> +		crtc_state->mode_changed = true;
> +	}
> +
>  	struct drm_bridge_connector *bridge_connector =
>  		to_drm_bridge_connector(connector);

[Severity: Low]
This isn't a bug, but does this placement mix declarations and executable
code?

The newly inserted if block pushes the declaration of bridge_connector down,
placing it after executable code. While supported in newer C standards, this
violates the Linux kernel coding style and will trigger warnings from
checkpatch.pl.

Could this declaration be moved to the top of the block?

>  
>  	if (bridge_connector->bridge_hdmi)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261011132024.2833-1-jan.claussen10@web.de?part=1