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 <[email protected]> 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/[email protected]?part=9
