Re: [PATCH 70/75] drm/nouveau: Convert to atomic_create_state

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] drm/nouveau: Assigning ERR_PTR to connector->state causes panic in cleanup
- [High] drm/nouveau: Invalid kfree of embedded connector state on non-atomic devices
- [Medium] drm/nouveau: MST connectors lose DRM properties on initialization
--

commit 5c80f0bbb55c00fa7eb36613e602205469912331
Author: Maxime Ripard <mripard@kernel.org>

drm/nouveau: Convert to atomic_create_state

The nouveau connector reset implementation creates a custom state
subclass, but only initializes a pristine state and sets
driver-specific defaults without resetting any hardware. This is
equivalent to what atomic_create_state expects. Convert to it.

> diff --git a/drivers/gpu/drm/nouveau/dispnv50/disp.c b/drivers/gpu/drm/nouveau/dispnv50/disp.c
> index e91130f93db67..39a23d0e9be56 100644
> --- a/drivers/gpu/drm/nouveau/dispnv50/disp.c
> +++ b/drivers/gpu/drm/nouveau/dispnv50/disp.c
[ ... ]
> @@ -1281,8 +1281,11 @@ nv50_mstc_new(struct nv50_mstm *mstm, struct drm_dp_mst_port *port,
>  
>  	drm_connector_helper_add(&mstc->connector, &nv50_mstc_help);
>  
> -	mstc->connector.funcs->reset(&mstc->connector);
> -	nouveau_conn_attach_properties(&mstc->connector);
> +	mstc->connector.state = mstc->connector.funcs->atomic_create_state(&mstc->connector);

[Severity: Medium]
Will dropping this call to nouveau_conn_attach_properties() cause MST
connectors to silently lose access to their DRM properties?

Before this patch, MST connectors would get properties like underscan_property,
scaling_mode_property, dithering_mode, and vibrant_hue registered here.
Without this call, userspace compositors might lose access to these standard
digital output properties.

[Severity: High]
If atomic_create_state() returns an error pointer, this assigns it directly
to mstc->connector.state.

> +	if (IS_ERR(mstc->connector.state)) {
> +		drm_connector_cleanup(&mstc->connector);

Could passing this error pointer to drm_connector_cleanup() result in a kernel
panic?

If we take this error path, drm_connector_cleanup() checks if connector->state
is set (which is true for ERR_PTR), and calls atomic_destroy_state().
This passes the error pointer down to nouveau_conn_atomic_destroy_state(),
which will dereference the invalid pointer and panic.

> +		return PTR_ERR(mstc->connector.state);
> +	}
>  
>  	drm_for_each_crtc(crtc, dev) {
>  		if (!(mstm->outp->dcb->heads & drm_crtc_mask(crtc)))
> diff --git a/drivers/gpu/drm/nouveau/nouveau_connector.c b/drivers/gpu/drm/nouveau/nouveau_connector.c
> index cc3caf8de64b0..59c8091c67967 100644
> --- a/drivers/gpu/drm/nouveau/nouveau_connector.c
> +++ b/drivers/gpu/drm/nouveau/nouveau_connector.c
[ ... ]
> @@ -242,21 +242,17 @@ nouveau_conn_atomic_duplicate_state(struct drm_connector *connector)
>  	return &asyc->state;
>  }
>  
> -void
> -nouveau_conn_reset(struct drm_connector *connector)
> +struct drm_connector_state *
> +nouveau_conn_atomic_create_state(struct drm_connector *connector)
>  {
>  	struct nouveau_connector *nv_connector = nouveau_connector(connector);
>  	struct nouveau_conn_atom *asyc;
>  
>  	if (drm_drv_uses_atomic_modeset(connector->dev)) {
>  		if (WARN_ON(!(asyc = kzalloc_obj(*asyc))))
> -			return;
> +			return ERR_PTR(-ENOMEM);
>  
> -		if (connector->state)
> -			nouveau_conn_atomic_destroy_state(connector,
> -							  connector->state);
> -
> -		__drm_atomic_helper_connector_reset(connector, &asyc->state);
> +		__drm_atomic_helper_connector_state_init(&asyc->state, connector);
>  	} else {
>  		asyc = &nv_connector->properties_state;
>  	}

[Severity: High]
On older non-atomic hardware (pre-NV50), this returns a pointer to the
statically embedded nv_connector->properties_state.

The caller nouveau_connector_create() now unconditionally assigns this returned
pointer to connector->state. When the connector is later destroyed or reset,
drm_connector_cleanup() will invoke nouveau_conn_atomic_destroy_state(),
which unconditionally calls kfree() on the state pointer.

Can calling kfree() on this statically embedded struct pointer cause memory
corruption and a kernel panic?

[ ... ]
> @@ -1521,7 +1519,12 @@ nouveau_connector_create(struct drm_device *dev, int index)
>  		}
>  	}
>  
> -	connector->funcs->reset(connector);
> +	connector->state = connector->funcs->atomic_create_state(connector);
> +	if (IS_ERR(connector->state)) {
> +		ret = PTR_ERR(connector->state);
> +		goto drm_conn_err;

[Severity: High]
Similar to the issue in nv50_mstc_new(), if atomic_create_state() fails,
the error pointer is assigned to connector->state.

The goto drm_conn_err path calls drm_connector_cleanup(), which will attempt to
clean up the state using the error pointer.

Does this lead to a panic during the error path cleanup?

> +	}
> +
>  	nouveau_conn_attach_properties(connector);
>  
>  	/* Default scaling mode */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261014-drm-no-more-connector-reset-v1-0-586c9c026823@kernel.org?part=70