Re: [PATCH v6 09/28] drm/atomic_sro: Add function to install state into drm objects

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] drm: NULL pointer dereference in drm_atomic_sro_state_free() on error path
- [High] drm: state corruption due to destroying object state before checking replacement
- [High] drm: heap OOB read and O(N^2) loop in drm_atomic_sro_install_state
- [Medium] drm: memory leak of uninstalled private states due to count zeroing
- [Medium] drm: connector reference leak when state installation is skipped
--

commit e822513db05b3712029cac237b3eaca7c2373096
Author: Maxime Ripard <mripard@kernel.org>
Subject: drm/atomic_sro: Add function to install state into drm objects

Introduces drm_atomic_sro_install_state() to assign SRO readout states to
KMS objects. It also adds an atomic_sro_install_state hook in the plane,
CRTC, connector, and private object function vtables.

> diff --git a/drivers/gpu/drm/drm_atomic_sro.c b/drivers/gpu/drm/drm_atomic_sro.c
> index 6921af73eb697..14fddd7601658 100644
> --- a/drivers/gpu/drm/drm_atomic_sro.c
> +++ b/drivers/gpu/drm/drm_atomic_sro.c
[ ... ]
> @@ -548,3 +549,145 @@ void drm_atomic_sro_set_private_obj_state(struct drm_atomic_sro_state *state,
> +void drm_atomic_sro_install_state(struct drm_atomic_sro_state *state)
> +{
> +	unsigned int i;
> +
> +	for (i = 0; i < state->dev->mode_config.num_connector; i++) {
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Does this loop condition risk a heap out-of-bounds read? If objects are added
dynamically between drm_atomic_sro_state_alloc() and here, the loop bound will
exceed the allocated array size.

> +		const struct drm_connector_funcs *conn_funcs;
> +		struct drm_connector_state *conn_state;
> +		struct drm_connector *conn;
> +
> +		conn = state->connectors[i].ptr;
> +		if (!conn)
> +			continue;
> +
> +		conn_funcs = conn->funcs;
> +		if (!conn_funcs)
> +			continue;
                 ^^^^^^^^

[Severity: High]
Can this defensive continue lead to a NULL pointer dereference later?
If we skip installation here, the connector is left in the state array.
When drm_atomic_sro_state_free() is subsequently called to clean up
uninstalled states, it checks if conn and conn_state are present but
unconditionally dereferences conn->funcs:

drm_atomic_sro_state_free()
	if (!conn || !conn_state)
		continue;
	conn->funcs->atomic_destroy_state(conn, conn_state);

This would panic because conn->funcs was NULL.

> +
> +		if (conn->state) {
> +			conn_funcs->atomic_destroy_state(conn, conn->state);
> +			conn->state = NULL;
> +		}
> +
> +		conn_state = state->connectors[i].state;
> +		if (!conn_state)
> +			continue;
                 ^^^^^^^^

[Severity: High]
Does this logic leave the connector in a corrupted state if conn_state is
missing? The existing valid state is destroyed and set to NULL just before
this check. If we continue here, the object is left permanently with a NULL
state pointer.

[Severity: Medium]
Additionally, does continuing here skip the drm_connector_put(conn) at the
end of the loop body? If so, when drm_atomic_sro_state_free() is later invoked,
it skips its cleanup block because it checks if (!conn || !conn_state),
bypassing drm_connector_put(conn) again and leaking the connector reference.

> +
> +		if (conn_funcs->atomic_sro_install_state)
> +			conn_funcs->atomic_sro_install_state(conn, conn_state);
> +
> +		conn->state = conn_state;
> +		state->connectors[i].state = NULL;
> +		state->connectors[i].ptr = NULL;
> +		drm_connector_put(conn);
> +	}
[ ... ]
> +	for (i = 0; i < count_private_obj(state->dev); i++) {
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: High]
Similar to the connector loop, does using count_private_obj() dynamically
risk a heap out-of-bounds read if a private object is added after allocation?

Also, since count_private_obj() traverses the list on each iteration, doesn't
this result in an O(N^2) complexity loop?

[ ... ]
> +	}
> +	state->num_private_objs = 0;
         ^^^^^^^^^^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Does zeroing this counter cause a memory leak of uninstalled private states?

If a private object fails to install during hardware state readout, it remains
in the state->private_objs array. By setting the counter to 0 here,
drm_atomic_sro_state_free() will bypass its cleanup loop entirely:

drm_atomic_sro_state_free()
	for (i = 0; i < state->num_private_objs; i++) {
		...

This would permanently leak any uninstalled private object states.

> +}
> +EXPORT_SYMBOL(drm_atomic_sro_install_state);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-drm-state-readout-v6-0-6cc02cd10998@kernel.org?part=9