Re: [PATCH RFC v2 05/24] drm/display: bridge-connector: use a dynamic connector

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm: NULL pointer dereference due to inverted drmm cleanup order
- [Medium] drm: memory leak on connector initialization error paths
- [Medium] drm: missing IS_ERR check causes invalid pointer dereference
--

commit a2cbdd2f3a09967f7b7cc240d15ab5e45033083e
Author: Luca Ceresoli <luca.ceresoli@bootlin.com>

drm/display: bridge-connector: use a dynamic connector

This commit switches the DRM bridge connector initialization from
drmm-managed functions to dynamic variants. This prepares the driver for
bridge hotplugging, allowing connectors to be removed without removing the
entire card.

> diff --git a/drivers/gpu/drm/display/drm_bridge_connector.c b/drivers/gpu/drm/display/drm_bridge_connector.c
> index 2aff745f01478..1c222e27767d4 100644
> --- a/drivers/gpu/drm/display/drm_bridge_connector.c
> +++ b/drivers/gpu/drm/display/drm_bridge_connector.c
[ ... ]
> @@ -1113,6 +1120,9 @@ drm_bridge_connector_initialize(struct drm_bridge_connector *bridge_connector,
>  
>  	drm_connector_helper_add(connector, &drm_bridge_connector_helper_funcs);
>  
> +	if (!connector->state)
> +		connector->state = drm_bridge_connector_create_state(connector);
> +

[Severity: Medium]
Does this code assign an error pointer directly to connector->state?

If drm_bridge_connector_create_state() returns ERR_PTR(-ENOMEM), this
will poison the state pointer without validation, potentially leading to an
invalid pointer dereference later.

>  	if (bridge_connector->bridge_hpd)
>  		connector->polled = DRM_CONNECTOR_POLL_HPD;
>  	else if (bridge_connector->bridge_detect)
[ ... ]
> @@ -1130,9 +1140,26 @@ drm_bridge_connector_initialize(struct drm_bridge_connector *bridge_connector,
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> +	ret = drm_connector_dynamic_register(connector);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

[Severity: Medium]
Does this code skip manual cleanup on this error path?

Since the switch to dynamic initialization, the connector is referenced and
has to be put if initialization fails (as noted in an earlier comment: "From
now on the connector is referenced and has to be put"). Returning
ERR_PTR(ret) directly skips drm_connector_put() and bridge cleanup,
leaking the initialized dynamic connector and bridge references.

This also seems to occur on earlier error paths in this function, such as
after drm_connector_hdmi_audio_init().

>  	return connector;
>  }
[ ... ]
> @@ -1155,6 +1182,7 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  {
>  	struct drm_bridge_connector *bridge_connector;
>  	struct drm_connector *connector;
> +	int ret;
>  
>  	bridge_connector = drmm_kzalloc(drm, sizeof(*bridge_connector), GFP_KERNEL);
>  	if (!bridge_connector)
> @@ -1164,6 +1192,10 @@ struct drm_connector *drm_bridge_connector_init(struct drm_device *drm,
>  	if (IS_ERR(connector))
>  		return connector;
>  
> +	ret = drmm_add_action_or_reset(drm, drmm_bridge_connector_fini, bridge_connector);
> +	if (ret)
> +		return ERR_PTR(ret);
> +

[Severity: High]
Does this inverted drmm cleanup order cause a NULL pointer dereference?

This drmm action is registered after the CEC drmm action (which happens
inside drm_bridge_connector_initialize()). Since drmm actions run in
LIFO order, the connector is destroyed and zeroed out before the CEC
adapter unregisters. The CEC unregister action would then dereference
connector->cec.data, which is already NULL, leading to a crash.

>  	return connector;
>  }

While reviewing this patch, I noticed a pre-existing issue in the HDMI
dynamic initialization path that this patch calls into:

drivers/gpu/drm/drm_connector.c:drm_connector_hdmi_dynamic_init()
{
	...
	ret = drm_connector_dynamic_init(dev, connector, funcs, connector_type, ddc);
	if (ret)
		return ret;

	if (ret)
		drm_connector_cleanup(connector);

	return drm_connector_hdmi_initialize(dev, connector, hdmi_funcs);
}

[Severity: Medium]
This is a pre-existing issue, but does this code contain a dead cleanup path?

The code evaluates if (ret) return ret; followed immediately by
if (ret) drm_connector_cleanup(connector);. The cleanup is dead code.
If the subsequent drm_connector_hdmi_initialize() fails, the successfully
initialized connector is leaked.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261001-drm-bridge-hotplug-v2-0-8e34986dcb68@bootlin.com?part=5