Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider: - [High] drm: heap out-of-bounds write in drm_atomic_sro_set_private_obj_state - [High] drm: OOB read/write in drm_atomic_sro_state_free due to dynamic num_connector - [High] drm: heap out-of-bounds write in drm_atomic_sro_set_connector_state() --
commit 7fa33b0c7c68e508f3bb44a6225923e6e800d132 Author: Maxime Ripard <[email protected]> drm/atomic_sro: Create drm_atomic_sro_state container The SRO infrastructure needs a state container to collect the readout states for all KMS objects. Unlike drm_atomic_commit which tracks old and new states for atomic commits, this container only holds a single state per object: the one read out from the hardware. Create struct drm_atomic_sro_state with arrays for planes, CRTCs, connectors, and private objects, along with accessors to get and set individual object states within it. > diff --git a/drivers/gpu/drm/drm_atomic_sro.c > b/drivers/gpu/drm/drm_atomic_sro.c > new file mode 100644 > index 0000000000000..096fdb7088190 > --- /dev/null > +++ b/drivers/gpu/drm/drm_atomic_sro.c [ ... ] > +void drm_atomic_sro_state_free(struct drm_atomic_sro_state *state) > +{ > + struct drm_device *dev = state->dev; > + unsigned int i; > + > + for (i = 0; i < state->num_private_objs; i++) { > + struct drm_private_obj *obj = state->private_objs[i].ptr; [ ... ] > + kfree(state->private_objs); > + > + for (i = 0; i < state->dev->mode_config.num_connector; i++) { > + struct drm_connector *conn = state->connectors[i].ptr; > + struct drm_connector_state *conn_state = > + state->connectors[i].state; > + > + if (!conn || !conn_state) > + continue; > + > + conn->funcs->atomic_destroy_state(conn, conn_state); ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this code read past the end of the state->connectors[] array? Because the loop uses the potentially updated state->dev->mode_config.num_connector as its upper bound, what happens if a concurrent hotplug (e.g. DP MST) increases num_connector between the array's allocation and this free operation? It seems this could dereference uninitialized memory. > + state->connectors[i].state = NULL; > + state->connectors[i].ptr = NULL; > + drm_connector_put(conn); > + } > + > + kfree(state->connectors); [ ... ] > +void drm_atomic_sro_set_connector_state(struct drm_atomic_sro_state *state, > + struct drm_connector *conn, > + struct drm_connector_state *conn_state) > +{ > + unsigned int index = drm_connector_index(conn); > + > + drm_connector_get(conn); > + state->connectors[index].ptr = conn; > + state->connectors[index].state = conn_state; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Can this code write out of bounds for state->connectors[]? Since the array is sized by dev->mode_config.num_connector, but accessed here using the sparse IDA-allocated drm_connector_index(conn), what happens if a connector is dynamically removed (e.g. unplugging an MST display)? If num_connector is decremented but the remaining connectors retain their higher IDA indices, won't index exceed the allocated array capacity? > + > + drm_dbg_atomic(conn->dev, > + "Added [CONNECTOR:%d:%s] %p state to readout state %p\n", > + conn->base.id, conn->name, conn_state, state); > +} [ ... ] > +void drm_atomic_sro_set_private_obj_state(struct drm_atomic_sro_state *state, > + struct drm_private_obj *obj, > + struct drm_private_state *obj_state) > +{ > + unsigned int index = state->num_private_objs; > + > + state->private_objs[index].ptr = obj; > + state->private_objs[index].state = obj_state; > + state->num_private_objs += 1; ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ [Severity: High] Does this code overflow state->private_objs[]? Since state->private_objs[] is allocated in drm_atomic_sro_state_init() strictly based on the lockless count_private_obj(dev) snapshot, what happens if the number of private objects dynamically increases before readout? There doesn't seem to be a capacity check to ensure index doesn't exceed the allocated size if a driver calls this for dynamically created private objects. > + > + drm_dbg_atomic(state->dev, > + "Added new private object %p state %p to readout state > %p\n", obj, > + obj_state, state); > +} -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=7
